amadan

rastrillo / aviso Public

Refuse non-canonical private key spellings; pin the key-id derivation

Codex's review of the key parser: RawURLEncoding tolerates newlines
and non-zero trailing bits, so two spellings of one scalar passed the
single-spelling rule the comment claimed. Require the decoded bytes to
re-encode to the input. Tests now cover the group order boundary
(n-1 accepted; n and n+1 refused), a padded but otherwise valid key,
and that the key id is SHA-256 of the public point rather than any
URL-safe string.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev 8f87a42861dca366d55f31ec39071bc89dd93abf parent 0f9b4ea
2 files changed, +40 −5
  • vapid.go +5 −0
  • vapid_test.go +35 −5
diff --git a/vapid.go b/vapid.go
index 9326733..d778283 100644
--- a/vapid.go
+++ b/vapid.go
@@ -48,6 +48,11 @@ func parsePrivateKey(s string) (pub, keyID string, err error) {
if err != nil || len(raw) != 32 {
return "", "", ErrInvalidPrivateKey
}
+ // The decoder tolerates newlines and non-zero trailing bits; only
+ // the spelling that round-trips is the one aviso-key printed.
+ if base64.RawURLEncoding.EncodeToString(raw) != s {
+ return "", "", ErrInvalidPrivateKey
+ }
// NewPrivateKey rejects zero and out-of-range scalars.
k, err := ecdh.P256().NewPrivateKey(raw)
if err != nil {
diff --git a/vapid_test.go b/vapid_test.go
index 59af685..50c316b 100644
--- a/vapid_test.go
+++ b/vapid_test.go
@@ -3,8 +3,10 @@ package aviso
import (
"crypto/ecdh"
"crypto/rand"
+ "crypto/sha256"
"encoding/base64"
"errors"
+ "math/big"
"strings"
"testing"
)
@@ -41,12 +43,27 @@ func TestParsePrivateKeyRefusesBadInput(t *testing.T) {
if _, _, err := parsePrivateKey(""); !errors.Is(err, ErrEmptyPrivateKey) {
t.Fatalf("empty: got %v, want ErrEmptyPrivateKey", err)
}
- zero := base64.RawURLEncoding.EncodeToString(make([]byte, 32))
+ enc := base64.RawURLEncoding.EncodeToString
+ zero := enc(make([]byte, 32))
+ // The P-256 group order n, and n-1 (the largest valid scalar).
+ n, _ := new(big.Int).SetString("ffffffff00000000ffffffffffffffffbce6faada7179e84f3b9cac2fc632551", 16)
+ nMinus1 := new(big.Int).Sub(n, big.NewInt(1))
+ nPlus1 := new(big.Int).Add(n, big.NewInt(1))
+ one := make([]byte, 32)
+ one[31] = 1
+ good := enc(one)
+ if _, _, err := parsePrivateKey(enc(nMinus1.FillBytes(make([]byte, 32)))); err != nil {
+ t.Errorf("n-1 refused: %v", err)
+ }
for name, in := range map[string]string{
- "not base64": "!!!",
- "short": base64.RawURLEncoding.EncodeToString([]byte("short")),
- "zero scalar": zero,
- "padded": zero + "=",
+ "not base64": "!!!",
+ "short": enc([]byte("short")),
+ "zero scalar": zero,
+ "order n": enc(n.FillBytes(make([]byte, 32))),
+ "above n": enc(nPlus1.FillBytes(make([]byte, 32))),
+ "padded": good + "=",
+ "newline": good[:10] + "\n" + good[10:],
+ "trailing bits": good[:len(good)-1] + "F", // canonical ending is "E"
} {
if _, _, err := parsePrivateKey(in); !errors.Is(err, ErrInvalidPrivateKey) {
t.Errorf("%s: got %v, want ErrInvalidPrivateKey", name, err)
@@ -54,6 +71,19 @@ func TestParsePrivateKeyRefusesBadInput(t *testing.T) {
}
}
+func TestKeyIDIsSHA256OfThePublicPoint(t *testing.T) {
+ priv, _ := GenerateKey()
+ pub, id, err := parsePrivateKey(priv)
+ if err != nil {
+ t.Fatal(err)
+ }
+ point, _ := base64.RawURLEncoding.DecodeString(pub)
+ sum := sha256.Sum256(point)
+ if want := base64.RawURLEncoding.EncodeToString(sum[:]); id != want {
+ t.Fatalf("key id = %s, want %s", id, want)
+ }
+}
+
func TestParsePrivateKeyAgreesWithECDH(t *testing.T) {
k, err := ecdh.P256().GenerateKey(rand.Reader)
if err != nil {