Address PR comments

Author: erm-g

security/advancedtls/crl.go

@@ -285,7 +285,7 @@ func fetchIssuerCRL(rawIssuer []byte, crlVerifyCrt []*x509.Certificate, cfg Revo
 		return nil, fmt.Errorf("fetchCRL() failed: %v", err)
 	}
 
-	if err := verifyCRL(crl, rawIssuer, crlVerifyCrt); err != nil {
+	if err := verifyCRL(crl, crlVerifyCrt); err != nil {
 		return nil, fmt.Errorf("verifyCRL() failed: %v", err)
 	}
 	if cfg.Cache != nil {
@@ -303,7 +303,7 @@ func fetchCRL(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revocat
 		if crl == nil {
 			return nil, fmt.Errorf("no CRL found for certificate's issuer")
 		}
-		if err := verifyCRL(crl, nil, crlVerifyCrt); err != nil {
+		if err := verifyCRL(crl, crlVerifyCrt); err != nil {
 			return nil, fmt.Errorf("verifyCRL() failed: %v", err)
 		}
 		return crl, nil
@@ -325,7 +325,7 @@ func checkCert(c *x509.Certificate, crlVerifyCrt []*x509.Certificate, cfg Revoca
 		// 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 that the CRL
-		// file to is invalid.
+		// file 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)
@@ -541,7 +541,7 @@ func fetchCRLOpenSSLHashDir(rawIssuer []byte, cfg RevocationConfig) (*CRL, error
 	return parsedCRL, nil
 }
 
-func verifyCRL(crl *CRL, rawIssuer []byte, chain []*x509.Certificate) error {
+func verifyCRL(crl *CRL, chain []*x509.Certificate) error {
 	// RFC5280, 6.3.3 (f) Obtain and validate the certification path for the issuer of the complete CRL
 	// We intentionally limit our CRLs to be signed with the same certificate path as the certificate
 	// so we can use the chain from the connection.

security/advancedtls/crl_test.go

@@ -515,7 +515,7 @@ func TestVerifyCrl(t *testing.T) {
 
 	for _, tt := range verifyTests {
 		t.Run(tt.desc, func(t *testing.T) {
-			err := verifyCRL(tt.crl, tt.cert.RawIssuer, tt.certs)
+			err := verifyCRL(tt.crl, tt.certs)
 			switch {
 			case tt.errWant == "" && err != nil:
 				t.Errorf("Valid CRL did not verify err = %v", err)

security/advancedtls/testdata/crl/README.md

@@ -49,8 +49,8 @@ Certificate chain where the leaf is revoked
 
 ## Test Data for testing CRL providers functionality
 
-To generate test data please follow the steps below or run provider_create.sh 
-script. All the files have `provider_` prefix.
+To generate test data please run provider_create.sh script. All the files have 
+`provider_` prefix.
 
 We need to generate the following artifacts for testing CRL provider:
 * server self signed CA cert
@@ -62,73 +62,31 @@ We need to generate the following artifacts for testing CRL provider:
 * crl file by 'malicious' CA which contains the same issuer with original CA 
 
 
-Please find the related commands below.
+All the commands are provided in provider_create.sh script. Please find the 
+description below.
 
-* Generate self signed CAs
-```
-$ openssl req -x509 -newkey rsa:4096 -keyout provider_server_trust_key.pem -out provider_server_trust_cert.pem -days 365 -subj "/C=US/ST=VA/O=Internet Widgits Pty Ltd/CN=foo.bar.hoo.ca.com" -nodes
-$ openssl req -x509 -newkey rsa:4096 -keyout provider_client_trust_key.pem -out provider_client_trust_cert.pem -days 365 -subj "/C=US/ST=CA/L=SVL/O=Internet Widgits Pty Ltd" -nodes
-```
-
-* Generate client and server certs signed by CAs
-```
-$ openssl req -newkey rsa:4096 -keyout provider_server_cert.key -out provider_new_cert.csr -nodes -subj "/C=US/ST=CA/L=DUMMYCITY/O=Internet Widgits Pty Ltd/CN=foo.bar.com" -sha256
-$ openssl x509 -req -in provider_new_cert.csr -out provider_server_cert.pem -CA provider_client_trust_cert.pem -CAkey provider_client_trust_key.pem -CAcreateserial -days 3650 -sha256 -extfile provider_extensions.conf
+1. The first two commands generate self signed CAs for client and server:
+   - provider_server_trust_key.pem 
+   - provider_server_trust_cert.pem 
+   - provider_client_trust_key.pem 
+   - provider_client_trust_cert.pem 
 
-$ openssl req -newkey rsa:4096 -keyout provider_client_cert.key -out provider_new_cert.csr -nodes -subj "/C=US/ST=CA/O=Internet Widgits Pty Ltd/CN=foo.bar.hoo.com" -sha256
-$ openssl x509 -req -in provider_new_cert.csr -out provider_client_cert.pem -CA provider_server_trust_cert.pem -CAkey provider_server_trust_key.pem -CAcreateserial -days 3650 -sha256 -extfile provider_extensions.conf
-```
-
-Here is the content of `provider_extensions.conf` -
-```
-[extensions]
-subjectKeyIdentifier = hash
-authorityKeyIdentifier = keyid,issuer
-basicConstraints = CA:FALSE
-keyUsage = digitalSignature, keyEncipherment
-```
+2. Generate client and server certs signed by the CAs above:
+   - provider_server_cert.pem 
+   - provider_client_cert.pem
 
-* Generate CRLs
-  For CRL generation we need 2 more files called `index.txt` and `crlnumber.txt`:
-```
-$ echo "1000" > provider_crlnumber.txt
-$ touch provider_index.txt
-```
-Also we need another config `provider_crl.cnf` -
-```
-[ ca ]
-default_ca = my_ca
-
-[ my_ca ]
-crl = crl.pem
-default_md = sha256
-database = provider_index.txt
-crlnumber = provider_crlnumber.txt
-default_crl_days = 30
-default_crl_hours = 1
-crl_extensions = crl_ext
-
-[crl_ext]
-# Authority Key Identifier extension
-authorityKeyIdentifier=keyid:always,issuer:always
-```
+3. The next 2 commands create 2 files needed for CRL issuing:
+   - provider_crlnumber.txt
+   - provider_index.txt
 
-The commands to generate empty CRL file and CRL file containing revoked server
-cert are below.
-```
-$ openssl ca -gencrl -keyfile provider_client_trust_key.pem -cert provider_client_trust_cert.pem -out provider_crl_empty.pem -config provider_crl.cnf
-$ openssl ca -revoke provider_server_cert.pem -keyfile provider_client_trust_key.pem -cert provider_client_trust_cert.pem -config provider_crl.cnf
-$ 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
-```
+4. The next 3 commands generate an empty CRL file and a CRL file containing 
+revoked server cert:
+   - provider_crl_empty.pem 
+   - provider_crl_server_revoked.pem 
 
-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;}') 
-$ sed -i "s/subjectKeyIdentifier = X/subjectKeyIdentifier = $SKI/g" provider_extensions.conf
-$ openssl req -new -key provider_malicious_client_trust_key.pem -out cert_malicious_request.csr -subj "/C=US/ST=CA/L=SVL/O=Internet Widgits Pty Ltd" -config provider_extensions.conf
-$ openssl x509 -req -in cert_malicious_request.csr -signkey provider_malicious_client_trust_key.pem -out provider_malicious_client_trust_cert.pem -days 365 -extfile provider_extensions.conf -extensions extensions
-$ openssl ca -gencrl -keyfile provider_malicious_client_trust_key.pem -cert provider_malicious_client_trust_cert.pem -out provider_malicious_crl_empty.pem -config provider_crl.cnf
-```
+5. The final section contains commands to generate CRL file by 'malicious' CA. 
+Note that we use Subject Key Identifier from previously created 
+provider_client_trust_cert.pem to generate malicious certs / CRL.
+   - provider_malicious_client_trust_key.pem
+   - provider_malicious_client_trust_cert.pem 
+   - provider_malicious_crl_empty.pem

security/advancedtls/testdata/crl/provider_create.sh

@@ -1,6 +1,7 @@
 #!/bin/bash
 
 # The script contains a sequence of commands described in README.md
+# Generate client/server self signed CAs and certs.
 openssl req -x509                                                      \
   -newkey rsa:4096                                                     \
   -keyout provider_server_trust_key.pem                                \
@@ -51,10 +52,14 @@ openssl x509 -req                      \
   -sha256                              \
   -extfile provider_extensions.conf
 
+# Generate files need for CRL issuing.
+
 echo "1000" > provider_crlnumber.txt
 
 touch provider_index.txt
 
+# Generate two CRLs.
+
 openssl ca -gencrl                       \
   -keyfile provider_client_trust_key.pem \
   -cert provider_client_trust_cert.pem   \
@@ -72,15 +77,17 @@ openssl ca -gencrl                       \
   -out provider_crl_server_revoked.pem   \
   -config provider_crl.cnf
 
+# Generate malicious CRLs.
+
 openssl genrsa                                      \
   -out provider_malicious_client_trust_key.pem 4096
 
-SKI=$(openssl x509 -in provider_client_trust_cert.pem \
+SubjectKeyIdentifier=$(openssl x509 -in provider_client_trust_cert.pem \
   -noout                                              \
   -text                                               \
   | awk '/Subject Key Identifier/ {getline; print $1;}')
 
-sed -i "s/subjectKeyIdentifier = hash/subjectKeyIdentifier = $SKI/g" \
+sed -i "s/subjectKeyIdentifier = hash/subjectKeyIdentifier = $SubjectKeyIdentifier/g" \
   provider_extensions.conf
 
 openssl req -new                                       \