-
Notifications
You must be signed in to change notification settings - Fork 40.1k
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
x509 authenticator: get groups from subject's organization field #30392
x509 authenticator: get groups from subject's organization field #30392
Conversation
cdc7c75
to
e5d7f01
Compare
@@ -92,7 +92,10 @@ var CommonNameUserConversion = UserConversionFunc(func(chain []*x509.Certificate | |||
if len(chain[0].Subject.CommonName) == 0 { | |||
return nil, false, nil | |||
} | |||
return &user.DefaultInfo{Name: chain[0].Subject.CommonName}, true, nil | |||
return &user.DefaultInfo{ |
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.
@liggitt we have some additional checks in ours and we separate this case from SubjectToUserConversion
, but I can't say that I remember why.
@liggitt this lgtm and I can't remember why we had a separate case instead of |
@erictune's point in #30260 (comment) is well taken, though I don't think this would preclude additional server-side groupification |
GCE e2e build/test passed for commit e5d7f01. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
GCE e2e build/test passed for commit e5d7f01. |
Automatic merge from submit-queue |
Note that the current X509 tests provide a bunch of certs but no private keys or commands to reproduce the testdata, so the new test case isn't added to the certificate chain.
Closes #30260
cc @treed @gtank @mikedanese @deads2k @kubernetes/sig-auth
This change is