-
Notifications
You must be signed in to change notification settings - Fork 45
Adding client usage extension for server cert (#305) #306
Adding client usage extension for server cert (#305) #306
Conversation
I had issues with make test. Don't think its related to this change |
/cc @justinsb |
/retest |
46f67f7
to
2470811
Compare
@justinsb I made the grpc sevices' cert (etcd-manager's service), and the etcd server's cert both available for using as a client cert during mutual TLS. Not sure if you have objections to allowing the grpc service's certificate being used in this way. |
@@ -63,7 +63,7 @@ func (p *etcdProcess) createKeypairs(peersCA *pki.Keypair, clientsCA *pki.Keypai | |||
|
|||
certConfig := certutil.Config{ | |||
CommonName: me.Name, | |||
Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | |||
Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, |
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.
Nit: A comment with a link to e.g. etcd-io/etcd#9785 would be helpful for people scratching their head about this one in future :-)
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.
added a comment with a link to the original issue
pkg/tlsconfig/options.go
Outdated
@@ -48,7 +48,7 @@ func GRPCServerConfig(keypairs *pki.Keypairs, myPeerID string) (*tls.Config, err | |||
|
|||
config := certutil.Config{ | |||
CommonName: "etcd-manager-server-" + myPeerID, | |||
Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, | |||
Usages: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth, x509.ExtKeyUsageClientAuth}, |
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 need client here? I think we set up our client certs separately (above in GRPCClientConfig)
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 just removed it, its not really needed per comments.
Thanks for figuring this out @mmerrill3 . I don't particularly object to adding the client flag, except that I don't think we need it - was there a reason to add it? |
@justinsb I put it in there to figure out which of the server certs was being used as a client cert, since I wasn't sure at first about which server cert was used where in etcd-manager. The gRPC service one for etcd-manager was changed first in my testing, and I just didn't undo it. If there's no need for the client attribute on it, I'll remove it. I wasn't sure, which is why I was asking if there was any use case where this cert was used as a client cert. I'll update my PR tonight. |
That's great @mmerrill3 - thanks again! Please ping the PR when you push the change so I can (be more likely to) notice it :-) |
2470811
to
ade4983
Compare
Signed-off-by: mmerrill3 <[email protected]>
@justinsb the PR has been updated and commits squashed down. |
Awesome @mmerrill3 - thank you so much! /approve |
Contains the workaround for 1-year certificate expiry. Full changes * Release notes for 3.0.20200307 [kubernetes#303](kopeio/etcd-manager#303) * Add support for etcd 3.3.17 [kubernetes#304](kopeio/etcd-manager#304) * Adding client usage extension for server cert (kubernetes#305) [kubernetes#306](kopeio/etcd-manager#306) * Add a check to renew certificates on startup if they expire in 60 days or less [kubernetes#309](kopeio/etcd-manager#309) * Try github actions [kubernetes#310](kopeio/etcd-manager#310) * Upgrade bazel to 2.2.0 [kubernetes#311](kopeio/etcd-manager#311) * Update to go 1.13.10 [kubernetes#314](kopeio/etcd-manager#314) * Bazel: update dependency [kubernetes#316](kopeio/etcd-manager#316) * e2e tests should wait for cluster readiness [kubernetes#318](kopeio/etcd-manager#318) * Remove old bazel versions from travis [kubernetes#317](kopeio/etcd-manager#317) * Always renew certificates [kubernetes#313](kopeio/etcd-manager#313)
Contains the workaround for 1-year certificate expiry. Full changes * Release notes for 3.0.20200307 [kubernetes#303](kopeio/etcd-manager#303) * Add support for etcd 3.3.17 [kubernetes#304](kopeio/etcd-manager#304) * Adding client usage extension for server cert (kubernetes#305) [kubernetes#306](kopeio/etcd-manager#306) * Add a check to renew certificates on startup if they expire in 60 days or less [kubernetes#309](kopeio/etcd-manager#309) * Try github actions [kubernetes#310](kopeio/etcd-manager#310) * Upgrade bazel to 2.2.0 [kubernetes#311](kopeio/etcd-manager#311) * Update to go 1.13.10 [kubernetes#314](kopeio/etcd-manager#314) * Bazel: update dependency [kubernetes#316](kopeio/etcd-manager#316) * e2e tests should wait for cluster readiness [kubernetes#318](kopeio/etcd-manager#318) * Remove old bazel versions from travis [kubernetes#317](kopeio/etcd-manager#317) * Always renew certificates [kubernetes#313](kopeio/etcd-manager#313)
Contains the workaround for 1-year certificate expiry. Full changes * Release notes for 3.0.20200307 [kubernetes#303](kopeio/etcd-manager#303) * Add support for etcd 3.3.17 [kubernetes#304](kopeio/etcd-manager#304) * Adding client usage extension for server cert (kubernetes#305) [kubernetes#306](kopeio/etcd-manager#306) * Add a check to renew certificates on startup if they expire in 60 days or less [kubernetes#309](kopeio/etcd-manager#309) * Try github actions [kubernetes#310](kopeio/etcd-manager#310) * Upgrade bazel to 2.2.0 [kubernetes#311](kopeio/etcd-manager#311) * Update to go 1.13.10 [kubernetes#314](kopeio/etcd-manager#314) * Bazel: update dependency [kubernetes#316](kopeio/etcd-manager#316) * e2e tests should wait for cluster readiness [kubernetes#318](kopeio/etcd-manager#318) * Remove old bazel versions from travis [kubernetes#317](kopeio/etcd-manager#317) * Always renew certificates [kubernetes#313](kopeio/etcd-manager#313)
Contains the workaround for 1-year certificate expiry. Full changes * Release notes for 3.0.20200307 [kubernetes#303](kopeio/etcd-manager#303) * Add support for etcd 3.3.17 [kubernetes#304](kopeio/etcd-manager#304) * Adding client usage extension for server cert (kubernetes#305) [kubernetes#306](kopeio/etcd-manager#306) * Add a check to renew certificates on startup if they expire in 60 days or less [kubernetes#309](kopeio/etcd-manager#309) * Try github actions [kubernetes#310](kopeio/etcd-manager#310) * Upgrade bazel to 2.2.0 [kubernetes#311](kopeio/etcd-manager#311) * Update to go 1.13.10 [kubernetes#314](kopeio/etcd-manager#314) * Bazel: update dependency [kubernetes#316](kopeio/etcd-manager#316) * e2e tests should wait for cluster readiness [kubernetes#318](kopeio/etcd-manager#318) * Remove old bazel versions from travis [kubernetes#317](kopeio/etcd-manager#317) * Always renew certificates [kubernetes#313](kopeio/etcd-manager#313)
PR to address issue (#305 )
Signed-off-by: mmerrill3 [email protected]