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.

Sign up for GitHub

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

Jump to bottom

authenticate clients based on certificate CommonName in v3 API #6881

Merged
merged 2 commits into from
Feb 1, 2017
Merged

authenticate clients based on certificate CommonName in v3 API #6881

merged 2 commits into from
Feb 1, 2017

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Nov 21, 2016

This PR lets v3 auth mechanism authenticate clients based on CommonName of certificate like v2 auth.

/cc @yudai

@xiang90 xiang90 added this to the v3.2.0 milestone Nov 22, 2016
@xiang90
Copy link
Contributor

xiang90 commented Jan 13, 2017

@mitake Can you resolve the conflict? Also we need to add documentation around it.

@mitake
Copy link
Contributor Author

mitake commented Jan 15, 2017

@xiang90 sure, I'll resume this PR and the jwt token support from next week.

@mitake mitake force-pushed the auth-v3-cn branch 2 times, most recently from 5ce930b to 3ecd88f Compare January 16, 2017 05:39
@yudai
Copy link
Contributor

yudai commented Jan 18, 2017

@mitake
I ran some tests and confirmed:

  • CommonName in client certs are read properly
  • values given to --user do not overwrite CommonName in client certs
    • but a token is generated internally, i'm not sure if it's safe
  • client certs signed by an invalid CA (not the one given to --trusted-ca-file) are rejected
    • btw, in this case, client gets Error: grpc: timed out when dialing which is a bit confusing

It looks good overall.

@mitake
Copy link
Contributor Author

mitake commented Jan 18, 2017

updated for e2e testing issue.

@yudai thanks for testing, I'll consider about the behaviors you pointed out tomorrow.

xiang90
xiang90 reviewed Jan 18, 2017
View reviewed changes
auth/store.go
@@ -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 {
Copy link
Contributor

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?

Copy link
Contributor

@heyitsanthony heyitsanthony Jan 18, 2017 •
edited
Loading

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.

Copy link
Contributor Author

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.

Copy link
Contributor Author

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()?

xiang90
xiang90 reviewed Jan 18, 2017
View reviewed changes
auth/store.go
@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

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

authInfoFromTLS?

Copy link
Contributor Author

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.

xiang90
xiang90 reviewed Jan 18, 2017
View reviewed changes
e2e/etcd_test.go
@@ -106,6 +106,13 @@ var (
isPeerTLS: true,
initialToken: "new",
}
configClientTLSCertAuth = etcdProcessClusterConfig{
clusterSize: 3,
Copy link
Contributor

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?

Copy link
Contributor Author

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.

@mitake mitake mentioned this pull request Jan 30, 2017
Closed
3 tasks
@mitake
auth, etcdserver: authenticate clients based on certificate CommonName
0191509 
This commit lets v3 auth mechanism authenticate clients based on
CommonName of certificate like v2 auth.
@mitake
e2e: add a case for CommonName auth of v3 API
cd9f0a1
@mitake
Copy link
Contributor Author

mitake commented Jan 31, 2017

@xiang90 @heyitsanthony updated for your comments, could you take a look? Now I separated auth.AuthInfoFromTLS() from auth.AuthInfoFromCtx() and let etcdserver call them based on its configuration for avoiding complicated initialization of AuthStore.

@xiang90
Copy link
Contributor

xiang90 commented Jan 31, 2017

LGTM

heyitsanthony
heyitsanthony reviewed Jan 31, 2017
View reviewed changes
etcdserver/v3_server.go
@@ -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 {
Copy link
Contributor

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?

Copy link
Contributor

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".

@heyitsanthony
Copy link
Contributor

lgtm thanks

@xiang90
Copy link
Contributor

xiang90 commented Feb 1, 2017

@mitake Merging this for you! Thanks a lot!

@xiang90 xiang90 merged commit 89bb904 into etcd-io:master Feb 1, 2017
@mitake
Copy link
Contributor Author

mitake commented Feb 1, 2017

@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)

@mitake mitake deleted the auth-v3-cn branch February 1, 2017 03:31
@jessfs
Copy link

jessfs commented Jun 2, 2017

@mitake it doesn't look like the documentation ever got completed for how to use this feature. I've checked all the related issues as well such as #7251 and #3916. I did find a comment from you here mentioning you were going to work on it soon, but that's where the trail seems to go cold.

@jesseendahl jesseendahl mentioned this pull request Jun 2, 2017
Closed
@gyuho gyuho mentioned this pull request Sep 21, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xiang90 xiang90 xiang90 left review comments

@heyitsanthony heyitsanthony heyitsanthony left review comments

Assignees
No one assigned
Labels
None yet
Milestone
v3.2.0
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@mitake @xiang90 @yudai @heyitsanthony @jessfs @edevil @gyuho