improve docs and implementation of Signature, PrivateKey and PublicKey

This commit improves the documentation for the core minisign types and
improves the implementation for various functions. It also adds an
additional test vector for (un)marshaling private keys.

Signed-off-by: Andreas Auernhammer <[email protected]>

Author: aead

.golangci.yml

@@ -1,7 +1,4 @@
 linters-settings:
-  golint:
-    min-confidence: 0
-
   misspell:
     locale: US
 
@@ -11,24 +8,27 @@ linters-settings:
 linters:
   disable-all: true
   enable:
-    - typecheck
+    - durationcheck
+    - gocritic
+    - gofmt
     - goimports
-    - misspell
-    - staticcheck
+    - gomodguard
     - govet
-    - revive
     - ineffassign
-    - gosimple
-    - unused
-    - prealloc
+    - misspell
+    - revive
+    - staticcheck
+    - tenv
+    - typecheck
     - unconvert
-    - gofumpt
+    - unused
 
 issues:
   exclude-use-default: false
   exclude:
-      - "var-naming: don't use ALL_CAPS in Go names; use CamelCase"
       - "package-comments: should have a package comment"
+      - "exitAfterDefer:"
+      - "captLocal:"
 
 service:
-  golangci-lint-version: 1.48.0 # use the fixed version to not introduce new linters unexpectedly
+  golangci-lint-version: 1.57.2 # use the fixed version to not introduce new linters unexpectedly

internal/testdata/minisign_unencrypted.key

@@ -0,0 +1,5 @@
+untrusted comment: minisign encrypted secret key
+RWQAAEIyAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAbuUYgQpHKDcmmMQj9cgqohWX321PrXUDFfCVWOXDZp8kLw2/qju66KnI28LcOaA7ZywNP5vDVtlHeyzit3lxeqirS5+2UImrAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=
+
+untrusted comment: minisign encrypted secret key
+RWQAAEIyAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAb/yydu4x5dcvbgaLZRtY5v8wFvgzMkvKyALUXUWcT+bvaqFvuvkUyUfMd7ozqYIs8zOaPqWf6EjnWSqkOpOQiD1UJpOgCFm0AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA=

internal/testdata/unencrypted.key

@@ -0,0 +1,2 @@
+untrusted comment: minisign public key D7E531EE76B2FC6F
+RWRv/LJ27jHl10fMd7ozqYIs8zOaPqWf6EjnWSqkOpOQiD1UJpOgCFm0

minisign.pub

