kubeadm: Validate only the first cert entry when external ca mode is used #123102

astundzia · 2024-02-02T20:54:05Z

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

kubeadm: allow to validate "ca.crt" files that contain multiple certificates.

linux-foundation-easycla · 2024-02-02T20:54:11Z

The committers listed above are authorized under a signed CLA.

✅ login: astundzia / name: Alex Stundzia (074a980)

k8s-ci-robot · 2024-02-02T20:54:14Z

Welcome @astundzia!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2024-02-02T20:54:15Z

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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

k8s-ci-robot · 2024-02-02T20:54:54Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: astundzia
Once this PR has been reviewed and has the lgtm label, please assign sataqiu for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

cmd/kubeadm/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

astundzia · 2024-02-02T21:20:48Z

Working on adding the necessary EasyCLA auth with my org

astundzia · 2024-02-02T21:53:48Z

Tested on kubedm 1.23.X and master.

astundzia · 2024-02-03T01:06:21Z

EasyCLA completed

neolit123 · 2024-02-05T07:03:29Z

/triage accepted
/priority backlog

astundzia · 2024-02-05T14:48:04Z

Hey, thanks for the comments. I will address the comments & add a unit test this week. Thank you!

neolit123 · 2024-02-08T19:28:20Z

/release-note-edit

kubeadm: allow to validate "ca.crt" files that contain multiple certificates.

neolit123 · 2024-02-16T12:00:10Z

Hey, thanks for the comments. I will address the comments & add a unit test this week. Thank you!

@astundzia any updates?

astundzia · 2024-02-17T07:01:25Z

Added unit test, addressed comments. Please review when you get a chance. Thanks!

neolit123

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.

cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go

cmd/kubeadm/app/util/certs/util.go

astundzia · 2024-02-17T20:53:49Z

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 ca.crt that includes Intermediate CA1 and Intermediate CA2. The trust chain must include all certificates tracing back to a common trust anchor, which in this case is Intermediate CA1. Certificates issued from Intermediate CA3 should validate against a bundle containing any of the following combinations: {Root CA, Intermediate CA1, Intermediate CA2}, {Root CA, Intermediate CA1}, or {Intermediate CA1}, as long as the trust chain can be resolved.

In this scenario, it would be beneficial to compare all the current ca.crt certificates against the first expected CA certificate. As long as there's a match between the first expected certificate & any of the certs in the current ca bundle, a trust chain should be established.

astundzia · 2024-02-17T21:27:17Z

/retest

astundzia · 2024-02-17T22:19:17Z

/retest

astundzia · 2024-02-17T23:47:49Z

/retest

neolit123

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.

neolit123 · 2024-02-18T08:45:10Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

 }
+ if !trustAnchorFound {
+ return errors.Errorf("a kubeconfig file %q exists already but could not find trust anchor", kubeConfigFilePath)


Suggested change

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.

neolit123 · 2024-02-18T08:45:19Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

+ // Only use the first certificate in the expected CA cert list
+ expectedCACert := expectedCACerts[0]
+
+ // find a common trust anchor


Suggested change

// find a common trust anchor

// Find a common trust anchor

neolit123 · 2024-02-18T09:24:44Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go

+ },
+ }
+
+ // creates CA, write to pkiDir and remove ca.key to get into external CA condition


Suggested change

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

neolit123 · 2024-02-18T09:26:01Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go

+ 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)


a good opportunity to fix this typo?

Suggested change

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)

neolit123 · 2024-02-18T09:26:31Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go

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


Suggested change

// create a config with a CA cert containing multiple certificates

// Create a config with a CA cert containing multiple certificates

neolit123 · 2024-02-18T09:28:25Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go

+ 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


Suggested change

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

neolit123 · 2024-02-18T09:30:07Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go

