Address PR comments

Author: erm-g

security/advancedtls/advancedtls_test.go

@@ -381,7 +381,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		return &GetRootCAsResults{TrustCerts: cs.ServerTrust3}, nil
 	}
 
-	makeStaticCRLProvider := func(crlPath string, allowUndetermined bool) *RevocationConfig {
+	makeStaticCRLRevocationConfig := func(crlPath string, allowUndetermined bool) *RevocationConfig {
 		rawCRL, err := os.ReadFile(crlPath)
 		if err != nil {
 			t.Fatalf("readFile(%v) failed err = %v", crlPath, err)
@@ -731,13 +731,13 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		// Expected Behavior: success, because none of the certificate chains sent in the connection are revoked
 		{
 			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},
+			clientCert:             []tls.Certificate{cs.ClientCertForCRL},
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_empty.pem"), true),
+			clientRevocationConfig: makeStaticCRLRevocationConfig(testdata.Path("crl/provider_crl_empty.pem"), true),
 			serverMutualTLS:        true,
-			serverCert:             []tls.Certificate{cs.ServerCert3},
+			serverCert:             []tls.Certificate{cs.ServerCertForCRL},
 			serverGetRoot:          getRootCAsForServerCRL,
 			serverVType:            CertVerification,
 		},
@@ -746,13 +746,13 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		// Expected Behavior: fail, server creds are revoked
 		{
 			desc:                   "Client sets peer cert, reload root function with verifyFuncGood; Server sets revoked cert; Client uses CRL; mutualTLS",
-			clientCert:             []tls.Certificate{cs.ClientCert3},
+			clientCert:             []tls.Certificate{cs.ClientCertForCRL},
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_crl_server_revoked.pem"), true),
+			clientRevocationConfig: makeStaticCRLRevocationConfig(testdata.Path("crl/provider_crl_server_revoked.pem"), true),
 			serverMutualTLS:        true,
-			serverCert:             []tls.Certificate{cs.ServerCert3},
+			serverCert:             []tls.Certificate{cs.ServerCertForCRL},
 			serverGetRoot:          getRootCAsForServerCRL,
 			serverVType:            CertVerification,
 			serverExpectError:      true,
@@ -763,13 +763,13 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		// 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},
+			clientCert:             []tls.Certificate{cs.ClientCertForCRL},
 			clientGetRoot:          getRootCAsForClientCRL,
 			clientVerifyFunc:       clientVerifyFuncGood,
 			clientVType:            CertVerification,
-			clientRevocationConfig: makeStaticCRLProvider(testdata.Path("crl/provider_malicious_crl_empty.pem"), false),
+			clientRevocationConfig: makeStaticCRLRevocationConfig(testdata.Path("crl/provider_malicious_crl_empty.pem"), false),
 			serverMutualTLS:        true,
-			serverCert:             []tls.Certificate{cs.ServerCert3},
+			serverCert:             []tls.Certificate{cs.ServerCertForCRL},
 			serverGetRoot:          getRootCAsForServerCRL,
 			serverVType:            CertVerification,
 			serverExpectError:      true,

security/advancedtls/crl.go

@@ -313,8 +313,9 @@ func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revocat
 
 // checkCert checks a single certificate against the CRL defined in the
 // certificate. It will fetch and verify the CRL(s) defined in the root
-// directory (or a CRLProvider) specified by cfg. If we can't load any valid
-// authoritative CRL files, the status is RevocationUndetermined.
+// directory (or a CRLProvider) specified by cfg. If we can't load (and verify -
+// see verifyCRL) any valid authoritative CRL files, the status is
+// RevocationUndetermined.
 // c is the certificate to check.
 // crlVerifyCrt is the group of possible certificates to verify the crl.
 func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg RevocationConfig) RevocationStatus {
@@ -323,8 +324,8 @@ func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revoca
 		// We couldn't load any valid CRL files for the certificate, so we don't
 		// know if it's RevocationUnrevoked or not. This is not necessarily a
 		// problem - it's not invalid to have no CRLs if you don't have any
-		// revocations for an issuer. It also might be an indication of CRL file to
-		// be invalid.
+		// revocations for an issuer. It also might be an indication that the CRL
+		// file to is invalid.
 		// We just return RevocationUndetermined and there is a setting for the user
 		// to control the handling of that.
 		grpclogLogger.Warningf("fetchCRL() err = %v", err)
@@ -555,7 +556,7 @@ func verifyCRL(crl *CRL, rawIssuer []byte, chain []*x509.Certificate) error {
 		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")
+				return fmt.Errorf("verifyCRL: The certificate can't be used for issuing CRLs")
 			}
 			// RFC5280, 6.3.3 (g) Validate signature.
 			return crl.certList.CheckSignatureFrom(c)

security/advancedtls/crl_test.go

@@ -509,7 +509,7 @@ func TestVerifyCrl(t *testing.T) {
 			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",
+			errWant: "certificate can't be used",
 		},
 	}
 

security/advancedtls/internal/testutils/testutils.go

@@ -35,18 +35,18 @@ type CertStore struct {
 	// ClientCert2 is the certificate sent by client to prove its identity.
 	// It is trusted by ServerTrust2.
 	ClientCert2 tls.Certificate
-	// ClientCert3 is the certificate sent by client to prove its identity.
+	// ClientCertForCRL is the certificate sent by client to prove its identity.
 	// It is trusted by ServerTrust3. Used in CRL tests
-	ClientCert3 tls.Certificate
+	ClientCertForCRL tls.Certificate
 	// ServerCert1 is the certificate sent by server to prove its identity.
 	// It is trusted by ClientTrust1.
 	ServerCert1 tls.Certificate
 	// ServerCert2 is the certificate sent by server to prove its identity.
 	// It is trusted by ClientTrust2.
 	ServerCert2 tls.Certificate
-	// ServerCert3 is a revoked certificate
-	// (this info is stored in crl_server_revoked.pem).
-	ServerCert3 tls.Certificate
+	// ServerCertForCRL is a revoked certificate
+	// (this info is stored in provider_crl_server_revoked.pem).
+	ServerCertForCRL tls.Certificate
 	// ServerPeer3 is the certificate sent by server to prove its identity.
 	ServerPeer3 tls.Certificate
 	// ServerPeerLocalhost1 is the certificate sent by server to prove its
@@ -89,7 +89,7 @@ func (cs *CertStore) LoadCerts() error {
 	if cs.ClientCert2, err = tls.LoadX509KeyPair(testdata.Path("client_cert_2.pem"), testdata.Path("client_key_2.pem")); err != nil {
 		return err
 	}
-	if cs.ClientCert3, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_client_cert.pem"), testdata.Path("crl/provider_client_cert.key")); err != nil {
+	if cs.ClientCertForCRL, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_client_cert.pem"), testdata.Path("crl/provider_client_cert.key")); err != nil {
 		return err
 	}
 	if cs.ServerCert1, err = tls.LoadX509KeyPair(testdata.Path("server_cert_1.pem"), testdata.Path("server_key_1.pem")); err != nil {
@@ -98,7 +98,7 @@ func (cs *CertStore) LoadCerts() error {
 	if cs.ServerCert2, err = tls.LoadX509KeyPair(testdata.Path("server_cert_2.pem"), testdata.Path("server_key_2.pem")); err != nil {
 		return err
 	}
-	if cs.ServerCert3, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_server_cert.pem"), testdata.Path("crl/provider_server_cert.key")); err != nil {
+	if cs.ServerCertForCRL, err = tls.LoadX509KeyPair(testdata.Path("crl/provider_server_cert.pem"), testdata.Path("crl/provider_server_cert.key")); err != nil {
 		return err
 	}
 	if cs.ServerPeer3, err = tls.LoadX509KeyPair(testdata.Path("server_cert_3.pem"), testdata.Path("server_key_3.pem")); err != nil {

security/advancedtls/testdata/crl/README.md

@@ -121,7 +121,9 @@ $ openssl ca -revoke provider_server_cert.pem -keyfile provider_client_trust_key
 $ openssl ca -gencrl -keyfile provider_client_trust_key.pem -cert provider_client_trust_cert.pem -out provider_crl_server_revoked.pem -config provider_crl.cnf
 ```
 
-The commands to generate CRL file by 'malicious' CA are below.
+The commands to generate CRL file by 'malicious' CA are below. Note that we use
+Subject Key Identifier from previously generated provider_client_trust_cert.pem
+to generate malicious certs / CRL.
 ```
 $ openssl genrsa -out provider_malicious_client_trust_key.pem 4096
 $ SKI=$(openssl x509 -in provider_client_trust_cert.pem -noout -text | awk '/Subject Key Identifier/ {getline; print $1;}') 

security/advancedtls/testdata/crl/provider_create.sh

@@ -103,4 +103,7 @@ openssl ca -gencrl                                 \
   -out provider_malicious_crl_empty.pem            \
   -config provider_crl.cnf
 
+sed -i "s/subjectKeyIdentifier = .*/subjectKeyIdentifier = hash/g" \
+  provider_extensions.conf
+
 rm *.csr

security/advancedtls/testdata/crl/provider_extensions.conf

@@ -1,5 +1,5 @@
 [extensions]
-subjectKeyIdentifier = 54:1B:29:00:6E:3C:B1:54:68:75:C2:F8:26:90:BB:46:D5:70:45:E9
+subjectKeyIdentifier = hash
 authorityKeyIdentifier = keyid,issuer
 basicConstraints = CA:FALSE
 keyUsage = digitalSignature, keyEncipherment