security: added CRL check [WIP] #5968

kayrus · 2016-07-18T13:52:30Z

resolves #4034, uses patches from cloudflare/cfssl#637

Comments are appreciated.

Test certs: https://github.com/endocode/etcd/raw/kayrus/test_certs/test_certs.tgz

Add /etc/hosts entry for test certs:

echo "127.0.0.1 coreos1" | sudo tee -a /etc/hosts

Test commands:

./bin/etcd --force-new-cluster=true \
 --trusted-ca-file cfssl/ca.pem \
 --client-cert-auth=true \
 --crl-file cfssl/crl.pem \
 --cert-file cfssl/server.pem \
 --key-file cfssl/server-key.pem \
 --listen-client-urls=https://localhost:2379 \
 --advertise-client-urls=https://localhost:2379 \
 --client-cert-auth=true \
 --peer-cert-file cfssl/server.pem \
 --peer-key-file cfssl/server-key.pem \
 --peer-trusted-ca-file cfssl/ca.pem \
 --peer-client-cert-auth=true \
 --peer-crl-file cfssl/crl.pem \
 --listen-peer-urls https://127.0.0.1:2380

curl -v --cacert cfssl/ca.pem \
 --cert cfssl/client.pem \
 --key cfssl/client-key.pem \
 https://coreos1:2379/v2/keys/message

heyitsanthony · 2016-07-18T16:56:41Z

etcdmain/config.go

 	fs.BoolVar(&cfg.ClientAutoTLS, "auto-tls", false, "Client TLS using generated certificates")
 	fs.StringVar(&cfg.PeerTLSInfo.CAFile, "peer-ca-file", "", "DEPRECATED: Path to the peer server TLS CA file.")
 	fs.StringVar(&cfg.PeerTLSInfo.CertFile, "peer-cert-file", "", "Path to the peer server TLS cert file.")
 	fs.StringVar(&cfg.PeerTLSInfo.KeyFile, "peer-key-file", "", "Path to the peer server TLS key file.")
 	fs.BoolVar(&cfg.PeerTLSInfo.ClientCertAuth, "peer-client-cert-auth", false, "Enable peer client cert authentication.")
 	fs.StringVar(&cfg.PeerTLSInfo.TrustedCAFile, "peer-trusted-ca-file", "", "Path to the peer server TLS trusted CA file.")
+	fs.StringVar(&cfg.PeerTLSInfo.CRLFile, "peer-crl-file", "", "Path to the peer server certificate revocation list file.")


these options should also be available via yaml conf in embed/config.go

xiang90 · 2016-07-18T20:39:50Z

Can we add a e2e or integration test for this? I think the direction is good!

heyitsanthony · 2016-07-18T20:47:43Z

embed/etcd.go

@@ -272,6 +275,27 @@ func startClientListeners(cfg *Config) (sctxs map[string]*serveCtx, err error) {
 	return sctxs, nil
 }

