Merge pull request #6542 from tanujd11/fix/name-constraints-csr-structure

fix: structure of nameconstraint in CSR
2023-12-12 16:07:16 +00:00 · 2023-12-12 16:07:16 +00:00 · 8da699a735
commit 8da699a735
parent 4ae25789a2 849b6bda9e
6 changed files with 467 additions and 170 deletions
--- a/make/e2e-setup.mk
+++ b/make/e2e-setup.mk
@ -263,7 +263,7 @@ comma = ,
 # Helm's "--set" interprets commas, which means we want to escape commas
 # for "--set featureGates". That's why we have "\$(comma)".
 feature_gates_controller := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% AdditionalCertificateOutputFormats=% ValidateCAA=% ExperimentalCertificateSigningRequestControllers=% ExperimentalGatewayAPISupport=% ServerSideApply=% LiteralCertificateSubject=% UseCertificateRequestBasicConstraints=% UseCertificateRequestNameConstraints=% SecretsFilteredCaching=%, $(subst $(comma),$(space),$(FEATURE_GATES))))
-feature_gates_webhook := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% AdditionalCertificateOutputFormats=% LiteralCertificateSubject=%, UseCertificateRequestNameConstraints=%  $(subst $(comma),$(space),$(FEATURE_GATES))))
+feature_gates_webhook := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% AdditionalCertificateOutputFormats=% LiteralCertificateSubject=% UseCertificateRequestNameConstraints=%, $(subst $(comma),$(space),$(FEATURE_GATES))))
 feature_gates_cainjector := $(subst $(space),\$(comma),$(filter AllAlpha=% AllBeta=% ServerSideApply=%, $(subst $(comma),$(space),$(FEATURE_GATES))))

 # Install cert-manager with E2E specific images and deployment settings.
--- a/pkg/util/pki/certificatetemplate.go
+++ b/pkg/util/pki/certificatetemplate.go
@ -198,16 +198,15 @@ func CertificateTemplateFromCSR(csr *x509.CertificateRequest, validatorMutators
 			if err != nil {
 				return err
 			}
-
-			template.PermittedDNSDomainsCritical = nameConstraints.PermittedDNSDomainsCritical
+			template.PermittedDNSDomainsCritical = val.Critical
 			template.PermittedDNSDomains = nameConstraints.PermittedDNSDomains
-			template.ExcludedDNSDomains = nameConstraints.ExcludedDNSDomains
-			template.PermittedIPRanges = convertIPNetSliceToIPNetPointerSlice(nameConstraints.PermittedIPRanges)
-			template.ExcludedIPRanges = convertIPNetSliceToIPNetPointerSlice(nameConstraints.ExcludedIPRanges)
+			template.PermittedIPRanges = nameConstraints.PermittedIPRanges
 			template.PermittedEmailAddresses = nameConstraints.PermittedEmailAddresses
-			template.ExcludedEmailAddresses = nameConstraints.ExcludedEmailAddresses
 			template.PermittedURIDomains = nameConstraints.PermittedURIDomains
-			template.ExcludedURIDomains = nameConstraints.ExcludedEmailAddresses
+			template.ExcludedDNSDomains = nameConstraints.ExcludedDNSDomains
+			template.ExcludedIPRanges = nameConstraints.ExcludedIPRanges
+			template.ExcludedEmailAddresses = nameConstraints.ExcludedEmailAddresses
+			template.ExcludedURIDomains = nameConstraints.ExcludedURIDomains
 		}

 		// RFC 5280, 4.2.1.3
