-
Notifications
You must be signed in to change notification settings - Fork 2k
support authentication indicators in GSSAPI #500
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
All failures seem to be about |
|
This looks pretty reasonable but I don't think any of the active developers have the GSSAPI knowledge to review the GSSAPI bits of this change. We'll have to find someone with more experience to look at it. |
|
@djmdjm I am one of FreeIPA, SSSD, and Samba developers and contribute to MIT Kerberos as well. This code is based on my indicators work for SSSD's If you want more external eyes, I can ask my fellow Red Hat colleagues who maintain OpenSSH and wrote GSSAPI code in past too. |
|
I'm working on libssh, MIT Kerberos and Samba. I can review the GSSAPI bits. |
|
Patch looks good to me. |
beldmit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall impression is fine, one place to change
Jakuje
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@abbra added one comment inline with one more potential issue.
We are now working with a GSoC student to implement test coverage for libssh and GSSAPI authentication in https://gitlab.com/libssh/libssh-mirror/-/merge_requests/490
I think it would be great if something similar could be in OpenSSH upstream testsuite, which could land a test for this functionality. Let me know if you would find it useful. If so, we can try to help contribute something similar.
|
I updated the code to follow recent discussions. Let me know whether this is all fine for merge. I have recorded a demo here: https://www.youtube.com/watch?v=OauHdZYKGKk. This demo shows the use of In the demo, first we attempt to access as |
|
I have been on vacation for past few weeks so may be I missed something... |
|
This code looks good to me after the changes I've requested are done and we are interested in having it upstream. |
cc9109d to
7a0bab8
Compare
b4e9e3b to
e4e2aa7
Compare
|
@abbra is it just a rebase or also changes? |
|
Just a rebase to clear a small conflict with the renamed option. |
e4e2aa7 to
e85372a
Compare
|
@bob-beck suggested to add a CI test. I'll look at this. |
gss-serv-krb5.c
Outdated
| krb5_free_error_message(krb_context, errmsg); | ||
| return 0; | ||
| } | ||
| if (krb5_kuserok(krb_context, princ, name)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO this function would be clearer if it shortcircuits when preconditions fail, i.e.
if (!krb5_kuserok(krb_context, princ, name))
goto out;
if (options.gss_indicators == NULL) {
retval = 1;
goto out;
}
if (!client->indicators) {
logit("GSSAPI authentication indicators enforced "
"but not matched. krb5 principal %s denied",
(char *)client->displayname.value);
goto out;
}
...
out:
if (retval == 1) {
logit("Authorized to %s, krb5 principal %s (%s)",
name, (char *)client->displayname.value, errmsg);
}
krb5_free_principal(krb_context, princ);
return retval;There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, it makes sense. I rewrote the logic to reduce if-conditions.
815ce02 to
0fa9399
Compare
0fa9399 to
3e44137
Compare
Signed-off-by: Alexander Bokovoy <[email protected]>
RFC 6680 defines a set of GSSAPI extensions to handle attributes associated with the GSSAPI names. MIT Kerberos and FreeIPA use name attributes to add information about pre-authentication methods used to acquire the initial Kerberos ticket. The attribute 'auth-indicators' may contain list of strings that KDC has associated with the ticket issuance process. Use authentication indicators to authorise or deny access to SSH server. GSSAPIIndicators setting allows to specify a list of possible indicators that a Kerberos ticket presented must or must not contain. More details on the syntax are provided in sshd_config(5) man page. Fixes: https://bugzilla.mindrot.org/show_bug.cgi?id=2696 Signed-off-by: Alexander Bokovoy <[email protected]>
3e44137 to
900a1af
Compare
|
@djmdjm thanks for the comments. I addressed them. We still need to add a test but it is a wider effort as you pointed out in https://lists.mindrot.org/pipermail/openssh-unix-dev/2025-December/042303.html. We have a lot of tests downstream in Fedora/RHEL and plan to adopt them to upstream CI. |
RFC 6680 defines a set of GSSAPI extensions to handle attributes associated with the GSSAPI names. MIT Kerberos and FreeIPA use name attributes to add information about pre-authentication methods used to acquire the initial Kerberos ticket. The attribute 'auth-indicators' may contain list of strings that KDC has associated with the ticket issuance process.
Use authentication indicators to authorise or deny access to SSH server. GSSAPIIndicators setting allows to specify a list of possible indicators that a Kerberos ticket presented must or must not contain. More details on the syntax are provided in sshd_config(5) man page.
Fixes: https://bugzilla.mindrot.org/show_bug.cgi?id=2696