+func revokeCheckHandler(req *http.Request, CRLpath string) error {


I think this should live in tlsutil? There's nothing etcd specific here.

heyitsanthony · 2016-07-18T20:50:33Z

Mostly looks good aside from some minor code stuff. Thanks!

kayrus · 2016-07-19T16:30:42Z

Can someone explain me why when I add debug info at the beginning of revokeCheckHandler function, I notice that the req contains nil TLS for the Peer listener? So it doesn't check whether cert is revocated for peers.

kayrus · 2016-07-19T19:54:13Z

@heyitsanthony @xiang90 ^

heyitsanthony · 2016-07-19T21:18:01Z

@kayrus I debugged this down to the go standard library. The TLS field is set depending on the tlsState which is set only if the connection type is tls.Conn (cf. https://golang.org/src/net/http/server.go#L1398). The peer listeners are created with rafthttp.NewListener which creates a transport.NewTimeoutListener, which creates a timeoutConn. So it looks like the type assertion is failing in the standard library and tlsState is never set.

kayrus · 2016-07-19T21:41:19Z

@heyitsanthony what can we do with this?

kayrus · 2016-07-20T00:05:13Z

@heyitsanthony nice! #5998 did the trick

heyitsanthony · 2016-07-20T16:30:40Z

cmd/vendor/github.com/cloudflare/cfssl/revoke/revoke.go

@@ -31,17 +31,20 @@ type Revoke struct {
 	// status of a certificate (i.e. due to network failure) causes
 	// verification to fail (a hard failure).
 	HardFail bool
+	// HardFailLck is a Mutex locker for the HardFail
+	HardFailLck sync.Mutex


please stick to whatever is in upstream?

which rev in the cfssl repo has type Revoke struct? vendored deps shouldn't diverge from upstream...

I work on several PRs in parallel. Please refer to the main PR message. It is still in progress I will let you know when it will be merged.

xiang90 · 2016-07-20T18:58:05Z

@kayrus Let us know when this is reviewable. Probably separate out the vendor and code commits. Thank you a lot!

kayrus · 2016-07-20T18:58:56Z

@xiang90 sure. That is why this PR has [WIP] postfix.

heyitsanthony · 2017-06-20T17:53:28Z

superseded by #8124

kayrus force-pushed the kayrus/crl_check branch from 5e0a942 to 0d47908 Compare July 18, 2016 13:58

heyitsanthony reviewed Jul 18, 2016
View reviewed changes

kayrus force-pushed the kayrus/crl_check branch from 0d47908 to 3cb4777 Compare July 18, 2016 20:36

heyitsanthony reviewed Jul 18, 2016
View reviewed changes

kayrus force-pushed the kayrus/crl_check branch from 3cb4777 to 19dcd0f Compare July 19, 2016 08:37

kayrus mentioned this pull request Jul 19, 2016

etcd should support signals handling #5987

Closed

kayrus force-pushed the kayrus/crl_check branch 3 times, most recently from aafbf79 to 03272cd Compare July 19, 2016 15:43

kayrus force-pushed the kayrus/crl_check branch 3 times, most recently from 52ab929 to 791f37d Compare July 20, 2016 08:28

heyitsanthony reviewed Jul 20, 2016
View reviewed changes

kayrus force-pushed the kayrus/crl_check branch from 6a17033 to 5f68732 Compare July 21, 2016 15:45

kayrus added 4 commits July 21, 2016 17:45

vendor: added cfssl/revoke, crypto/ocsp and crypto/pkcs12 packages

717f444

security: Implemented certificate revocation list

461cece

CRL: Update cfssl dependency and fixed remarks

f08dfd8

Updated deps

2b066b8

kayrus added 5 commits July 21, 2016 17:45

Added --crl-check flag for the certs with built-in CRL info

ecb04ea

Updated deps

b7dea95

Added a hardfail option

829fb81

Updated cfssl/revoke

23c6d2a

Updated PR

e016f6d

kayrus force-pushed the kayrus/crl_check branch from 5f68732 to 1b82996 Compare July 21, 2016 15:45

embed: Fixed serve() err return

c6baf70

kayrus force-pushed the kayrus/crl_check branch from 1b82996 to c6baf70 Compare July 21, 2016 16:07

kayrus added 2 commits July 21, 2016 18:35

Updated cfssl/revoke

9e0dba8

Updated cfssl/revoke pkg

a8b7511

xiang90 added the kind/release-note label Aug 9, 2016

gyuho force-pushed the master branch 2 times, most recently from 44ca396 to 4301f49 Compare June 2, 2017 15:53

heyitsanthony mentioned this pull request Jun 17, 2017

reject connections based on CRL file #8124

Merged

heyitsanthony closed this Jun 20, 2017

gyuho removed the kind/need-release-note-v3.3 label Nov 22, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

security: added CRL check [WIP] #5968

security: added CRL check [WIP] #5968

kayrus commented Jul 18, 2016 •

edited

Loading

heyitsanthony Jul 18, 2016

kayrus Jul 18, 2016

xiang90 commented Jul 18, 2016

heyitsanthony Jul 18, 2016

heyitsanthony commented Jul 18, 2016

kayrus commented Jul 19, 2016 •

edited

Loading

kayrus commented Jul 19, 2016

heyitsanthony commented Jul 19, 2016

kayrus commented Jul 19, 2016

kayrus commented Jul 20, 2016

heyitsanthony Jul 20, 2016

kayrus Jul 20, 2016

heyitsanthony Jul 20, 2016

kayrus Jul 20, 2016

xiang90 commented Jul 20, 2016

kayrus commented Jul 20, 2016 •

edited

Loading

heyitsanthony commented Jun 20, 2017

security: added CRL check [WIP] #5968

security: added CRL check [WIP] #5968

Conversation

kayrus commented Jul 18, 2016 • edited Loading

heyitsanthony Jul 18, 2016

Choose a reason for hiding this comment

kayrus Jul 18, 2016

Choose a reason for hiding this comment

xiang90 commented Jul 18, 2016

heyitsanthony Jul 18, 2016

Choose a reason for hiding this comment

heyitsanthony commented Jul 18, 2016

kayrus commented Jul 19, 2016 • edited Loading

kayrus commented Jul 19, 2016

heyitsanthony commented Jul 19, 2016

kayrus commented Jul 19, 2016

kayrus commented Jul 20, 2016

heyitsanthony Jul 20, 2016

Choose a reason for hiding this comment

kayrus Jul 20, 2016

Choose a reason for hiding this comment

heyitsanthony Jul 20, 2016

Choose a reason for hiding this comment

kayrus Jul 20, 2016

Choose a reason for hiding this comment

xiang90 commented Jul 20, 2016

kayrus commented Jul 20, 2016 • edited Loading

heyitsanthony commented Jun 20, 2017

kayrus commented Jul 18, 2016 •

edited

Loading

kayrus commented Jul 19, 2016 •

edited

Loading

kayrus commented Jul 20, 2016 •

edited

Loading