clientupdate/distsign: use distinct PEM types for root/signing keys (#9045)

To make key management less error-prone, use different PEM block types
for root and signing keys. As a result, separate out most of the Go code
between root/signing keys too.

Updates #8760

Signed-off-by: Andrew Lytvynov <awly@tailscale.com>

clientupdate/distsign/distsign.go

@@ -38,7 +38,6 @@
 package distsign
 
 import (
-	"crypto"
 	"crypto/ed25519"
 	"crypto/rand"
 	"encoding/binary"
@@ -56,49 +55,93 @@ import (
 )
 
 const (
-	pemTypePrivate = "PRIVATE KEY"
+	pemTypeRootPrivate    = "ROOT PRIVATE KEY"
-	pemTypePublic  = "PUBLIC KEY"
+	pemTypeRootPublic     = "ROOT PUBLIC KEY"
+	pemTypeSigningPrivate = "SIGNING PRIVATE KEY"
+	pemTypeSigningPublic  = "SIGNING PUBLIC KEY"
 
 	downloadSizeLimit    = 1 << 29 // 512MB
 	signingKeysSizeLimit = 1 << 20 // 1MB
 	signatureSizeLimit   = ed25519.SignatureSize
 )
 
-// GenerateKey generates a new key pair and encodes it as PEM.
+// RootKey is a root key used to sign signing keys.
-func GenerateKey() (priv, pub []byte, err error) {
+type RootKey struct {
+	k ed25519.PrivateKey
+}
+
+// GenerateRootKey generates a new root key pair and encodes it as PEM.
+func GenerateRootKey() (priv, pub []byte, err error) {
 	pub, priv, err = ed25519.GenerateKey(rand.Reader)
 	if err != nil {
 		return nil, nil, err
 	}
 	return pem.EncodeToMemory(&pem.Block{
-			Type:  pemTypePrivate,
+			Type:  pemTypeRootPrivate,
 			Bytes: []byte(priv),
 		}), pem.EncodeToMemory(&pem.Block{
-			Type:  pemTypePublic,
+			Type:  pemTypeRootPublic,
 			Bytes: []byte(pub),
 		}), nil
 }
 
-// RootKey is a root key Signer used to sign signing keys.
+// ParseRootKey parses the PEM-encoded private root key. The key must be in the
-type RootKey Signer
+// same format as returned by GenerateRootKey.
+func ParseRootKey(privKey []byte) (*RootKey, error) {
+	k, err := parsePrivateKey(privKey, pemTypeRootPrivate)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse root key: %w", err)
+	}
+	return &RootKey{k: k}, nil
+}
 
 // SignSigningKeys signs the bundle of public signing keys. The bundle must be
 // a sequence of PEM blocks joined with newlines.
-func (s *RootKey) SignSigningKeys(pubBundle []byte) ([]byte, error) {
+func (r *RootKey) SignSigningKeys(pubBundle []byte) ([]byte, error) {
-	return s.Sign(nil, pubBundle, crypto.Hash(0))
+	if _, err := parseSigningKeyBundle(pubBundle); err != nil {
+		return nil, err
+	}
+	return ed25519.Sign(r.k, pubBundle), nil
+}
+
+// SigningKey is a signing key used to sign packages.
+type SigningKey struct {
+	k ed25519.PrivateKey
+}
+
+// GenerateSigningKey generates a new signing key pair and encodes it as PEM.
+func GenerateSigningKey() (priv, pub []byte, err error) {
+	pub, priv, err = ed25519.GenerateKey(rand.Reader)
+	if err != nil {
+		return nil, nil, err
+	}
+	return pem.EncodeToMemory(&pem.Block{
+			Type:  pemTypeSigningPrivate,
+			Bytes: []byte(priv),
+		}), pem.EncodeToMemory(&pem.Block{
+			Type:  pemTypeSigningPublic,
+			Bytes: []byte(pub),
+		}), nil
 }
 
-// SigningKey is a signing key Signer used to sign packages.
+// ParseSigningKey parses the PEM-encoded private signing key. The key must be
-type SigningKey Signer
+// in the same format as returned by GenerateSigningKey.
+func ParseSigningKey(privKey []byte) (*SigningKey, error) {
+	k, err := parsePrivateKey(privKey, pemTypeSigningPrivate)
+	if err != nil {
+		return nil, fmt.Errorf("failed to parse root key: %w", err)
+	}
+	return &SigningKey{k: k}, nil
+}
 
 // SignPackageHash signs the hash and the length of a package. Use PackageHash
 // to compute the inputs.
-func (s SigningKey) SignPackageHash(hash []byte, len int64) ([]byte, error) {
+func (s *SigningKey) SignPackageHash(hash []byte, len int64) ([]byte, error) {
 	if len <= 0 {
 		return nil, fmt.Errorf("package length must be positive, got %d", len)
 	}
 	msg := binary.LittleEndian.AppendUint64(hash, uint64(len))
-	return s.Sign(nil, msg, crypto.Hash(0))
+	return ed25519.Sign(s.k, msg), nil
 }
 
 // PackageHash is a hash.Hash that counts the number of bytes written. Use it
@@ -132,26 +175,6 @@ func (ph *PackageHash) Reset() {
 // Len returns the total number of bytes written.
 func (ph *PackageHash) Len() int64 { return ph.len }
 
-// Signer is crypto.Signer using a single key (root or signing).
-type Signer struct {
-	crypto.Signer
-}
-
-// NewSigner parses the PEM-encoded private key stored in the file named
-// privKeyPath and creates a Signer for it. The key is expected to be in the
-// same format as returned by GenerateKey.
-func NewSigner(privKeyPath string) (Signer, error) {
-	raw, err := os.ReadFile(privKeyPath)
-	if err != nil {
-		return Signer{}, err
-	}
-	k, err := parsePrivateKey(raw)
-	if err != nil {
-		return Signer{}, fmt.Errorf("failed to parse %q: %w", privKeyPath, err)
-	}
-	return Signer{Signer: k}, nil
-}
-
 // Client downloads and validates files from a distribution server.
 type Client struct {
 	roots    []ed25519.PublicKey
@@ -229,18 +252,9 @@ func (c *Client) signingKeys() ([]ed25519.PublicKey, error) {
 		return nil, fmt.Errorf("signature %q for key %q does not validate with any known root key; either you are under attack, or running a very old version of Tailscale with outdated root keys", sigURL, keyURL)
 	}
 
-	// Parse the bundle of public signing keys.
+	keys, err := parseSigningKeyBundle(raw)
-	var keys []ed25519.PublicKey
-	for len(raw) > 0 {
-		pub, rest, err := parsePublicKey(raw)
 	if err != nil {
-			return nil, err
+		return nil, fmt.Errorf("cannot parse signing key bundle from %q: %w", keyURL, err)
-		}
-		keys = append(keys, pub)
-		raw = rest
-	}
-	if len(keys) == 0 {
-		return nil, fmt.Errorf("no signing keys found at %q", keyURL)
 	}
 	return keys, nil
 }
@@ -284,7 +298,7 @@ func download(url, dst string, limit int64) ([]byte, int64, error) {
 	return h.Sum(nil), h.Len(), nil
 }
 
-func parsePrivateKey(data []byte) (ed25519.PrivateKey, error) {
+func parsePrivateKey(data []byte, typeTag string) (ed25519.PrivateKey, error) {
 	b, rest := pem.Decode(data)
 	if b == nil {
 		return nil, errors.New("failed to decode PEM data")
@@ -292,8 +306,8 @@ func parsePrivateKey(data []byte) (ed25519.PrivateKey, error) {
 	if len(rest) > 0 {
 		return nil, errors.New("trailing PEM data")
 	}
-	if b.Type != pemTypePrivate {
+	if b.Type != typeTag {
-		return nil, fmt.Errorf("PEM type is %q, want %q", b.Type, pemTypePrivate)
+		return nil, fmt.Errorf("PEM type is %q, want %q", b.Type, typeTag)
 	}
 	if len(b.Bytes) != ed25519.PrivateKeySize {
 		return nil, errors.New("private key has incorrect length for an Ed25519 private key")
@@ -301,8 +315,24 @@ func parsePrivateKey(data []byte) (ed25519.PrivateKey, error) {
 	return ed25519.PrivateKey(b.Bytes), nil
 }
 
-func parseSinglePublicKey(data []byte) (ed25519.PublicKey, error) {
+func parseSigningKeyBundle(bundle []byte) ([]ed25519.PublicKey, error) {
-	pub, rest, err := parsePublicKey(data)
+	var keys []ed25519.PublicKey
+	for len(bundle) > 0 {
+		pub, rest, err := parsePublicKey(bundle, pemTypeSigningPublic)
+		if err != nil {
+			return nil, err
+		}
+		keys = append(keys, pub)
+		bundle = rest
+	}
+	if len(keys) == 0 {
+		return nil, errors.New("no signing keys found in the bundle")
+	}
+	return keys, nil
+}
+
+func parseSinglePublicKey(data []byte, typeTag string) (ed25519.PublicKey, error) {
+	pub, rest, err := parsePublicKey(data, typeTag)
 	if err != nil {
 		return nil, err
 	}
@@ -312,13 +342,13 @@ func parseSinglePublicKey(data []byte) (ed25519.PublicKey, error) {
 	return pub, err
 }
 
-func parsePublicKey(data []byte) (pub ed25519.PublicKey, rest []byte, retErr error) {
+func parsePublicKey(data []byte, typeTag string) (pub ed25519.PublicKey, rest []byte, retErr error) {
 	b, rest := pem.Decode(data)
 	if b == nil {
 		return nil, nil, errors.New("failed to decode PEM data")
 	}
-	if b.Type != pemTypePublic {
+	if b.Type != typeTag {
-		return nil, nil, fmt.Errorf("PEM type is %q, want %q", b.Type, pemTypePublic)
+		return nil, nil, fmt.Errorf("PEM type is %q, want %q", b.Type, typeTag)
 	}
 	if len(b.Bytes) != ed25519.PublicKeySize {
 		return nil, nil, errors.New("public key has incorrect length for an Ed25519 public key")

clientupdate/distsign/distsign_test.go

@@ -72,7 +72,7 @@ func TestDownload(t *testing.T) {
 			desc: "signed with root key",
 			before: func(t *testing.T) {
 				srv.add("hello", []byte("world"))
-				srv.add("hello.sig", srv.roots[0].sign([]byte("world")))
+				srv.add("hello.sig", ed25519.Sign(srv.roots[0].k, []byte("world")))
 			},
 			src:     "hello",
 			wantErr: true,
@@ -202,6 +202,122 @@ func TestRotateSigning(t *testing.T) {
 	}
 }
 
+func TestParseRootKey(t *testing.T) {
+	tests := []struct {
+		desc     string
+		generate func() ([]byte, []byte, error)
+		wantErr  bool
+	}{
+		{
+			desc:     "valid",
+			generate: GenerateRootKey,
+		},
+		{
+			desc:     "signing",
+			generate: GenerateSigningKey,
+			wantErr:  true,
+		},
+		{
+			desc:     "nil",
+			generate: func() ([]byte, []byte, error) { return nil, nil, nil },
+			wantErr:  true,
+		},
+		{
+			desc: "invalid PEM tag",
+			generate: func() ([]byte, []byte, error) {
+				priv, pub, err := GenerateRootKey()
+				priv = bytes.Replace(priv, []byte("ROOT "), nil, -1)
+				return priv, pub, err
+			},
+			wantErr: true,
+		},
+		{
+			desc:     "not PEM",
+			generate: func() ([]byte, []byte, error) { return []byte("s3cr3t"), nil, nil },
+			wantErr:  true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			priv, _, err := tt.generate()
+			if err != nil {
+				t.Fatal(err)
+			}
+			r, err := ParseRootKey(priv)
+			if err != nil {
+				if tt.wantErr {
+					return
+				}
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if tt.wantErr {
+				t.Fatal("expected non-nil error")
+			}
+			if r == nil {
+				t.Errorf("got nil error and nil RootKey")
+			}
+		})
+	}
+}
+
+func TestParseSigningKey(t *testing.T) {
+	tests := []struct {
+		desc     string
+		generate func() ([]byte, []byte, error)
+		wantErr  bool
+	}{
+		{
+			desc:     "valid",
+			generate: GenerateSigningKey,
+		},
+		{
+			desc:     "root",
+			generate: GenerateRootKey,
+			wantErr:  true,
+		},
+		{
+			desc:     "nil",
+			generate: func() ([]byte, []byte, error) { return nil, nil, nil },
+			wantErr:  true,
+		},
+		{
+			desc: "invalid PEM tag",
+			generate: func() ([]byte, []byte, error) {
+				priv, pub, err := GenerateSigningKey()
+				priv = bytes.Replace(priv, []byte("SIGNING "), nil, -1)
+				return priv, pub, err
+			},
+			wantErr: true,
+		},
+		{
+			desc:     "not PEM",
+			generate: func() ([]byte, []byte, error) { return []byte("s3cr3t"), nil, nil },
+			wantErr:  true,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			priv, _, err := tt.generate()
+			if err != nil {
+				t.Fatal(err)
+			}
+			r, err := ParseSigningKey(priv)
+			if err != nil {
+				if tt.wantErr {
+					return
+				}
+				t.Fatalf("unexpected error: %v", err)
+			}
+			if tt.wantErr {
+				t.Fatal("expected non-nil error")
+			}
+			if r == nil {
+				t.Errorf("got nil error and nil SigningKey")
+			}
+		})
+	}
+}
+
 type testServer struct {
 	roots []rootKeyPair
 	sign  []signingKeyPair
@@ -228,7 +344,7 @@ func newTestServer(t *testing.T) *testServer {
 func (s *testServer) client(t *testing.T) *Client {
 	roots := make([]ed25519.PublicKey, 0, len(s.roots))
 	for _, r := range s.roots {
-		pub, err := parseSinglePublicKey(r.pubRaw)
+		pub, err := parseSinglePublicKey(r.pubRaw, pemTypeRootPublic)
 		if err != nil {
 			t.Fatalf("parsePublicKey: %v", err)
 		}
@@ -285,13 +401,20 @@ type rootKeyPair struct {
 }
 
 func newRootKeyPair(t *testing.T) rootKeyPair {
-	kp := newKeyPair(t)
+	privRaw, pubRaw, err := GenerateRootKey()
-	priv, err := parsePrivateKey(kp.privRaw)
+	if err != nil {
+		t.Fatalf("GenerateRootKey: %v", err)
+	}
+	kp := keyPair{
+		privRaw: privRaw,
+		pubRaw:  pubRaw,
+	}
+	priv, err := parsePrivateKey(kp.privRaw, pemTypeRootPrivate)
 	if err != nil {
 		t.Fatalf("parsePrivateKey: %v", err)
 	}
 	return rootKeyPair{
-		RootKey: &RootKey{Signer: priv},
+		RootKey: &RootKey{k: priv},
 		keyPair: kp,
 	}
 }
@@ -310,13 +433,20 @@ type signingKeyPair struct {
 }
 
 func newSigningKeyPair(t *testing.T) signingKeyPair {
-	kp := newKeyPair(t)
+	privRaw, pubRaw, err := GenerateSigningKey()
-	priv, err := parsePrivateKey(kp.privRaw)
+	if err != nil {
+		t.Fatalf("GenerateSigningKey: %v", err)
+	}
+	kp := keyPair{
+		privRaw: privRaw,
+		pubRaw:  pubRaw,
+	}
+	priv, err := parsePrivateKey(kp.privRaw, pemTypeSigningPrivate)
 	if err != nil {
 		t.Fatalf("parsePrivateKey: %v", err)
 	}
 	return signingKeyPair{
-		SigningKey: &SigningKey{Signer: priv},
+		SigningKey: &SigningKey{k: priv},
 		keyPair:    kp,
 	}
 }
@@ -334,14 +464,3 @@ type keyPair struct {
 	privRaw []byte
 	pubRaw  []byte
 }
-
-func newKeyPair(t *testing.T) keyPair {
-	privRaw, pubRaw, err := GenerateKey()
-	if err != nil {
-		t.Fatalf("GenerateKey: %v", err)
-	}
-	return keyPair{
-		privRaw: privRaw,
-		pubRaw:  pubRaw,
-	}
-}

clientupdate/distsign/roots.go

@@ -41,7 +41,7 @@ func parseRoots() ([]ed25519.PublicKey, error) {
 		if err != nil {
 			return nil, err
 		}
-		key, err := parseSinglePublicKey(raw)
+		key, err := parseSinglePublicKey(raw, pemTypeRootPublic)
 		if err != nil {
 			return nil, fmt.Errorf("parsing root key %q: %w", f.Name(), err)
 		}

clientupdate/distsign/roots/to-be-replaced.pub

@@ -1,3 +1,3 @@
------BEGIN PUBLIC KEY-----
+-----BEGIN ROOT PUBLIC KEY-----
-JNBgo4EFQ+DpRcESM2xU19xQWGffvLcmxtBMT4I+Qo0=
+xFykOJAkOlBoMOXA4UKYtaSGYsOY8r1+0wJzQE5mzUo=
------END PUBLIC KEY-----
+-----END ROOT PUBLIC KEY-----

release/dist/cli/cli.go

@@ -88,6 +88,8 @@ func CLI(getTargets func(unixpkgs.Signers) ([]dist.Target, error)) *ffcli.Comman
 				ShortHelp:  "Generate root or signing key pair",
 				FlagSet: (func() *flag.FlagSet {
 					fs := flag.NewFlagSet("gen-key", flag.ExitOnError)
+					fs.BoolVar(&genKeyArgs.root, "root", false, "generate a root key")
+					fs.BoolVar(&genKeyArgs.signing, "signing", false, "generate a signing key")
 					fs.StringVar(&genKeyArgs.privPath, "priv-path", "private-key.pem", "output path for the private key")
 					fs.StringVar(&genKeyArgs.pubPath, "pub-path", "public-key.pem", "output path for the public key")
 					return fs
@@ -190,12 +192,25 @@ func parseSigningKey(path string) (crypto.Signer, error) {
 }
 
 var genKeyArgs struct {
+	root     bool
+	signing  bool
 	privPath string
 	pubPath  string
 }
 
 func runGenKey(ctx context.Context) error {
-	priv, pub, err := distsign.GenerateKey()
+	var pub, priv []byte
+	var err error
+	switch {
+	case genKeyArgs.root && genKeyArgs.signing:
+		return errors.New("only one of --root or --signing can be set")
+	case !genKeyArgs.root && !genKeyArgs.signing:
+		return errors.New("set either --root or --signing")
+	case genKeyArgs.root:
+		priv, pub, err = distsign.GenerateRootKey()
+	case genKeyArgs.signing:
+		priv, pub, err = distsign.GenerateSigningKey()
+	}
 	if err != nil {
 		return err
 	}