Gracefully reject malformed API keys (#8846)

We revoke API keys leaked on GitHub. The leak detection is a best effort and may discover malformed API keys, like API keys that aren't properly base32 encoded. Currently these malformed API keys will cause the admin panel to respond with HTTP 500. We should gracefully reject these malformed API keys.

Addresses #8845

Author: loic-sharma

src/NuGetGallery.Services/Authentication/ApiKeyV4.cs

@@ -121,7 +121,16 @@ private bool TryParseInternal(string plaintextApiKey)
             try
             {
                 var id = plaintextApiKey.Substring(0, IdPartBase32Length);
-                var idBytes = id.AppendBase32Padding().ToUpper().FromBase32String();
+                var validId = id
+                    .AppendBase32Padding()
+                    .ToUpper()
+                    .TryDecodeBase32String(out var idBytes);
+
+                if (!validId)
+                {
+                    return false;
+                }
+
                 bool success = idBytes[0] == IdPrefix[0] && idBytes[1] == IdPrefix[1];
 
                 if (success)

src/NuGetGallery.Services/Extensions/Base32Encoder.cs

@@ -18,6 +18,20 @@ public static string ToBase32String(this byte[] data)
             return Encode(data);
         }
 
+        public static bool TryDecodeBase32String(this string base32String, out byte[] result)
+        {
+            try
+            {
+                result = Decode(base32String);
+                return true;
+            }
+            catch (ArgumentException)
+            {
+                result = Array.Empty<byte>();
+                return false;
+            }
+        }
+
         public static byte[] FromBase32String(this string base32String)
         {
             return Decode(base32String);
@@ -42,7 +56,7 @@ public static string Encode(byte[] data)
         {
             if (data == null)
             {
-                throw new NullReferenceException(nameof(data));
+                throw new ArgumentNullException(nameof(data));
             }
 
             int ncTokens = GetTokenCount(data);
@@ -63,7 +77,7 @@ public static byte[] Decode(string base32String)
         {
             if (base32String == null)
             {
-                throw new NullReferenceException(nameof(base32String));
+                throw new ArgumentNullException(nameof(base32String));
             }
 
             // Validate base32 format

tests/NuGetGallery.Facts/Infrastructure/Authentication/ApiKeyV4Facts.cs

@@ -35,9 +35,10 @@ public void CreatesAValidApiKey()
         [InlineData(" ")]
         [InlineData("abc")]
         [InlineData("SEMTXET5UU6UZDD4AMK57TR46I==")]
+        [InlineData("0000thisis46charactersbutnotvalidbase32encoded")]
         public void TryParseFailsForIllegalApiKeys(string inputApiKey)
         {
-            // Act 
+            // Act
             bool result = ApiKeyV4.TryParse(inputApiKey, out var apiKey);
 
             // Assert

tests/NuGetGallery.Facts/Infrastructure/Authentication/Base32EncoderTests.cs

@@ -2,6 +2,7 @@
 // Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System;
+using System.Collections.Generic;
 using System.Linq;
 using System.Text;
 using NuGetGallery.Infrastructure.Authentication;
@@ -13,55 +14,84 @@ public class Base32EncoderTests
     {
         private Random _random = new Random(0);
 
+        // Maps string to its base32 encoding
+        public static IEnumerable<object[]> ValidBase32 => new List<object[]>
+        {
+            new object[] { "", "" },
+            new object[] { "f", "MY======" },
+            new object[] { "fo", "MZXQ====" },
+            new object[] { "foo", "MZXW6===" },
+            new object[] { "foob", "MZXW6YQ=" },
+            new object[] { "fooba", "MZXW6YTB" },
+            new object[] { "foobar", "MZXW6YTBOI======" },
+        };
+
+        public static IEnumerable<object[]> InvalidBase32 => new List<object[]>
+        {
+            new object[] { "a" },
+            new object[] { "abc" },
+            new object[] { "SEMTXET5UU6UZDD4AMK57TR46I==" },
+            new object[] { "mjwgc===" },
+            new object[] { "GEYq====" },
+        };
+
+        [Fact]
+        public void EncodeThrowsNull()
+        {
+            Assert.Throws<ArgumentNullException>(() => Base32Encoder.Encode(data: null));
+        }
+
         [Fact]
-        public void WhenDataIsNullEncodeThrowsNullReferenceException()
+        public void DecodeThrowsNull()
         {
-            Assert.Throws<NullReferenceException>(() => Base32Encoder.Encode(data: null));
+            Assert.Throws<ArgumentNullException>(() => Base32Encoder.Decode(base32String: null));
         }
 
         [Fact]
-        public void WhenDataIsNullDecodeThrowsNullReferenceException()
+        public void TryDecodeRejectsNull()
         {
-            Assert.Throws<NullReferenceException>(() => Base32Encoder.Decode(base32String: null));
+            string input = null;
+
+            Assert.False(input.TryDecodeBase32String(out var result));
         }
 
         [Theory]
-        [InlineData("a")]
-        [InlineData("abc")]
-        [InlineData("SEMTXET5UU6UZDD4AMK57TR46I==")]
-        [InlineData("mjwgc===")]
-        [InlineData("GEYq====")]
-        public void WhenDataIsNotLegalBase32DecodeThrows(string input)
+        [MemberData(nameof(InvalidBase32))]
+        public void DecodeThrowsInvalidArgument(string input)
         {
             Assert.Throws<ArgumentException>(() => Base32Encoder.Decode(base32String: input));
         }
 
         [Theory]
-        [InlineData("", "")]
-        [InlineData("f", "MY======")]
-        [InlineData("fo", "MZXQ====")]
-        [InlineData("foo", "MZXW6===")]
-        [InlineData("foob", "MZXW6YQ=")]
-        [InlineData("fooba", "MZXW6YTB")]
-        [InlineData("foobar", "MZXW6YTBOI======")]
-        public void WhenValidBase32StringProvideDecodeSucceeds(string decodedString, string base32String)
+        [MemberData(nameof(InvalidBase32))]
+        public void TryParseRejectsInvalidBase32(string input)
+        {
+            Assert.False(input.TryDecodeBase32String(out var result));
+        }
+
+        [Theory]
+        [MemberData(nameof(ValidBase32))]
+        public void DecodesBase32(string decodedString, string base32String)
         {
             Assert.Equal(decodedString, Encoding.ASCII.GetString(base32String.FromBase32String()));
         }
 
         [Theory]
-        [InlineData("", "")]
-        [InlineData("f", "MY======")]
-        [InlineData("fo", "MZXQ====")]
-        [InlineData("foo", "MZXW6===")]
-        [InlineData("foob", "MZXW6YQ=")]
-        [InlineData("fooba", "MZXW6YTB")]
-        [InlineData("foobar", "MZXW6YTBOI======")]
-        public void WhenDataIsEncodedTheResultIsCorrect(string input, string base32String)
+        [MemberData(nameof(ValidBase32))]
+        public void TryDecodesBase32(string decodedString, string base32String)
         {
-            Assert.Equal(base32String, Base32Encoder.ToBase32String(Encoding.ASCII.GetBytes(input)));
+            var success = base32String.TryDecodeBase32String(out var result);
+
+            Assert.True(success);
+            Assert.Equal(decodedString, Encoding.ASCII.GetString(result));
         }
 
+        [Theory]
+        [MemberData(nameof(ValidBase32))]
+        public void EncodesBase32(string input, string base32String)
+        {
+            Assert.Equal(base32String, Base32Encoder.ToBase32String(Encoding.ASCII.GetBytes(input)));
+        }
 
         [Theory]
         [InlineData("")]
@@ -75,10 +105,13 @@ public void WhenDataIsEncodedItCanBeDecoded(string input)
 
             // Act
             var encoded = byteArr.ToBase32String();
-            var decoded = encoded.FromBase32String();
+            var decoded1 = encoded.FromBase32String();
+            var success = encoded.TryDecodeBase32String(out var decoded2);
 
             // Assert           
-            Assert.True(byteArr.SequenceEqual(decoded));
+            Assert.True(success);
+            Assert.True(byteArr.SequenceEqual(decoded1));
+            Assert.True(byteArr.SequenceEqual(decoded2));
         }
 
         [Theory]
@@ -98,10 +131,13 @@ public void WhenPaddingIsRemovedItCanBeAppendedBack(int byteArrayLength)
 
             // Act
             var withPadding = noPadding.AppendBase32Padding();
-            var decoded = withPadding.FromBase32String();
+            var decoded1 = withPadding.FromBase32String();
+            var success = withPadding.TryDecodeBase32String(out var decoded2);
 
             // Assert
-            Assert.True(byteArr.SequenceEqual(decoded));
+            Assert.True(success);
+            Assert.True(byteArr.SequenceEqual(decoded1));
+            Assert.True(byteArr.SequenceEqual(decoded2));
         }
     }
 }