+ tests := map[string]struct {
+ filesToWrite map[string]*clientcmdapi.Config
+ initConfig *kubeadmapi.InitConfiguration
+ expectedError bool


all expected errors in this test are "false", i guess we should have at least one with "true"?

neolit123 · 2024-02-18T09:33:09Z

cmd/kubeadm/app/util/certs/util.go

+ Config: certutil.Config{CommonName: "kubernetes intermediate CA"},
+ })
+ if err != nil {
+ t.Fatalf("failure while generating intermediate CA certificate and key: %v", err)


Suggested change

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

neolit123 · 2024-02-18T09:33:28Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go

+ multipleCAConfigNoRootCA.Clusters[clusterName].CertificateAuthorityData = caCertBytesNoRootCA
+
+ if err := pkiutil.WriteCertBundle(pkiDir, kubeadmconstants.CACertAndKeyBaseName, caCertBundleNoRootCA); err != nil {
+ t.Fatalf("failure while saving CA certificate: %v", err)


Suggested change

t.Fatalf("failure while saving CA certificate: %v", err)

t.Fatalf("Failure while saving CA certificate: %v", err)

neolit123 · 2024-02-18T09:36:45Z

cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go

+ // 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)


this would print the whole base64 string?
if so i think it should be at the end of the message:

Suggested change

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.

astundzia · 2024-03-01T02:54:02Z

Hey, will get the comments addressed and a commit up in the next few days.

neolit123 · 2024-03-01T03:30:19Z

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.

neolit123 · 2024-05-20T10:43:01Z

Hey, will get the comments addressed and a commit up in the next few days.

@astundzia do you have plans to finish this PR?

astundzia · 2024-05-29T00:15:39Z

Hey, apologies for the lack of commits here. I will try to get this merged in this week!

k8s-ci-robot added needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 2, 2024

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 2, 2024

k8s-ci-robot requested review from chendave and SataQiu February 2, 2024 20:55

k8s-ci-robot added the area/kubeadm label Feb 2, 2024

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 3, 2024

astundzia force-pushed the master branch from f5673cf to fc47ccd Compare February 3, 2024 02:48

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 3, 2024

astundzia force-pushed the master branch from fc47ccd to 2ba540a Compare February 3, 2024 03:56

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 3, 2024

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 17, 2024

astundzia force-pushed the master branch from 277fb81 to 2711763 Compare February 17, 2024 07:00

astundzia force-pushed the master branch 2 times, most recently from 4de9f48 to c859cd0 Compare February 17, 2024 07:07

neolit123 reviewed Feb 17, 2024

View reviewed changes

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 17, 2024

astundzia force-pushed the master branch 2 times, most recently from a171b55 to ffbebf2 Compare February 17, 2024 21:26

When using external CA, look for common trust anchor within CA bundle.

074a980

astundzia force-pushed the master branch from ffbebf2 to 074a980 Compare February 17, 2024 22:18

neolit123 reviewed Feb 18, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm: Validate only the first cert entry when external ca mode is used #123102

kubeadm: Validate only the first cert entry when external ca mode is used #123102

astundzia commented Feb 2, 2024 •

edited by k8s-ci-robot

linux-foundation-easycla bot commented Feb 2, 2024 •

edited

k8s-ci-robot commented Feb 2, 2024

k8s-ci-robot commented Feb 2, 2024

k8s-ci-robot commented Feb 2, 2024

astundzia commented Feb 2, 2024

astundzia commented Feb 2, 2024

astundzia commented Feb 3, 2024

neolit123 commented Feb 5, 2024

astundzia commented Feb 5, 2024

neolit123 commented Feb 8, 2024

neolit123 commented Feb 16, 2024

astundzia commented Feb 17, 2024

neolit123 left a comment

astundzia commented Feb 17, 2024 •

edited

astundzia commented Feb 17, 2024

astundzia commented Feb 17, 2024

astundzia commented Feb 17, 2024

neolit123 left a comment

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024

neolit123 Feb 18, 2024 •

edited

astundzia commented Mar 1, 2024

neolit123 commented Mar 1, 2024

neolit123 commented May 20, 2024

astundzia commented May 29, 2024

	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))

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

	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)

	// create a config with a CA cert containing multiple certificates
	// Create a config with a CA cert containing multiple certificates

	t.Fatalf("failure while generating intermediate CA certificate and key: %v", err)
	t.Fatalf("Failure while generating intermediate CA certificate and key: %v", err)

	t.Fatalf("failure while saving CA certificate: %v", err)
	t.Fatalf("Failure while saving CA certificate: %v", err)

	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)

kubeadm: Validate only the first cert entry when external ca mode is used #123102

Are you sure you want to change the base?

kubeadm: Validate only the first cert entry when external ca mode is used #123102

Conversation

astundzia commented Feb 2, 2024 • edited by k8s-ci-robot

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

linux-foundation-easycla bot commented Feb 2, 2024 • edited

k8s-ci-robot commented Feb 2, 2024

k8s-ci-robot commented Feb 2, 2024

k8s-ci-robot commented Feb 2, 2024

astundzia commented Feb 2, 2024

astundzia commented Feb 2, 2024

astundzia commented Feb 3, 2024

neolit123 commented Feb 5, 2024

astundzia commented Feb 5, 2024

neolit123 commented Feb 8, 2024

neolit123 commented Feb 16, 2024

astundzia commented Feb 17, 2024

neolit123 left a comment

Choose a reason for hiding this comment

astundzia commented Feb 17, 2024 • edited

astundzia commented Feb 17, 2024

astundzia commented Feb 17, 2024

astundzia commented Feb 17, 2024

neolit123 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neolit123 Feb 18, 2024 • edited

Choose a reason for hiding this comment

astundzia commented Mar 1, 2024

neolit123 commented Mar 1, 2024

neolit123 commented May 20, 2024

astundzia commented May 29, 2024

astundzia commented Feb 2, 2024 •

edited by k8s-ci-robot

linux-foundation-easycla bot commented Feb 2, 2024 •

edited

astundzia commented Feb 17, 2024 •

edited

neolit123 Feb 18, 2024 •

edited