From da46aaa92737474d0e6d291022a8170f84d863e0 Mon Sep 17 00:00:00 2001 From: Tarun Pothulapati Date: Thu, 19 Nov 2020 05:00:39 +0530 Subject: [PATCH] cli: Don't check for SAN in root and intermediate certs (#5237) As discussed in #5228, it is not correct for root and intermediate certs to have SAN. This PR updates the check to not verify the intermediate issuer cert with the identity dns name (which checks with SAN and not CN as the the `verify` func is used to verify leaf certs and not root and intermediate certs). This PR also avoids setting a SAN field when generating certs in the `install` command. Fixes #5228 --- cli/cmd/install_test.go | 1 - cli/cmd/options.go | 4 ++-- pkg/healthcheck/healthcheck.go | 6 +----- pkg/healthcheck/healthcheck_test.go | 9 --------- pkg/identity/service.go | 6 ++++-- pkg/issuercerts/issuercerts.go | 4 ++-- pkg/tls/ca.go | 2 -- pkg/tls/cred_test.go | 4 ++-- 8 files changed, 11 insertions(+), 25 deletions(-) diff --git a/cli/cmd/install_test.go b/cli/cmd/install_test.go index 3ce799ea22945..5b3f5043c62e3 100644 --- a/cli/cmd/install_test.go +++ b/cli/cmd/install_test.go @@ -462,7 +462,6 @@ func TestValidate(t *testing.T) { {"valid", ""}, {"expired", "failed to validate issuer credentials: not valid anymore. Expired on 1990-01-01T01:01:11Z"}, {"not-valid-yet", "failed to validate issuer credentials: not valid before: 2100-01-01T01:00:51Z"}, - {"wrong-domain", "failed to validate issuer credentials: x509: certificate is valid for wrong.linkerd.cluster.local, not identity.linkerd.cluster.local"}, {"wrong-algo", "failed to validate issuer credentials: must use P-256 curve for public key, instead P-521 was used"}, } for _, tc := range testCases { diff --git a/cli/cmd/options.go b/cli/cmd/options.go index 57dd94d2675a1..393a088a4f79d 100644 --- a/cli/cmd/options.go +++ b/cli/cmd/options.go @@ -571,7 +571,7 @@ func validateValues(ctx context.Context, k *k8s.KubernetesAPI, values *l5dcharts if err != nil { return err } - _, err = externalIssuerData.VerifyAndBuildCreds(issuerName(values.Global.IdentityTrustDomain)) + _, err = externalIssuerData.VerifyAndBuildCreds() if err != nil { return fmt.Errorf("failed to validate issuer credentials: %s", err) } @@ -583,7 +583,7 @@ func validateValues(ctx context.Context, k *k8s.KubernetesAPI, values *l5dcharts IssuerKey: values.Identity.Issuer.TLS.KeyPEM, TrustAnchors: values.Global.IdentityTrustAnchorsPEM, } - _, err := issuerData.VerifyAndBuildCreds(issuerName(values.Global.IdentityTrustDomain)) + _, err := issuerData.VerifyAndBuildCreds() if err != nil { return fmt.Errorf("failed to validate issuer credentials: %s", err) } diff --git a/pkg/healthcheck/healthcheck.go b/pkg/healthcheck/healthcheck.go index 5267d38b928f9..19fc02172172c 100644 --- a/pkg/healthcheck/healthcheck.go +++ b/pkg/healthcheck/healthcheck.go @@ -1022,7 +1022,7 @@ func (hc *HealthChecker) allCategories() []category { description: "issuer cert is issued by the trust anchor", hintAnchor: "l5d-identity-issuer-cert-issued-by-trust-anchor", check: func(ctx context.Context) error { - return hc.issuerCert.Verify(tls.CertificatesToPool(hc.trustAnchors), hc.issuerIdentity(), time.Time{}) + return hc.issuerCert.Verify(tls.CertificatesToPool(hc.trustAnchors), "", time.Time{}) }, }, }, @@ -1446,10 +1446,6 @@ func (hc *HealthChecker) checkMinReplicasAvailable(ctx context.Context) error { return nil } -func (hc *HealthChecker) issuerIdentity() string { - return fmt.Sprintf("identity.%s.%s", hc.ControlPlaneNamespace, hc.linkerdConfig.Global.IdentityTrustDomain) -} - // Add adds an arbitrary checker. This should only be used for testing. For // production code, pass in the desired set of checks when calling // NewHealthChecker. diff --git a/pkg/healthcheck/healthcheck_test.go b/pkg/healthcheck/healthcheck_test.go index e4d103ea71730..0518a5974baf9 100644 --- a/pkg/healthcheck/healthcheck_test.go +++ b/pkg/healthcheck/healthcheck_test.go @@ -3077,15 +3077,6 @@ func TestLinkerdIdentityCheckCertValidity(t *testing.T) { } } -func TestLinkerdIdentityCheckWrongDns(t *testing.T) { - expectedOutput := []string{"linkerd-identity-test-cat issuer cert is issued by the trust anchor: x509: certificate is valid for wrong.linkerd.cluster.local, not identity.linkerd.cluster.local"} - issuerData := createIssuerData("wrong.linkerd.cluster.local", time.Now().AddDate(-1, 0, 0), time.Now().AddDate(1, 0, 0)) - fakeConfigMap := getFakeConfigMap(k8s.IdentityIssuerSchemeLinkerd, issuerData) - fakeSecret := getFakeSecret(k8s.IdentityIssuerSchemeLinkerd, issuerData) - runIdentityCheckTestCase(context.Background(), t, 0, "fails when cert dns is wrong", "issuer cert is issued by the trust anchor", fakeConfigMap, fakeSecret, expectedOutput) - -} - type fakeCniResourcesOpts struct { hasConfigMap bool hasPodSecurityPolicy bool diff --git a/pkg/identity/service.go b/pkg/identity/service.go index 208eefefac3c5..5fb4bf44280e9 100644 --- a/pkg/identity/service.go +++ b/pkg/identity/service.go @@ -114,7 +114,8 @@ func (svc *Service) loadCredentials() (tls.Issuer, error) { return nil, fmt.Errorf("failed to read CA from disk: %s", err) } - if err := creds.Crt.Verify(svc.trustAnchors, svc.expectedName, time.Time{}); err != nil { + // Don't verify with dns name as this is not a leaf certificate + if err := creds.Crt.Verify(svc.trustAnchors, "", time.Time{}); err != nil { return nil, fmt.Errorf("failed to verify issuer credentials for '%s' with trust anchors: %s", svc.expectedName, err) } @@ -149,7 +150,8 @@ func (svc *Service) ensureIssuerStillValid() error { issuer := *svc.issuer switch is := issuer.(type) { case *tls.CA: - return is.Cred.Verify(svc.trustAnchors, svc.expectedName, time.Time{}) + // Don't verify with dns name as this is not a leaf certificate + return is.Cred.Verify(svc.trustAnchors, "", time.Time{}) default: return fmt.Errorf("unsupported issuer type. Expected *tls.CA, got %v", is) } diff --git a/pkg/issuercerts/issuercerts.go b/pkg/issuercerts/issuercerts.go index fdab74d71d1ae..216d0040dd209 100644 --- a/pkg/issuercerts/issuercerts.go +++ b/pkg/issuercerts/issuercerts.go @@ -155,7 +155,7 @@ func CheckCertAlgoRequirements(cert *x509.Certificate) error { } // VerifyAndBuildCreds builds and validates the creds out of the data in IssuerCertData -func (ic *IssuerCertData) VerifyAndBuildCreds(dnsName string) (*tls.Cred, error) { +func (ic *IssuerCertData) VerifyAndBuildCreds() (*tls.Cred, error) { creds, err := tls.ValidateAndCreateCreds(ic.IssuerCrt, ic.IssuerKey) if err != nil { return nil, fmt.Errorf("failed to read CA: %s", err) @@ -180,7 +180,7 @@ func (ic *IssuerCertData) VerifyAndBuildCreds(dnsName string) (*tls.Cred, error) return nil, err } - if err := creds.Verify(anchors, dnsName, time.Time{}); err != nil { + if err := creds.Verify(anchors, "", time.Time{}); err != nil { return nil, err } diff --git a/pkg/tls/ca.go b/pkg/tls/ca.go index 806075e574b7a..0ae8fc414185c 100644 --- a/pkg/tls/ca.go +++ b/pkg/tls/ca.go @@ -118,7 +118,6 @@ func CreateRootCA( // Configure the root certificate. t := createTemplate(1, &key.PublicKey, validity) t.Subject = pkix.Name{CommonName: name} - t.DNSNames = []string{name} t.IsCA = true t.MaxPathLen = -1 t.BasicConstraintsValid = true @@ -167,7 +166,6 @@ func (ca *CA) GenerateCA(name string, maxPathLen int) (*CA, error) { t := ca.createTemplate(&key.PublicKey) t.Subject = pkix.Name{CommonName: name} - t.DNSNames = []string{name} t.IsCA = true t.MaxPathLen = maxPathLen t.MaxPathLenZero = true // 0-values are actually 0 diff --git a/pkg/tls/cred_test.go b/pkg/tls/cred_test.go index a076ffa0bbade..a89876610cef4 100644 --- a/pkg/tls/cred_test.go +++ b/pkg/tls/cred_test.go @@ -28,7 +28,7 @@ func TestCrtRoundtrip(t *testing.T) { t.Fatalf("Failed to decode PEM Crt: %s", err) } - if err := crt.Verify(rootTrust, "endentity.test", time.Time{}); err != nil { + if err := crt.Verify(rootTrust, "", time.Time{}); err != nil { t.Fatal("Failed to verify round-tripped certificate") } } @@ -103,7 +103,7 @@ func TestCrtExpiry(t *testing.T) { crt.Certificate.NotBefore = tc.notBefore crt.Certificate.NotAfter = tc.notAfter - err := crt.Verify(rootTrust, "expired.test", tc.currentTime) + err := crt.Verify(rootTrust, "", tc.currentTime) if tc.valid && err != nil { t.Fatalf("expected certificate to be valid but was invalid: %s", err.Error()) }