From 270d5a12d0e02d0defbdaf2af5449d612e853930 Mon Sep 17 00:00:00 2001 From: Daniel Adam Date: Wed, 23 Oct 2024 16:08:47 +0200 Subject: [PATCH] fixup! Extend signers to set CRL Distribution Points --- pkg/security/generateCertificate/config.go | 12 ++-- pkg/security/signer/basicCertificateSigner.go | 17 ++---- .../signer/basicCertificateSigner_test.go | 60 +++++++++++++++++-- .../signer/identityCertificateSigner.go | 17 ++---- .../signer/identityCertificateSigner_test.go | 30 +++++++++- pkg/security/x509/parse.go | 17 ++++++ pkg/security/x509/validate.go | 15 +++++ 7 files changed, 129 insertions(+), 39 deletions(-) create mode 100644 pkg/security/x509/validate.go diff --git a/pkg/security/generateCertificate/config.go b/pkg/security/generateCertificate/config.go index 032f7b2b..42cedd01 100644 --- a/pkg/security/generateCertificate/config.go +++ b/pkg/security/generateCertificate/config.go @@ -9,11 +9,12 @@ import ( "encoding/asn1" "fmt" "net" - "net/url" "slices" "strconv" "strings" "time" + + pkgX509 "github.com/plgd-dev/device/v2/pkg/security/x509" ) type ( @@ -308,13 +309,10 @@ func (cfg Configuration) ToIPAddresses() ([]net.IP, error) { } func (cfg Configuration) ToCRLDistributionPoints() ([]string, error) { - cdp := make([]string, 0, len(cfg.CRLDistributionPoints)) - for _, crl := range cfg.CRLDistributionPoints { - if _, err := url.ParseRequestURI(crl); err != nil { - return nil, fmt.Errorf("invalid CRL distribution point URL %q: %w", crl, err) - } - cdp = append(cdp, crl) + if err := pkgX509.ValidateCRLDistributionPoints(cfg.CRLDistributionPoints); err != nil { + return nil, err } + cdp := slices.Clone(cfg.CRLDistributionPoints) slices.Sort(cdp) return slices.Compact(cdp), nil } diff --git a/pkg/security/signer/basicCertificateSigner.go b/pkg/security/signer/basicCertificateSigner.go index f8743482..de1852ca 100644 --- a/pkg/security/signer/basicCertificateSigner.go +++ b/pkg/security/signer/basicCertificateSigner.go @@ -5,7 +5,6 @@ import ( "crypto" "crypto/rand" "crypto/x509" - "encoding/pem" "errors" "math/big" "time" @@ -32,17 +31,7 @@ func NewBasicCertificateSigner(caCert []*x509.Certificate, caKey crypto.PrivateK } func (s *BasicCertificateSigner) Sign(_ context.Context, csr []byte) ([]byte, error) { - csrBlock, _ := pem.Decode(csr) - if csrBlock == nil { - return nil, errors.New("pem not found") - } - - certificateRequest, err := x509.ParseCertificateRequest(csrBlock.Bytes) - if err != nil { - return nil, err - } - - err = certificateRequest.CheckSignature() + certificateRequest, err := pkgX509.ParseAndCheckCertificateRequest(csr) if err != nil { return nil, err } @@ -54,7 +43,9 @@ func (s *BasicCertificateSigner) Sign(_ context.Context, csr []byte) ([]byte, er if err != nil { return nil, err } - + if err = pkgX509.ValidateCRLDistributionPoints(s.crlDistributionPoints); err != nil { + return nil, err + } template := x509.Certificate{ SerialNumber: serialNumber, NotBefore: notBefore, diff --git a/pkg/security/signer/basicCertificateSigner_test.go b/pkg/security/signer/basicCertificateSigner_test.go index 293964c1..46844e3f 100644 --- a/pkg/security/signer/basicCertificateSigner_test.go +++ b/pkg/security/signer/basicCertificateSigner_test.go @@ -2,6 +2,7 @@ package signer_test import ( "context" + "crypto" "crypto/ecdsa" "crypto/elliptic" "crypto/rand" @@ -33,21 +34,41 @@ func TestBasicCertificateSignerSign(t *testing.T) { csr, err := generateCertificate.GenerateCSR(csrCfg, priv) require.NoError(t, err) + type fields struct { + caCert []*x509.Certificate + caKey crypto.PrivateKey + validNotBefore time.Time + validNotAfter time.Time + crlPoints []string + } type args struct { csr []byte } tests := []struct { name string + fields fields args args want []*x509.Certificate wantErr bool }{ { - name: "invalid", + name: "invalid", + fields: fields{ + caCert: caCert, + caKey: caKey, + validNotBefore: time.Now(), + validNotAfter: time.Now().Add(time.Hour * 86400), + }, wantErr: true, }, { name: "valid", + fields: fields{ + caCert: caCert, + caKey: caKey, + validNotBefore: time.Now(), + validNotAfter: time.Now().Add(time.Hour * 86400), + }, args: args{ csr: csr, }, @@ -70,14 +91,42 @@ func TestBasicCertificateSignerSign(t *testing.T) { }, }, }, + { + name: "valid with CRL points", + fields: fields{ + caCert: caCert, + caKey: caKey, + validNotBefore: time.Now().Add(-time.Second), + validNotAfter: time.Now().Add(time.Hour), + crlPoints: []string{"http://example.com/crl"}, + }, + args: args{ + csr: csr, + }, + want: []*x509.Certificate{ + { + Subject: pkix.Name{ + CommonName: "uuid:00000000-0000-0000-0000-000000000001", + }, + ExtKeyUsage: []x509.ExtKeyUsage{ + x509.ExtKeyUsageServerAuth, + x509.ExtKeyUsageClientAuth, + }, + UnknownExtKeyUsage: []asn1.ObjectIdentifier{coap.ExtendedKeyUsage_IDENTITY_CERTIFICATE}, + CRLDistributionPoints: []string{"http://example.com/crl"}, + }, + { + Subject: pkix.Name{ + CommonName: "intermediateCA", + }, + }, + }, + }, } - - s := signer.NewBasicCertificateSigner(caCert, caKey, time.Now(), time.Now().Add(time.Hour*86400), nil) - for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + s := signer.NewBasicCertificateSigner(tt.fields.caCert, tt.fields.caKey, tt.fields.validNotBefore, tt.fields.validNotAfter, tt.fields.crlPoints) got, err := s.Sign(context.Background(), tt.args.csr) - if tt.wantErr { require.Error(t, err) return @@ -91,6 +140,7 @@ func TestBasicCertificateSignerSign(t *testing.T) { require.Equal(t, tt.want[i].Subject.CommonName, cert.Subject.CommonName) require.Equal(t, tt.want[i].ExtKeyUsage, cert.ExtKeyUsage) require.Equal(t, tt.want[i].UnknownExtKeyUsage, cert.UnknownExtKeyUsage) + require.Equal(t, tt.want[i].CRLDistributionPoints, cert.CRLDistributionPoints) got = rest } }) diff --git a/pkg/security/signer/identityCertificateSigner.go b/pkg/security/signer/identityCertificateSigner.go index dc8ad217..1b0d349f 100644 --- a/pkg/security/signer/identityCertificateSigner.go +++ b/pkg/security/signer/identityCertificateSigner.go @@ -22,7 +22,6 @@ import ( "crypto/rand" "crypto/x509" "encoding/asn1" - "encoding/pem" "errors" "fmt" "math/big" @@ -72,17 +71,7 @@ func (s *OCFIdentityCertificate) Sign(_ context.Context, csr []byte) ([]byte, er return nil, fmt.Errorf("expired: current time %v is out of time range: %v <-> %v", now, notBefore.Format(time.RFC3339), notAfter.Format(time.RFC3339)) } - csrBlock, _ := pem.Decode(csr) - if csrBlock == nil { - return nil, errors.New("pem not found") - } - - certificateRequest, err := x509.ParseCertificateRequest(csrBlock.Bytes) - if err != nil { - return nil, err - } - - err = certificateRequest.CheckSignature() + certificateRequest, err := pkgX509.ParseAndCheckCertificateRequest(csr) if err != nil { return nil, err } @@ -92,7 +81,9 @@ func (s *OCFIdentityCertificate) Sign(_ context.Context, csr []byte) ([]byte, er if err != nil { return nil, err } - + if err = pkgX509.ValidateCRLDistributionPoints(s.crlDistributionPoints); err != nil { + return nil, err + } template := x509.Certificate{ SerialNumber: serialNumber, NotBefore: notBefore, diff --git a/pkg/security/signer/identityCertificateSigner_test.go b/pkg/security/signer/identityCertificateSigner_test.go index efe409b4..919518de 100644 --- a/pkg/security/signer/identityCertificateSigner_test.go +++ b/pkg/security/signer/identityCertificateSigner_test.go @@ -24,6 +24,7 @@ func TestOCFIdentityCertificateSign(t *testing.T) { caKey crypto.PrivateKey validNotBefore time.Time validNotAfter time.Time + crlPoints []string } type args struct { csr []byte @@ -107,10 +108,37 @@ func TestOCFIdentityCertificateSign(t *testing.T) { }, wantErr: true, }, + { + name: "valid with CRL points", + fields: fields{ + caCert: caCert, + caKey: caKey, + validNotBefore: time.Now().Add(-time.Second), + validNotAfter: time.Now().Add(time.Hour), + crlPoints: []string{"http://example.com/crl"}, + }, + args: args{ + csr: csr, + }, + }, + { + name: "invalid CRL point format", + fields: fields{ + caCert: caCert, + caKey: caKey, + validNotBefore: time.Now().Add(-time.Second), + validNotAfter: time.Now().Add(time.Hour), + crlPoints: []string{"invalid-url"}, + }, + args: args{ + csr: csr, + }, + wantErr: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - s := signer.NewOCFIdentityCertificate(tt.fields.caCert, tt.fields.caKey, tt.fields.validNotBefore, tt.fields.validNotAfter, nil) + s := signer.NewOCFIdentityCertificate(tt.fields.caCert, tt.fields.caKey, tt.fields.validNotBefore, tt.fields.validNotAfter, tt.fields.crlPoints) gotSignedCsr, err := s.Sign(context.Background(), tt.args.csr) if tt.wantErr { require.Error(t, err) diff --git a/pkg/security/x509/parse.go b/pkg/security/x509/parse.go index 85d8c46b..b3dd952b 100644 --- a/pkg/security/x509/parse.go +++ b/pkg/security/x509/parse.go @@ -106,3 +106,20 @@ func ParseCertificates(cert *tls.Certificate) ([]*x509.Certificate, error) { } return caChain, nil } + +func ParseAndCheckCertificateRequest(csr []byte) (*x509.CertificateRequest, error) { + csrBlock, _ := pem.Decode(csr) + if csrBlock == nil { + return nil, errors.New("pem not found") + } + + certificateRequest, err := x509.ParseCertificateRequest(csrBlock.Bytes) + if err != nil { + return nil, err + } + err = certificateRequest.CheckSignature() + if err != nil { + return nil, err + } + return certificateRequest, nil +} diff --git a/pkg/security/x509/validate.go b/pkg/security/x509/validate.go new file mode 100644 index 00000000..78b456a0 --- /dev/null +++ b/pkg/security/x509/validate.go @@ -0,0 +1,15 @@ +package x509 + +import ( + "fmt" + "net/url" +) + +func ValidateCRLDistributionPoints(crlDistributionPoints []string) error { + for _, crl := range crlDistributionPoints { + if _, err := url.ParseRequestURI(crl); err != nil { + return fmt.Errorf("invalid CRL distribution point URL %q: %w", crl, err) + } + } + return nil +}