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

Add canonical client principal to PAC #902

Merged
merged 7 commits into from Dec 21, 2021

Conversation

lhoward
Copy link
Member

@lhoward lhoward commented Dec 19, 2021

KDC and libkrb5 support for the UPN_DNS_INFO_EX PAC info buffer, which contains the canonical client name from the TGT.

@lhoward lhoward force-pushed the lukeh/canon-name-pac branch 2 times, most recently from 5d45cc1 to f1e456a Compare December 19, 2021 07:08
@lhoward lhoward self-assigned this Dec 19, 2021
@lhoward lhoward added this to the Heimdal 8 milestone Dec 19, 2021
@lhoward lhoward force-pushed the lukeh/canon-name-pac branch 2 times, most recently from a7521cf to 0686b52 Compare December 19, 2021 10:14
kdc/kerberos5.c Outdated Show resolved Hide resolved
@lhoward lhoward force-pushed the lukeh/canon-name-pac branch 8 times, most recently from 67aff24 to c1b82f6 Compare December 20, 2021 06:28
@lhoward
Copy link
Member Author

lhoward commented Dec 20, 2021

cc @josephsutton1

@lhoward lhoward force-pushed the lukeh/canon-name-pac branch 11 times, most recently from e618149 to 73a8447 Compare December 20, 2021 20:36
@lhoward lhoward force-pushed the lukeh/canon-name-pac branch 2 times, most recently from fc1d314 to d9ddd62 Compare December 20, 2021 21:48
@lhoward lhoward force-pushed the lukeh/canon-name-pac branch 4 times, most recently from 8f4cdb5 to 8ea73e5 Compare December 21, 2021 05:26
@@ -518,6 +518,10 @@ If this flag is true, then all application protocol authentication
requests will be flagged to indicate that the application supports
channel bindings when operating over a secure channel.
The default value is false.
.It Li gss_canonicalize_source_name = Va boolean
Copy link
Contributor

Choose a reason for hiding this comment

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

Why default to false? Why not true? What might break? Why have this be configurable? Why is this in GSS and not in the rd_req side in lib/krb5? Wouldn't we want this to be used for non-GSS apps? (Well, we don't want non-GSS krb5 apps, but that's another story.)

If this should move to lib/krb5, maybe rename this param to use_client_name_from_pac.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, with the Principal decoration I'm adding, maybe we could stuff the canonical and alias names in there and provide an accessor (name attributes).

Copy link
Member Author

Choose a reason for hiding this comment

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

Why default to false? Why not true? What might break? Why have this be configurable? Why is this in GSS and not in the rd_req side in lib/krb5? Wouldn't we want this to be used for non-GSS apps? (Well, we don't want non-GSS krb5 apps, but that's another story.)

If this should move to lib/krb5, maybe rename this param to use_client_name_from_pac.

Can move to lib/krb5, good idea. Defaulting to TRUE will change behaviour where aliases are used without the canonicalize flag which would break @jaltman's use case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, note, because of the rd_req API, this will involve replacing ticket->client with the canonical name from the PAC; the ticket client will be lost forever.

@nicowilliams
Copy link
Contributor

LGTM except for the config matter (see commentary).

If the UPN_DNS_INFO buffer in the Windows PAC contains a canonical principal
name, use it in lieu of the ticket client name to determine the GSS-API
initiator name.
Use the UPN_DNS_INFO buffer of the PAC to include the canonical principal name.

Arguably we should use AD-LOGIN-ALIAS as defined in RFC6806, but we may not
always know all the principal's aliases, and this approach allows us to share
application service logic with Windows.
Add PAC_ATTRIBUTES_INFO to the PAC. This info buffer indicates whether the user
explicitly requested a PAC be present or absent.

Note: this changes the windc plugin ABI.
Note the selected pre-authentication mechanism, and add a callback to allow the
pre-authentication mechanism to update the PAC immediately prior to signing.
Update the sample GSS pre-authentication authorizer plugin to allow the PAC to
be pinned to the authenticating user's SID.

There is still a race condition between the time the user authenticates and the
time the SID is looked up via LDAP, but it should be sufficient as an example;
if more security is required, then users should be enrolled with their SIDs.
@lhoward lhoward merged commit 8590499 into heimdal:master Dec 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants