Skip to content
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

support authentication indicators in GSSAPI #500

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

abbra
Copy link

@abbra abbra commented Jun 12, 2024

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

@abbra
Copy link
Author

abbra commented Jun 12, 2024

All failures seem to be about penalty.sh dependency of a t-exec which is unrelated to the PR changes, it seems.

gss-serv-krb5.c Outdated Show resolved Hide resolved
@djmdjm
Copy link
Contributor

djmdjm commented Jun 14, 2024

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.

@abbra
Copy link
Author

abbra commented Jun 14, 2024

@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 pam_sss_gss which we have in production since 2021.

If you want more external eyes, I can ask my fellow Red Hat colleagues who maintain OpenSSH and wrote GSSAPI code in past too.

@cryptomilk
Copy link

I'm working on libssh, MIT Kerberos and Samba. I can review the GSSAPI bits.

gss-serv.c Outdated Show resolved Hide resolved
@cryptomilk
Copy link

Patch looks good to me.

Copy link

@beldmit beldmit left a 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

if (*matched) {
return (1 - denial);
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use match_pattern_list ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rewrote to use match_pattern_list. This forced a change in the way how a rule is presented, though not too different:

GSSAPIIndicators [!]name1,[!]name2,...

Where a negation would mean a denial to the presence of the indicator.
A result is that a rule like

GSSAPIIndicators !hardened,pkinit,idp,passkey,otp

would only allow access with passwordless pre-authentication method in FreeIPA (either using smartcards, external IdP authentication, FIDO2 token or TOTP/HOTP token).

This simplifies a bit both configuration part and matching rule.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added this as a fixup on top of the original commit to show the difference. Let me know if this is better one. I also would like to add it to Match handling so that one could have per-group or per-user indicator rules. Let me know if this would be useful.

Copy link
Contributor

@Jakuje Jakuje left a 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.

gss-serv.c Outdated
Comment on lines 303 to 306
if (strncmp(AUTH_INDICATORS_TAG,
attrs->elements[i].value,
sizeof(AUTH_INDICATORS_TAG) - 1) != 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the indicator guaranteed to be NULL terminated to be safely comparable with strncmp when it will be shorter than the name we look for? I see in the header of this function that it should be utf8, which might imply this, but I would rather be safe to make sure we do not go behind the buffer boundary.

Additionally, are there any prefix names? The way how it is written matches just the non-null characters, which could theoretically match attributes such as auth-indicators-something if I see right.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, indicators are UTF-8 strings and encoded so in the tickets, according to RFC 8129 (sequence of utf8string ASN.1 encoding, to be precise). In both Heimdal and MIT Kerberos any ASN.1-encoded utf8string is converted into a nul-terminated string.

As for the prefixes, you might be right that we should do exact comparison and not the prefixed one. There is nothing stopping a KDC to send another name attributes that start with auth-indicators- prefix although nothing is done like that today.

@abbra
Copy link
Author

abbra commented Jul 2, 2024

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 GSSAPIIndicators option in a Match block to constrain two users (unenforced-user and enforced-user) to authenticate with GSSAPI when their initial Kerberos tickets were obtained in a particular way. In this demo we just deny any ticket that was not obtained with OTP or a smartcard-based pre-authentication mechanism. Since FreeIPA associated a hardened indicator with SPAKE pre-authentication method (used for password-based auth), we also reject hardened indicator.

In the demo, first we attempt to access as unenforced-user, with a ticket that contains hardened indicator. Then we access as enforced-user, with a ticket that contains otp indicator. In the first case we fail and system logs show that clearly. In the second case we succeed. Again, logs demonstrate that a particular indicator was inspected and accepted.

@abbra
Copy link
Author

abbra commented Aug 7, 2024

I have been on vacation for past few weeks so may be I missed something...
@djmdjm Is there anything else preventing a further review and inclusion of the GSSAPI authentication indicators work?

@beldmit
Copy link

beldmit commented Aug 14, 2024

This code looks good to me after the changes I've requested are done and we are interested in having it upstream.
@djmdjm could you please look at it if time permits?

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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants