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.

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

RFC: etcdmain, pkg: CN based auth for inter peer connection #8616

Merged
merged 3 commits into from
Oct 4, 2017

Conversation

mitake
Copy link
Contributor

@mitake mitake commented Sep 27, 2017

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-cn is passed, an etcd process denies a peer
connection whose CN doesn't match.

/cc @luxas this is what we talked before, will it be useful for kubeadm?

@mitake mitake changed the title etcdmain, pkg: CN based auth for inter peer connection RFC: etcdmain, pkg: CN based auth for inter peer connection Sep 27, 2017
@mitake
Copy link
Contributor Author

mitake commented Sep 27, 2017

This PR is related to #8262 , so I'm ccing @ericchiang
If it is valuable for k8s and acceptable by etcd, I'll add tests and docs.

if info.RequireCN != "" {
cfg.VerifyPeerCertificate = func(rawCerts [][]byte, verifiedChains [][]*x509.Certificate) error {
if len(verifiedChains) != 0 {
chains := verifiedChains[0]
Copy link
Contributor

Choose a reason for hiding this comment

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

Q. why verifiedChains[0]? Do we ever get more than 1 []*x509.Certificate?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the first certificate is the leaf certificate.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mitake Let's document that on the code, then.
Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, you're right. The first cert of an individual chain is the leaf. Checking each chain sounds correct.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gyuho @ericchiang @jpbetz ok, I'll change the mechanism of extracting CN later. Thanks for your comments.

@mitake
Copy link
Contributor Author

mitake commented Sep 29, 2017

@gyuho @ericchiang @jpbetz updated the way of looking CN based on the discussion. Also I renamed the command line flag to --peer-cert-allowed-cn as originally proposed in #8262. I'm adding an e2e test and doc.

@mitake
Copy link
Contributor Author

mitake commented Sep 29, 2017

@gyuho added the doc and test, could you take a look?

for _, chain := range chains {
if info.AllowedCN == chain.Subject.CommonName {
return nil
} else {
Copy link
Contributor

Choose a reason for hiding this comment

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

else is not needed?

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

Add period . at the end of comment?

}

var args []string

Copy link
Contributor

Choose a reason for hiding this comment

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

remove this empty line?

@mitake
Copy link
Contributor Author

mitake commented Sep 29, 2017

@gyuho updated for your comments, could you take a look?

return nil
}

return fmt.Errorf("CommonName authentication failed (allowed: %s, client: %s)", info.AllowedCN, chains[0].Subject.CommonName)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.
@mitake
Copy link
Contributor Author

mitake commented Oct 2, 2017

@gyuho updated for the CN checking part, could you take a look?

@gyuho
Copy link
Contributor

gyuho commented Oct 2, 2017

lgtm. Thanks!
/cc @xiang90 @jpbetz

@gyuho gyuho merged commit 863dfd1 into etcd-io:master Oct 4, 2017
@mitake mitake deleted the peer-cn-auth branch October 5, 2017 01:32
@luxas
Copy link
Contributor

luxas commented Oct 28, 2017

Thank you @gyuho & @mitake!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants