-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
authenticate clients based on certificate CommonName in v3 API #6881
Conversation
@mitake Can you resolve the conflict? Also we need to add documentation around it. |
@xiang90 sure, I'll resume this PR and the jwt token support from next week. |
5ce930b
to
3ecd88f
Compare
@mitake
It looks good overall. |
updated for e2e testing issue. @yudai thanks for testing, I'll consider about the behaviors you pointed out tomorrow. |
@@ -879,7 +883,7 @@ func (as *authStore) isAuthEnabled() bool { | |||
return as.enabled | |||
} | |||
|
|||
func NewAuthStore(be backend.Backend, indexWaiter func(uint64) <-chan struct{}) *authStore { | |||
func NewAuthStore(be backend.Backend, indexWaiter func(uint64) <-chan struct{}, clientCertAuthEnabled bool) *authStore { |
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.
i dislike boolean arg in a public api.
@heyitsanthony opinions?
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.
Is there any reason why it should be optional? Could just hardwire it to always be enabled.
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.
Yes, on the second thought, the condition based on Cfg.ClientCertAuthEnabled
seems to be not so meaningful. I'd like to remove it. Maybe making a clear rule about priorities of auth methods (a token generated by Authenticate()
and common name of cert) and documenting it is required.
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.
On the third thought, the condition would be needed for not using common names in auth accidentally (the option is provided for this purpose in v2 auth). How about passing Cfg.ClientCertAuthEnabled
via ctx
of AuthInfoFromCtx()
?
@@ -950,7 +955,30 @@ func (as *authStore) isValidSimpleToken(token string, ctx context.Context) bool | |||
return false | |||
} | |||
|
|||
func (as *authStore) authInfoFromPeer(tlsInfo credentials.TLSInfo) (*AuthInfo, error) { |
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.
authInfoFromTLS?
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.
Seems good, I'll rename the function.
@@ -106,6 +106,13 @@ var ( | |||
isPeerTLS: true, | |||
initialToken: "new", | |||
} | |||
configClientTLSCertAuth = etcdProcessClusterConfig{ | |||
clusterSize: 3, |
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.
do we really need a 3 member cluster for this test?
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.
Yes, 3 nodes are needless. I'll reduce the number of nodes.
This commit lets v3 auth mechanism authenticate clients based on CommonName of certificate like v2 auth.
@xiang90 @heyitsanthony updated for your comments, could you take a look? Now I separated |
LGTM |
@@ -802,3 +802,14 @@ func (s *EtcdServer) linearizableReadNotify(ctx context.Context) error { | |||
return ErrStopped | |||
} | |||
} | |||
|
|||
func (s *EtcdServer) AuthInfoFromCtx(ctx context.Context) (*auth.AuthInfo, error) { | |||
if s.Cfg.ClientCertAuthEnabled { |
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.
@xiang90 is there any reason to keep this as optional in v3 if it'll just fall back to regular user auth?
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.
admin can disable cert auth. If we do not check here we might make mistake, like now command name "foo" == user "foo".
lgtm thanks |
@mitake Merging this for you! Thanks a lot! |
@xiang90 @heyitsanthony thanks! @yudai I'll work on the points you mentioned later because they would be independent from this PR (they are related to client and transport, I think) |
This PR lets v3 auth mechanism authenticate clients based on CommonName of certificate like v2 auth.
/cc @yudai