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

x509 authenticator: get groups from subject's organization field #30392

Merged

Conversation

ericchiang
Copy link
Contributor

@ericchiang ericchiang commented Aug 10, 2016

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 Reviewable

@ericchiang ericchiang force-pushed the x509-get-groups-from-org branch from cdc7c75 to e5d7f01 Compare August 10, 2016 19:32
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 10, 2016
@@ -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{
Copy link
Contributor

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 liggitt added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-label-needed labels Aug 11, 2016
@deads2k
Copy link
Contributor

deads2k commented Aug 17, 2016

@liggitt this lgtm and I can't remember why we had a separate case instead of CommonNameUserConversion. I plan to tag it today unless you have something to add.

@liggitt
Copy link
Member

liggitt commented Aug 17, 2016

@erictune's point in #30260 (comment) is well taken, though I don't think this would preclude additional server-side groupification

@deads2k
Copy link
Contributor

deads2k commented Aug 17, 2016

@erictune's point in #30260 (comment) is well taken, though I don't think this would preclude additional server-side groupification

I agree. Nothing precludes us from allowing both when we have groupification.

@deads2k deads2k added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 17, 2016
@liggitt
Copy link
Member

liggitt commented Aug 17, 2016

@k8s-bot unit test this issue #30788

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2016

GCE e2e build/test passed for commit e5d7f01.

@k8s-github-robot
Copy link

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

@k8s-bot
Copy link

k8s-bot commented Aug 21, 2016

GCE e2e build/test passed for commit e5d7f01.

@k8s-github-robot
Copy link

Automatic merge from submit-queue

@k8s-github-robot k8s-github-robot merged commit d9705f8 into kubernetes:master Aug 21, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants