Optimize DecodePubCurveKey and fix off-by-one (#23)

mtmk · web-flow · commit 73d147d62e3d · 2026-03-17T12:28:24.000Z
* Optimize DecodePubCurveKey

* fix format

* Refactor DecodePubCurveKey to use stack allocation and optimize key decoding logic.

* Validate curve key during DecodePubCurveKey and add test to reject non-curve keys.
diff --git a/NATS.NKeys.Tests/NATS.NKeys.Tests.csproj b/NATS.NKeys.Tests/NATS.NKeys.Tests.csproj
@@ -28,13 +28,6 @@
         <Compile Include="..\NATS.NKeys.Benchmarks\NKeysReference1.cs" />
         <Compile Include="..\NATS.NKeys.Benchmarks\NKeysReference2.cs" />
         <Compile Include="..\NATS.NKeys.Benchmarks\FixedRng.cs" />
-        <Compile Include="..\NATS.NKeys\Internal\Crc16.cs" />
-        <Compile Include="..\NATS.NKeys\NaCl\*.cs">
-            <Link>NaCl\%(Filename)%(Extension)</Link>
-        </Compile>
-        <Compile Include="..\NATS.NKeys\NaCl\Internal\Ed25519Ref10\*.cs">
-            <Link>NaCl\Internal\Ed25519Ref10\%(Filename)%(Extension)</Link>
-        </Compile>
     </ItemGroup>
 
     <ItemGroup>
diff --git a/NATS.NKeys.Tests/NKeysTest.cs b/NATS.NKeys.Tests/NKeysTest.cs
@@ -236,6 +236,287 @@ public void Api()
         Assert.False(kp5.Verify(corrupt, signature));
     }
 
