Resolve some TODOs

cert/ca_pool.go

@@ -3,6 +3,8 @@ package cert
 import (
 	"errors"
 	"fmt"
+	"net/netip"
+	"slices"
 	"strings"
 	"time"
 )
@@ -12,7 +14,7 @@ type CAPool struct {
 	certBlocklist map[string]struct{}
 }
 
-// NewCAPool creates a CAPool
+// NewCAPool creates an empty CAPool
 func NewCAPool() *CAPool {
 	ca := CAPool{
 		CAs:           make(map[string]*CachedCertificate),
@@ -51,7 +53,7 @@ func NewCAPoolFromPEM(caPEMs []byte) (*CAPool, error) {
 	return pool, nil
 }
 
-// AddCAFromPEM verifies a Nebula CA certificate and adds it to the pool
+// AddCAFromPEM verifies a Nebula CA certificate and adds it to the pool.
 // Only the first pem encoded object will be consumed, any remaining bytes are returned.
 // Parsed certificates will be verified and must be a CA
 func (ncp *CAPool) AddCAFromPEM(pemBytes []byte) ([]byte, error) {
@@ -68,7 +70,7 @@ func (ncp *CAPool) AddCAFromPEM(pemBytes []byte) ([]byte, error) {
 	return pemBytes, nil
 }
 
-// TODO:
+// AddCA verifies a Nebula CA certificate and adds it to the pool.
 func (ncp *CAPool) AddCA(c Certificate) error {
 	if !c.IsCA() {
 		return fmt.Errorf("%s: %w", c.Name(), ErrNotCA)
@@ -78,7 +80,7 @@ func (ncp *CAPool) AddCA(c Certificate) error {
 		return fmt.Errorf("%s: %w", c.Name(), ErrNotSelfSigned)
 	}
 
-	sum, err := c.Sha256Sum()
+	sum, err := c.Fingerprint()
 	if err != nil {
 		return fmt.Errorf("could not calculate shasum for provided CA; error: %s; %s", err, c.Name())
 	}
@@ -112,9 +114,10 @@ func (ncp *CAPool) ResetCertBlocklist() {
 	ncp.certBlocklist = make(map[string]struct{})
 }
 
-// TODO:
-func (ncp *CAPool) IsBlocklisted(sha string) bool {
-	if _, ok := ncp.certBlocklist[sha]; ok {
+// IsBlocklisted tests the provided fingerprint against the pools blocklist.
+// Returns true if the fingerprint is blocked.
+func (ncp *CAPool) IsBlocklisted(fingerprint string) bool {
+	if _, ok := ncp.certBlocklist[fingerprint]; ok {
 		return true
 	}
 
@@ -125,7 +128,7 @@ func (ncp *CAPool) IsBlocklisted(sha string) bool {
 // If the certificate is valid then the returned CachedCertificate can be used in subsequent verification attempts
 // to increase performance.
 func (ncp *CAPool) VerifyCertificate(now time.Time, c Certificate) (*CachedCertificate, error) {
-	sha, err := c.Sha256Sum()
+	sha, err := c.Fingerprint()
 	if err != nil {
 		return nil, fmt.Errorf("could not calculate shasum to verify: %w", err)
 	}
@@ -141,14 +144,16 @@ func (ncp *CAPool) VerifyCertificate(now time.Time, c Certificate) (*CachedCerti
 		ShaSum:         sha,
 		signerShaSum:   signer.ShaSum,
 	}
-	
+
 	for _, g := range c.Groups() {
 		cc.InvertedGroups[g] = struct{}{}
 	}
 
 	return &cc, nil
 }
 
+// VerifyCachedCertificate is the same as VerifyCertificate other than it operates on a pre-verified structure and
+// is a cheaper operation to perform as a result.
 func (ncp *CAPool) VerifyCachedCertificate(now time.Time, c *CachedCertificate) error {
 	_, err := ncp.verify(c.Certificate, now, c.ShaSum, c.signerShaSum)
 	return err
@@ -174,6 +179,9 @@ func (ncp *CAPool) verify(c Certificate, now time.Time, certSha string, signerSh
 
 	// If we are checking a cached certificate then we can bail early here
 	// Either the root is no longer trusted or everything is fine
+	//TODO: this is slightly different than v1.9.3 and earlier where we were matching the public key
+	// The reason to switch is that the public key can be reused and the constraints in the ca can change
+	// but there may be history here, double check
 	if len(signerSha) > 0 {
 		if signerSha != signer.ShaSum {
 			return nil, ErrSignatureMismatch
@@ -184,7 +192,7 @@ func (ncp *CAPool) verify(c Certificate, now time.Time, certSha string, signerSh
 		return nil, ErrSignatureMismatch
 	}
 
-	err = c.CheckRootConstraints(signer.Certificate)
+	err = CheckCAConstraints(signer.Certificate, c)
 	if err != nil {
 		return nil, err
 	}
@@ -219,3 +227,70 @@ func (ncp *CAPool) GetFingerprints() []string {
 
 	return fp
 }
+
+// CheckCAConstraints returns an error if the sub certificate violates constraints present in the signer certificate.
+func CheckCAConstraints(signer Certificate, sub Certificate) error {
+	return checkCAConstraints(signer, sub.NotAfter(), sub.NotBefore(), sub.Groups(), sub.Networks(), sub.UnsafeNetworks())
+}
+
+// checkCAConstraints is a very generic function allowing both Certificates and TBSCertificates to be tested.
+func checkCAConstraints(signer Certificate, notBefore, notAfter time.Time, groups []string, networks, unsafeNetworks []netip.Prefix) error {
+	// Make sure this cert wasn't valid before the root
+	if signer.NotAfter().Before(notAfter) {
+		return fmt.Errorf("certificate expires after signing certificate")
+	}
+
+	// Make sure this cert isn't valid after the root
+	if signer.NotBefore().After(notBefore) {
+		return fmt.Errorf("certificate is valid before the signing certificate")
+	}
+
+	// If the signer has a limited set of groups make sure the cert only contains a subset
+	signerGroups := signer.Groups()
+	if len(signerGroups) > 0 {
+		for _, g := range groups {
+			if !slices.Contains(signerGroups, g) {
+				//TODO: since we no longer pre-compute the inverted groups then this is kind of slow
+				return fmt.Errorf("certificate contained a group not present on the signing ca: %s", g)
+			}
+		}
+	}
+
+	// If the signer has a limited set of ip ranges to issue from make sure the cert only contains a subset
+	signingNetworks := signer.Networks()
+	if len(signingNetworks) > 0 {
+		for _, subNetwork := range networks {
+			found := false
+			for _, signingNetwork := range signingNetworks {
+				if signingNetwork.Contains(subNetwork.Addr()) && signingNetwork.Bits() <= subNetwork.Bits() {
+					found = true
+					break
+				}
+			}
+
+			if !found {
+				return fmt.Errorf("certificate contained an ip assignment outside the limitations of the signing ca: %s", subNetwork.String())
+			}
+		}
+	}
+
+	// If the signer has a limited set of subnet ranges to issue from make sure the cert only contains a subset
+	signingUnsafeNetworks := signer.UnsafeNetworks()
+	if len(signingUnsafeNetworks) > 0 {
+		for _, subUnsafeNetwork := range unsafeNetworks {
+			found := false
+			for _, caNetwork := range signingUnsafeNetworks {
+				if caNetwork.Contains(subUnsafeNetwork.Addr()) && caNetwork.Bits() <= subUnsafeNetwork.Bits() {
+					found = true
+					break
+				}
+			}
+
+			if !found {
+				return fmt.Errorf("certificate contained a subnet assignment outside the limitations of the signing ca: %s", subUnsafeNetwork.String())
+			}
+		}
+	}
+
+	return nil
+}

cert/cert.go

@@ -13,7 +13,9 @@ const (
 )
 
 type Certificate interface {
-	//TODO: describe this
+	// Version defines the underlying certificate structure and wire protocol version
+	// Version1 certificates are ipv4 only and uses protobuf serialization
+	// Version2 certificates are ipv4 or ipv6 and uses asn.1 serialization
 	Version() Version
 
 	// Name is the human-readable name that identifies this certificate.
@@ -65,20 +67,14 @@ type Certificate interface {
 	// computed signature. A true result means this certificate has not been tampered with.
 	CheckSignature(signingPublicKey []byte) bool
 
-	// Sha256Sum returns the hex encoded sha256 sum of the certificate.
+	// Fingerprint returns the hex encoded sha256 sum of the certificate.
 	// This acts as a unique fingerprint and can be used to blocklist certificates.
-	Sha256Sum() (string, error)
+	Fingerprint() (string, error)
 
 	// Expired tests if the certificate is valid for the provided time.
 	Expired(t time.Time) bool
 
-	// CheckRootConstraints tests if the certificate meets all constraints in the
-	// signing certificate, returning the first violated constraint or nil if the
-	// certificate conforms to all constraints.
-	//TODO: feels better to have this on the CAPool I think
-	CheckRootConstraints(signer Certificate) error
-
-	//TODO
+	// VerifyPrivateKey returns an error if the private key is not a pair with the certificates public key.
 	VerifyPrivateKey(curve Curve, privateKey []byte) error
 
 	// Marshal will return the byte representation of this certificate
@@ -88,18 +84,17 @@ type Certificate interface {
 	// MarshalForHandshakes prepares the bytes needed to use directly in a handshake
 	MarshalForHandshakes() ([]byte, error)
 
-	// MarshalToPEM will return a PEM encoded representation of this certificate
+	// MarshalPEM will return a PEM encoded representation of this certificate
 	// This is primarily the format stored on disk
-	//TODO: MarshalPEM?
-	MarshalToPEM() ([]byte, error)
+	MarshalPEM() ([]byte, error)
 
 	// MarshalJSON will return the json representation of this certificate
 	MarshalJSON() ([]byte, error)
 
 	// String will return a human-readable representation of this certificate
 	String() string
 
-	//TODO
+	// Copy creates a copy of the certificate
 	Copy() Certificate
 }
 
@@ -112,7 +107,7 @@ type CachedCertificate struct {
 	signerShaSum   string
 }
 
-// TODO:
+// UnmarshalCertificate will attempt to unmarshal a wire protocol level certificate.
 func UnmarshalCertificate(b []byte) (Certificate, error) {
 	c, err := unmarshalCertificateV1(b, true)
 	if err != nil {
@@ -121,7 +116,9 @@ func UnmarshalCertificate(b []byte) (Certificate, error) {
 	return c, nil
 }
 
-// TODO:
+// UnmarshalCertificateFromHandshake will attempt to unmarshal a certificate received in a handshake.
+// Handshakes save space by placing the peers public key in a different part of the packet, we have to
+// reassemble the actual certificate structure with that in mind.
 func UnmarshalCertificateFromHandshake(b []byte, publicKey []byte) (Certificate, error) {
 	c, err := unmarshalCertificateV1(b, false)
 	if err != nil {

cert/cert_test.go

@@ -212,7 +212,7 @@ func TestNebulaCertificate_Verify(t *testing.T) {
 	caPool := NewCAPool()
 	assert.NoError(t, caPool.AddCA(ca))
 
-	f, err := c.Sha256Sum()
+	f, err := c.Fingerprint()
 	assert.Nil(t, err)
 	caPool.BlocklistFingerprint(f)
 
@@ -235,7 +235,7 @@ func TestNebulaCertificate_Verify(t *testing.T) {
 	ca, _, caKey, err = newTestCaCert(time.Now(), time.Now().Add(10*time.Minute), nil, nil, []string{"test1", "test2"})
 	assert.Nil(t, err)
 
-	caPem, err := ca.MarshalToPEM()
+	caPem, err := ca.MarshalPEM()
 	assert.Nil(t, err)
 
 	caPool = NewCAPool()
@@ -264,7 +264,7 @@ func TestNebulaCertificate_VerifyP256(t *testing.T) {
 	caPool := NewCAPool()
 	assert.NoError(t, caPool.AddCA(ca))
 
-	f, err := c.Sha256Sum()
+	f, err := c.Fingerprint()
 	assert.Nil(t, err)
 	caPool.BlocklistFingerprint(f)
 
@@ -287,7 +287,7 @@ func TestNebulaCertificate_VerifyP256(t *testing.T) {
 	ca, _, caKey, err = newTestCaCertP256(time.Now(), time.Now().Add(10*time.Minute), nil, nil, []string{"test1", "test2"})
 	assert.Nil(t, err)
 
-	caPem, err := ca.MarshalToPEM()
+	caPem, err := ca.MarshalPEM()
 	assert.Nil(t, err)
 
 	caPool = NewCAPool()
@@ -312,7 +312,7 @@ func TestNebulaCertificate_Verify_IPs(t *testing.T) {
 	ca, _, caKey, err := newTestCaCert(time.Now(), time.Now().Add(10*time.Minute), []netip.Prefix{caIp1, caIp2}, nil, []string{"test"})
 	assert.Nil(t, err)
 
-	caPem, err := ca.MarshalToPEM()
+	caPem, err := ca.MarshalPEM()
 	assert.Nil(t, err)
 
 	caPool := NewCAPool()
@@ -385,7 +385,7 @@ func TestNebulaCertificate_Verify_Subnets(t *testing.T) {
 	ca, _, caKey, err := newTestCaCert(time.Now(), time.Now().Add(10*time.Minute), nil, []netip.Prefix{caIp1, caIp2}, []string{"test"})
 	assert.Nil(t, err)
 
-	caPem, err := ca.MarshalToPEM()
+	caPem, err := ca.MarshalPEM()
 	assert.Nil(t, err)
 
 	caPool := NewCAPool()
@@ -643,7 +643,7 @@ func newTestCaCertP256(before, after time.Time, ips, subnets []netip.Prefix, gro
 }
 
 func newTestCert(ca *certificateV1, key []byte, before, after time.Time, ips, subnets []netip.Prefix, groups []string) (*certificateV1, []byte, []byte, error) {
-	issuer, err := ca.Sha256Sum()
+	issuer, err := ca.Fingerprint()
 	if err != nil {
 		return nil, nil, nil, err
 	}