-
Notifications
You must be signed in to change notification settings - Fork 38.8k
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
kubeadm: Validate only the first cert entry when external ca mode is used #123102
base: master
Are you sure you want to change the base?
Conversation
|
Welcome @astundzia! |
Hi @astundzia. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: astundzia The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Working on adding the necessary EasyCLA auth with my org |
Tested on kubedm 1.23.X and |
EasyCLA completed |
/triage accepted |
Hey, thanks for the comments. I will address the comments & add a unit test this week. Thank you! |
/release-note-edit
|
@astundzia any updates? |
Added unit test, addressed comments. Please review when you get a chance. Thanks! |
4de9f48
to
c859cd0
Compare
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 some more minor comments.
please keep the commit squashed to one.
a question that i have:
i recall discussions that taking the first cert from a bundle maybe is arbitrary under certain conditions, we are already doing that in at least another location in the code base. but would it make more sense to compare entire bundles i.e. bundleCurrentCert1 == bundleExpCert1, undleCurrentCert2 == bundleExpCert12, ...
what do official specs say about that?
i don't think we should apply the change like that, but asking out of curiosity.
comparing only the first cert ensure we are comparing the CA root, supposedly, while intermediates follow and they can be perhaps different count between the current and expected.
Hey, I will address comments and squash the commits. I added some additional tests to cover this scenario. The Root Certificate Authority (Root CA) is the fundamental element of the trust model. In terms of certificate validation, it's possible to create a Certificate Authority (CA) bundle that doesn't include the Root CA. As long as the CA bundle and the leaf certificates can trace back to a common trust anchor (a CA), the certificate chain should be valid. However, you're correct that I should compare more than just the first certificate. Let's consider this (convoluted) scenario: We have a Certificate Authority chain that includes the Root CA, Intermediate CA1, and Intermediate CA2. At some point, we issue Intermediate CA3 from Intermediate CA1. In this case, we can have a In this scenario, it would be beneficial to compare all the current |
a171b55
to
ffbebf2
Compare
/retest |
/retest |
1 similar comment
/retest |
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.
thanks for the detailed explanation!
yes, i think that the extended validation against all current CAs makes sense.
the logic and new tests SGTM.
i did another pass over the code, found some minor prior nits and also some new ones.
i think i might request another reviewer to have a quick look, in case we missed something.
} | ||
if !trustAnchorFound { | ||
return errors.Errorf("a kubeconfig file %q exists already but could not find trust anchor", kubeConfigFilePath) |
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.
return errors.Errorf("a kubeconfig file %q exists already but could not find trust anchor", kubeConfigFilePath) | |
return errors.Errorf("a kubeconfig file %q exists but does not contain a common trust anchor CA in its current context's cluster. Total CA certificates found: %d", kubeConfigFilePath, len(currentCACerts)) |
trying to formulate an error message that is sufficiently descriptive for users, so that they can resolve potential problems without logging a ticket for us.
my idea is to cover a couple of potential mistakes:
- not having the right context / cluster in the kubeconfig
- not having the right CA embedded for a given cluster in the context / cluster.
please adjust if you think it can be worded better.
// Only use the first certificate in the expected CA cert list | ||
expectedCACert := expectedCACerts[0] | ||
|
||
// find a common trust anchor |
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.
// find a common trust anchor | |
// Find a common trust anchor |
}, | ||
} | ||
|
||
// creates CA, write to pkiDir and remove ca.key to get into external CA condition |
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.
// creates CA, write to pkiDir and remove ca.key to get into external CA condition | |
// Creates CA, write to pkiDir and remove ca.key to get into external CA mode |
that's how we call it (mode) across docs
also capitalization
clusterName := "myOrg1" | ||
|
||
// Create a kube config and assign the CA data to the CA bundle, issued from root CA | ||
multipleCAConfigRootCAIssuer := setupdKubeConfigWithClientAuth(t, caCert, caKey, "https://1.2.3.4:1234", "test-cluster", clusterName) |
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.
a good opportunity to fix this typo?
multipleCAConfigRootCAIssuer := setupdKubeConfigWithClientAuth(t, caCert, caKey, "https://1.2.3.4:1234", "test-cluster", clusterName) | |
multipleCAConfigRootCAIssuer := setupKubeConfigWithClientAuth(t, caCert, caKey, "https://1.2.3.4:1234", "test-cluster", clusterName) |
// creates CA, write to pkiDir and remove ca.key to get into external CA condition | ||
caCert, caKey := certstestutil.SetupCertificateAuthority(t) | ||
|
||
// create a config with a CA cert containing multiple certificates |
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.
// create a config with a CA cert containing multiple certificates | |
// Create a config with a CA cert containing multiple certificates |
multipleCAConfigIntermediateCA2Issuer := setupdKubeConfigWithClientAuth(t, intermediateCACert2, intermediateCAKey2, "https://1.2.3.4:1234", "test-cluster", clusterName) | ||
multipleCAConfigIntermediateCA2Issuer.Clusters[clusterName].CertificateAuthorityData = caCertBytes | ||
|
||
// create a config with a CA cert containing multiple certificates, omitting the root CA |
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.
// create a config with a CA cert containing multiple certificates, omitting the root CA | |
// Create a config with a CA cert containing multiple certificates, omitting the root CA |
trying to use consistent capitalization of comments within a function (or even file)
tests := map[string]struct { | ||
filesToWrite map[string]*clientcmdapi.Config | ||
initConfig *kubeadmapi.InitConfiguration | ||
expectedError bool |
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.
all expected errors in this test are "false", i guess we should have at least one with "true"?
Config: certutil.Config{CommonName: "kubernetes intermediate CA"}, | ||
}) | ||
if err != nil { | ||
t.Fatalf("failure while generating intermediate CA certificate and key: %v", err) |
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.
t.Fatalf("failure while generating intermediate CA certificate and key: %v", err) | |
t.Fatalf("Failure while generating intermediate CA certificate and key: %v", err) |
test output messages should be capitalized unlike non-test errors
multipleCAConfigNoRootCA.Clusters[clusterName].CertificateAuthorityData = caCertBytesNoRootCA | ||
|
||
if err := pkiutil.WriteCertBundle(pkiDir, kubeadmconstants.CACertAndKeyBaseName, caCertBundleNoRootCA); err != nil { | ||
t.Fatalf("failure while saving CA certificate: %v", err) |
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.
t.Fatalf("failure while saving CA certificate: %v", err) | |
t.Fatalf("Failure while saving CA certificate: %v", err) |
// Parse the expected certificate authority data | ||
expectedCACerts, err := certutil.ParseCertsPEM(caExpected) | ||
if err != nil { | ||
return errors.Errorf("the expected base64 encoded CA cert %q could not be parsed as a PEM", caExpected) |
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.
this would print the whole base64 string?
if so i think it should be at the end of the message:
return errors.Errorf("the expected base64 encoded CA cert %q could not be parsed as a PEM", caExpected) | |
return errors.Errorf("the expected base64 encoded CA cert could not be parsed as a PEM:\n%s\n", caExpected) |
generally users should not see this error as it would be a preparation error on our side - i.e. expected kubeconfig file was malformed. i think we already have tests for that.
Hey, will get the comments addressed and a commit up in the next few days. |
ok, note that code freeze for 1.30 is march 6th. |
@astundzia do you have plans to finish this PR? |
Hey, apologies for the lack of commits here. I will try to get this merged in this week! |
/sig cluster-lifecycle
What type of PR is this?
/kind bug
What this PR does / why we need it:
We are deploying kubernetes using kubeadm using an external ca cert file / external ca mode. This external ca cert file contains multiple certificates. When kubeadm runs in external CA mode, kubeadm runs validation logic that compares the entire existing ca certificate file (
/etc/kubernetes/pki/ca.crt
) to the newly rendered 'dummy' kubeconfig that only reads in the first certificate fromca.crt
. The bytes are then compared & fails the check since they are different -- 1 contains 1+N certificates, the other only contains 1 certificate.The
ca.crt
used within validation is read here:https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/util/pkiutil/pki_helpers.go#L292
The original
ca.crt
is read here:https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go#L257
The check is happening here:
https://github.com/kubernetes/kubernetes/blob/master/cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go#L268
Which issue(s) this PR fixes:
Fixes kubernetes/kubeadm#3011
Special notes for your reviewer:
Does this PR introduce a user-facing change?
YES