From 2601debb2246e662a44ec4a8d0358ab7b11d1608 Mon Sep 17 00:00:00 2001 From: Kenneth Jenkins <51246568+kenjenkins@users.noreply.github.com> Date: Thu, 10 Aug 2023 14:20:31 -0700 Subject: [PATCH] cryptutil: update CRL parsing Move the parseCRLs() method from package 'authorize/evaluator' to 'pkg/cryptutil', replacing the existing DecodeCRL() method. This method will parse all CRLs found in the PEM input, rather than just the first. This removes our usage of the deprecated method x509.ParseDERCRL(). Update this method to return an error if there is non-PEM data found in the input, to satisfy the existing test that raw DER-encoded CRLs are not permitted. Delete the CRLFromBase64() and CRLFromFile() methods, as these are no longer used. --- authorize/evaluator/functions.go | 22 +---------- config/mtls.go | 7 +--- config/mtls_test.go | 2 +- pkg/cryptutil/certificates.go | 51 +++++++++----------------- pkg/cryptutil/certificates_test.go | 25 ++++--------- pkg/cryptutil/testdata/example-crl.pem | 13 ------- 6 files changed, 29 insertions(+), 91 deletions(-) delete mode 100644 pkg/cryptutil/testdata/example-crl.pem diff --git a/authorize/evaluator/functions.go b/authorize/evaluator/functions.go index d5858dc31..4c1f351b1 100644 --- a/authorize/evaluator/functions.go +++ b/authorize/evaluator/functions.go @@ -11,6 +11,7 @@ import ( lru "github.com/hashicorp/golang-lru/v2" "github.com/pomerium/pomerium/internal/log" + "github.com/pomerium/pomerium/pkg/cryptutil" ) // ClientCertConstraints contains additional constraints to validate when @@ -61,7 +62,7 @@ func isValidClientCertificate( return false, err } - crls, err := parseCRLs([]byte(crl)) + crls, err := cryptutil.ParseCRLs([]byte(crl)) if err != nil { return false, err } @@ -168,22 +169,3 @@ func parseCertificate(pemStr string) (*x509.Certificate, error) { } return x509.ParseCertificate(block.Bytes) } - -func parseCRLs(crl []byte) (map[string]*x509.RevocationList, error) { - m := make(map[string]*x509.RevocationList) - for { - var block *pem.Block - block, crl = pem.Decode(crl) - if block == nil { - return m, nil - } - if block.Type != "X509 CRL" { - continue - } - l, err := x509.ParseRevocationList(block.Bytes) - if err != nil { - return nil, err - } - m[string(l.RawIssuer)] = l - } -} diff --git a/config/mtls.go b/config/mtls.go index e4886f887..fb9fecc94 100644 --- a/config/mtls.go +++ b/config/mtls.go @@ -123,11 +123,8 @@ func (s *DownstreamMTLSSettings) validate() error { crl, err := s.GetCRL() if err != nil { return err - } - if len(crl) > 0 { - if _, err := cryptutil.DecodeCRL(crl); err != nil { - return fmt.Errorf("CRL: %w", err) - } + } else if _, err := cryptutil.ParseCRLs(crl); err != nil { + return fmt.Errorf("CRL: %w", err) } switch s.Enforcement { diff --git a/config/mtls_test.go b/config/mtls_test.go index 1f4cc7f98..330759876 100644 --- a/config/mtls_test.go +++ b/config/mtls_test.go @@ -127,7 +127,7 @@ func TestDownstreamMTLSSettingsValidate(t *testing.T) { {"bad CA file", DownstreamMTLSSettings{CAFile: "-"}, "CA file: open -: no such file or directory"}, {"bad CRL", DownstreamMTLSSettings{CRL: "dGhpc2lzZmluZQo="}, - "CRL: cryptutil: invalid crl, no X509 CRL block found"}, + "CRL: cryptutil: non-PEM data in CRL bundle"}, {"bad CRL file", DownstreamMTLSSettings{CRLFile: "-"}, "CRL file: open -: no such file or directory"}, {"bad enforcement mode", DownstreamMTLSSettings{Enforcement: "whatever"}, diff --git a/pkg/cryptutil/certificates.go b/pkg/cryptutil/certificates.go index 426267249..94d6c81ad 100644 --- a/pkg/cryptutil/certificates.go +++ b/pkg/cryptutil/certificates.go @@ -4,7 +4,6 @@ import ( "crypto/ecdsa" "crypto/tls" "crypto/x509" - "crypto/x509/pkix" "encoding/base64" "encoding/pem" "errors" @@ -16,7 +15,6 @@ import ( ) const ( - crlPemType = "X509 CRL" maxCertFileSize = 1 << 16 ) @@ -41,43 +39,28 @@ func CertificateFromFile(certFile, keyFile string) (*tls.Certificate, error) { return &cert, err } -// CRLFromBase64 parses a certificate revocation list from a base64 encoded blob. -func CRLFromBase64(rawCRL string) (*pkix.CertificateList, error) { - bs, err := base64.StdEncoding.DecodeString(rawCRL) - if err != nil { - return nil, fmt.Errorf("cryptutil: failed to decode base64 crl: %w", err) - } - return DecodeCRL(bs) -} - -// CRLFromFile parses a certificate revocation list from a file. -func CRLFromFile(fileName string) (*pkix.CertificateList, error) { - bs, err := os.ReadFile(fileName) - if err != nil { - return nil, fmt.Errorf("cryptutil: failed to read crl file (%s): %w", fileName, err) - } - return DecodeCRL(bs) -} - -// DecodeCRL decodes a PEM-encoded certificate revocation list. -func DecodeCRL(encodedCRL []byte) (*pkix.CertificateList, error) { - data := encodedCRL - for len(data) > 0 { +// ParseCRLs parses PEM-encoded certificate revocation lists, returning a map +// of the parsed CRLs keyed by the raw issuer name. +func ParseCRLs(crl []byte) (map[string]*x509.RevocationList, error) { + m := make(map[string]*x509.RevocationList) + for { var block *pem.Block - block, data = pem.Decode(data) + block, crl = pem.Decode(crl) if block == nil { - break - } - - if block.Type == crlPemType { - lst, err := x509.ParseDERCRL(block.Bytes) - if err != nil { - return nil, fmt.Errorf("cryptutil: failed to parse crl: %w", err) + if len(crl) > 0 { + return nil, errors.New("cryptutil: non-PEM data in CRL bundle") } - return lst, nil + return m, nil } + if block.Type != "X509 CRL" { + continue + } + l, err := x509.ParseRevocationList(block.Bytes) + if err != nil { + return nil, fmt.Errorf("cryptutil: failed to parse crl: %w", err) + } + m[string(l.RawIssuer)] = l } - return nil, fmt.Errorf("cryptutil: invalid crl, no %s block found", crlPemType) } // DecodePublicKey decodes a PEM-encoded ECDSA public key. diff --git a/pkg/cryptutil/certificates_test.go b/pkg/cryptutil/certificates_test.go index e62e752d8..abac16a9c 100644 --- a/pkg/cryptutil/certificates_test.go +++ b/pkg/cryptutil/certificates_test.go @@ -106,29 +106,18 @@ func TestCertificateFromFile(t *testing.T) { _ = listener } -func TestCRLFromBase64(t *testing.T) { - bs := base64.StdEncoding.EncodeToString([]byte(pemCRL)) - lst, err := CRLFromBase64(bs) +func TestParseCRLs(t *testing.T) { + crls, err := ParseCRLs([]byte(pemCRL)) assert.NoError(t, err) - assert.NotNil(t, lst) -} - -func TestCRLFromFile(t *testing.T) { - lst, err := CRLFromFile("testdata/example-crl.pem") - assert.NoError(t, err) - assert.NotNil(t, lst) -} - -func TestDecodeCRL(t *testing.T) { - lst, err := DecodeCRL([]byte(pemCRL)) - assert.NoError(t, err) - assert.NotNil(t, lst) + assert.Equal(t, 1, len(crls)) + crl := crls["0l1\x1a0\x18\x06\x03U\x04\n\x13\x11RSA Security Inc.1\x1e0\x1c\x06\x03U\x04\x03\x13\x15RSA Public Root CA v11.0,\x06\t*\x86H\x86\xf7\r\x01\t\x01\x16\x1frsakeonrootsign@rsasecurity.com"] + assert.NotNil(t, crl) t.Run("der", func(t *testing.T) { bs, _ := base64.StdEncoding.DecodeString(derCRLBase64) - lst, err := DecodeCRL(bs) + crls, err := ParseCRLs(bs) assert.Error(t, err, "should not allow DER encoded CRL") - assert.Nil(t, lst) + assert.Nil(t, crls) }) } diff --git a/pkg/cryptutil/testdata/example-crl.pem b/pkg/cryptutil/testdata/example-crl.pem deleted file mode 100644 index bf4988988..000000000 --- a/pkg/cryptutil/testdata/example-crl.pem +++ /dev/null @@ -1,13 +0,0 @@ ------BEGIN X509 CRL----- -MIIB9jCCAV8CAQEwDQYJKoZIhvcNAQEFBQAwbDEaMBgGA1UEChMRUlNBIFNlY3Vy -aXR5IEluYy4xHjAcBgNVBAMTFVJTQSBQdWJsaWMgUm9vdCBDQSB2MTEuMCwGCSqG -SIb3DQEJARYfcnNha2VvbnJvb3RzaWduQHJzYXNlY3VyaXR5LmNvbRcNMTEwMjIz -MTkyODMwWhcNMTEwODIyMTkyODMwWjCBjDBKAhEArDqoh9FHJHXT7OPguun4+BcN -MDkxMTAyMTQyNzA5WjAmMAoGA1UdFQQDCgEJMBgGA1UdGAQRGA8yMDA5MTEwMjE0 -MjQ1NVowPgIRALGznZ095PB5aAOLPg57fMMXDTAyMTAyMzE0NTAxNFowGjAYBgNV -HRgEERgPMjAwMjEwMjMxNDUwMTRaoDAwLjAfBgNVHSMEGDAWgBT1TDF6UQM/LNeL -l5lvqHGQq3g9mzALBgNVHRQEBAICAIQwDQYJKoZIhvcNAQEFBQADgYEAFU5As6Mz -q5PRsifaobQPGh1aJLyC+Ms5Agc0bWyA3GAdxur5SpPZeRWCBjiP/MEHBWJClBHP -GRcq5yId3EjDkaEyxRa+i67LzvhI6c29Ee6K9pSYwji/7RUhmmnPrXtTxlL0lrLr -mQQJ6xhDRa5G3QA4CmUdsHNvbrzgmCYpvVE= ------END X509 CRL-----