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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
31 changes: 28 additions & 3 deletions cmd/kubeadm/app/phases/kubeconfig/kubeconfig.go
Original file line number Diff line number Diff line change
Expand Up @@ -264,10 +264,35 @@ func validateKubeConfig(outDir, filename string, config *clientcmdapi.Config) er
}
caExpected := bytes.TrimSpace(config.Clusters[expectedCluster].CertificateAuthorityData)

// If the current CA cert on disk doesn't match the expected CA cert, error out because we have a file, but it's stale
if !bytes.Equal(caCurrent, caExpected) {
return errors.Errorf("a kubeconfig file %q exists already but has got the wrong CA cert", kubeConfigFilePath)
// Parse the current certificate authority data
currentCACerts, err := certutil.ParseCertsPEM(caCurrent)
if err != nil {
return errors.Errorf("the kubeconfig file %q contains an invalid CA cert", kubeConfigFilePath)
}

// 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)
Copy link
Member

@neolit123 neolit123 Feb 18, 2024

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:

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.

}

// Only use the first certificate in the expected CA cert list
expectedCACert := expectedCACerts[0]

// find a common trust anchor
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// find a common trust anchor
// Find a common trust anchor

trustAnchorFound := false
for _, currentCACert := range currentCACerts {
// Compare the current CA cert to the expected CA cert.
// If the certificates match then a common trust anchor was found.
if currentCACert.Equal(expectedCACert) {
trustAnchorFound = true
break
}
}
if !trustAnchorFound {
return errors.Errorf("a kubeconfig file %q exists already but could not find trust anchor", kubeConfigFilePath)
Copy link
Member

Choose a reason for hiding this comment

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

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.

}