+    [Fact]
+    public void DecodePubCurveKey_returns_exactly_32_bytes()
+    {
+        var kp = KeyPair.CreatePair(PrefixByte.Curve);
+        var decoded = KeyPair.DecodePubCurveKey(kp.GetPublicKey());
+        Assert.Equal(32, decoded.Length);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_returns_32_bytes_for_multiple_keys()
+    {
+        // Verify across several independently generated keys
+        for (var i = 0; i < 10; i++)
+        {
+            var kp = KeyPair.CreatePair(PrefixByte.Curve);
+            var decoded = KeyPair.DecodePubCurveKey(kp.GetPublicKey());
+            Assert.Equal(32, decoded.Length);
+        }
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_deterministic_for_known_seed()
+    {
+        var kp = KeyPair.FromSeed("SXAD4F52S2XAJTJ3TGDJ4VXQVW7TU35XJUSVKF25ZRXIWCIUK6NLANRHVY".ToCharArray());
+        var decoded1 = KeyPair.DecodePubCurveKey(kp.GetPublicKey());
+        var decoded2 = KeyPair.DecodePubCurveKey(kp.GetPublicKey());
+        Assert.Equal(decoded1, decoded2);
+        Assert.Equal(32, decoded1.Length);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_matches_known_public_key()
+    {
+        // Known seed/public key pair from existing tests
+        var kp = KeyPair.FromSeed("SXANLPW5OPP62ISXMPTH26DBYM4BGT3U6P2FEALGW3BZAVXQRWCX3KH2ZM".ToCharArray());
+        Assert.Equal("XBIGHMCJSHWYFW6ZY4WWONQ3FRIS5A3OQAUMPPKYBNKRYQTVX4PEAIZA", kp.GetPublicKey());
+        var decoded = KeyPair.DecodePubCurveKey(kp.GetPublicKey());
+        Assert.Equal(32, decoded.Length);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_does_not_include_prefix_byte()
+    {
+        var kp = KeyPair.CreatePair(PrefixByte.Curve);
+        var publicKey = kp.GetPublicKey();
+
+        // Decode the full base32 to get raw bytes including prefix and CRC
+        var rawLen = Base32.GetDataLength(publicKey.ToCharArray());
+        var raw = new byte[rawLen];
+        Base32.FromBase32(publicKey.ToCharArray(), raw);
+
+        // First byte is the prefix
+        var prefixByte = raw[0];
+        Assert.Equal((byte)PrefixByte.Curve, prefixByte);
+
+        // DecodePubCurveKey should return bytes [1..33), not include prefix or CRC
+        var decoded = KeyPair.DecodePubCurveKey(publicKey);
+        Assert.Equal(raw.AsSpan().Slice(1, 32).ToArray(), decoded);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_does_not_include_crc_bytes()
+    {
+        var kp = KeyPair.CreatePair(PrefixByte.Curve);
+        var publicKey = kp.GetPublicKey();
+
+        // Get the raw decoded bytes
+        var rawLen = Base32.GetDataLength(publicKey.ToCharArray());
+        var raw = new byte[rawLen];
+        Base32.FromBase32(publicKey.ToCharArray(), raw);
+
+        // Decoded key should be exactly raw[1..33) — the 32-byte key without prefix or CRC
+        var decoded = KeyPair.DecodePubCurveKey(publicKey);
+        Assert.Equal(32, decoded.Length);
+        Assert.Equal(raw.AsSpan().Slice(1, 32).ToArray(), decoded);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_round_trips_with_seal_open()
+    {
+        var kp1 = KeyPair.FromSeed("SXAD4F52S2XAJTJ3TGDJ4VXQVW7TU35XJUSVKF25ZRXIWCIUK6NLANRHVY".ToCharArray());
+        var kp2 = KeyPair.CreatePair(PrefixByte.Curve);
+
+        var message = new byte[] { 1, 2, 3, 4, 5 };
+        var sealed1 = kp1.Seal(message, kp2.GetPublicKey());
+        var opened = kp2.Open(sealed1, kp1.GetPublicKey());
+        Assert.Equal(message, opened);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_rejects_non_curve_key()
+    {
+        // A valid User public key has the same decoded length but wrong prefix
+        var kp = KeyPair.CreatePair(PrefixByte.User);
+        var ex = Assert.Throws<NKeysException>(() => KeyPair.DecodePubCurveKey(kp.GetPublicKey()));
+        Assert.Equal("Not a valid curve key", ex.Message);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_rejects_too_short()
+    {
+        var ex = Assert.Throws<NKeysException>(() => KeyPair.DecodePubCurveKey("XAAA"));
+        Assert.Equal("Not a valid curve key", ex.Message);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_rejects_too_long()
+    {
+        // Take a valid key and append extra base32 characters
+        var kp = KeyPair.CreatePair(PrefixByte.Curve);
+        var tooLong = kp.GetPublicKey() + "AAAAAAAAAA";
+        Assert.Throws<NKeysException>(() => KeyPair.DecodePubCurveKey(tooLong));
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_rejects_empty_string()
+    {
+        Assert.ThrowsAny<Exception>(() => KeyPair.DecodePubCurveKey(string.Empty));
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_rejects_corrupted_crc()
+    {
+        var kp = KeyPair.CreatePair(PrefixByte.Curve);
+        var publicKey = kp.GetPublicKey();
+
+        // Corrupt a character in the middle of the key
+        var chars = publicKey.ToCharArray();
+        chars[10] = chars[10] == 'A' ? 'B' : 'A';
+        var corrupted = new string(chars);
+
+        var ex = Assert.Throws<NKeysException>(() => KeyPair.DecodePubCurveKey(corrupted));
+        Assert.Equal("Invalid CRC", ex.Message);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_rejects_invalid_base32_characters()
+    {
+        var kp = KeyPair.CreatePair(PrefixByte.Curve);
+        var publicKey = kp.GetPublicKey();
+
+        // Replace with invalid base32 chars (lowercase, digits 0/1/8/9, symbols)
+        var chars = publicKey.ToCharArray();
+        chars[5] = '!';
+        Assert.Throws<ArgumentException>(() => KeyPair.DecodePubCurveKey(new string(chars)));
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_different_keys_produce_different_results()
+    {
+        var kp1 = KeyPair.CreatePair(PrefixByte.Curve);
+        var kp2 = KeyPair.CreatePair(PrefixByte.Curve);
+
+        var decoded1 = KeyPair.DecodePubCurveKey(kp1.GetPublicKey());
+        var decoded2 = KeyPair.DecodePubCurveKey(kp2.GetPublicKey());
+
+        Assert.NotEqual(decoded1, decoded2);
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_cross_validates_with_go_test_data()
+    {
+        // Use the Go-generated test data to verify decode produces
+        // keys that work correctly in seal/open operations
+        var json = JsonNode.Parse(File.ReadAllText("test_data.json"));
+        foreach (var data in json!["xkeys"]!.AsArray())
+        {
+            var pk1 = data!["pk1"]!.GetValue<string>();
+            var pk2 = data!["pk2"]!.GetValue<string>();
+
+            var decoded1 = KeyPair.DecodePubCurveKey(pk1);
+            var decoded2 = KeyPair.DecodePubCurveKey(pk2);
+
+            Assert.Equal(32, decoded1.Length);
+            Assert.Equal(32, decoded2.Length);
+        }
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_blackbox_seal_open_with_decoded_key()
+    {
+        // Black box: if DecodePubCurveKey returns the correct raw key,
+        // then encrypting for that key and decrypting must round-trip
+        var sender = KeyPair.CreatePair(PrefixByte.Curve);
+        var receiver = KeyPair.CreatePair(PrefixByte.Curve);
+
+        var payloads = new byte[][]
+        {
+            Array.Empty<byte>(),
+            new byte[] { 0 },
+            new byte[] { 0xFF },
+            Encoding.UTF8.GetBytes("hello world"),
+            new byte[1024],
+            new byte[8192],
+        };
+
+        foreach (var payload in payloads)
+        {
+            var sealed1 = sender.Seal(payload, receiver.GetPublicKey());
+            var opened = receiver.Open(sealed1, sender.GetPublicKey());
+            Assert.Equal(payload, opened);
+        }
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_blackbox_two_parties_cross_seal()
+    {
+        // Black box: both parties can seal for each other and open
+        var alice = KeyPair.CreatePair(PrefixByte.Curve);
+        var bob = KeyPair.CreatePair(PrefixByte.Curve);
+        var message = Encoding.UTF8.GetBytes("secret message");
+
+        // Alice seals for Bob
+        var sealedForBob = alice.Seal(message, bob.GetPublicKey());
+        Assert.Equal(message, bob.Open(sealedForBob, alice.GetPublicKey()));
+
+        // Bob seals for Alice
+        var sealedForAlice = bob.Seal(message, alice.GetPublicKey());
+        Assert.Equal(message, alice.Open(sealedForAlice, bob.GetPublicKey()));
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_blackbox_wrong_sender_fails_open()
+    {
+        // Black box: opening with wrong sender key must fail
+        var alice = KeyPair.CreatePair(PrefixByte.Curve);
+        var bob = KeyPair.CreatePair(PrefixByte.Curve);
+        var eve = KeyPair.CreatePair(PrefixByte.Curve);
+
+        var sealed1 = alice.Seal(Encoding.UTF8.GetBytes("secret"), bob.GetPublicKey());
+
+        // Bob tries to open with Eve's public key as sender — must fail
+        Assert.ThrowsAny<Exception>(() => bob.Open(sealed1, eve.GetPublicKey()));
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_blackbox_wrong_receiver_fails_open()
+    {
+        // Black box: wrong receiver cannot open
+        var alice = KeyPair.CreatePair(PrefixByte.Curve);
+        var bob = KeyPair.CreatePair(PrefixByte.Curve);
+        var eve = KeyPair.CreatePair(PrefixByte.Curve);
+
+        var sealed1 = alice.Seal(Encoding.UTF8.GetBytes("secret"), bob.GetPublicKey());
+
+        // Eve tries to open something meant for Bob
+        Assert.ThrowsAny<Exception>(() => eve.Open(sealed1, alice.GetPublicKey()));
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_blackbox_tampered_ciphertext_fails()
+    {
+        var alice = KeyPair.CreatePair(PrefixByte.Curve);
+        var bob = KeyPair.CreatePair(PrefixByte.Curve);
+
+        var sealed1 = alice.Seal(Encoding.UTF8.GetBytes("secret"), bob.GetPublicKey());
+
+        // Flip a byte in the encrypted payload (after version + nonce header)
+        sealed1[30] ^= 0xFF;
+
+        Assert.ThrowsAny<Exception>(() => bob.Open(sealed1, alice.GetPublicKey()));
+    }
+
+    [Fact]
+    public void DecodePubCurveKey_blackbox_each_seal_produces_different_ciphertext()
+    {
+        // Black box: nonce randomization means same plaintext encrypts differently
+        var alice = KeyPair.CreatePair(PrefixByte.Curve);
+        var bob = KeyPair.CreatePair(PrefixByte.Curve);
+        var message = Encoding.UTF8.GetBytes("hello");
+
+        var sealed1 = alice.Seal(message, bob.GetPublicKey());
+        var sealed2 = alice.Seal(message, bob.GetPublicKey());
+
+        Assert.NotEqual(sealed1, sealed2);
+
+        // But both decrypt to the same message
+        Assert.Equal(message, bob.Open(sealed1, alice.GetPublicKey()));
+        Assert.Equal(message, bob.Open(sealed2, alice.GetPublicKey()));
+    }
+
     [Fact]
     public void Public_key_does_not_have_seed_nor_secret_key()
     {
diff --git a/NATS.NKeys/KeyPair.cs b/NATS.NKeys/KeyPair.cs
@@ -231,7 +231,6 @@ public byte[] Seal(byte[] data, string receiver)
             throw new NKeysException("Curve key only operation");
         }
 
-        // TODO optimize
         var rpub = DecodePubCurveKey(receiver);
 
         var nonce = new byte[CurveNonceLen];
@@ -268,7 +267,6 @@ public byte[] Open(byte[] input, string sender)
             throw new NKeysException("Curve key only operation");
         }
 
-        // TODO optimize
         if (input.Length <= Vlen + CurveNonceLen)
         {
             throw new NKeysException("Encrypted input is not valid");
@@ -302,6 +300,40 @@ public void Dispose()
         CryptoBytes.Wipe(_pk);
     }
 
+    internal static byte[] DecodePubCurveKey(string key)
+    {
+        var pub = new byte[CurveKeyLen];
+        DecodePubCurveKey(key, pub);
+        return pub;
+    }
+
+    internal static void DecodePubCurveKey(string key, Span<byte> pub)
+    {
+        Span<byte> buf = stackalloc byte[CurveDecodeLen];
+        var keySpan = key.AsSpan();
+        var length = Base32.GetDataLength(keySpan);
+
+        if (length != CurveDecodeLen)
+        {
+            throw new NKeysException("Not a valid curve key");
+        }
+
+        Base32.FromBase32(keySpan, buf);
+
+        if (buf[0] != (byte)PrefixByte.Curve)
+        {
+            throw new NKeysException("Not a valid curve key");
+        }
+
+        var crc = (ushort)(buf[CurveDecodeLen - 2] | buf[CurveDecodeLen - 1] << 8);
+        if (crc != Crc16.Checksum(buf.Slice(0, CurveDecodeLen - 2)))
+        {
+            ThrowInvalidCrcException();
+        }
+
+        buf.Slice(1, CurveKeyLen).CopyTo(pub);
+    }
+
     private static string Encode(byte prefixByte, bool seed, byte[] src)
     {
         if (!IsValidPublicPrefixByte(prefixByte))
@@ -443,27 +475,4 @@ or NKeysConstants.PrefixByteUser
 
     [MethodImpl(MethodImplOptions.NoInlining)]
     private static void ThrowInvalidCurveKeyOperationException() => throw new NKeysException("Invalid curve key operation");
-
-    private static byte[] DecodePubCurveKey(string key)
-    {
-        // TODO optimize
-        var length = Base32.GetDataLength(key.ToCharArray());
-        var buf = new Span<byte>(new byte[length]);
-        Base32.FromBase32(key.ToCharArray(), buf);
-
-        if (buf.Length != CurveDecodeLen)
-        {
-            throw new NKeysException("Not a valid curve key");
-        }
-
-        var crc = (ushort)(buf[length - 2] | buf[length - 1] << 8);
-        if (crc != Crc16.Checksum(buf.Slice(0, length - 2)))
-        {
-            ThrowInvalidCrcException();
-        }
-
-        var pub = buf.Slice(1, length - 2).ToArray();
-
-        return pub;
-    }
 }
diff --git a/NATS.NKeys/NATS.NKeys.csproj b/NATS.NKeys/NATS.NKeys.csproj
@@ -24,4 +24,9 @@
         <PackageReference Include="Microsoft.CodeAnalysis.PublicApiAnalyzers" Version="3.3.4" ExcludeAssets="Compile" PrivateAssets="All" />
     </ItemGroup>
 
+    <ItemGroup>
+        <InternalsVisibleTo Include="NATS.NKeys.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100DB7DA1F2F89089327B47D26D69666FAD20861F24E9ACDB13965FB6C64DFEE8DA589B495DF37A75E934DDBACB0752A42C40F3DBC79614EEC9BB2A0B6741F9E2AD2876F95E74D54C23EEF0063EB4EFB1E7D824EE8A695B647C113C92834F04A3A83FB60F435814DDF5C4E5F66A168139C4C1B1A50A3E60C164D180E265B1F000CD" />
+        <InternalsVisibleTo Include="NATS.NKeys.NaCl.Tests, PublicKey=0024000004800000940000000602000000240000525341310004000001000100DB7DA1F2F89089327B47D26D69666FAD20861F24E9ACDB13965FB6C64DFEE8DA589B495DF37A75E934DDBACB0752A42C40F3DBC79614EEC9BB2A0B6741F9E2AD2876F95E74D54C23EEF0063EB4EFB1E7D824EE8A695B647C113C92834F04A3A83FB60F435814DDF5C4E5F66A168139C4C1B1A50A3E60C164D180E265B1F000CD" />
+    </ItemGroup>
+
 </Project>
