From f61dd12f052de6ba674607a143e4dbc22ddd7012 Mon Sep 17 00:00:00 2001 From: Andrew Lytvynov Date: Wed, 23 Aug 2023 17:13:03 -0600 Subject: [PATCH] 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 --- clientupdate/distsign/distsign.go | 140 ++++++++++------ clientupdate/distsign/distsign_test.go | 157 +++++++++++++++--- clientupdate/distsign/roots.go | 2 +- .../distsign/roots/to-be-replaced.pub | 6 +- release/dist/cli/cli.go | 17 +- 5 files changed, 243 insertions(+), 79 deletions(-) diff --git a/clientupdate/distsign/distsign.go b/clientupdate/distsign/distsign.go index 9d5fb106c..1efe1e5f6 100644 --- a/clientupdate/distsign/distsign.go +++ b/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" - pemTypePublic = "PUBLIC KEY" + pemTypeRootPrivate = "ROOT PRIVATE 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. -func GenerateKey() (priv, pub []byte, err error) { +// RootKey is a root key used to sign signing keys. +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. -type RootKey Signer +// ParseRootKey parses the PEM-encoded private root key. The key must be in the +// 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) { - return s.Sign(nil, pubBundle, crypto.Hash(0)) +func (r *RootKey) SignSigningKeys(pubBundle []byte) ([]byte, error) { + if _, err := parseSigningKeyBundle(pubBundle); err != nil { + return nil, err + } + return ed25519.Sign(r.k, pubBundle), nil } -// SigningKey is a signing key Signer used to sign packages. -type SigningKey Signer +// 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 +} + +// ParseSigningKey parses the PEM-encoded private signing key. The key must be +// 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. - var keys []ed25519.PublicKey - for len(raw) > 0 { - pub, rest, err := parsePublicKey(raw) - if err != nil { - return nil, err - } - keys = append(keys, pub) - raw = rest - } - if len(keys) == 0 { - return nil, fmt.Errorf("no signing keys found at %q", keyURL) + keys, err := parseSigningKeyBundle(raw) + if err != nil { + return nil, fmt.Errorf("cannot parse signing key bundle from %q: %w", keyURL, err) } 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 { - return nil, fmt.Errorf("PEM type is %q, want %q", b.Type, pemTypePrivate) + if b.Type != typeTag { + 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) { - pub, rest, err := parsePublicKey(data) +func parseSigningKeyBundle(bundle []byte) ([]ed25519.PublicKey, error) { + 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 { - return nil, nil, fmt.Errorf("PEM type is %q, want %q", b.Type, pemTypePublic) + if b.Type != typeTag { + 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") diff --git a/clientupdate/distsign/distsign_test.go b/clientupdate/distsign/distsign_test.go index 2d450b86a..16f71b2e8 100644 --- a/clientupdate/distsign/distsign_test.go +++ b/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) - priv, err := parsePrivateKey(kp.privRaw) + privRaw, pubRaw, err := GenerateRootKey() + 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) - priv, err := parsePrivateKey(kp.privRaw) + privRaw, pubRaw, err := GenerateSigningKey() + 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, - } -} diff --git a/clientupdate/distsign/roots.go b/clientupdate/distsign/roots.go index b148b2189..ed6330ca1 100644 --- a/clientupdate/distsign/roots.go +++ b/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) } diff --git a/clientupdate/distsign/roots/to-be-replaced.pub b/clientupdate/distsign/roots/to-be-replaced.pub index d9e7989f8..293fec3fd 100644 --- a/clientupdate/distsign/roots/to-be-replaced.pub +++ b/clientupdate/distsign/roots/to-be-replaced.pub @@ -1,3 +1,3 @@ ------BEGIN PUBLIC KEY----- -JNBgo4EFQ+DpRcESM2xU19xQWGffvLcmxtBMT4I+Qo0= ------END PUBLIC KEY----- +-----BEGIN ROOT PUBLIC KEY----- +xFykOJAkOlBoMOXA4UKYtaSGYsOY8r1+0wJzQE5mzUo= +-----END ROOT PUBLIC KEY----- diff --git a/release/dist/cli/cli.go b/release/dist/cli/cli.go index 7370da7a1..9329dcd14 100644 --- a/release/dist/cli/cli.go +++ b/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 }