--- a/pkg/util/pki/csr.go
+++ b/pkg/util/pki/csr.go
@ -319,11 +319,36 @@ func GenerateCSR(crt *v1.Certificate, optFuncs ...GenerateCSROption) (*x509.Cert
 	}

 	if opts.EncodeNameConstraintsInRequest && crt.Spec.NameConstraints != nil {
-		extension, err := MarshalNameConstraints(crt.Spec.NameConstraints)
-		if err != nil {
-			return nil, err
+		nameConstraints := &NameConstraints{}
+
+		if crt.Spec.NameConstraints.Permitted != nil {
+			nameConstraints.PermittedDNSDomains = crt.Spec.NameConstraints.Permitted.DNSDomains
+			nameConstraints.PermittedIPRanges, err = parseCIDRs(crt.Spec.NameConstraints.Permitted.IPRanges)
+			if err != nil {
+				return nil, err
+			}
+			nameConstraints.PermittedEmailAddresses = crt.Spec.NameConstraints.Permitted.EmailAddresses
+			nameConstraints.ExcludedURIDomains = crt.Spec.NameConstraints.Permitted.URIDomains
+		}
+
+		if crt.Spec.NameConstraints.Excluded != nil {
+			nameConstraints.ExcludedDNSDomains = crt.Spec.NameConstraints.Excluded.DNSDomains
+			nameConstraints.ExcludedIPRanges, err = parseCIDRs(crt.Spec.NameConstraints.Excluded.IPRanges)
+			if err != nil {
+				return nil, err
+			}
+			nameConstraints.ExcludedEmailAddresses = crt.Spec.NameConstraints.Excluded.EmailAddresses
+			nameConstraints.ExcludedURIDomains = crt.Spec.NameConstraints.Excluded.URIDomains
+		}
+
+		if !nameConstraints.IsEmpty() {
+			extension, err := MarshalNameConstraints(nameConstraints, crt.Spec.NameConstraints.Critical)
+			if err != nil {
+				return nil, err
+			}
+
+			extraExtensions = append(extraExtensions, extension)
 		}
-		extraExtensions = append(extraExtensions, extension)
 	}

 	cr := &x509.CertificateRequest{
--- a/pkg/util/pki/csr_test.go
+++ b/pkg/util/pki/csr_test.go
@ -346,28 +346,6 @@ func TestGenerateCSR(t *testing.T) {
 		t.Fatal(err)
 	}

-	_, permittedIPNet, err := net.ParseCIDR("10.10.0.0/16")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	_, excludedIPNet, err := net.ParseCIDR("10.10.0.0/24")
-	if err != nil {
-		t.Fatal(err)
-	}
-
-	nameConstraints := NameConstraints{
-		PermittedDNSDomainsCritical: true,
-		PermittedDNSDomains:         []string{"example.org"},
-		PermittedIPRanges:           []net.IPNet{*permittedIPNet},
-		PermittedEmailAddresses:     []string{"email@email.org"},
-		ExcludedIPRanges:            []net.IPNet{*excludedIPNet},
-	}
-	asn1NameConstraints, err := asn1.Marshal(nameConstraints)
-	if err != nil {
-		t.Fatal(err)
-	}
-
 	// 0xa0 = DigitalSignature, Encipherment and KeyCertSign usage
 	asn1KeyUsageWithCa, err := asn1.Marshal(asn1.BitString{Bytes: []byte{0xa4}, BitLength: asn1BitLength([]byte{0xa4})})
 	if err != nil {
@ -652,7 +630,7 @@ func TestGenerateCSR(t *testing.T) {
 					},
 					{
 						Id:       OIDExtensionNameConstraints,
-						Value:    asn1NameConstraints,
+						Value:    []byte{0x30, 0x3e, 0xa0, 0x2e, 0x30, 0xd, 0x82, 0xb, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x6f, 0x72, 0x67, 0x30, 0xa, 0x87, 0x8, 0xa, 0xa, 0x0, 0x0, 0xff, 0xff, 0x0, 0x0, 0x30, 0x11, 0x81, 0xf, 0x65, 0x6d, 0x61, 0x69, 0x6c, 0x40, 0x65, 0x6d, 0x61, 0x69, 0x6c, 0x2e, 0x6f, 0x72, 0x67, 0xa1, 0xc, 0x30, 0xa, 0x87, 0x8, 0xa, 0xa, 0x0, 0x0, 0xff, 0xff, 0xff, 0x0},
 						Critical: true,
 					},
 				},
@ -690,7 +668,7 @@ func TestSignCSRTemplate(t *testing.T) {
 		require.NoError(t, err)
 		var permittedIPRanges []*net.IPNet
 		if nameConstraints != nil {
-			permittedIPRanges = convertIPNetSliceToIPNetPointerSlice(nameConstraints.PermittedIPRanges)
+			permittedIPRanges = nameConstraints.PermittedIPRanges
 		}
 		tmpl := &x509.Certificate{
 			Version:               3,
@ -731,7 +709,7 @@ func TestSignCSRTemplate(t *testing.T) {

 	// vars for testing name constraints
 	_, permittedIPNet, _ := net.ParseCIDR("10.10.0.0/16")
-	_, ncRootCert, _, ncRootPK := mustCreatePair(nil, nil, "ncroot", true, &NameConstraints{PermittedIPRanges: []net.IPNet{*permittedIPNet}})
+	_, ncRootCert, _, ncRootPK := mustCreatePair(nil, nil, "ncroot", true, &NameConstraints{PermittedIPRanges: []*net.IPNet{permittedIPNet}})
 	_, _, ncLeafTmpl, _ := mustCreatePair(ncRootCert, ncRootPK, "ncleaf", false, nil)
 	ncLeafTmpl.IPAddresses = []net.IP{net.ParseIP("10.20.0.5")}

--- a/pkg/util/pki/nameconstraints.go
+++ b/pkg/util/pki/nameconstraints.go
@ -18,11 +18,13 @@ package pki

 import (
 	"crypto/x509/pkix"
-	"encoding/asn1"
 	"errors"
+	"fmt"
 	"net"
+	"unicode"

-	v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
+	"golang.org/x/crypto/cryptobyte"
+	cryptobyte_asn1 "golang.org/x/crypto/cryptobyte/asn1"
 )

 // Copied from x509.go
@ -32,58 +34,146 @@ var (

 // NameConstraints represents the NameConstraints extension.
 type NameConstraints struct {
-	PermittedDNSDomainsCritical bool        `asn1:"optional,explicit,tag:0"`
-	PermittedDNSDomains         []string    `asn1:"optional,explicit,tag:1"`
-	ExcludedDNSDomains          []string    `asn1:"optional,explicit,tag:2"`
-	PermittedIPRanges           []net.IPNet `asn1:"optional,explicit,tag:3"`
-	ExcludedIPRanges            []net.IPNet `asn1:"optional,explicit,tag:4"`
-	PermittedEmailAddresses     []string    `asn1:"optional,explicit,tag:5"`
-	ExcludedEmailAddresses      []string    `asn1:"optional,explicit,tag:6"`
-	PermittedURIDomains         []string    `asn1:"optional,explicit,tag:7"`
-	ExcludedURIDomains          []string    `asn1:"optional,explicit,tag:8"`
+	PermittedDNSDomains     []string
+	ExcludedDNSDomains      []string
+	PermittedIPRanges       []*net.IPNet
+	ExcludedIPRanges        []*net.IPNet
+	PermittedEmailAddresses []string
+	ExcludedEmailAddresses  []string
+	PermittedURIDomains     []string
+	ExcludedURIDomains      []string
+}
+
+func (nc NameConstraints) IsEmpty() bool {
+	return len(nc.PermittedDNSDomains) == 0 &&
+		len(nc.PermittedIPRanges) == 0 &&
+		len(nc.PermittedEmailAddresses) == 0 &&
+		len(nc.PermittedURIDomains) == 0 &&
+		len(nc.ExcludedDNSDomains) == 0 &&
+		len(nc.ExcludedIPRanges) == 0 &&
+		len(nc.ExcludedEmailAddresses) == 0 &&
+		len(nc.ExcludedURIDomains) == 0
 }

 // Adapted from x509.go
-func MarshalNameConstraints(nameConstraints *v1.NameConstraints) (pkix.Extension, error) {
-	ext := pkix.Extension{Id: OIDExtensionNameConstraints, Critical: true}
-	var nameConstraintsForMarshalling NameConstraints
-	if nameConstraints.Permitted != nil {
-		permittedIPRanges, err := parseCIDRs(nameConstraints.Permitted.IPRanges)
-		if err != nil {
-			return pkix.Extension{}, err
-		}
-		nameConstraintsForMarshalling = NameConstraints{
-			PermittedDNSDomainsCritical: nameConstraints.Critical,
-			PermittedDNSDomains:         nameConstraints.Permitted.DNSDomains,
-			PermittedIPRanges:           permittedIPRanges,
-			PermittedEmailAddresses:     nameConstraints.Permitted.EmailAddresses,
-			PermittedURIDomains:         nameConstraints.Permitted.URIDomains,
-		}
+func MarshalNameConstraints(nameConstraints *NameConstraints, critical bool) (pkix.Extension, error) {
+	ipAndMask := func(ipNet *net.IPNet) []byte {
+		maskedIP := ipNet.IP.Mask(ipNet.Mask)
+		ipAndMask := make([]byte, 0, len(maskedIP)+len(ipNet.Mask))
+		ipAndMask = append(ipAndMask, maskedIP...)
+		ipAndMask = append(ipAndMask, ipNet.Mask...)
+		return ipAndMask
 	}

-	if nameConstraints.Excluded != nil {
-		excludedIPRanges, err := parseCIDRs(nameConstraints.Excluded.IPRanges)
-		if err != nil {
-			return pkix.Extension{}, err
+	serialiseConstraints := func(dns []string, ips []*net.IPNet, emails []string, uriDomains []string) (der []byte, err error) {
+		var b cryptobyte.Builder
+
+		for _, name := range dns {
+			if err = isIA5String(name); err != nil {
+				return nil, err
+			}
+
+			b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+				b.AddASN1(cryptobyte_asn1.Tag(2).ContextSpecific(), func(b *cryptobyte.Builder) {
+					b.AddBytes([]byte(name))
+				})
+			})
 		}
-		nameConstraintsForMarshalling.ExcludedDNSDomains = nameConstraints.Excluded.DNSDomains
-		nameConstraintsForMarshalling.ExcludedIPRanges = excludedIPRanges
-		nameConstraintsForMarshalling.ExcludedEmailAddresses = nameConstraints.Excluded.EmailAddresses
-		nameConstraintsForMarshalling.ExcludedURIDomains = nameConstraints.Excluded.URIDomains
+
+		for _, ipNet := range ips {
+			b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+				b.AddASN1(cryptobyte_asn1.Tag(7).ContextSpecific(), func(b *cryptobyte.Builder) {
+					b.AddBytes(ipAndMask(ipNet))
+				})
+			})
+		}
+
+		for _, email := range emails {
+			if err = isIA5String(email); err != nil {
+				return nil, err
+			}
+
+			b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+				b.AddASN1(cryptobyte_asn1.Tag(1).ContextSpecific(), func(b *cryptobyte.Builder) {
+					b.AddBytes([]byte(email))
+				})
+			})
+		}
+
+		for _, uriDomain := range uriDomains {
+			if err = isIA5String(uriDomain); err != nil {
+				return nil, err
+			}
+
+			b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+				b.AddASN1(cryptobyte_asn1.Tag(6).ContextSpecific(), func(b *cryptobyte.Builder) {
+					b.AddBytes([]byte(uriDomain))
+				})
+			})
+		}
+
+		return b.Bytes()
 	}
+
+	var permitted []byte
 	var err error
-	ext.Value, err = asn1.Marshal(nameConstraintsForMarshalling)
-	return ext, err
+	permitted, err = serialiseConstraints(nameConstraints.PermittedDNSDomains, nameConstraints.PermittedIPRanges, nameConstraints.PermittedEmailAddresses, nameConstraints.PermittedURIDomains)
+	if err != nil {
+		return pkix.Extension{}, err
+	}
+
+	var excluded []byte
+	excluded, err = serialiseConstraints(nameConstraints.ExcludedDNSDomains, nameConstraints.ExcludedIPRanges, nameConstraints.ExcludedEmailAddresses, nameConstraints.ExcludedURIDomains)
+	if err != nil {
+		return pkix.Extension{}, err
+	}
+
+	var b cryptobyte.Builder
+	b.AddASN1(cryptobyte_asn1.SEQUENCE, func(b *cryptobyte.Builder) {
+		if len(permitted) > 0 {
+			b.AddASN1(cryptobyte_asn1.Tag(0).ContextSpecific().Constructed(), func(b *cryptobyte.Builder) {
+				b.AddBytes(permitted)
+			})
+		}
+
+		if len(excluded) > 0 {
+			b.AddASN1(cryptobyte_asn1.Tag(1).ContextSpecific().Constructed(), func(b *cryptobyte.Builder) {
+				b.AddBytes(excluded)
+			})
+		}
+	})
+
+	bytes, err := b.Bytes()
+	if err != nil {
+		return pkix.Extension{}, err
+	}
+
+	return pkix.Extension{
+		Id:       OIDExtensionNameConstraints,
+		Critical: critical,
+		Value:    bytes,
+	}, nil
 }

-func parseCIDRs(cidrs []string) ([]net.IPNet, error) {
-	ipRanges := []net.IPNet{}
+func isIA5String(s string) error {
+	for _, r := range s {
+		// Per RFC5280 "IA5String is limited to the set of ASCII characters"
+		if r > unicode.MaxASCII {
+			return fmt.Errorf("x509: %q cannot be encoded as an IA5String", s)
+		}
+	}
+
+	return nil
+}
+
+func parseCIDRs(cidrs []string) ([]*net.IPNet, error) {
+	ipRanges := []*net.IPNet{}
 	for _, cidr := range cidrs {
 		_, ipNet, err := net.ParseCIDR(cidr)
 		if err != nil {
 			return nil, err
 		}
-		ipRanges = append(ipRanges, net.IPNet{
+		ipRanges = append(ipRanges, &net.IPNet{
 			IP:   ipNet.IP,
 			Mask: ipNet.Mask,
 		})
@ -91,27 +181,145 @@ func parseCIDRs(cidrs []string) ([]net.IPNet, error) {
 	return ipRanges, nil
 }

-func UnmarshalNameConstraints(value []byte) (NameConstraints, error) {
-	var constraints NameConstraints
-	var rest []byte
+// Adapted from crypto/x509/parser.go
+func UnmarshalNameConstraints(value []byte) (*NameConstraints, error) {
+	// RFC 5280, 4.2.1.10
+
+	// NameConstraints ::= SEQUENCE {
+	//      permittedSubtrees       [0]     GeneralSubtrees OPTIONAL,
+	//      excludedSubtrees        [1]     GeneralSubtrees OPTIONAL }
+	//
+	// GeneralSubtrees ::= SEQUENCE SIZE (1..MAX) OF GeneralSubtree
+	//
+	// GeneralSubtree ::= SEQUENCE {
+	//      base                    GeneralName,
+	//      minimum         [0]     BaseDistance DEFAULT 0,
+	//      maximum         [1]     BaseDistance OPTIONAL }
+	//
+	// BaseDistance ::= INTEGER (0..MAX)
+
+	outer := cryptobyte.String(value)
+	var toplevel, permitted, excluded cryptobyte.String
+	var havePermitted, haveExcluded bool
+	if !outer.ReadASN1(&toplevel, cryptobyte_asn1.SEQUENCE) ||
+		!outer.Empty() ||
+		!toplevel.ReadOptionalASN1(&permitted, &havePermitted, cryptobyte_asn1.Tag(0).ContextSpecific().Constructed()) ||
+		!toplevel.ReadOptionalASN1(&excluded, &haveExcluded, cryptobyte_asn1.Tag(1).ContextSpecific().Constructed()) ||
+		!toplevel.Empty() {
+		return nil, errors.New("x509: invalid NameConstraints extension")
+	}
+
+	if !havePermitted && !haveExcluded || len(permitted) == 0 && len(excluded) == 0 {
+		// From RFC 5280, Section 4.2.1.10:
+		//   “either the permittedSubtrees field
+		//   or the excludedSubtrees MUST be
+		//   present”
+		return nil, errors.New("x509: empty name constraints extension")
+	}
+
+	getValues := func(subtrees cryptobyte.String) (dnsNames []string, ips []*net.IPNet, emails, uriDomains []string, err error) {
+		for !subtrees.Empty() {
+			var seq, value cryptobyte.String
+			var tag cryptobyte_asn1.Tag
+			if !subtrees.ReadASN1(&seq, cryptobyte_asn1.SEQUENCE) ||
+				!seq.ReadAnyASN1(&value, &tag) {
+				return nil, nil, nil, nil, fmt.Errorf("x509: invalid NameConstraints extension")
+			}
+
+			var (
+				dnsTag   = cryptobyte_asn1.Tag(2).ContextSpecific()
+				emailTag = cryptobyte_asn1.Tag(1).ContextSpecific()
+				ipTag    = cryptobyte_asn1.Tag(7).ContextSpecific()
+				uriTag   = cryptobyte_asn1.Tag(6).ContextSpecific()
+			)
+
+			switch tag {
+			case dnsTag:
+				domain := string(value)
+				if err := isIA5String(domain); err != nil {
+					return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+				}
+
+				dnsNames = append(dnsNames, domain)
+
+			case ipTag:
+				l := len(value)
+				var ip, mask []byte
+
+				switch l {
+				case 2 * net.IPv4len:
+					ip = value[:net.IPv4len]
+					mask = value[net.IPv4len:]
+
+				case 2 * net.IPv6len:
+					ip = value[:net.IPv6len]
+					mask = value[net.IPv6len:]
+
+				default:
+					return nil, nil, nil, nil, fmt.Errorf("x509: IP constraint contained value of length %d", l)
+				}
+
+				if !isValidIPMask(mask) {
+					return nil, nil, nil, nil, fmt.Errorf("x509: IP constraint contained invalid mask %x", mask)
+				}
+
+				ips = append(ips, &net.IPNet{IP: net.IP(ip), Mask: net.IPMask(mask)})
+
+			case emailTag:
+				constraint := string(value)
+				if err := isIA5String(constraint); err != nil {
+					return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+				}
+
+				emails = append(emails, constraint)
+
+			case uriTag:
+				domain := string(value)
+				if err := isIA5String(domain); err != nil {
+					return nil, nil, nil, nil, errors.New("x509: invalid constraint value: " + err.Error())
+				}
+
+				uriDomains = append(uriDomains, domain)
+			}
+		}
+
+		return dnsNames, ips, emails, uriDomains, nil
+	}
+
+	out := &NameConstraints{}
+
 	var err error
-	if rest, err = asn1.Unmarshal(value, &constraints); err != nil {
-		return constraints, err
-	} else if len(rest) != 0 {
-		return constraints, errors.New("x509: trailing data after X.509 NameConstraints")
+	if out.PermittedDNSDomains, out.PermittedIPRanges, out.PermittedEmailAddresses, out.PermittedURIDomains, err = getValues(permitted); err != nil {
+		return nil, err
+	}
+	if out.ExcludedDNSDomains, out.ExcludedIPRanges, out.ExcludedEmailAddresses, out.ExcludedURIDomains, err = getValues(excluded); err != nil {
+		return nil, err
 	}

-	return constraints, nil
+	return out, nil
 }

-// convertIPNetSliceToIPNetPointerSlice converts []net.IPNet to []*net.IPNet.
-func convertIPNetSliceToIPNetPointerSlice(ipNetPointerSlice []net.IPNet) []*net.IPNet {
-	if ipNetPointerSlice == nil {
-		return nil
+// isValidIPMask reports whether mask consists of zero or more 1 bits, followed by zero bits.
+func isValidIPMask(mask []byte) bool {
+	seenZero := false
+
+	for _, b := range mask {
+		if seenZero {
+			if b != 0 {
+				return false
+			}
+
+			continue
+		}
+
+		switch b {
+		case 0x00, 0x80, 0xc0, 0xe0, 0xf0, 0xf8, 0xfc, 0xfe:
+			seenZero = true
+		case 0xff:
+		default:
+			return false
+		}
 	}
-	var ipNets []*net.IPNet
-	for _, ipNet := range ipNetPointerSlice {
-		ipNets = append(ipNets, &ipNet)
-	}
-	return ipNets
+
+	return true
 }
--- a/pkg/util/pki/nameconstraints_test.go
+++ b/pkg/util/pki/nameconstraints_test.go
@ -17,119 +17,206 @@ limitations under the License.
 package pki

 import (
+	"bytes"
+	"crypto/x509"
 	"crypto/x509/pkix"
 	"encoding/pem"
 	"fmt"
+	"net"
+	"strings"
 	"testing"

-	v1 "github.com/cert-manager/cert-manager/pkg/apis/certmanager/v1"
 	"github.com/stretchr/testify/assert"
 )

-func TestMarshalNameConstraints(t *testing.T) {
+// TestMarshalNameConstraints tests the MarshalNameConstraints function
+// To generate the expectedPEM, do something like this:
+// openssl req -new -key private_key.pem -out csr1.pem -subj "/CN=example.org" -config config.cnf
+//
+// where config.cnf is(replace nameConstraints with the values mentioned in the testcase):
+// [req]
+// default_bits        = 2048
+// prompt              = no
+// default_md          = sha256
+// req_extensions      = req_ext
+
+// [req_ext]
+// nameConstraints = critical,permitted;DNS:example.com,permitted;IP:192.168.1.0/255.255.255.0,permitted;email:user@example.com,permitted;URI:https://example.com,excluded;DNS:excluded.com,excluded;IP:192.168.0.0/255.255.255.0,excluded;email:user@excluded.com,excluded;URI:https://excluded.com
+func TestMarshalUnmarshalNameConstraints(t *testing.T) {
 	// Test data
 	testCases := []struct {
-		name           string
-		input          *v1.NameConstraints
-		expectedErr    error
-		expectedResult pkix.Extension
+		name        string
+		input       *NameConstraints
+		expectedErr error
+		expectedPEM string
 	}{
 		{
 			name: "Permitted constraints",
-			input: &v1.NameConstraints{
-				Critical: true,
-				Permitted: &v1.NameConstraintItem{
-					DNSDomains:     []string{"example.com"},
-					IPRanges:       []string{"192.168.0.1/24"},
-					EmailAddresses: []string{"user@example.com"},
-					URIDomains:     []string{"https://example.com"},
-				},
+			input: &NameConstraints{
+				PermittedDNSDomains:     []string{"example.com"},
+				PermittedIPRanges:       []*net.IPNet{{IP: net.IPv4(192, 168, 1, 0), Mask: net.IPv4Mask(255, 255, 255, 0)}},
+				PermittedEmailAddresses: []string{"user@example.com"},
+				PermittedURIDomains:     []string{"https://example.com"},
 			},
 			expectedErr: nil,
-			expectedResult: pkix.Extension{
-				Id:       OIDExtensionNameConstraints,
-				Critical: true,
-				Value:    []byte{0x30, 0x57, 0xa0, 0x3, 0x1, 0x1, 0xff, 0xa1, 0xf, 0x30, 0xd, 0x13, 0xb, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d, 0xa3, 0x10, 0x30, 0xe, 0x30, 0xc, 0x4, 0x4, 0xc0, 0xa8, 0x0, 0x0, 0x4, 0x4, 0xff, 0xff, 0xff, 0x0, 0xa5, 0x14, 0x30, 0x12, 0xc, 0x10, 0x75, 0x73, 0x65, 0x72, 0x40, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d, 0xa7, 0x17, 0x30, 0x15, 0x13, 0x13, 0x68, 0x74, 0x74, 0x70, 0x73, 0x3a, 0x2f, 0x2f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d},
-			},
+			// nameConstraints = critical,permitted;DNS:example.com,permitted;IP:192.168.1.0/255.255.255.0,permitted;email:user@example.com,permitted;URI:https://example.com
+			expectedPEM: `-----BEGIN CERTIFICATE REQUEST-----
+MIICwjCCAaoCAQAwFjEUMBIGA1UEAwwLZXhhbXBsZS5vcmcwggEiMA0GCSqGSIb3
+DQEBAQUAA4IBDwAwggEKAoIBAQCXy2XEkqESyr8/Y2x1A7AQaQlu3wry8QSmVwcb
+QYQ12xpA9derxd6f2qV+UZq/7tSwvaFfcdzbY4MTG+dq3QmlyXNEpVmzg/CbQJpQ
+ae/aacnb7MEvPGQpD8eHBt14QdoH0B5qreARa/IND4I+BazEAn9yAWc9o5BQMqPb
+5OGa5PMWR8apRyJrMfupMS0R3Nnmi+BP0fWepbOZHzRA6d2rbwkPBNBHQUyinxXS
+oIMg/WbrG0tbps8H6PTZg3Ki+XutPm5rFJ3CKVCzIfWLFIa3jHDNbeRc359EgBI9
+r1H7ecuPKxhxewugl0NirKIaEgzc609FIP++pmm3J5P10HF7AgMBAAGgZzBlBgkq
+hkiG9w0BCQ4xWDBWMFQGA1UdHgEB/wRKMEigRjANggtleGFtcGxlLmNvbTAKhwjA
+qAEA////ADASgRB1c2VyQGV4YW1wbGUuY29tMBWGE2h0dHBzOi8vZXhhbXBsZS5j
+b20wDQYJKoZIhvcNAQELBQADggEBAG4mhMt9iOGu1LInHW7oZyD8/FILhhafO7NF
+OLPLNK37yZmPWn3idIei/oooFspKspLSMqyCGgibr6jo613+6ENCHgzM/MUDrbfP
+i0VmriogMVB6qF73Qozylk1HPMcNe32aKsZygFAzKT586aO/F/exMx3NlKWa36m2
+rXKPgtD+T4R+hBxmsYAGVWFlvish+L1UIXtxddna4dYHSbLBz+uZXzrxyuJgSQV3
+2wF++GJ1zOi47CEUukqQOAZKPCE59erY+vUas8hwMTHMT22D5ZGbdjg6qVBCQdqW
+Nu6OGP4KFgW0HWyeGeNBzioGUeyIHFKILLvj2n94WJMqXNyT5eE=
+-----END CERTIFICATE REQUEST-----`,
 		},
 		{
 			name: "Mixed constraints",
-			input: &v1.NameConstraints{
-				Critical: true,
-				Permitted: &v1.NameConstraintItem{
-					DNSDomains:     []string{"example.com"},
-					IPRanges:       []string{"192.168.0.1/24"},
-					EmailAddresses: []string{"user@example.com"},
-					URIDomains:     []string{"https://example.com"},
-				},
-				Excluded: &v1.NameConstraintItem{
-					DNSDomains:     []string{"excluded.com"},
-					IPRanges:       []string{"192.168.0.0/24"},
-					EmailAddresses: []string{"user@excluded.com"},
-					URIDomains:     []string{"https://excluded.com"},
-				},
+			input: &NameConstraints{
+				PermittedDNSDomains:     []string{"example.com"},
+				PermittedIPRanges:       []*net.IPNet{{IP: net.IPv4(192, 168, 1, 0), Mask: net.IPv4Mask(255, 255, 255, 0)}},
+				PermittedEmailAddresses: []string{"user@example.com"},
+				PermittedURIDomains:     []string{"https://example.com"},
+				ExcludedDNSDomains:      []string{"excluded.com"},
+				ExcludedIPRanges:        []*net.IPNet{{IP: net.IPv4(192, 168, 0, 0), Mask: net.IPv4Mask(255, 255, 255, 0)}},
+				ExcludedEmailAddresses:  []string{"user@excluded.com"},
+				ExcludedURIDomains:      []string{"https://excluded.com"},
 			},
 			expectedErr: nil,
-			expectedResult: pkix.Extension{
-				Id:       OIDExtensionNameConstraints,
-				Critical: true,
-				Value:    []byte{0x30, 0x81, 0xac, 0xa0, 0x3, 0x1, 0x1, 0xff, 0xa1, 0xf, 0x30, 0xd, 0x13, 0xb, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d, 0xa2, 0x10, 0x30, 0xe, 0x13, 0xc, 0x65, 0x78, 0x63, 0x6c, 0x75, 0x64, 0x65, 0x64, 0x2e, 0x63, 0x6f, 0x6d, 0xa3, 0x10, 0x30, 0xe, 0x30, 0xc, 0x4, 0x4, 0xc0, 0xa8, 0x0, 0x0, 0x4, 0x4, 0xff, 0xff, 0xff, 0x0, 0xa4, 0x10, 0x30, 0xe, 0x30, 0xc, 0x4, 0x4, 0xc0, 0xa8, 0x0, 0x0, 0x4, 0x4, 0xff, 0xff, 0xff, 0x0, 0xa5, 0x14, 0x30, 0x12, 0xc, 0x10, 0x75, 0x73, 0x65, 0x72, 0x40, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d, 0xa6, 0x15, 0x30, 0x13, 0xc, 0x11, 0x75, 0x73, 0x65, 0x72, 0x40, 0x65, 0x78, 0x63, 0x6c, 0x75, 0x64, 0x65, 0x64, 0x2e, 0x63, 0x6f, 0x6d, 0xa7, 0x17, 0x30, 0x15, 0x13, 0x13, 0x68, 0x74, 0x74, 0x70, 0x73, 0x3a, 0x2f, 0x2f, 0x65, 0x78, 0x61, 0x6d, 0x70, 0x6c, 0x65, 0x2e, 0x63, 0x6f, 0x6d, 0xa8, 0x18, 0x30, 0x16, 0x13, 0x14, 0x68, 0x74, 0x74, 0x70, 0x73, 0x3a, 0x2f, 0x2f, 0x65, 0x78, 0x63, 0x6c, 0x75, 0x64, 0x65, 0x64, 0x2e, 0x63, 0x6f, 0x6d},
-			},
-		},
-		{
-			name:        "Empty constraints",
-			input:       &v1.NameConstraints{},
-			expectedErr: nil,
-			expectedResult: pkix.Extension{
-				Id:       OIDExtensionNameConstraints,
-				Critical: true,
-				Value:    []byte{0x30, 0x0},
-			},
+			// nameConstraints = critical,permitted;DNS:example.com,permitted;IP:192.168.1.0/255.255.255.0,permitted;email:user@example.com,permitted;URI:https://example.com,excluded;DNS:excluded.com,excluded;IP:192.168.0.0/255.255.255.0,excluded;email:user@excluded.com,excluded;URI:https://excluded.com
+			expectedPEM: `-----BEGIN CERTIFICATE REQUEST-----
+MIIDFDCCAfwCAQAwFjEUMBIGA1UEAwwLZXhhbXBsZS5vcmcwggEiMA0GCSqGSIb3
+DQEBAQUAA4IBDwAwggEKAoIBAQCXy2XEkqESyr8/Y2x1A7AQaQlu3wry8QSmVwcb
+QYQ12xpA9derxd6f2qV+UZq/7tSwvaFfcdzbY4MTG+dq3QmlyXNEpVmzg/CbQJpQ
+ae/aacnb7MEvPGQpD8eHBt14QdoH0B5qreARa/IND4I+BazEAn9yAWc9o5BQMqPb
+5OGa5PMWR8apRyJrMfupMS0R3Nnmi+BP0fWepbOZHzRA6d2rbwkPBNBHQUyinxXS
+oIMg/WbrG0tbps8H6PTZg3Ki+XutPm5rFJ3CKVCzIfWLFIa3jHDNbeRc359EgBI9
+r1H7ecuPKxhxewugl0NirKIaEgzc609FIP++pmm3J5P10HF7AgMBAAGggbgwgbUG
+CSqGSIb3DQEJDjGBpzCBpDCBoQYDVR0eAQH/BIGWMIGToEYwDYILZXhhbXBsZS5j
+b20wCocIwKgBAP///wAwEoEQdXNlckBleGFtcGxlLmNvbTAVhhNodHRwczovL2V4
+YW1wbGUuY29toUkwDoIMZXhjbHVkZWQuY29tMAqHCMCoAAD///8AMBOBEXVzZXJA
+ZXhjbHVkZWQuY29tMBaGFGh0dHBzOi8vZXhjbHVkZWQuY29tMA0GCSqGSIb3DQEB
+CwUAA4IBAQCEBMhHw4wbP+aBDViKtvpaMar3ZWYVuV7j2qck5yDlXYGhpTQlwg5C
+XEIP7zKM1yGgCITEpA5KML4PV55rEU6TCa2E9oQfy51QQcmSTGYLjolOahpALwzn
+38n9e4WBiHwDVMVsSR5Zhw2dy9tqSslAHjp3TFFCcx7gaKoTs6OOJzv784PzX7xp
+Vbm68hvWwkdD0lwGJlNkykPmNGxpC1kVn6L1p7LUubWOkkqBHwgny+DW3fPtKpvO
+AHpUq+yDI0oaIz6BIfn2Vs7jUSXCZIoQBwajALg9kGqh3O6+ds617+AzxGXk0LBQ
+0GsHVWCimOgcqgU5Qg4K6iMUtlDU2WAW
+-----END CERTIFICATE REQUEST-----`,
 		},
 		{
 			name: "Excluded constraints",
-			input: &v1.NameConstraints{
-				Excluded: &v1.NameConstraintItem{
-					DNSDomains:     []string{"excluded.com"},
-					IPRanges:       []string{"192.168.0.0/24"},
-					EmailAddresses: []string{"user@excluded.com"},
-					URIDomains:     []string{"https://excluded.com"},
-				},
+			input: &NameConstraints{
+				ExcludedDNSDomains:     []string{"excluded.com"},
+				ExcludedIPRanges:       []*net.IPNet{{IP: net.IPv4(192, 168, 0, 0), Mask: net.IPv4Mask(255, 255, 255, 0)}},
+				ExcludedEmailAddresses: []string{"user@excluded.com"},
+				ExcludedURIDomains:     []string{"https://excluded.com"},
 			},
 			expectedErr: nil,
-			expectedResult: pkix.Extension{
-				Id:       OIDExtensionNameConstraints,
-				Critical: true,
-				Value:    []byte{0x30, 0x55, 0xa2, 0x10, 0x30, 0xe, 0x13, 0xc, 0x65, 0x78, 0x63, 0x6c, 0x75, 0x64, 0x65, 0x64, 0x2e, 0x63, 0x6f, 0x6d, 0xa4, 0x10, 0x30, 0xe, 0x30, 0xc, 0x4, 0x4, 0xc0, 0xa8, 0x0, 0x0, 0x4, 0x4, 0xff, 0xff, 0xff, 0x0, 0xa6, 0x15, 0x30, 0x13, 0xc, 0x11, 0x75, 0x73, 0x65, 0x72, 0x40, 0x65, 0x78, 0x63, 0x6c, 0x75, 0x64, 0x65, 0x64, 0x2e, 0x63, 0x6f, 0x6d, 0xa8, 0x18, 0x30, 0x16, 0x13, 0x14, 0x68, 0x74, 0x74, 0x70, 0x73, 0x3a, 0x2f, 0x2f, 0x65, 0x78, 0x63, 0x6c, 0x75, 0x64, 0x65, 0x64, 0x2e, 0x63, 0x6f, 0x6d},
-			},
-		},
-		{
-			name: "Invalid NameConstraints",
-			input: &v1.NameConstraints{
-				Excluded: &v1.NameConstraintItem{
-					IPRanges: []string{"invalidCIDR"},
-				},
-			},
-			expectedErr:    fmt.Errorf("invalid CIDR address: invalidCIDR"),
-			expectedResult: pkix.Extension{},
+			// nameConstraints = critical,excluded;DNS:excluded.com,excluded;IP:192.168.0.0/255.255.255.0,excluded;email:user@excluded.com,excluded;URI:https://excluded.com
+			expectedPEM: `-----BEGIN CERTIFICATE REQUEST-----
+MIICxTCCAa0CAQAwFjEUMBIGA1UEAwwLZXhhbXBsZS5vcmcwggEiMA0GCSqGSIb3
+DQEBAQUAA4IBDwAwggEKAoIBAQCXy2XEkqESyr8/Y2x1A7AQaQlu3wry8QSmVwcb
+QYQ12xpA9derxd6f2qV+UZq/7tSwvaFfcdzbY4MTG+dq3QmlyXNEpVmzg/CbQJpQ
+ae/aacnb7MEvPGQpD8eHBt14QdoH0B5qreARa/IND4I+BazEAn9yAWc9o5BQMqPb
+5OGa5PMWR8apRyJrMfupMS0R3Nnmi+BP0fWepbOZHzRA6d2rbwkPBNBHQUyinxXS
+oIMg/WbrG0tbps8H6PTZg3Ki+XutPm5rFJ3CKVCzIfWLFIa3jHDNbeRc359EgBI9
+r1H7ecuPKxhxewugl0NirKIaEgzc609FIP++pmm3J5P10HF7AgMBAAGgajBoBgkq
+hkiG9w0BCQ4xWzBZMFcGA1UdHgEB/wRNMEuhSTAOggxleGNsdWRlZC5jb20wCocI
+wKgAAP///wAwE4ERdXNlckBleGNsdWRlZC5jb20wFoYUaHR0cHM6Ly9leGNsdWRl
+ZC5jb20wDQYJKoZIhvcNAQELBQADggEBABQGXpovgvk8Ag+FSv0fVcHAalNrNHkL
+8kJmLjJKMjYhrI4KwkrVDwRvm96ueSfDYLMu56Vd/cLzVbqgFNEeGY+7/fwty/PK
+PwjPjMC3i09D1JZjrpc2gpIxmrwP/vf1DpxPUVF5wzE9xRiYvKu3/ZHy1d3FYYgT
+cpf+w2cqzt2J8imToJUtjbVTACqBwhwRrn7xyP0trvAo1tfHS4qK7urJxbuT+OAf
+mYfy24EOPhpvyIyYS+lbkc9wdYT4BSIjQCFNAjcBD+/04SkHgtbFLy0i8xsKcfOy
+3haWYno4zTZ0v6LAdn3CgtbvUtFBfIMjmEfsldVZpIbpuSEqjMFDGls=
+-----END CERTIFICATE REQUEST-----`,
 		},
 	}

+	compareIPArrays := func(a, b []*net.IPNet) bool {
+		if len(a) != len(b) {
+			return false
+		}
+
+		for i, ipNet := range a {
+			if !ipNet.IP.Equal(b[i].IP) || !bytes.Equal(ipNet.Mask, b[i].Mask) {
+				return false
+			}
+		}
+
+		return true
+	}
+
 	for _, tc := range testCases {
-		t.Run(tc.name, func(t *testing.T) {
-			result, err := MarshalNameConstraints(tc.input)
+		t.Run(tc.name+"_marshal", func(t *testing.T) {
+			expectedResult, err := getExtensionFromPem(tc.expectedPEM)
+			assert.NoError(t, err)
+			result, err := MarshalNameConstraints(tc.input, expectedResult.Critical)
 			if tc.expectedErr != nil {
 				assert.Error(t, err)
 				assert.EqualError(t, err, tc.expectedErr.Error())
 			} else {
 				assert.NoError(t, err)
-				assert.Equal(t, tc.expectedResult.Id, result.Id)
-				assert.Equal(t, tc.expectedResult.Critical, result.Critical)
+				assert.Equal(t, expectedResult.Id, result.Id)
+				assert.Equal(t, expectedResult.Critical, result.Critical)
+				assert.Equal(t, expectedResult.Value, result.Value)
+			}
+		})

-				expectedPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE EXTENSION", Bytes: tc.expectedResult.Value})
-				actualPEM := pem.EncodeToMemory(&pem.Block{Type: "CERTIFICATE EXTENSION", Bytes: result.Value})
-				assert.Equal(t, expectedPEM, actualPEM)
+		t.Run(tc.name+"_unmarshal", func(t *testing.T) {
+			expectedResult, err := getExtensionFromPem(tc.expectedPEM)
+			assert.NoError(t, err)
+			constraints, err := UnmarshalNameConstraints(expectedResult.Value)
+			if tc.expectedErr != nil {
+				assert.Error(t, err)
+				assert.EqualError(t, err, tc.expectedErr.Error())
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, constraints.ExcludedDNSDomains, tc.input.ExcludedDNSDomains)
+				assert.Equal(t, constraints.ExcludedEmailAddresses, tc.input.ExcludedEmailAddresses)
+				assert.True(t, compareIPArrays(constraints.ExcludedIPRanges, tc.input.ExcludedIPRanges))
+				assert.Equal(t, constraints.ExcludedURIDomains, tc.input.ExcludedURIDomains)
+				assert.Equal(t, constraints.PermittedDNSDomains, tc.input.PermittedDNSDomains)
+				assert.Equal(t, constraints.PermittedEmailAddresses, tc.input.PermittedEmailAddresses)
+				assert.True(t, compareIPArrays(constraints.PermittedIPRanges, tc.input.PermittedIPRanges))
+				assert.Equal(t, constraints.PermittedURIDomains, tc.input.PermittedURIDomains)
 			}
 		})
 	}
 }
+
+func getExtensionFromPem(pemData string) (pkix.Extension, error) {
+	if pemData == "" {
+		return pkix.Extension{}, nil
+	}
+	pemData = strings.TrimSpace(pemData)
+	fmt.Println(pemData)
+	csrPEM := []byte(pemData)
+
+	block, _ := pem.Decode(csrPEM)
+	if block == nil || block.Type != "CERTIFICATE REQUEST" {
+		return pkix.Extension{}, fmt.Errorf("Failed to decode PEM block or the type is not 'CERTIFICATE REQUEST'")
+	}
+
+	csr, err := x509.ParseCertificateRequest(block.Bytes)
+	if err != nil {
+		return pkix.Extension{}, fmt.Errorf("Error parsing CSR: %v", err)
+	}
+
+	for _, ext := range csr.Extensions {
+		if ext.Id.Equal(OIDExtensionNameConstraints) {
+			return ext, nil
+		}
+	}
+
+	return pkix.Extension{}, nil
+}