Merge branch 'cert-v2' into cert-v2-sign-rules

Author: nbrownus

cert/ca_pool.go

@@ -213,7 +213,7 @@ func (ncp *CAPool) GetCAForCert(c Certificate) (*CachedCertificate, error) {
 		return signer, nil
 	}
 
-	return nil, fmt.Errorf("could not find ca for the certificate")
+	return nil, ErrCaNotFound
 }
 
 // GetFingerprints returns an array of trusted CA fingerprints

cert/cert_v2.go

@@ -137,8 +137,10 @@ func (c *certificateV2) Fingerprint() (string, error) {
 }
 
 func (c *certificateV2) CheckSignature(key []byte) bool {
+	if len(c.rawDetails) == 0 {
+		return false
+	}
 	b := make([]byte, len(c.rawDetails)+1+len(c.publicKey))
-	//TODO: double check this, panic on empty raw details
 	copy(b, c.rawDetails)
 	b[len(c.rawDetails)] = byte(c.curve)
 	copy(b[len(c.rawDetails)+1:], c.publicKey)
@@ -147,7 +149,6 @@ func (c *certificateV2) CheckSignature(key []byte) bool {
 	case Curve_CURVE25519:
 		return ed25519.Verify(key, b, c.signature)
 	case Curve_P256:
-		//TODO: NewPublicKey
 		x, y := elliptic.Unmarshal(elliptic.P256(), key)
 		pubKey := &ecdsa.PublicKey{Curve: elliptic.P256(), X: x, Y: y}
 		hashed := sha256.Sum256(b)
@@ -229,12 +230,14 @@ func (c *certificateV2) String() string {
 }
 
 func (c *certificateV2) MarshalForHandshakes() ([]byte, error) {
+	if c.rawDetails == nil {
+		return nil, ErrEmptyRawDetails
+	}
 	var b cryptobyte.Builder
 	// Outermost certificate
 	b.AddASN1(asn1.SEQUENCE, func(b *cryptobyte.Builder) {
 
 		// Add the cert details which is already marshalled
-		//TODO: panic on nil rawDetails
 		b.AddBytes(c.rawDetails)
 
 		// Skipping the curve and public key since those come across in a different part of the handshake
@@ -249,6 +252,9 @@ func (c *certificateV2) MarshalForHandshakes() ([]byte, error) {
 }
 
 func (c *certificateV2) Marshal() ([]byte, error) {
+	if c.rawDetails == nil {
+		return nil, ErrEmptyRawDetails
+	}
 	var b cryptobyte.Builder
 	// Outermost certificate
 	b.AddASN1(asn1.SEQUENCE, func(b *cryptobyte.Builder) {
@@ -447,13 +453,11 @@ func (c *certificateV2) validate() error {
 func (c *certificateV2) marshalForSigning() ([]byte, error) {
 	d, err := c.details.Marshal()
 	if err != nil {
-		//TODO: annotate?
-		return nil, err
+		return nil, fmt.Errorf("marshalling certificate details failed: %w", err)
 	}
 	c.rawDetails = d
 
 	b := make([]byte, len(c.rawDetails)+1+len(c.publicKey))
-	//TODO: double check this
 	copy(b, c.rawDetails)
 	b[len(c.rawDetails)] = byte(c.curve)
 	copy(b[len(c.rawDetails)+1:], c.publicKey)
@@ -587,6 +591,10 @@ func unmarshalCertificateV2(b []byte, publicKey []byte, curve Curve) (*certifica
 		return nil, ErrBadFormat
 	}
 
+	if len(rawPublicKey) == 0 {
+		return nil, ErrBadFormat
+	}
+
 	// Grab the signature
 	var rawSignature cryptobyte.String
 	if !input.ReadASN1(&rawSignature, TagCertSignature) || rawSignature.Empty() {

cert/cert_v2_test.go

@@ -3,6 +3,7 @@ package cert
 import (
 	"crypto/ed25519"
 	"crypto/rand"
+	"encoding/hex"
 	"net/netip"
 	"slices"
 	"testing"
@@ -224,3 +225,43 @@ func TestUnmarshalCertificateV2(t *testing.T) {
 	_, err := unmarshalCertificateV2(data, nil, Curve_CURVE25519)
 	assert.EqualError(t, err, "bad wire format")
 }
+
+func TestCertificateV2_marshalForSigningStability(t *testing.T) {
+	before := time.Date(1996, time.May, 5, 0, 0, 0, 0, time.UTC)
+	after := before.Add(time.Second * 60).Round(time.Second)
+	pubKey := []byte("1234567890abcedfghij1234567890ab")
+
+	nc := certificateV2{
+		details: detailsV2{
+			name: "testing",
+			networks: []netip.Prefix{
+				mustParsePrefixUnmapped("10.1.1.2/16"),
+				mustParsePrefixUnmapped("10.1.1.1/24"),
+			},
+			unsafeNetworks: []netip.Prefix{
+				mustParsePrefixUnmapped("9.1.1.3/16"),
+				mustParsePrefixUnmapped("9.1.1.2/24"),
+			},
+			groups:    []string{"test-group1", "test-group2", "test-group3"},
+			notBefore: before,
+			notAfter:  after,
+			isCA:      false,
+			issuer:    "1234567890abcdef1234567890abcdef",
+		},
+		signature: []byte("1234567890abcdef1234567890abcdef"),
+		publicKey: pubKey,
+	}
+
+	const expectedRawDetailsStr = "a070800774657374696e67a10e04050a0101021004050a01010118a20e0405090101031004050901010218a3270c0b746573742d67726f7570310c0b746573742d67726f7570320c0b746573742d67726f7570338504318bef808604318befbc87101234567890abcdef1234567890abcdef"
+	expectedRawDetails, err := hex.DecodeString(expectedRawDetailsStr)
+	require.NoError(t, err)
+
+	db, err := nc.details.Marshal()
+	require.NoError(t, err)
+	assert.Equal(t, expectedRawDetails, db)
+
+	expectedForSigning, err := hex.DecodeString(expectedRawDetailsStr + "00313233343536373839306162636564666768696a313233343536373839306162")
+	b, err := nc.marshalForSigning()
+	require.NoError(t, err)
+	assert.Equal(t, expectedForSigning, b)
+}

cert/errors.go

@@ -19,6 +19,7 @@ var (
 	ErrPublicPrivateCurveMismatch = errors.New("public key does not match private key curve")
 	ErrPublicPrivateKeyMismatch   = errors.New("public key and private key are not a pair")
 	ErrPrivateKeyEncrypted        = errors.New("private key must be decrypted")
+	ErrCaNotFound                 = errors.New("could not find ca for the certi
 
 	ErrInvalidPEMBlock                   = errors.New("input did not contain a valid PEM encoded block")
 	ErrInvalidPEMCertificateBanner       = errors.New("bytes did not contain a proper certificate banner")
@@ -30,8 +31,9 @@ var (
 	ErrNoPeerStaticKey = errors.New("no peer static key was present")
 	ErrNoPayload       = errors.New("provided payload was empty")
 
-	ErrMissingDetails = errors.New("certificate did not contain details")
-	ErrEmptySignature = errors.New("empty signature")
+	ErrMissingDetails  = errors.New("certificate did not contain details")
+	ErrEmptySignature  = errors.New("empty signature")
+	ErrEmptyRawDetails = errors.New("empty rawDetails not allowed")
 )
 
 type ErrInvalidCertificateProperties struct {

cert/sign_test.go

@@ -73,6 +73,7 @@ func TestCertificateV1_SignP256(t *testing.T) {
 	}
 
 	priv, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
+	assert.NoError(t, err)
 	pub := elliptic.Marshal(elliptic.P256(), priv.PublicKey.X, priv.PublicKey.Y)
 	rawPriv := priv.D.FillBytes(make([]byte, 32))
 

cmd/nebula-cert/verify.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"errors"
 	"flag"
 	"fmt"
 	"io"
@@ -41,14 +42,14 @@ func verify(args []string, out io.Writer, errOut io.Writer) error {
 
 	rawCACert, err := os.ReadFile(*vf.caPath)
 	if err != nil {
-		return fmt.Errorf("error while reading ca: %s", err)
+		return fmt.Errorf("error while reading ca: %w", err)
 	}
 
 	caPool := cert.NewCAPool()
 	for {
 		rawCACert, err = caPool.AddCAFromPEM(rawCACert)
 		if err != nil {
-			return fmt.Errorf("error while adding ca cert to pool: %s", err)
+			return fmt.Errorf("error while adding ca cert to pool: %w", err)
 		}
 
 		if rawCACert == nil || len(rawCACert) == 0 || strings.TrimSpace(string(rawCACert)) == "" {
@@ -58,20 +59,30 @@ func verify(args []string, out io.Writer, errOut io.Writer) error {
 
 	rawCert, err := os.ReadFile(*vf.certPath)
 	if err != nil {
-		return fmt.Errorf("unable to read crt; %s", err)
+		return fmt.Errorf("unable to read crt: %w", err)
 	}
-
-	c, _, err := cert.UnmarshalCertificateFromPEM(rawCert)
-	if err != nil {
-		return fmt.Errorf("error while parsing crt: %s", err)
-	}
-
-	_, err = caPool.VerifyCertificate(time.Now(), c)
-	if err != nil {
-		return err
+	var errs []error
+	for {
+		if len(rawCert) == 0 {
+			break
+		}
+		c, extra, err := cert.UnmarshalCertificateFromPEM(rawCert)
+		if err != nil {
+			return fmt.Errorf("error while parsing crt: %w", err)
+		}
+		rawCert = extra
+		_, err = caPool.VerifyCertificate(time.Now(), c)
+		if err != nil {
+			switch {
+			case errors.Is(err, cert.ErrCaNotFound):
+				errs = append(errs, fmt.Errorf("error while verifying certificate v%d %s with issuer %s: %w", c.Version(), c.Name(), c.Issuer(), err))
+			default:
+				errs = append(errs, fmt.Errorf("error while verifying certificate %+v: %w", c, err))
+			}
+		}
 	}
 
-	return nil
+	return errors.Join(errs...)
 }
 
 func verifySummary() string {
@@ -80,7 +91,7 @@ func verifySummary() string {
 
 func verifyHelp(out io.Writer) {
 	vf := newVerifyFlags()
-	out.Write([]byte("Usage of " + os.Args[0] + " " + verifySummary() + "\n"))
+	_, _ = out.Write([]byte("Usage of " + os.Args[0] + " " + verifySummary() + "\n"))
 	vf.set.SetOutput(out)
 	vf.set.PrintDefaults()
 }

cmd/nebula-cert/verify_test.go

@@ -3,10 +3,12 @@ package main
 import (
 	"bytes"
 	"crypto/rand"
+	"errors"
 	"os"
 	"testing"
 	"time"
 
+	"github.com/slackhq/nebula/cert"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/crypto/ed25519"
 )
@@ -76,7 +78,7 @@ func Test_verify(t *testing.T) {
 	err = verify([]string{"-ca", caFile.Name(), "-crt", "does_not_exist"}, ob, eb)
 	assert.Equal(t, "", ob.String())
 	assert.Equal(t, "", eb.String())
-	assert.EqualError(t, err, "unable to read crt; open does_not_exist: "+NoSuchFileError)
+	assert.EqualError(t, err, "unable to read crt: open does_not_exist: "+NoSuchFileError)
 
 	// invalid crt at path
 	ob.Reset()
@@ -106,7 +108,7 @@ func Test_verify(t *testing.T) {
 	err = verify([]string{"-ca", caFile.Name(), "-crt", certFile.Name()}, ob, eb)
 	assert.Equal(t, "", ob.String())
 	assert.Equal(t, "", eb.String())
-	assert.EqualError(t, err, "certificate signature did not match")
+	assert.True(t, errors.Is(err, cert.ErrSignatureMismatch))
 
 	// verified cert at path
 	crt, _ = NewTestCert(ca, caPriv, "test-cert", time.Now().Add(time.Hour*-1), time.Now().Add(time.Hour), nil, nil, nil)

connection_manager.go

@@ -426,17 +426,17 @@ func (n *connectionManager) shouldSwapPrimary(current, primary *HostInfo) bool {
 	// If we are here then we have multiple tunnels for a host pair and neither side believes the same tunnel is primary.
 	// Let's sort this out.
 
-	//TODO: current.vpnIp should become an array of vpnIps
+	// Only one side should swap because if both swap then we may never resolve to a single tunnel.
+	// vpn addr is static across all tunnels for this host pair so lets
+	// use that to determine if we should consider swapping.
 	if current.vpnAddrs[0].Compare(n.intf.myVpnAddrs[0]) < 0 {
-		// Only one side should flip primary because if both flip then we may never resolve to a single tunnel.
-		// vpn ip is static across all tunnels for this host pair so lets use that to determine who is flipping.
-		// The remotes vpn ip is lower than mine. I will not flip.
+		// Their primary vpn addr is less than mine. Do not swap.
 		return false
 	}
 
-	//TODO: we should favor v2 over v1 certificates if configured to send them
-
-	crt := n.intf.pki.getCertificate(current.ConnectionState.myCert.Version())
+	crt := n.intf.pki.getCertState().getCertificate(current.ConnectionState.myCert.Version())
+	// If this tunnel is using the latest certificate then we should swap it to primary for a bit and see if things
+	// settle down.
 	return bytes.Equal(current.ConnectionState.myCert.Signature(), crt.Signature())
 }
 
@@ -495,13 +495,14 @@ func (n *connectionManager) sendPunch(hostinfo *HostInfo) {
 }
 
 func (n *connectionManager) tryRehandshake(hostinfo *HostInfo) {
-	crt := n.intf.pki.getCertificate(hostinfo.ConnectionState.myCert.Version())
-	if bytes.Equal(hostinfo.ConnectionState.myCert.Signature(), crt.Signature()) {
+	cs := n.intf.pki.getCertState()
+	curCrt := hostinfo.ConnectionState.myCert
+	myCrt := cs.getCertificate(curCrt.Version())
+	if curCrt.Version() >= cs.defaultVersion && bytes.Equal(curCrt.Signature(), myCrt.Signature()) == true {
+		// The current tunnel is using the latest certificate and version, no need to rehandshake.
 		return
 	}
 
-	//TODO: we should favor v2 over v1 certificates if configured to send them
-
 	n.l.WithField("vpnAddrs", hostinfo.vpnAddrs).
 		WithField("reason", "local certificate is not current").
 		Info("Re-handshaking with remote")

control.go

@@ -133,9 +133,9 @@ func (c *Control) ListHostmapIndexes(pendingMap bool) []ControlHostInfo {
 func (c *Control) GetCertByVpnIp(vpnIp netip.Addr) cert.Certificate {
 	_, found := c.f.myVpnAddrsTable.Lookup(vpnIp)
 	if found {
-		//TODO: we might have 2 certs....
-		//TODO: this should return our latest version cert
-		return c.f.pki.getDefaultCertificate().Copy()
+		// Only returning the default certificate since its impossible
+		// for any other host but ourselves to have more than 1
+		return c.f.pki.getCertState().GetDefaultCertificate().Copy()
 	}
 	hi := c.f.hostMap.QueryVpnAddr(vpnIp)
 	if hi == nil {
@@ -228,13 +228,9 @@ func (c *Control) CloseTunnel(vpnIp netip.Addr, localOnly bool) bool {
 // the int returned is a count of tunnels closed
 func (c *Control) CloseAllTunnels(excludeLighthouses bool) (closed int) {
 	//TODO: this is probably better as a function in ConnectionManager or HostMap directly
-	lighthouses := c.f.lightHouse.GetLighthouses()
-
 	shutdown := func(h *HostInfo) {
-		if excludeLighthouses {
-			if _, ok := lighthouses[h.vpnAddrs[0]]; ok {
-				return
-			}
+		if excludeLighthouses && c.f.lightHouse.IsAnyLighthouseAddr(h.vpnAddrs) {
+			return
 		}
 		c.f.send(header.CloseTunnel, 0, h.ConnectionState, h, []byte{}, make([]byte, 12, 12), make([]byte, mtu))
 		c.f.closeTunnel(h)

control_tester.go

@@ -70,7 +70,7 @@ func (c *Control) InjectRelays(vpnIp netip.Addr, relayVpnIps []netip.Addr) {
 	defer remoteList.Unlock()
 	c.f.lightHouse.Unlock()
 
-	remoteList.unlockedSetRelay(vpnIp, vpnIp, relayVpnIps)
+	remoteList.unlockedSetRelay(vpnIp, relayVpnIps)
 }
 
 // GetFromTun will pull a packet off the tun side of nebula