// If the current API Server location on disk doesn't match the expected API server, show a warning
if currentConfig.Clusters[currentCluster].Server != config.Clusters[expectedCluster].Server {
klog.Warningf("a kubeconfig file %q exists already but has an unexpected API Server URL: expected: %s, got: %s",
Expand Down
147 changes: 140 additions & 7 deletions cmd/kubeadm/app/phases/kubeconfig/kubeconfig_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -588,7 +588,11 @@ func TestValidateKubeConfig(t *testing.T) {

func TestValidateKubeconfigsForExternalCA(t *testing.T) {
tmpDir := testutil.SetupTempDir(t)
defer os.RemoveAll(tmpDir)
defer func() {
if err := os.RemoveAll(tmpDir); err != nil {
t.Error(err)
}
}()
pkiDir := filepath.Join(tmpDir, "pki")

initConfig := &kubeadmapi.InitConfiguration{
Expand All @@ -603,11 +607,9 @@ func TestValidateKubeconfigsForExternalCA(t *testing.T) {

// creates CA, write to pkiDir and remove ca.key to get into external CA condition
caCert, caKey := certstestutil.SetupCertificateAuthority(t)
if err := pkiutil.WriteCertAndKey(pkiDir, kubeadmconstants.CACertAndKeyBaseName, caCert, caKey); err != nil {
t.Fatalf("failure while saving CA certificate and key: %v", err)
}
if err := os.Remove(filepath.Join(pkiDir, kubeadmconstants.CAKeyName)); err != nil {
t.Fatalf("failure while deleting ca.key: %v", err)

if err := pkiutil.WriteCertBundle(pkiDir, kubeadmconstants.CACertAndKeyBaseName, []*x509.Certificate{caCert}); err != nil {
t.Fatalf("failure while saving CA certificate: %v", err)
}

// create a valid config
Expand Down Expand Up @@ -675,7 +677,138 @@ func TestValidateKubeconfigsForExternalCA(t *testing.T) {
for name, test := range tests {
t.Run(name, func(t *testing.T) {
tmpdir := testutil.SetupTempDir(t)
defer os.RemoveAll(tmpdir)
defer func() {
if err := os.RemoveAll(tmpdir); err != nil {
t.Error(err)
}
}()

for name, config := range test.filesToWrite {
if err := createKubeConfigFileIfNotExists(tmpdir, name, config); err != nil {
t.Errorf("createKubeConfigFileIfNotExists failed: %v", err)
}
}

err := ValidateKubeconfigsForExternalCA(tmpdir, test.initConfig)
if (err != nil) != test.expectedError {
t.Fatalf(dedent.Dedent(
"ValidateKubeconfigsForExternalCA failed\n%s\nexpected error: %t\n\tgot: %t\nerror: %v"),
name,
test.expectedError,
(err != nil),
err,
)
}
})
}
}

func TestValidateKubeconfigsForExternalCAMissingRoot(t *testing.T) {
tmpDir := testutil.SetupTempDir(t)
defer func() {
if err := os.RemoveAll(tmpDir); err != nil {
t.Error(err)
}
}()
pkiDir := filepath.Join(tmpDir, "pki")

initConfig := &kubeadmapi.InitConfiguration{
ClusterConfiguration: kubeadmapi.ClusterConfiguration{
CertificatesDir: pkiDir,
},
LocalAPIEndpoint: kubeadmapi.APIEndpoint{
BindPort: 1234,
AdvertiseAddress: "1.2.3.4",
},
}

// creates CA, write to pkiDir and remove ca.key to get into external CA condition
Copy link
Member

Choose a reason for hiding this comment

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

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

caCert, caKey := certstestutil.SetupCertificateAuthority(t)

// create a config with a CA cert containing multiple certificates
Copy link
Member

Choose a reason for hiding this comment

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

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

intermediateCACert1, intermediateCAKey1 := certstestutil.SetupIntermediateCertificateAuthority(t, caCert, caKey)
intermediateCACert2, intermediateCAKey2 := certstestutil.SetupIntermediateCertificateAuthority(t, intermediateCACert1, intermediateCAKey1)

var caCertBundle []*x509.Certificate
caCertBundle = append(caCertBundle, caCert, intermediateCACert1, intermediateCACert2)
caCertBytes, _ := pkiutil.EncodeCertBundlePEM(caCertBundle)

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)
Copy link
Member

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?

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)

multipleCAConfigRootCAIssuer.Clusters[clusterName].CertificateAuthorityData = caCertBytes

// Create a kube config and assign the CA data to the CA bundle, issued from intermediate CA 1
multipleCAConfigIntermediateCA1Issuer := setupdKubeConfigWithClientAuth(t, intermediateCACert1, intermediateCAKey1, "https://1.2.3.4:1234", "test-cluster", clusterName)
multipleCAConfigIntermediateCA1Issuer.Clusters[clusterName].CertificateAuthorityData = caCertBytes

// Create a kube config and assign the CA data to the CA bundle, issued from intermediate CA 2
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
Copy link
Member

Choose a reason for hiding this comment

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

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)

var caCertBundleNoRootCA []*x509.Certificate
caCertBundleNoRootCA = append(caCertBundleNoRootCA, intermediateCACert1, intermediateCACert2)
caCertBytesNoRootCA, _ := pkiutil.EncodeCertBundlePEM(caCertBundleNoRootCA)

// Create a kube config and assign the CA data to the CA bundle, issued from intermediate CA 2,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Create a kube config and assign the CA data to the CA bundle, issued from intermediate CA 2,
// Create a kubeconfig and assign the CA data to the CA bundle, issued from intermediate CA 2,

// CA is missing root CA certificate.
multipleCAConfigNoRootCA := setupdKubeConfigWithClientAuth(t, intermediateCACert2, intermediateCAKey2, "https://1.2.3.4:1234", "test-cluster", clusterName)
multipleCAConfigNoRootCA.Clusters[clusterName].CertificateAuthorityData = caCertBytesNoRootCA

if err := pkiutil.WriteCertBundle(pkiDir, kubeadmconstants.CACertAndKeyBaseName, caCertBundleNoRootCA); err != nil {
t.Fatalf("failure while saving CA certificate: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

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

}

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

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"?

}{
"all files are valid for CA with multiple certificates issued from RootCA": {
filesToWrite: map[string]*clientcmdapi.Config{
kubeadmconstants.AdminKubeConfigFileName: multipleCAConfigNoRootCA,
kubeadmconstants.SuperAdminKubeConfigFileName: multipleCAConfigNoRootCA,
kubeadmconstants.KubeletKubeConfigFileName: multipleCAConfigNoRootCA,
kubeadmconstants.ControllerManagerKubeConfigFileName: multipleCAConfigNoRootCA,
kubeadmconstants.SchedulerKubeConfigFileName: multipleCAConfigNoRootCA,
},
initConfig: initConfig,
expectedError: false,
},
"all files are valid for CA with multiple certificates issued from IntermediateCA1": {
filesToWrite: map[string]*clientcmdapi.Config{
kubeadmconstants.AdminKubeConfigFileName: multipleCAConfigIntermediateCA1Issuer,
kubeadmconstants.SuperAdminKubeConfigFileName: multipleCAConfigIntermediateCA1Issuer,
kubeadmconstants.KubeletKubeConfigFileName: multipleCAConfigIntermediateCA1Issuer,
kubeadmconstants.ControllerManagerKubeConfigFileName: multipleCAConfigIntermediateCA1Issuer,
kubeadmconstants.SchedulerKubeConfigFileName: multipleCAConfigIntermediateCA1Issuer,
},
initConfig: initConfig,
expectedError: false,
},
"all files are valid for CA with multiple certificates issued from IntermediateCA2": {
filesToWrite: map[string]*clientcmdapi.Config{
kubeadmconstants.AdminKubeConfigFileName: multipleCAConfigIntermediateCA2Issuer,
kubeadmconstants.SuperAdminKubeConfigFileName: multipleCAConfigIntermediateCA2Issuer,
kubeadmconstants.KubeletKubeConfigFileName: multipleCAConfigIntermediateCA2Issuer,
kubeadmconstants.ControllerManagerKubeConfigFileName: multipleCAConfigIntermediateCA2Issuer,
kubeadmconstants.SchedulerKubeConfigFileName: multipleCAConfigIntermediateCA2Issuer,
},
initConfig: initConfig,
expectedError: false,
},
}

for name, test := range tests {
t.Run(name, func(t *testing.T) {
tmpdir := testutil.SetupTempDir(t)
defer func() {
if err := os.RemoveAll(tmpdir); err != nil {
t.Error(err)
}
}()

for name, config := range test.filesToWrite {
if err := createKubeConfigFileIfNotExists(tmpdir, name, config); err != nil {
Expand Down
13 changes: 13 additions & 0 deletions cmd/kubeadm/app/util/certs/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,19 @@ func SetupCertificateAuthority(t *testing.T) (*x509.Certificate, crypto.Signer)
return caCert, caKey
}

// SetupIntermediateCertificateAuthority is a utility function for kubeadm testing that creates a
// Intermediate CertificateAuthority cert/key pair
func SetupIntermediateCertificateAuthority(t *testing.T, parentCert *x509.Certificate, parentKey crypto.Signer) (*x509.Certificate, crypto.Signer) {
caCert, caKey, err := pkiutil.NewIntermediateCertificateAuthority(parentCert, parentKey, &pkiutil.CertConfig{
Config: certutil.Config{CommonName: "kubernetes intermediate CA"},
})
if err != nil {
t.Fatalf("failure while generating intermediate CA certificate and key: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

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

}

return caCert, caKey
}

// AssertCertificateIsSignedByCa is a utility function for kubeadm testing that asserts if a given certificate is signed
// by the expected CA
func AssertCertificateIsSignedByCa(t *testing.T, cert *x509.Certificate, signingCa *x509.Certificate) {
Expand Down