Change Static provider behavior to match C Core, address other PR com…

…ments
grpc · erm-g · Oct 31, 2023 · Jun 29, 2023 · Jun 29, 2023 · Jun 29, 2023
commit f844c8cd889638d1388947780ac9786e65bf55e6
diff --git a/security/advancedtls/advancedtls_test.go b/security/advancedtls/advancedtls_test.go
@@ -25,6 +25,7 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"os"
 	"testing"
 
 	lru "github.com/hashicorp/golang-lru"
@@ -381,15 +382,20 @@ func (s) TestClientServerHandshake(t *testing.T) {
 	}
 
 	makeStaticCRLProvider := func(containsRevoked bool) *RevocationConfig {
-		cRLProvider := MakeStaticCRLProvider()
-		var crl *CRL
+
+		rawCRLs := make([][]byte, 1)
+		var path string
 		if containsRevoked {
-			crl = loadCRL(t, testdata.Path("crl/provider/crl_server_revoked.pem"))
+			path = testdata.Path("crl/provider/crl_server_revoked.pem")
 		} else {
-			crl = loadCRL(t, testdata.Path("crl/provider/crl_empty.pem"))
+			path = testdata.Path("crl/provider/crl_empty.pem")
 		}
-		cRLProvider.AddCRL(crl)
-
+		rawCRL, err := os.ReadFile(path)
+		if err != nil {
+			t.Fatalf("readFile(%v) failed err = %v", path, err)
+		}
+		rawCRLs = append(rawCRLs, rawCRL)
+		cRLProvider := MakeStaticCRLProvider(rawCRLs)
 		return &RevocationConfig{
 			AllowUndetermined: true,
 			CRLProvider:       cRLProvider,
@@ -619,14 +625,14 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		// server custom check fails
 		{
 			desc:                       "Client sets peer cert, reload root function with verifyFuncGood; Server sets bad custom check; mutualTLS",
-			clientCert:                 []tls.Certificate{cs.ClientCert3},
-			clientGetRoot:              getRootCAsForClientCRL,
+			clientCert:                 []tls.Certificate{cs.ClientCert1},
+			clientGetRoot:              getRootCAsForClient,
 			clientVerifyFunc:           clientVerifyFuncGood,
 			clientVType:                CertVerification,
 			clientExpectHandshakeError: true,
 			serverMutualTLS:            true,
-			serverCert:                 []tls.Certificate{cs.ServerCert3},
-			serverGetRoot:              getRootCAsForServerCRL,
+			serverCert:                 []tls.Certificate{cs.ServerCert1},
+			serverGetRoot:              getRootCAsForServer,
 			serverVerifyFunc:           verifyFuncBad,
 			serverVType:                CertVerification,
 			serverExpectError:          true,
@@ -707,7 +713,7 @@ func (s) TestClientServerHandshake(t *testing.T) {
 		},
 		// Client: set valid credentials with the revocation config
 		// Server: set valid credentials with the revocation config
-		// Expected Behavior: success, because non of the certificate chains sent in the connection are revoked
+		// 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; mutualTLS",
 			clientCert:       []tls.Certificate{cs.ClientCert1},

diff --git a/security/advancedtls/crl_provider.go b/security/advancedtls/crl_provider.go
@@ -45,24 +45,33 @@ type CRLProvider interface {
 	CRL(cert *x509.Certificate) (*CRL, error)
 }
 
-// StaticCRLProvider implements CRLProvider interface by accepting CRL structs
-// and storing them in-memory.
+// StaticCRLProvider implements CRLProvider interface by accepting raw content
+// of CRL files at creation time and storing parsed CRL structs  in-memory.
 type StaticCRLProvider struct {
 	// TODO CRL is sort of our internal representation - provide an API for
 	// people to read into it, or provide a simpler type in the API then
 	// internally convert to this form
 	crls map[string]*CRL
 }
 
-// MakeStaticCRLProvider returns a new instance of the StaticCRLProvider.
-func MakeStaticCRLProvider() *StaticCRLProvider {
+// MakeStaticCRLProvider processes raw content of CRL files, adds parsed CRL
+// structs into in-memory, and returns a new instance of the StaticCRLProvider.
+func MakeStaticCRLProvider(rawCRLs [][]byte) *StaticCRLProvider {
 	p := StaticCRLProvider{}
 	p.crls = make(map[string]*CRL)
+	for idx, rawCRL := range rawCRLs {
+		cRL, err := NewCRL(rawCRL)
+		if err != nil {
+			grpclogLogger.Warningf("Can't parse raw CRL number %v from the slice: %v", idx, err)
+			continue
+		}
+		p.addCRL(cRL)
+	}
 	return &p
 }
 
 // AddCRL adds/updates provided CRL to in-memory storage.
-func (p *StaticCRLProvider) AddCRL(crl *CRL) {
+func (p *StaticCRLProvider) addCRL(crl *CRL) {
 	key := crl.CertList.Issuer.ToRDNSequence().String()
 	p.crls[key] = crl
 }

diff --git a/security/advancedtls/crl_provider_test.go b/security/advancedtls/crl_provider_test.go
@@ -21,6 +21,7 @@ package advancedtls
 import (
 	"crypto/x509"
 	"fmt"
+	"os"
 	"testing"
 	"time"
 
@@ -34,11 +35,15 @@ const nonCRLFilesUnderCRLDirectory = 5
 // unrevoked, revoked leaf, and revoked intermediate chains, as well as a chain
 // without CRL for issuer, and checks that it’s correctly processed.
 func TestStaticCRLProvider(t *testing.T) {
-	p := MakeStaticCRLProvider()
+	rawCRLs := make([][]byte, 6)
 	for i := 1; i <= 6; i++ {
-		crl := loadCRL(t, testdata.Path(fmt.Sprintf("crl/%d.crl", i)))
-		p.AddCRL(crl)
+		rawCRL, err := os.ReadFile(testdata.Path(fmt.Sprintf("crl/%d.crl", i)))
+		if err != nil {
+			t.Fatalf("readFile(%v) failed err = %v", fmt.Sprintf("crl/%d.crl", i), err)
+		}
+		rawCRLs = append(rawCRLs, rawCRL)
 	}
+	p := MakeStaticCRLProvider(rawCRLs)
 	// Each test data entry contains a description of a certificate chain,
 	// certificate chain itself, and if CRL is not expected to be found.
 	tests := []struct {

diff --git a/security/advancedtls/crl_test.go b/security/advancedtls/crl_test.go
@@ -509,11 +509,15 @@ func TestRevokedCert(t *testing.T) {
 	revokedLeafChain := makeChain(t, testdata.Path("crl/revokedLeaf.pem"))
 	validChain := makeChain(t, testdata.Path("crl/unrevoked.pem"))
 	cache, err := lru.New(5)
-	cRLProvider := MakeStaticCRLProvider()
+	rawCRLs := make([][]byte, 6)
 	for i := 1; i <= 6; i++ {
-		crl := loadCRL(t, testdata.Path(fmt.Sprintf("crl/%d.crl", i)))
-		cRLProvider.AddCRL(crl)
+		rawCRL, err := os.ReadFile(testdata.Path(fmt.Sprintf("crl/%d.crl", i)))
+		if err != nil {
+			t.Fatalf("readFile(%v) failed err = %v", fmt.Sprintf("crl/%d.crl", i), err)
+		}
+		rawCRLs = append(rawCRLs, rawCRL)
 	}
+	cRLProvider := MakeStaticCRLProvider(rawCRLs)
 	if err != nil {
 		t.Fatalf("lru.New: err = %v", err)
 	}