RFC: etcdmain, pkg: CN based auth for inter peer connection #8616
Conversation
mitake
changed the title
|
This PR is related to #8262 , so I'm ccing @ericchiang |
pkg/transport/listener.go
Outdated
| if info.RequireCN != "" { | ||
| cfg.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error { | ||
| if len(verifiedChains) != 0 { | ||
| chains := verifiedChains[0] |
There was a problem hiding this comment.
Q. why verifiedChains[0]? Do we ever get more than 1 []*x509.Certificate?
There was a problem hiding this comment.
I believe the first certificate is the leaf certificate.
There was a problem hiding this comment.
@mitake Let's document that on the code, then.
Thanks!
There was a problem hiding this comment.
verifiedChains is documented as any verified chains that normal processing found. Wouldn't it be safest to verify them all (for _, chains := range verifiedChains { ...), just in-case there is more than one? I can't think of any down side.
There was a problem hiding this comment.
Sorry, you're right. The first cert of an individual chain is the leaf. Checking each chain sounds correct.
There was a problem hiding this comment.
@gyuho @ericchiang @jpbetz ok, I'll change the mechanism of extracting CN later. Thanks for your comments.
mitake
force-pushed
the
peer-cn-auth
branch
from
8d33f2f
to
427c582
Compare
|
@gyuho @ericchiang @jpbetz updated the way of looking CN based on the discussion. Also I renamed the command line flag to |
mitake
force-pushed
the
peer-cn-auth
branch
from
427c582
to
bcbc6d1
Compare
|
@gyuho added the doc and test, could you take a look? |
pkg/transport/listener.go
Outdated
| for _, chain := range chains { | ||
| if info.AllowedCN == chain.Subject.CommonName { | ||
| return nil | ||
| } else { |
pkg/transport/listener.go
Outdated
| @@ -76,6 +77,9 @@ type TLSInfo struct { | |||
| // parseFunc exists to simplify testing. Typically, parseFunc | |||
| // should be left nil. In that case, tls.X509KeyPair will be used. | |||
| parseFunc func([]byte, []byte) (tls.Certificate, error) | |||
|
|
|||
| // AllowedCN is a CN which must be provided by a client | |||
There was a problem hiding this comment.
Add period . at the end of comment?
e2e/etcd_config_test.go
Outdated
| } | ||
|
|
||
| var args []string | ||
|
|
mitake
force-pushed
the
peer-cn-auth
branch
from
bcbc6d1
to
ca03c96
Compare
|
@gyuho updated for your comments, could you take a look? |
pkg/transport/listener.go
Outdated
| return nil | ||
| } | ||
|
|
||
| return fmt.Errorf("CommonName authentication failed (allowed: %s, client: %s)", info.AllowedCN, chains[0].Subject.CommonName) |
There was a problem hiding this comment.
Just remove this return fmt.Errorf entirely because it's possible that one of the chain has different Subject.CommonName?
If we still need to keep this error, it should be only when chain.Subject.CommonName != "" and also s/chains[0].Subject.CommonName/chain.Subject.CommonName/.
There was a problem hiding this comment.
OK I'll change the behaviour for checking a leaf of every verified chain. Probably checking non-leaf certs should be avoided because they wouldn't be user supplied. If the parameter passed with --peer-cert-allowed-cn and CNs in the non leaf certs match accidentally, a node can be authenticated in a wrong manner.
I'm still not sure how the case of multiple verified chains can happen yet... I'm digging it.
This commit adds an authentication mechanism to inter peer connection (rafthttp). If the cert based peer auth is enabled and a new option `--peer-cert-allowed-cn` is passed, an etcd process denies a peer connection whose CN doesn't match.
|
@gyuho updated for the CN checking part, could you take a look? |
This commit adds an authentication mechanism to inter peer connection
(rafthttp). If the cert based peer auth is enabled and a new option
--peer-require-cnis passed, an etcd process denies a peerconnection whose CN doesn't match.
/cc @luxas this is what we talked before, will it be useful for kubeadm?