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

v2http: client certificate auth via common name #5991

Merged
merged 9 commits into from
Jul 21, 2016
Merged

v2http: client certificate auth via common name #5991

merged 9 commits into from
Jul 21, 2016

Conversation

gyuho
Copy link
Contributor

@gyuho gyuho commented Jul 19, 2016

Cherry-picked from #3916.

@gyuho
Copy link
Contributor Author

gyuho commented Jul 19, 2016

/cc @roboll @mitake @xiang90 @heyitsanthony

xiang90
xiang90 reviewed Jul 19, 2016
View reviewed changes
etcdserver/api/v2http/client_auth.go
return false
var rootUser *auth.User
if r.Header.Get("Authorization") == "" {
rootUser = userFromClientCertificate(sec, r)
Copy link
Contributor

@xiang90 xiang90 Jul 19, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

we probably should only enable this checking when client auth is set: https://github.com/coreos/etcd/blob/master/etcdmain/config.go#L174

Or we trust random client certs.

/cc @gtank @heyitsanthony any opinion?

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree it should ensure the cert's been signed by the client ca

@gyuho
Copy link
Contributor Author

gyuho commented Jul 19, 2016

@xiang90 @heyitsanthony Added ClientTLSCertEnabled flag to use common name only when the client TLS cert auth is enabled.

PTAL.

Thanks.

xiang90
xiang90 reviewed Jul 19, 2016
View reviewed changes
embed/etcd.go
@@ -116,6 +116,7 @@ func StartEtcd(inCfg *Config) (e *Etcd, err error) {
QuotaBackendBytes: cfg.QuotaBackendBytes,
StrictReconfigCheck: cfg.StrictReconfigCheck,
EnablePprof: cfg.EnablePprof,
ClientTLSCertEnabled: cfg.ClientTLSInfo.ClientCertAuth,
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably we should just name this as ClientCertAuthEnabled

@gyuho gyuho mentioned this pull request Jul 19, 2016
Closed
@gyuho
Copy link
Contributor Author

gyuho commented Jul 19, 2016

@xiang90 Just addressed. PTAL.

Thanks.

@gyuho gyuho changed the title etcdserver/api/v2http: client certificate auth via common name DO NOT MERGE: etcdserver/api/v2http: client certificate auth via common name Jul 20, 2016
@gyuho
Copy link
Contributor Author

gyuho commented Jul 20, 2016

Do not merge yet. Will add e2e tests for this.

@gyuho gyuho changed the title DO NOT MERGE: etcdserver/api/v2http: client certificate auth via common name v2http: client certificate auth via common name Jul 20, 2016
@gyuho
Copy link
Contributor Author

gyuho commented Jul 20, 2016

@xiang90 Just added e2e tests. Also confirmed manually that it works without username and password when CN is provided:

$ ./bin/etcdctl \
  --endpoints=https://localhost:2379 \
  --cert-file=hack/tls-setup/certs/ca.pem \
  --key-file=hack/tls-setup/certs/ca-key.pem \
  --ca-file=hack/tls-setup/certs/ca.pem \
  set foo bar
bar

$ ./bin/etcdctl \
  --endpoints=https://localhost:2379 \
  --cert-file=hack/tls-setup/certs/ca.pem \
  --key-file=hack/tls-setup/certs/ca-key.pem \
  --ca-file=hack/tls-setup/certs/ca.pem \
  set hello world
Error:  110: The request requires user authentication (Insufficient credentials) [0]

@gyuho
Copy link
Contributor Author

gyuho commented Jul 21, 2016

Also confirmed that if different key given, auth feature correctly rejects the request

ctl_v2_test.go:311: failed to write (read /dev/ptmx: input/output error (expected "bar", got ["Error:  110: The request requires user authentication (Insufficient credentials) [0]\r\n"]))

@xiang90
Copy link
Contributor

xiang90 commented Jul 21, 2016

lgtm.

@gyuho gyuho merged commit de638a5 into etcd-io:master Jul 21, 2016
@gyuho gyuho deleted the manual branch July 21, 2016 15:02
This was referenced Jul 21, 2016
Closed
Closed
@swsnider
Copy link

swsnider commented Nov 3, 2016

I see that this has been assigned the kind/backport label. Will this be backported?

@gyuho
Copy link
Contributor Author

gyuho commented Nov 3, 2016

@swsnider Yes this has been backported to https://github.com/coreos/etcd/releases/tag/v3.0.4.

You need etcd v3.0.4+ for this feature.

Thanks.

@geekofalltrades
Copy link

Is this feature documented anywhere?

@geekofalltrades geekofalltrades mentioned this pull request Jan 30, 2017
Closed
@lumjjb lumjjb mentioned this pull request Feb 9, 2017
Closed
@jesseendahl
Copy link

@geekofalltrades it doesn't appear to be. I just opened a new issue at #8020.

@jesseendahl jesseendahl mentioned this pull request Jun 2, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gyuho @xiang90 @swsnider @geekofalltrades @jesseendahl @heyitsanthony @roboll