From 2a417d0ad66fc51c4011d05f11dd521026b8625e Mon Sep 17 00:00:00 2001 From: Nick Guerrera Date: Fri, 17 Jan 2025 13:34:08 -0600 Subject: [PATCH] PR feedback --- devdocs/Questions.md | 7 -- docs/PrimaryKey.md | 8 +- src/Cask/Cask.cs | 15 ++-- src/Cask/CaskKey.cs | 13 +--- src/Cask/CaskVersion.cs | 12 --- src/Cask/Helpers.cs | 34 ++------ src/Cask/InternalConstants.cs | 77 +++++++++---------- src/Cask/KeyKind.cs | 1 + .../Cask.Benchmarks/CompareHashBenchmarks.cs | 2 +- .../Cask.Benchmarks/GenerateHashBenchmarks.cs | 2 +- .../Cask.Benchmarks/GenerateKeyBenchmarks.cs | 2 +- src/Tests/Cask.Tests/CaskSecretsTests.cs | 13 ++-- 12 files changed, 68 insertions(+), 118 deletions(-) delete mode 100644 src/Cask/CaskVersion.cs diff --git a/devdocs/Questions.md b/devdocs/Questions.md index 7b661af..c3d95fb 100644 --- a/devdocs/Questions.md +++ b/devdocs/Questions.md @@ -1,11 +1,4 @@ # Questions -1. MD5 vs. CRC32 (or something else)? - - CRC32 is a good choice, we're using it for its intended purpose. - - Can we make room for all four bytes? There isn't a lot of prior art for truncating CRC-32 like SHAs. - - Reserving a byte or two for future use by us might be a good idea anyway? - - Possible uses: - - Record secret entropy length. Right now, it's a bit of a footgun that you have to pass the correct length to Generate/CompareHash. - 1. Should provider data be allowed to be unaligned. Should we pad it instead of throwing? 1. Should we combine Cask and CaskKey, putting the statics as helpers on the struct? diff --git a/docs/PrimaryKey.md b/docs/PrimaryKey.md index 4b264ca..baa698e 100644 --- a/docs/PrimaryKey.md +++ b/docs/PrimaryKey.md @@ -63,8 +63,8 @@ |decodedKey[^15..^12]| 37, 4, 9 |0x25, 0x04, 0x09| 00100101b, 00000100b, 00001001b | Decoded 'JQQJ' signature. |decodedKey[^12..^9]|0...255|0x0...0xFF|00000000b...11111111b| Provider identifier, e.g. , '0x4c', '0x44', '0x93' (base64 encoded as 'TEST') |decodedKey[^9..^6]||||Time stamp data encoded in 4 six-bit blocks for YMDH. -|decodedKey[^6]|0, 28, 32|0x00, 0x1c, 0x20|00000000b, 00011100b, 00100000b| Leading 6 bits comprises kind enum followed by 2 bits of reserved zero padding. -|decodedKey[^5]|0|0x00|00000000b| Leading 4 bits comprise version number (currenty 0 = Cask 1.0), followed by 4 bits of reserved zero padding. +|decodedKey[^6]|0, 28, 32|0x00, 0x1c, 0x20|00000000b, 00011100b, 00100000b | Leading 6 bits comprises kind enum followed by 2 bits of reserved zero padding. +|decodedKey[^5]|0|0x00|00000000b| Reserved. Must be zero. |decodedKey[^4..]|0...255|0x0...0xFF|00000000b..11111111b|CRC32(key[..^4]) ## Primary 256-bit Key Base64-Encoded Rendering @@ -82,7 +82,7 @@ |encodedKey[^10]|'A'...'Z'\|'a'..'e'|Represents the day of key allocation, 'A' (0) to 'e' (31)| |encodedKey[^9]|'A'...'X'|Represents the hour of key allocation, 'A' (hour 0 or midnight) to 'X' (hour 23). [TBD: This value could be used for half hour increments instead]. |encodedKey[^8]|'A', 'H', 'I'|Represents the key kind, a 256-bit primary key, HMAC256 or HMAC384. -|encodedKey[^7]|'A'|Cask version 1.0 -|encodedKey[^6]| 'A'...'D'| Encoded character of four leading zero bits followed by the first two bits of the CRC32 checksum. +|encodedKey[^7]|'A'| Reserved. Must be 'A'. +|encodedKey[^6]|'A'...'D'| Encoded character of four leading zero bits followed by the first two bits of the CRC32 checksum. |encodedKey[^5..]|'A'...'_'|The final five encoded checksum characters representing the remaining 30 bits of the CRC32 checksum. ``` diff --git a/src/Cask/Cask.cs b/src/Cask/Cask.cs index 2a01a78..0a249ee 100644 --- a/src/Cask/Cask.cs +++ b/src/Cask/Cask.cs @@ -35,6 +35,7 @@ public static bool IsCask(ReadOnlySpan key) return false; } + // Check for CASK signature, "JQQJ". if (!key[CaskSignatureCharRange].SequenceEqual(CaskSignature)) { return false; @@ -74,6 +75,7 @@ public static bool IsCaskUtf8(ReadOnlySpan keyUtf8) return false; } + // Check for CASK signature, "JQQJ" UTF-8 encoded. if (!keyUtf8[CaskSignatureCharRange].SequenceEqual(CaskSignatureUtf8)) { return false; @@ -108,26 +110,27 @@ public static bool IsCaskUtf8(ReadOnlySpan keyUtf8) /// public static bool IsCaskBytes(ReadOnlySpan keyBytes) { - // Check length is within limits and 3-byte aligned. if (keyBytes.Length < MinKeyLengthInBytes || keyBytes.Length > MaxKeyLengthInBytes || !Is3ByteAligned(keyBytes.Length)) { return false; } - // Check CASK signature + // Check for CASK signature. "JQQJ" base64-decoded. if (!keyBytes[CaskSignatureByteRange].SequenceEqual(CaskSignatureBytes)) { return false; } - // Check kind. NOTE: Hash384Bit is not implemented yet. + // Check that kind is valid. NOTE: 'Hash384Bit' is not implemented yet + // and is therefore treated as invalid here for now. if (!TryByteToKind(keyBytes[KindByteIndex], out KeyKind kind) || kind is not KeyKind.Key256Bit and not KeyKind.Hash256Bit) { return false; } - // Check version: must be 1.0.0 currently. - if (!TryByteToVersion(keyBytes[VersionByteIndex], out CaskVersion version) || version is not CaskVersion.OneZeroZero) + // Check that reserved version byte is zeroed out. If not, this might be + // a CASK key from a future version that we do not support. + if (keyBytes[ReservedVersionByteIndex] != 0) { return false; } @@ -257,7 +260,7 @@ private static void FinalizeKey(Span key, KeyKind kind, ReadOnlySpan timestamp.CopyTo(key[TimestampByteRange]); } - key[VersionByteIndex] = VersionToByte(CaskVersion.OneZeroZero); + key[ReservedVersionByteIndex] = 0; key[KindByteIndex] = KindToByte(kind); Crc32.Hash(key[..Crc32ByteRange.Start], key[Crc32ByteRange]); } diff --git a/src/Cask/CaskKey.cs b/src/Cask/CaskKey.cs index 4bf32f2..57555c3 100644 --- a/src/Cask/CaskKey.cs +++ b/src/Cask/CaskKey.cs @@ -7,6 +7,8 @@ using System.Text; using System.Text.RegularExpressions; +using static System.Text.RegularExpressions.RegexOptions; + namespace CommonAnnotatedSecurityKeys; /// @@ -36,15 +38,6 @@ public KeyKind Kind } } - public CaskVersion CaskVersion - { - get - { - ThrowIfUnitialized(); - return CharToVersion(_key[VersionCharIndex]); - } - } - public int SizeInBytes { get @@ -186,7 +179,7 @@ private void ThrowIfUnitialized() // language=regex private const string RegexPattern = """(^|[^A-Za-z0-9+/-_])([A-Za-z0-9-_]{4}){6,}JQQJ[A-Za-z0-9-_]{16}($|[^A-Za-z0-9+/-_])"""; - private const RegexOptions RegexFlags = RegexOptions.Compiled | RegexOptions.ExplicitCapture | RegexOptions.CultureInvariant; + private const RegexOptions RegexFlags = Compiled | ExplicitCapture | CultureInvariant | NonBacktracking; [GeneratedRegex(RegexPattern, RegexFlags)] private static partial Regex CompiledRegex(); diff --git a/src/Cask/CaskVersion.cs b/src/Cask/CaskVersion.cs deleted file mode 100644 index 361b5d7..0000000 --- a/src/Cask/CaskVersion.cs +++ /dev/null @@ -1,12 +0,0 @@ -// Copyright (c) Microsoft. All rights reserved. -// Licensed under the MIT license. See LICENSE file in the project root for full license information. - -namespace CommonAnnotatedSecurityKeys; - -public enum CaskVersion -{ - /// - /// Specifies version 1.0.0 of CASK. - /// - OneZeroZero = 0, -} diff --git a/src/Cask/Helpers.cs b/src/Cask/Helpers.cs index ac08ff1..2fc93a9 100644 --- a/src/Cask/Helpers.cs +++ b/src/Cask/Helpers.cs @@ -46,8 +46,7 @@ public static bool Is4CharAligned(int charLength) public static int GetKeyLengthInBytes(int providerDataLengthInBytes) { Debug.Assert(Is3ByteAligned(providerDataLengthInBytes), $"{nameof(providerDataLengthInBytes)} should have been validated to 3-byte aligned already."); - int paddedEntropyInBytes = RoundUpTo3ByteAlignment(SecretEntropyInBytes); - int keyLengthInBytes = paddedEntropyInBytes + providerDataLengthInBytes + FixedKeyComponentSizeInBytes; + int keyLengthInBytes = PaddedSecretEntropyInBytes + providerDataLengthInBytes + FixedKeyComponentSizeInBytes; Debug.Assert(Is3ByteAligned(keyLengthInBytes)); return keyLengthInBytes; } @@ -63,40 +62,15 @@ public static int GetHashLengthInBytes(int secretSizeInBytes, out int providerDa public static KeyKind CharToKind(char kindChar) { + Debug.Assert(kindChar == 'A' || kindChar == 'H' || kindChar == 'I', "This is only meant to be called using the kind char of a known valid key."); return (KeyKind)(kindChar - 'A'); } - public static CaskVersion CharToVersion(char versionChar) - { - return (CaskVersion)(versionChar - 'A'); - } - public static byte KindToByte(KeyKind kind) { return (byte)((int)kind << KindReservedBits); } - public static byte VersionToByte(CaskVersion version) - { - return (byte)((int)version << VersionReservedBits); - } - - /// - /// Converts a byte that encodes the CASK version to the CaskVersion enum. - /// Returns false if the reserved bits in that byte are non-zero. - /// - public static bool TryByteToVersion(byte value, out CaskVersion version) - { - if ((value & VersionReservedMask) != 0) - { - version = default; - return false; - } - - version = (CaskVersion)(value >> VersionReservedBits); - return true; - } - /// /// Converts a byte that encodeds the key kind to the KeyKind enum. /// Returns false if the reserved bits in that byte are non-zero. @@ -151,7 +125,8 @@ private static int RoundUpToMultipleOf(int value, int multiple) return (value + multiple - 1) / multiple * multiple; } - public static void ThrowIfUnitialized(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) where T : struct, IIsInitialized + public static void ThrowIfUnitialized(T value, [CallerArgumentExpression(nameof(value))] string? paramName = null) + where T : struct, IIsInitialized { if (!value.IsInitialized) { @@ -174,6 +149,7 @@ public static void ThrowIfNotHash(CaskKey value, [CallerArgumentExpression(nameo ThrowNotHash(paramName); } } + public static void ThrowIfDestinationTooSmall(Span destination, int requiredSize, [CallerArgumentExpression(nameof(destination))] string? paramName = null) { if (destination.Length < requiredSize) diff --git a/src/Cask/InternalConstants.cs b/src/Cask/InternalConstants.cs index 4a090b2..470a96a 100644 --- a/src/Cask/InternalConstants.cs +++ b/src/Cask/InternalConstants.cs @@ -9,20 +9,22 @@ namespace CommonAnnotatedSecurityKeys; /// Move things elsewhere if/when they need to be made public, and avoid `const` in /// public API in favor of static readonly properties. /// -internal static class InternalConstants +internal static partial class InternalConstants { + /// + /// The base64-encoded CASK signature "JQQJ" in UTF-16.) + /// public static ReadOnlySpan CaskSignature => "JQQJ".AsSpan(); - public static ReadOnlySpan CaskSignatureUtf8 => "JQQJ"u8; /// - /// The bytes that make up the CASK signature. "JQQJ", base64-decoded. + /// The base64-encoded CASK signature "JQQJ" in UTF-8. /// - public static ReadOnlySpan CaskSignatureBytes => [0x25, 0x04, 0x09]; + public static ReadOnlySpan CaskSignatureUtf8 => "JQQJ"u8; /// - /// Current version of CASK spec that we implement. + /// The base64-decoded CASK signature "JQQJ" in bytes. /// - public const int CaskVersion = 0; + public static ReadOnlySpan CaskSignatureBytes => [0x25, 0x04, 0x09]; /// /// The number of bytes in a CASK signature @@ -35,12 +37,12 @@ internal static class InternalConstants public const int TimestampSizeInBytes = 3; /// - /// The number of bytes the provider signature. + /// The number of bytes in a provider signature. /// public const int ProviderSignatureSizeInBytes = 3; /// - /// The nubmer of bytes reserved in the key footer for future use. + /// The number of bytes reserved in the key footer for future use. /// public const int VersionAndKindSizeInBytes = 2; @@ -60,25 +62,28 @@ internal static class InternalConstants Crc32SizeInBytes; /// - /// The number of bytes in the fixed componet of a hash key, from the C3ID to the end of the key. + /// The number of bytes in the fixed components of a hash key, from the C3ID to the end of the key. /// public const int FixedHashComponentSizeInBytes = FixedKeyComponentSizeInBytes + CrossCompanyCorrelatingId.RawSizeInBytes; /// - /// The number of bytes of entropy in a primary key. - /// Currently fixed, but may become configurable in future versions. + /// The number of bytes of entropy in a primary key. 32-bytes (256 bits) of + /// entropy generated by a cryptographically secure RNG are currently deemed + /// unbreakable even in a post-quantum world. + /// + /// Currently this value is fixed, but may become configurable in future versions. /// public const int SecretEntropyInBytes = 32; /// /// The size of the entropy in a primary after padding to 3-byte alignment. /// - public const int PaddedSecretEntropyInBytes = 33; + public static int PaddedSecretEntropyInBytes { get; } = RoundUpTo3ByteAlignment(SecretEntropyInBytes); /// /// The size of the HMAC-SHA256 hash after padding to 3-byte alignment. /// - public const int PaddedHmacSha256SizeInBytes = 33; + public static int PaddedHmacSha256SizeInBytes { get; } = RoundUpTo3ByteAlignment(PaddedSecretEntropyInBytes); /// /// The maximum amount of bytes that the implementation will stackalloc. @@ -86,58 +91,50 @@ internal static class InternalConstants public const int MaxStackAlloc = 256; /// - /// The range of byte indices in a key for the bytes that contain the CASK signature. + /// The number of least significant bits reserved in the key kind byte. /// - public static Range CaskSignatureByteRange => ^15..^12; - - /// - /// The range of byte indices in a key for the bytes that contain the provider signature. - /// - public static Range ProviderSignatureByteRange => ^12..^9; + public const int KindReservedBits = 2; /// - /// The range of byte indices in a key for the bytes that contain the timestamp. + /// A bit mask to obtain the reserved bits from the key kind. /// - public static Range TimestampByteRange => ^9..^6; + public const int KindReservedMask = (1 << KindReservedBits) - 1; /// - /// The index of the byte in a key that contains the key kind. + /// The range of byte indices in a hash for the bytes that contain the C3ID of the secret. /// - public static Index KindByteIndex = ^6; + public static Range C3IdByteRange => ^27..^15; /// - /// The index of the byte in a key that contains the CASK version. - /// - public static Index VersionByteIndex => ^5; - /// - /// The range of byte indices in a key for the bytes that contain the CRC32 of the key. + /// The range of byte indices in a key for the bytes that contain the CASK signature. /// - public static Range Crc32ByteRange => ^4..; + public static Range CaskSignatureByteRange => ^15..^12; /// - /// The range of byte indices in a hash for the bytes that contain the C3ID of the secret. + /// The range of byte indices in a key for the bytes that contain the provider signature. /// - public static Range C3IdByteRange => ^27..^15; + public static Range ProviderSignatureByteRange => ^12..^9; /// - /// The number of least significant bits reserved in the version byte. + /// The range of byte indices in a key for the bytes that contain the timestamp. /// - public const int VersionReservedBits = 4; + public static Range TimestampByteRange => ^9..^6; /// - /// A bit mask to obtain the reserved bits from the version byte + /// The index of the byte in a key that contains the key kind. /// - public const int VersionReservedMask = (1 << VersionReservedBits) - 1; + public static Index KindByteIndex = ^6; /// - /// The number of least significant bits reserved in the key kind + /// The index of the byte in a key that is currently reserved to be zeroed + /// out, and may be used in the future to indicate a format change. /// - public const int KindReservedBits = 2; + public static Index ReservedVersionByteIndex => ^5; /// - /// A bit mask to obtain the reserved bits from the key kind. + /// The range of byte indices in a key for the bytes that contain the CRC32 of the key. /// - public const int KindReservedMask = (1 << KindReservedBits) - 1; + public static Range Crc32ByteRange => ^4..; /// /// The range of chars in a base64-encoded key that hold the Cask signature. diff --git a/src/Cask/KeyKind.cs b/src/Cask/KeyKind.cs index 288395e..3d6f1c6 100644 --- a/src/Cask/KeyKind.cs +++ b/src/Cask/KeyKind.cs @@ -10,6 +10,7 @@ public enum KeyKind /// Key256Bit = ('A' - 'A'), // Base64: index 0 == 'A' // Bytewise: << 2 == 0, 0x00, 0b0000_0000 + /// /// Specifies a CASK hashed signature that incorporates a Message /// Authentication code of a derivation input generated by SHA-256. diff --git a/src/Tests/Cask.Benchmarks/CompareHashBenchmarks.cs b/src/Tests/Cask.Benchmarks/CompareHashBenchmarks.cs index 757c972..ca64892 100644 --- a/src/Tests/Cask.Benchmarks/CompareHashBenchmarks.cs +++ b/src/Tests/Cask.Benchmarks/CompareHashBenchmarks.cs @@ -29,7 +29,7 @@ public bool CompareHash_Floor() Span candidateHash = stackalloc byte[HMACSHA256.HashSizeInBytes]; Base64Url.DecodeFromChars(TestNonIdentifiableHash.AsSpan(), candidateHash); - Span secret = stackalloc byte[32]; + Span secret = stackalloc byte[TestSecretEntropyInBytes]; Base64Url.DecodeFromChars(TestNonIdentifiableSecret.AsSpan(), secret); Span hash = stackalloc byte[HMACSHA256.HashSizeInBytes]; diff --git a/src/Tests/Cask.Benchmarks/GenerateHashBenchmarks.cs b/src/Tests/Cask.Benchmarks/GenerateHashBenchmarks.cs index db0592d..28fd4e3 100644 --- a/src/Tests/Cask.Benchmarks/GenerateHashBenchmarks.cs +++ b/src/Tests/Cask.Benchmarks/GenerateHashBenchmarks.cs @@ -26,7 +26,7 @@ public string GenerateHash_Cask() [Benchmark] public string GenerateHash_Floor() { - Span secret = stackalloc byte[32]; + Span secret = stackalloc byte[TestSecretEntropyInBytes]; Base64Url.DecodeFromChars(TestNonIdentifiableSecret.AsSpan(), secret); Span hash = stackalloc byte[HMACSHA256.HashSizeInBytes]; diff --git a/src/Tests/Cask.Benchmarks/GenerateKeyBenchmarks.cs b/src/Tests/Cask.Benchmarks/GenerateKeyBenchmarks.cs index 0ab7974..191ca93 100644 --- a/src/Tests/Cask.Benchmarks/GenerateKeyBenchmarks.cs +++ b/src/Tests/Cask.Benchmarks/GenerateKeyBenchmarks.cs @@ -27,7 +27,7 @@ public string GenerateKey_Cask() [Benchmark] public string GenerateKey_Floor() { - Span bytes = stackalloc byte[32]; + Span bytes = stackalloc byte[TestSecretEntropyInBytes]; RandomNumberGenerator.Fill(bytes); return Base64Url.EncodeToString(bytes); } diff --git a/src/Tests/Cask.Tests/CaskSecretsTests.cs b/src/Tests/Cask.Tests/CaskSecretsTests.cs index 3addad3..c1b150d 100644 --- a/src/Tests/Cask.Tests/CaskSecretsTests.cs +++ b/src/Tests/Cask.Tests/CaskSecretsTests.cs @@ -100,9 +100,8 @@ private void TestEncodedMatchedDecoded(string encodedKey, KeyKind expectedKind, Span optionalData = keyBytes.AsSpan()[(optionalDataIndex)..^15]; Assert.Equal(Base64Url.EncodeToString(optionalData), encodedOptionalData); - char encodedVersion = encodedKey[^7]; - var version = (CaskVersion)(keyBytes.AsSpan()[^5]); - Assert.Equal(CaskVersion.OneZeroZero, version); + char encodedReservedForVersion = encodedKey[^7]; + Assert.Equal('A', encodedReservedForVersion); // Our checksum buffer here is 6 bytes because the 4-byte checksum // must itself be decoded from a buffer that properly pads the @@ -113,13 +112,13 @@ private void TestEncodedMatchedDecoded(string encodedKey, KeyKind expectedKind, Crc32.Hash(keyBytes.AsSpan()[..^4], crc32[2..]); Assert.Equal(Base64Url.EncodeToString(crc32)[2..], encodedChecksum); - // This follow-on demonstrates how to get the key kind and version - // from the bytewise form by shifting out the reserved bits. + // This follow-on demonstrates how to get the key kind and reservd version + // byte from the bytewise form. var kind = (KeyKind)(keyBytes[^6] >> KindReservedBits); Assert.Equal(expectedKind, kind); - version = (CaskVersion)(keyBytes[^5] >> VersionReservedBits); - Assert.Equal(CaskVersion.OneZeroZero, version); + byte reservedForVersion = keyBytes[^5]; + Assert.Equal(0, reservedForVersion); } enum BytewiseKeyKind : byte