From 2b84890d59c6015edf2ddfd9e09e38a4382f0160 Mon Sep 17 00:00:00 2001 From: Andrey Ermolov Date: Wed, 7 Feb 2024 02:12:52 +0000 Subject: [PATCH] Tests for KeyUsage and crl bit --- security/advancedtls/advancedtls_test.go | 14 ++++++++------ security/advancedtls/crl.go | 15 +++++---------- security/advancedtls/crl_test.go | 12 +++++++++--- 3 files changed, 22 insertions(+), 19 deletions(-) diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go index f81ae4dfa0b1..4cdd8446c225 100644 --- a/security/advancedtls/advancedtls_test.go +++ b/security/advancedtls/advancedtls_test.go @@ -381,14 +381,14 @@ func (s) TestClientServerHandshake(t *testing.T) { return &GetRootCAsResults{TrustCerts: cs.ServerTrust3}, nil } - makeStaticCRLProvider := func(crlPath string) *RevocationConfig { + makeStaticCRLProvider := func(crlPath string, allowUndetermined bool) *RevocationConfig { rawCRL, err := os.ReadFile(crlPath) if err != nil { t.Fatalf("readFile(%v) failed err = %v", crlPath, err) } cRLProvider := NewStaticCRLProvider([][]byte{rawCRL}) return &RevocationConfig{ - AllowUndetermined: true, + AllowUndetermined: allowUndetermined, CRLProvider: cRLProvider, } } @@ -735,7 +735,7 @@ func (s) TestClientServerHandshake(t *testing.T) { clientGetRoot: getRootCAsForClientCRL, clientVerifyFunc: clientVerifyFuncGood, clientVType: CertVerification, - clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem")), + clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem"), true), serverMutualTLS: true, serverCert: []tls.Certificate{cs.ServerCert3}, serverGetRoot: getRootCAsForServerCRL, @@ -750,7 +750,7 @@ func (s) TestClientServerHandshake(t *testing.T) { clientGetRoot: getRootCAsForClientCRL, clientVerifyFunc: clientVerifyFuncGood, clientVType: CertVerification, - clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_server_revoked.pem")), + clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_server_revoked.pem"), true), serverMutualTLS: true, serverCert: []tls.Certificate{cs.ServerCert3}, serverGetRoot: getRootCAsForServerCRL, @@ -759,18 +759,20 @@ func (s) TestClientServerHandshake(t *testing.T) { }, // Client: set valid credentials with the revocation config // Server: set valid credentials with the revocation config - // Expected Behavior: fail, because CRL is issued by some malicious CA + // Expected Behavior: fail, because CRL is issued by the malicious CA. It + // can't be properly processed, and we don't allow RevocationUndetermined. { desc: "Client sets peer cert, reload root function with verifyFuncGood; Server sets peer cert, reload root function; Client uses CRL; mutualTLS", clientCert: []tls.Certificate{cs.ClientCert3}, clientGetRoot: getRootCAsForClientCRL, clientVerifyFunc: clientVerifyFuncGood, clientVType: CertVerification, - clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem")), + clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_malicious_crl_empty.pem"), false), serverMutualTLS: true, serverCert: []tls.Certificate{cs.ServerCert3}, serverGetRoot: getRootCAsForServerCRL, serverVType: CertVerification, + serverExpectError: true, }, } { test := test diff --git a/security/advancedtls/crl.go b/security/advancedtls/crl.go index a3e59147d733..863a6ce485ac 100644 --- a/security/advancedtls/crl.go +++ b/security/advancedtls/crl.go @@ -552,21 +552,16 @@ func verifyCRL(crl *CRL, rawIssuer []byte, chain []*x509.Certificate) error { // "Conforming CRL issuers MUST use the key identifier method, and MUST // include this extension in all CRLs issued." // So, this is much simpler than RFC4158 and should be compatible. - str1 := cryptobyte.String(c.SubjectKeyId) - fmt.Println("SubjectKeyId =", str1) - str2 := cryptobyte.String(crl.authorityKeyID) - fmt.Println("SubjectKeyId =", str2) - str3 := cryptobyte.String(c.RawSubject) - fmt.Println("SubjectKeyId =", str3) - str4 := cryptobyte.String(crl.rawIssuer) - fmt.Println("SubjectKeyId =", str4) - if bytes.Equal(c.SubjectKeyId, crl.authorityKeyID) && bytes.Equal(c.RawSubject, crl.rawIssuer) { + // RFC5280, 6.3.3 (f) Key usage and cRLSign bit. + if c.KeyUsage != 0 && c.KeyUsage&x509.KeyUsageCRLSign == 0 { + return fmt.Errorf("verifyCRL: The trust anchor can't be used for issuing CRLs") + } // RFC5280, 6.3.3 (g) Validate signature. return crl.certList.CheckSignatureFrom(c) } } - return fmt.Errorf("verifyCRL: No certificates mached CRL issuer (%v)", crl.certList.Issuer) + return fmt.Errorf("verifyCRL: No certificates matched CRL issuer (%v)", crl.certList.Issuer) } // pemType is the type of a PEM encoded CRL. diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go index 4e9f60d9964c..e62807ded4f4 100644 --- a/security/advancedtls/crl_test.go +++ b/security/advancedtls/crl_test.go @@ -471,14 +471,14 @@ func TestVerifyCrl(t *testing.T) { crl: loadCRL(t, testdata.Path("crl/3.crl")), certs: makeChain(t, testdata.Path("crl/unrevoked.pem")), cert: makeChain(t, testdata.Path("crl/revokedInt.pem"))[1], - errWant: "No certificates mached", + errWant: "No certificates matched", }, { desc: "Fail no certs", crl: loadCRL(t, testdata.Path("crl/1.crl")), certs: []*x509.Certificate{}, cert: makeChain(t, testdata.Path("crl/unrevoked.pem"))[1], - errWant: "No certificates mached", + errWant: "No certificates matched", }, { desc: "Fail Tampered signature", @@ -501,6 +501,13 @@ func TestVerifyCrl(t *testing.T) { cert: makeChain(t, testdata.Path("crl/provider_client_trust_cert.pem"))[0], errWant: "verification error", }, + { + desc: "Fail KeyUsage without cRLSign bit", + crl: loadCRL(t, testdata.Path("crl/provider_malicious_crl_empty.pem")), + certs: makeChain(t, testdata.Path("crl/provider_malicious_client_trust_cert.pem")), + cert: makeChain(t, testdata.Path("crl/provider_malicious_client_trust_cert.pem"))[0], + errWant: "trust anchor can't be used", + }, } for _, tt := range verifyTests { @@ -662,7 +669,6 @@ func setupTLSConn(t *testing.T) (net.Listener, *x509.Certificate, *ecdsa.Private } // TestVerifyConnection will setup a client/server connection and check revocation in the real TLS dialer -// TODO add CRL provider tests here? func TestVerifyConnection(t *testing.T) { lis, cert, key := setupTLSConn(t) defer func() {