@@ -106,6 +106,8 @@ func (p PrivateKey) Equal(x crypto.PrivateKey) bool {
 //
 // For password-protected private keys refer to [EncryptKey].
 func (p PrivateKey) MarshalText() ([]byte, error) {
+	// A non-encrypted private key has the same format as an encrypted one.
+	// However, the salt, and auth. tag are set to all zero.
 	var b [privateKeySize]byte
 
 	binary.LittleEndian.PutUint16(b[:], EdDSA)
@@ -115,6 +117,8 @@ func (p PrivateKey) MarshalText() ([]byte, error) {
 	binary.LittleEndian.PutUint64(b[54:], p.id)
 	copy(b[62:], p.bytes[:])
 
+	// It seems odd that the comment says: "encrypted secret key".
+	// However, the original C implementation behaves like this.
 	const comment = "untrusted comment: minisign encrypted secret key\n"
 	encodedBytes := make([]byte, len(comment)+base64.StdEncoding.EncodedLen(len(b)))
 	copy(encodedBytes, []byte(comment))
@@ -140,7 +144,7 @@ func (p *PrivateKey) UnmarshalText(text []byte) error {
 	}
 
 	var (
-		empty [32]byte
+		empty [32]byte // For checking that the salt/tag are empty
 
 		kType     = binary.LittleEndian.Uint16(b)
 		kdf       = binary.LittleEndian.Uint16(b[2:])
@@ -149,7 +153,7 @@ func (p *PrivateKey) UnmarshalText(text []byte) error {
 		scryptOps = binary.LittleEndian.Uint64(b[38:])
 		scryptMem = binary.LittleEndian.Uint64(b[46:])
 		key       = b[54:126]
-		checksum  = b[126:privateKeySize]
+		tag       = b[126:privateKeySize]
 	)
 	if kType != EdDSA {
 		return fmt.Errorf("minisign: invalid private key: invalid key type '%d'", kType)
@@ -163,7 +167,7 @@ func (p *PrivateKey) UnmarshalText(text []byte) error {
 	if hType != algorithmBlake2b {
 		return fmt.Errorf("minisign: invalid private key: invalid hash type '%d'", hType)
 	}
-	if !bytes.Equal(salt[:], empty[:]) {
+	if !bytes.Equal(salt, empty[:]) {
 		return errors.New("minisign: invalid private key: salt is not empty")
 	}
 	if scryptOps != 0 {
@@ -172,11 +176,11 @@ func (p *PrivateKey) UnmarshalText(text []byte) error {
 	if scryptMem != 0 {
 		return errors.New("minisign: invalid private key: scrypt mem parameter is not zero")
 	}
-	if !bytes.Equal(checksum, empty[:]) {
+	if !bytes.Equal(tag, empty[:]) {
 		return errors.New("minisign: invalid private key: salt is not empty")
 	}
 
-	p.id = binary.LittleEndian.Uint64(key[:8])
+	p.id = binary.LittleEndian.Uint64(key)
 	copy(p.bytes[:], key[8:])
 	return nil
 }
@@ -235,10 +239,7 @@ func IsEncrypted(privateKey []byte) bool {
 	}
 	bytes = bytes[:n]
 
-	if len(bytes) != privateKeySize {
-		return false
-	}
-	return binary.LittleEndian.Uint16(bytes[2:4]) == algorithmScrypt
+	return len(bytes) >= 4 && binary.LittleEndian.Uint16(bytes[2:]) == algorithmScrypt
 }
 
 var errDecrypt = errors.New("minisign: decryption failed")
@@ -247,47 +248,50 @@ var errDecrypt = errors.New("minisign: decryption failed")
 // the given password.
 func DecryptKey(password string, privateKey []byte) (PrivateKey, error) {
 	privateKey = trimUntrustedComment(privateKey)
-	bytes := make([]byte, base64.StdEncoding.DecodedLen(len(privateKey)))
-	n, err := base64.StdEncoding.Decode(bytes, privateKey)
+	b := make([]byte, base64.StdEncoding.DecodedLen(len(privateKey)))
+	n, err := base64.StdEncoding.Decode(b, privateKey)
 	if err != nil {
 		return PrivateKey{}, err
 	}
-	bytes = bytes[:n]
+	b = b[:n]
 
-	if len(bytes) != privateKeySize {
+	if len(b) != privateKeySize {
 		return PrivateKey{}, errDecrypt
 	}
-	if a := binary.LittleEndian.Uint16(bytes[:2]); a != EdDSA {
+	var (
+		kType      = binary.LittleEndian.Uint16(b)
+		kdf        = binary.LittleEndian.Uint16(b[2:])
+		hType      = binary.LittleEndian.Uint16(b[4:])
+		salt       = b[6:38]
+		scryptOps  = binary.LittleEndian.Uint64(b[38:])
+		scryptMem  = binary.LittleEndian.Uint64(b[46:])
+		ciphertext = b[54:]
+	)
+	if kType != EdDSA {
 		return PrivateKey{}, errDecrypt
 	}
-	if a := binary.LittleEndian.Uint16(bytes[2:4]); a != algorithmScrypt {
+	if kdf != algorithmScrypt {
 		return PrivateKey{}, errDecrypt
 	}
-	if a := binary.LittleEndian.Uint16(bytes[4:6]); a != algorithmBlake2b {
+	if hType != algorithmBlake2b {
 		return PrivateKey{}, errDecrypt
 	}
-
-	var (
-		scryptOps = binary.LittleEndian.Uint64(bytes[38:46])
-		scryptMem = binary.LittleEndian.Uint64(bytes[46:54])
-	)
 	if scryptOps > scryptOpsLimit {
 		return PrivateKey{}, errDecrypt
 	}
 	if scryptMem > scryptMemLimit {
 		return PrivateKey{}, errDecrypt
 	}
-	var salt [32]byte
-	copy(salt[:], bytes[6:38])
-	privateKeyBytes, err := decryptKey(password, salt[:], scryptOps, scryptMem, bytes[54:])
+
+	plaintext, err := decryptKey(password, salt, scryptOps, scryptMem, ciphertext)
 	if err != nil {
 		return PrivateKey{}, err
 	}
 
 	key := PrivateKey{
-		id: binary.LittleEndian.Uint64(privateKeyBytes[:8]),
+		id: binary.LittleEndian.Uint64(plaintext),
 	}
-	copy(key.bytes[:], privateKeyBytes[8:])
+	copy(key.bytes[:], plaintext[8:])
 	return key, nil
 }
 
@@ -368,7 +372,7 @@ func decryptKey(password string, salt []byte, ops, mem uint64, ciphertext []byte
 	binary.LittleEndian.PutUint16(message[:2], EdDSA)
 	copy(message[2:], privateKeyBytes)
 
-	if sum := blake2b.Sum256(message[:]); subtle.ConstantTimeCompare(sum[:], checksum[:]) != 1 {
+	if sum := blake2b.Sum256(message[:]); subtle.ConstantTimeCompare(sum[:], checksum) != 1 {
 		return nil, errDecrypt
 	}
 	return privateKeyBytes, nil

private.go

@@ -6,21 +6,88 @@ package minisign
 
 import (
 	"bytes"
+	"encoding/base64"
 	"os"
+	"strconv"
 	"testing"
 )
 
+var marshalPrivateKeyTests = []struct {
+	ID    uint64
+	Bytes []byte
+}{
+	{
+		ID:    htoi("3728470A8118E56E"),
+		Bytes: b64("JpjEI/XIKqIVl99tT611AxXwlVjlw2afJC8Nv6o7uuipyNvC3DmgO2csDT+bw1bZR3ss4rd5cXqoq0uftlCJqw=="),
+	},
+	{
+		ID:    htoi("D7E531EE76B2FC6F"),
+		Bytes: b64("L24Gi2UbWOb/MBb4MzJLysgC1F1FnE/m72qhb7r5FMlHzHe6M6mCLPMzmj6ln+hI51kqpDqTkIg9VCaToAhZtA=="),
+	},
+}
+
+func TestPrivateKey_Marshal(t *testing.T) {
+	raw, err := os.ReadFile("./internal/testdata/unencrypted.key")
+	if err != nil {
+		t.Fatalf("Failed to read private key: %v", err)
+	}
+	raw = bytes.ReplaceAll(raw, []byte{'\r', '\n'}, []byte{'\n'})
+	raw = bytes.TrimSuffix(raw, []byte{'\n'})
+
+	keys := bytes.Split(raw, []byte{'\n', '\n'}) // Private keys are separated by a newline
+	if len(keys) != len(marshalPrivateKeyTests) {
+		t.Fatalf("Test vectors don't match: got %d - want %d", len(marshalPrivateKeyTests), len(keys))
+	}
+	for i, test := range marshalPrivateKeyTests {
+		key := PrivateKey{
+			id: test.ID,
+		}
+		copy(key.bytes[:], test.Bytes)
+
+		text, err := key.MarshalText()
+		if err != nil {
+			t.Fatalf("Test %d: failed to marshal private key: %v", i, err)
+		}
+		if !bytes.Equal(text, keys[i]) {
+			t.Log(len(text), len(keys[i]))
+			t.Log(string(keys[i][len(keys[i])-1]))
+			t.Fatalf("Test %d: failed to marshal private key:\nGot: %v\nWant: %v\n", i, text, keys[i])
+		}
+	}
+}
+
 func TestPrivateKey_Unmarshal(t *testing.T) {
-	raw, err := os.ReadFile("./internal/testdata/minisign_unencrypted.key")
+	raw, err := os.ReadFile("./internal/testdata/unencrypted.key")
 	if err != nil {
 		t.Fatalf("Failed to read private key: %v", err)
 	}
+	raw = bytes.ReplaceAll(raw, []byte{'\r', '\n'}, []byte{'\n'})
+	raw = bytes.TrimSuffix(raw, []byte{'\n'})
 
-	keys := bytes.Split(raw, []byte("\n\n")) // Private keys are separated by a newline
+	keys := bytes.Split(raw, []byte{'\n', '\n'}) // Private keys are separated by a newline
 	for _, k := range keys {
 		var key PrivateKey
 		if err := key.UnmarshalText(k); err != nil {
 			t.Fatalf("Failed to unmarshal private key: %v\nPrivate key:\n%s", err, string(k))
 		}
+
+		// Print test vector for marshaling:
+		// t.Logf("\n{\n\tID: htoi(\"%X\"),\n\tBytes: b64(\"%s\"),\n}", key.id, base64.StdEncoding.EncodeToString(key.bytes[:]))
+	}
+}
+
+func htoi(s string) uint64 {
+	i, err := strconv.ParseUint(s, 16, 64)
+	if err != nil {
+		panic(err)
+	}
+	return i
+}
+
+func b64(s string) []byte {
+	b, err := base64.StdEncoding.DecodeString(s)
+	if err != nil {
+		panic(err)
 	}
+	return b
 }

private_test.go

@@ -16,10 +16,11 @@ import (
 	"strings"
 )
 
-// PublicKeyFromFile reads a new PublicKey from the
-// given file.
-func PublicKeyFromFile(path string) (PublicKey, error) {
-	bytes, err := os.ReadFile(path)
+const publicKeySize = 2 + 8 + ed25519.PublicKeySize
+
+// PublicKeyFromFile reads a PublicKey from the given file.
+func PublicKeyFromFile(filename string) (PublicKey, error) {
+	bytes, err := os.ReadFile(filename)
 	if err != nil {
 		return PublicKey{}, err
 	}
@@ -57,7 +58,7 @@ func (p PublicKey) Equal(x crypto.PublicKey) bool {
 
 // String returns a base64 string representation of the PublicKey p.
 func (p PublicKey) String() string {
-	var bytes [2 + 8 + ed25519.PublicKeySize]byte
+	var bytes [publicKeySize]byte
 	binary.LittleEndian.PutUint16(bytes[:2], EdDSA)
 	binary.LittleEndian.PutUint64(bytes[2:10], p.ID())
 	copy(bytes[10:], p.bytes[:])
@@ -69,22 +70,27 @@ func (p PublicKey) String() string {
 //
 // It never returns an error.
 func (p PublicKey) MarshalText() ([]byte, error) {
-	comment := "untrusted comment: minisign public key: " + strings.ToUpper(strconv.FormatUint(p.ID(), 16)) + "\n"
-	return []byte(comment + p.String()), nil
+	s := make([]byte, 0, 113) // Size of a public key in text format
+	s = append(s, "untrusted comment: minisign public key: "...)
+	s = append(s, strings.ToUpper(strconv.FormatUint(p.ID(), 16))...)
+	s = append(s, '\n')
+	s = append(s, p.String()...)
+	return s, nil
 }
 
-// UnmarshalText parses text as textual-encoded public key.
-// It returns an error if text is not a well-formed public key.
+// UnmarshalText decodes a textual representation of a public key into p.
+//
+// It returns an error in case of a malformed key.
 func (p *PublicKey) UnmarshalText(text []byte) error {
 	text = trimUntrustedComment(text)
 	bytes := make([]byte, base64.StdEncoding.DecodedLen(len(text)))
 	n, err := base64.StdEncoding.Decode(bytes, text)
 	if err != nil {
 		return fmt.Errorf("minisign: invalid public key: %v", err)
 	}
-	bytes = bytes[:n] // Adjust b/c text may contain '\r' or '\n' which would have been ignored during decoding.
+	bytes = bytes[:n] // Adjust since text may contain '\r' or '\n' which would have been ignored during decoding.
 
-	if n = len(bytes); n != 2+8+ed25519.PublicKeySize {
+	if n = len(bytes); n != publicKeySize {
 		return errors.New("minisign: invalid public key length " + strconv.Itoa(n))
 	}
 	if a := binary.LittleEndian.Uint16(bytes[:2]); a != EdDSA {