control/noise: use key.Machine{Public,Private} as appropriate.

Signed-off-by: David Anderson <danderson@tailscale.com>

control/noise/conn.go

@@ -42,7 +42,7 @@ const (
 type Conn struct {
 	conn          net.Conn
 	version       uint16
-	peer          key.Public
+	peer          key.MachinePublic
 	handshakeHash [blake2s.Size]byte
 	rx            rxState
 	tx            txState
@@ -83,7 +83,7 @@ func (c *Conn) HandshakeHash() [blake2s.Size]byte {
 }
 
 // Peer returns the peer's long-term public key.
-func (c *Conn) Peer() key.Public {
+func (c *Conn) Peer() key.MachinePublic {
 	return c.peer
 }
 

control/noise/conn_test.go

@@ -197,8 +197,8 @@ func TestConnStd(t *testing.T) {
 	t.Skip("not all tests can pass on this Conn, see https://github.com/golang/go/issues/46977")
 	nettest.TestConn(t, func() (c1 net.Conn, c2 net.Conn, stop func(), err error) {
 		s1, s2 := tsnettest.NewConn("noise", 4096)
-		controlKey := key.NewPrivate()
+		controlKey := key.NewMachine()
-		machineKey := key.NewPrivate()
+		machineKey := key.NewMachine()
 		serverErr := make(chan error, 1)
 		go func() {
 			var err error
@@ -312,8 +312,8 @@ func (s *readSink) Total() int {
 
 func pairWithConns(t *testing.T, clientConn, serverConn net.Conn) (*Conn, *Conn) {
 	var (
-		controlKey = key.NewPrivate()
+		controlKey = key.NewMachine()
-		machineKey = key.NewPrivate()
+		machineKey = key.NewMachine()
 		server     *Conn
 		serverErr  = make(chan error, 1)
 	)

control/noise/handshake.go

@@ -14,6 +14,7 @@ import (
 	"strconv"
 	"time"
 
+	"go4.org/mem"
 	"golang.org/x/crypto/blake2s"
 	chp "golang.org/x/crypto/chacha20poly1305"
 	"golang.org/x/crypto/curve25519"
@@ -23,22 +24,21 @@ import (
 )
 
 const (
-	// protocolName is the name of the specific instantiation of the
+	// protocolName is the name of the specific instantiation of Noise
-	// Noise protocol we're using. Each field is defined in the Noise
+	// that the control protocol uses. This string's value is fixed by
-	// spec, and shouldn't be changed unless we're switching to a
+	// the Noise spec, and shouldn't be changed unless we're updating
-	// different Noise protocol instance.
+	// the control protocol to use a different Noise instance.
 	protocolName = "Noise_IK_25519_ChaChaPoly_BLAKE2s"
-	// protocolVersion is the version of the Tailscale base
+	// protocolVersion is the version of the control protocol that
-	// protocol that Client will use when initiating a handshake.
+	// Client will use when initiating a handshake.
 	protocolVersion uint16 = 1
 	// protocolVersionPrefix is the name portion of the protocol
-	// name+version string that gets mixed into the Noise handshake as
+	// name+version string that gets mixed into the handshake as a
-	// a prologue.
+	// prologue.
 	//
-	// This mixing verifies that both clients agree that
+	// This mixing verifies that both clients agree that they're
-	// they're executing the Tailscale control protocol at a specific
+	// executing the control protocol at a specific version that
-	// version that matches the advertised version in the cleartext
+	// matches the advertised version in the cleartext packet header.
-	// packet header.
 	protocolVersionPrefix = "Tailscale Control Protocol v"
 	invalidNonce          = ^uint64(0)
 )
@@ -49,12 +49,12 @@ func protocolVersionPrologue(version uint16) []byte {
 	return strconv.AppendUint(ret, uint64(version), 10)
 }
 
-// Client initiates a Noise client handshake, returning the resulting
+// Client initiates a control client handshake, returning the resulting
-// Noise connection.
+// control connection.
 //
 // The context deadline, if any, covers the entire handshaking
 // process. Any preexisting Conn deadline is removed.
-func Client(ctx context.Context, conn net.Conn, machineKey key.Private, controlKey key.Public) (*Conn, error) {
+func Client(ctx context.Context, conn net.Conn, machineKey key.MachinePrivate, controlKey key.MachinePublic) (*Conn, error) {
 	if deadline, ok := ctx.Deadline(); ok {
 		if err := conn.SetDeadline(deadline); err != nil {
 			return nil, fmt.Errorf("setting conn deadline: %w", err)
@@ -72,20 +72,20 @@ func Client(ctx context.Context, conn net.Conn, machineKey key.Private, controlK
 
 	// <- s
 	// ...
-	s.MixHash(controlKey[:])
+	s.MixHash(controlKey.UntypedBytes())
 
 	// -> e, es, s, ss
 	init := mkInitiationMessage()
-	machineEphemeral := key.NewPrivate()
+	machineEphemeral := key.NewMachine()
 	machineEphemeralPub := machineEphemeral.Public()
-	copy(init.EphemeralPub(), machineEphemeralPub[:])
+	copy(init.EphemeralPub(), machineEphemeralPub.UntypedBytes())
-	s.MixHash(machineEphemeralPub[:])
+	s.MixHash(machineEphemeralPub.UntypedBytes())
 	cipher, err := s.MixDH(machineEphemeral, controlKey)
 	if err != nil {
 		return nil, fmt.Errorf("computing es: %w", err)
 	}
 	machineKeyPub := machineKey.Public()
-	s.EncryptAndHash(cipher, init.MachinePub(), machineKeyPub[:])
+	s.EncryptAndHash(cipher, init.MachinePub(), machineKeyPub.UntypedBytes())
 	cipher, err = s.MixDH(machineKey, controlKey)
 	if err != nil {
 		return nil, fmt.Errorf("computing ss: %w", err)
@@ -122,9 +122,8 @@ func Client(ctx context.Context, conn net.Conn, machineKey key.Private, controlK
 	}
 
 	// <- e, ee, se
-	var controlEphemeralPub key.Public
+	controlEphemeralPub := key.MachinePublicFromRaw32(mem.B(resp.EphemeralPub()))
-	copy(controlEphemeralPub[:], resp.EphemeralPub())
+	s.MixHash(controlEphemeralPub.UntypedBytes())
-	s.MixHash(controlEphemeralPub[:])
 	if _, err = s.MixDH(machineEphemeral, controlEphemeralPub); err != nil {
 		return nil, fmt.Errorf("computing ee: %w", err)
 	}
@@ -156,12 +155,12 @@ func Client(ctx context.Context, conn net.Conn, machineKey key.Private, controlK
 	return c, nil
 }
 
-// Server initiates a Noise server handshake, returning the resulting
+// Server initiates a control server handshake, returning the resulting
-// Noise connection.
+// control connection.
 //
 // The context deadline, if any, covers the entire handshaking
 // process.
-func Server(ctx context.Context, conn net.Conn, controlKey key.Private) (*Conn, error) {
+func Server(ctx context.Context, conn net.Conn, controlKey key.MachinePrivate) (*Conn, error) {
 	if deadline, ok := ctx.Deadline(); ok {
 		if err := conn.SetDeadline(deadline); err != nil {
 			return nil, fmt.Errorf("setting conn deadline: %w", err)
@@ -215,20 +214,20 @@ func Server(ctx context.Context, conn net.Conn, controlKey key.Private) (*Conn,
 	// <- s
 	// ...
 	controlKeyPub := controlKey.Public()
-	s.MixHash(controlKeyPub[:])
+	s.MixHash(controlKeyPub.UntypedBytes())
 
 	// -> e, es, s, ss
-	var machineEphemeralPub key.Public
+	machineEphemeralPub := key.MachinePublicFromRaw32(mem.B(init.EphemeralPub()))
-	copy(machineEphemeralPub[:], init.EphemeralPub())
+	s.MixHash(machineEphemeralPub.UntypedBytes())
-	s.MixHash(machineEphemeralPub[:])
 	cipher, err := s.MixDH(controlKey, machineEphemeralPub)
 	if err != nil {
 		return nil, fmt.Errorf("computing es: %w", err)
 	}
-	var machineKey key.Public
+	var machineKeyBytes [32]byte
-	if err := s.DecryptAndHash(cipher, machineKey[:], init.MachinePub()); err != nil {
+	if err := s.DecryptAndHash(cipher, machineKeyBytes[:], init.MachinePub()); err != nil {
 		return nil, fmt.Errorf("decrypting machine key: %w", err)
 	}
+	machineKey := key.MachinePublicFromRaw32(mem.B(machineKeyBytes[:]))
 	cipher, err = s.MixDH(controlKey, machineKey)
 	if err != nil {
 		return nil, fmt.Errorf("computing ss: %w", err)
@@ -239,10 +238,10 @@ func Server(ctx context.Context, conn net.Conn, controlKey key.Private) (*Conn,
 
 	// <- e, ee, se
 	resp := mkResponseMessage()
-	controlEphemeral := key.NewPrivate()
+	controlEphemeral := key.NewMachine()
 	controlEphemeralPub := controlEphemeral.Public()
-	copy(resp.EphemeralPub(), controlEphemeralPub[:])
+	copy(resp.EphemeralPub(), controlEphemeralPub.UntypedBytes())
-	s.MixHash(controlEphemeralPub[:])
+	s.MixHash(controlEphemeralPub.UntypedBytes())
 	if _, err := s.MixDH(controlEphemeral, machineEphemeralPub); err != nil {
 		return nil, fmt.Errorf("computing ee: %w", err)
 	}
@@ -276,14 +275,12 @@ func Server(ctx context.Context, conn net.Conn, controlKey key.Private) (*Conn,
 	return c, nil
 }
 
-// symmetricState is the SymmetricState object from the Noise protocol
+// symmetricState contains the state of an in-flight handshake.
-// spec. It contains all the symmetric cipher state of an in-flight
-// handshake. Field names match the variable names in the spec.
 type symmetricState struct {
 	finished bool
 
-	h  [blake2s.Size]byte
+	h  [blake2s.Size]byte // hash of currently-processed handshake state
-	ck [blake2s.Size]byte
+	ck [blake2s.Size]byte // chaining key used to construct session keys at the end of the handshake
 }
 
 func (s *symmetricState) checkFinished() {
@@ -319,9 +316,9 @@ func (s *symmetricState) MixHash(data []byte) {
 // reduce the risk of error in the caller (e.g. invoking X25519 with
 // two private keys, or two public keys), and thus producing the wrong
 // calculation.
-func (s *symmetricState) MixDH(priv key.Private, pub key.Public) (*singleUseCHP, error) {
+func (s *symmetricState) MixDH(priv key.MachinePrivate, pub key.MachinePublic) (*singleUseCHP, error) {
 	s.checkFinished()
-	keyData, err := curve25519.X25519(priv[:], pub[:])
+	keyData, err := curve25519.X25519(priv.UntypedBytes(), pub.UntypedBytes())
 	if err != nil {
 		return nil, fmt.Errorf("computing X25519: %w", err)
 	}

control/noise/handshake_test.go

@@ -19,8 +19,8 @@ import (
 func TestHandshake(t *testing.T) {
 	var (
 		clientConn, serverConn = tsnettest.NewConn("noise", 128000)
-		serverKey              = key.NewPrivate()
+		serverKey              = key.NewMachine()
-		clientKey              = key.NewPrivate()
+		clientKey              = key.NewMachine()
 		server                 *Conn
 		serverErr              = make(chan error, 1)
 	)
@@ -71,8 +71,8 @@ func TestNoReuse(t *testing.T) {
 			clientBuf, serverBuf bytes.Buffer
 			clientConn           = &readerConn{clientRaw, io.TeeReader(clientRaw, &clientBuf)}
 			serverConn           = &readerConn{serverRaw, io.TeeReader(serverRaw, &serverBuf)}
-			serverKey            = key.NewPrivate()
+			serverKey            = key.NewMachine()
-			clientKey            = key.NewPrivate()
+			clientKey            = key.NewMachine()
 			server               *Conn
 			serverErr            = make(chan error, 1)
 		)
@@ -164,8 +164,8 @@ func TestTampering(t *testing.T) {
 		var (
 			clientConn, serverRaw = tsnettest.NewConn("noise", 128000)
 			serverConn            = &readerConn{serverRaw, &tamperReader{serverRaw, i, 0}}
-			serverKey             = key.NewPrivate()
+			serverKey             = key.NewMachine()
-			clientKey             = key.NewPrivate()
+			clientKey             = key.NewMachine()
 			serverErr             = make(chan error, 1)
 		)
 		go func() {
@@ -192,8 +192,8 @@ func TestTampering(t *testing.T) {
 		var (
 			clientRaw, serverConn = tsnettest.NewConn("noise", 128000)
 			clientConn            = &readerConn{clientRaw, &tamperReader{clientRaw, i, 0}}
-			serverKey             = key.NewPrivate()
+			serverKey             = key.NewMachine()
-			clientKey             = key.NewPrivate()
+			clientKey             = key.NewMachine()
 			serverErr             = make(chan error, 1)
 		)
 		go func() {
@@ -217,8 +217,8 @@ func TestTampering(t *testing.T) {
 		var (
 			clientRaw, serverConn = tsnettest.NewConn("noise", 128000)
 			clientConn            = &readerConn{clientRaw, &tamperReader{clientRaw, 53 + i, 0}}
-			serverKey             = key.NewPrivate()
+			serverKey             = key.NewMachine()
-			clientKey             = key.NewPrivate()
+			clientKey             = key.NewMachine()
 			serverErr             = make(chan error, 1)
 		)
 		go func() {
@@ -258,8 +258,8 @@ func TestTampering(t *testing.T) {
 		var (
 			clientConn, serverRaw = tsnettest.NewConn("noise", 128000)
 			serverConn            = &readerConn{serverRaw, &tamperReader{serverRaw, 101 + i, 0}}
-			serverKey             = key.NewPrivate()
+			serverKey             = key.NewMachine()
-			clientKey             = key.NewPrivate()
+			clientKey             = key.NewMachine()
 			serverErr             = make(chan error, 1)
 		)
 		go func() {

control/noise/interop_test.go

@@ -20,8 +20,8 @@ import (
 func TestInteropClient(t *testing.T) {
 	var (
 		s1, s2      = tsnettest.NewConn("noise", 128000)
-		controlKey  = key.NewPrivate()
+		controlKey  = key.NewMachine()
-		machineKey  = key.NewPrivate()
+		machineKey  = key.NewMachine()
 		serverErr   = make(chan error, 2)
 		serverBytes = make(chan []byte, 1)
 		c2s         = "client>server"
@@ -68,8 +68,8 @@ func TestInteropClient(t *testing.T) {
 func TestInteropServer(t *testing.T) {
 	var (
 		s1, s2      = tsnettest.NewConn("noise", 128000)
-		controlKey  = key.NewPrivate()
+		controlKey  = key.NewMachine()
-		machineKey  = key.NewPrivate()
+		machineKey  = key.NewMachine()
 		clientErr   = make(chan error, 2)
 		clientBytes = make(chan []byte, 1)
 		c2s         = "client>server"
@@ -115,12 +115,13 @@ func TestInteropServer(t *testing.T) {
 // noiseExplorerClient uses the Noise Explorer implementation of Noise
 // IK to handshake as a Noise client on conn, transmit payload, and
 // read+return a payload from the peer.
-func noiseExplorerClient(conn net.Conn, controlKey key.Public, machineKey key.Private, payload []byte) ([]byte, error) {
+func noiseExplorerClient(conn net.Conn, controlKey key.MachinePublic, machineKey key.MachinePrivate, payload []byte) ([]byte, error) {
-	mk := keypair{
+	var mk keypair
-		private_key: machineKey,
+	copy(mk.private_key[:], machineKey.UntypedBytes())
-		public_key:  machineKey.Public(),
+	copy(mk.public_key[:], machineKey.Public().UntypedBytes())
-	}
+	var peerKey [32]byte
-	session := InitSession(true, protocolVersionPrologue(protocolVersion), mk, controlKey)
+	copy(peerKey[:], controlKey.UntypedBytes())
+	session := InitSession(true, protocolVersionPrologue(protocolVersion), mk, peerKey)
 
 	_, msg1 := SendMessage(&session, nil)
 	var hdr [headerLen]byte
@@ -185,11 +186,10 @@ func noiseExplorerClient(conn net.Conn, controlKey key.Public, machineKey key.Pr
 	return p, nil
 }
 
-func noiseExplorerServer(conn net.Conn, controlKey key.Private, wantMachineKey key.Public, payload []byte) ([]byte, error) {
+func noiseExplorerServer(conn net.Conn, controlKey key.MachinePrivate, wantMachineKey key.MachinePublic, payload []byte) ([]byte, error) {
-	mk := keypair{
+	var mk keypair
-		private_key: controlKey,
+	copy(mk.private_key[:], controlKey.UntypedBytes())
-		public_key:  controlKey.Public(),
+	copy(mk.public_key[:], controlKey.Public().UntypedBytes())
-	}
 	session := InitSession(false, protocolVersionPrologue(protocolVersion), mk, [32]byte{})
 
 	var buf [1024]byte

control/noise/key.go

@@ -1,26 +0,0 @@
-// Copyright (c) 2020 Tailscale Inc & AUTHORS All rights reserved.
-// Use of this source code is governed by a BSD-style
-// license that can be found in the LICENSE file.
-
-package noise
-
-// Note that these types are deliberately separate from the types/key
-// package. That package defines generic curve25519 keys, without
-// consideration for how those keys are used. We don't want to
-// encourage mixing machine keys, node keys, and whatever else we
-// might use curve25519 for.
-//
-// Furthermore, the implementation in types/key does some work that is
-// unnecessary for machine keys, and results in a harder to follow
-// implementation. In particular, machine keys do not need to be
-// clamped per the curve25519 spec because they're only used with the
-// X25519 operation, and the X25519 operation defines its own clamping
-// and sanity checking logic. Thus, these keys must be used only with
-// this Noise protocol implementation, and the easiest way to ensure
-// that is a different type.
-
-// PrivateKey is a Tailscale machine private key.
-type PrivateKey [32]byte
-
-// PublicKey is a Tailscale machine public key.
-type PublicKey [32]byte

control/noise/messages.go

@@ -6,12 +6,12 @@ package noise
 
 import "encoding/binary"
 
-// The transport protocol is mostly Noise messages encapsulated in a
+// The control protocol wire format is mostly Noise messages
-// small header describing the payload's type and length. The one
+// encapsulated in a small header describing the payload's type and
-// place we deviate from pure Noise+header is that we also support
+// length. The one place we deviate from pure Noise+header is that we
-// sending an unauthenticated plaintext error as payload, to provide
+// also support sending an unauthenticated plaintext error as payload,
-// an explanation for a connection error that happens before the
+// to provide an explanation for a connection error that happens
-// handshake completes.
+// before the handshake completes.
 //
 // All frames in our protocol have a 5-byte header:
 //
@@ -31,9 +31,10 @@ import "encoding/binary"
 // version numbers. At minimum, the version number must change
 // whenever any particulars of the Noise handshake change
 // (e.g. switching from Noise IK to Noise IKpsk1 or Noise XX), and
-// when security-critical aspects of the "uppper" protocol within the
+// when security-critical aspects of the "uppper" protocol (the one
-// Noise frames change (e.g. how further authentication data is bound
+// running inside the established base protocol session) change
-// to the underlying Noise session).
+// (e.g. how further authentication data is bound to the underlying
+// session).
 
 // headerLen is the size of the header that gets prepended to Noise
 // messages.
@@ -51,7 +52,7 @@ const (
 	// hints only. They are not encrypted or authenticated, and so can
 	// be seen and tampered with on the wire.
 	msgTypeError = 3
-	// msgTypeRecord frames carry a Noise transport message (i.e. "user data").
+	// msgTypeRecord frames carry session data bytes.
 	msgTypeRecord = 4
 )
 
@@ -64,10 +65,8 @@ func hdrVersion(bs []byte) uint16 { return binary.LittleEndian.Uint16(bs[:2]) }
 func hdrType(bs []byte) byte      { return bs[2] }
 func hdrLen(bs []byte) int        { return int(binary.LittleEndian.Uint16(bs[3:5])) }
 
-// initiationMessage is the Noise protocol message sent from a client
+// initiationMessage is the protocol message sent from a client
-// machine to a control server. Aside from the message header, the
+// machine to a control server.
-// values are as specified in the Noise specification for the IK
-// handshake pattern.
 //
 // 5b: header (see headerLen for fields)
 // 32b: client ephemeral public key (cleartext)
@@ -92,10 +91,8 @@ func (m *initiationMessage) EphemeralPub() []byte { return m[headerLen : headerL
 func (m *initiationMessage) MachinePub() []byte   { return m[headerLen+32 : headerLen+32+48] }
 func (m *initiationMessage) Tag() []byte          { return m[headerLen+32+48:] }
 
-// responseMessage is the Noise protocol message sent from a control
+// responseMessage is the protocol message sent from a control server
-// server to a client machine. Aside from the message header, the
+// to a client machine.
-// values are as specified in the Noise specification for the IK
-// handshake pattern.
 //
 // 5b: header (see headerLen for fields)
 // 32b: control ephemeral public key (cleartext)

types/key/machine.go

@@ -77,6 +77,19 @@ func (k *MachinePrivate) UnmarshalText(b []byte) error {
 	return parseHex(k.k[:], mem.B(b), mem.S(machinePrivateHexPrefix))
 }
 
+// UntypedBytes returns k, encoded as an untyped 64-character hex
+// string.
+//
+// Deprecated: this function is risky to use, because it produces
+// serialized values that do not identify themselves as a
+// MachinePrivate, allowing other code to potentially parse it back in
+// as the wrong key type. For new uses that don't require this
+// specific raw byte serialization, please use
+// MarshalText/UnmarshalText.
+func (k MachinePrivate) UntypedBytes() []byte {
+	return append([]byte(nil), k.k[:]...)
+}
+
 // SealTo wraps cleartext into a NaCl box (see
 // golang.org/x/crypto/nacl) to p, authenticated from k, using a
 // random nonce.
@@ -112,6 +125,19 @@ type MachinePublic struct {
 	k [32]byte
 }
 
+// MachinePublicFromRaw32 parses a 32-byte raw value as a MachinePublic.
+//
+// This should be used only when deserializing a MachinePublic from a
+// binary protocol.
+func MachinePublicFromRaw32(raw mem.RO) MachinePublic {
+	if raw.Len() != 32 {
+		panic("input has wrong size")
+	}
+	var ret MachinePublic
+	raw.Copy(ret.k[:])
+	return ret
+}
+
 // ParseMachinePublicUntyped parses an untyped 64-character hex value
 // as a MachinePublic.
 //
@@ -153,6 +179,19 @@ func (k MachinePublic) UntypedHexString() string {
 	return hex.EncodeToString(k.k[:])
 }
 
+// UntypedBytes returns k, encoded as an untyped 64-character hex
+// string.
+//
+// Deprecated: this function is risky to use, because it produces
+// serialized values that do not identify themselves as a
+// MachinePublic, allowing other code to potentially parse it back in
+// as the wrong key type. For new uses that don't require this
+// specific raw byte serialization, please use
+// MarshalText/UnmarshalText.
+func (k MachinePublic) UntypedBytes() []byte {
+	return append([]byte(nil), k.k[:]...)
+}
+
 // String returns the output of MarshalText as a string.
 func (k MachinePublic) String() string {
 	bs, err := k.MarshalText()