Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[API Proposal]: Hash and HMAC one-shots for HashAlgorithName #91407

Closed
vcsjones opened this issue Aug 31, 2023 · 3 comments · Fixed by #92430
Closed

[API Proposal]: Hash and HMAC one-shots for HashAlgorithName #91407

vcsjones opened this issue Aug 31, 2023 · 3 comments · Fixed by #92430
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@vcsjones
Copy link
Member

Background and motivation

Over the past few releases of .NET, we've added a number of "one shots" for cryptographic hashing and HMACs (#62489, #40012, #17590).

All of these one-shots live off of their respective type. For a SHA1 one-shot, use SHA1, for HMAC-SHA256, use HMACSHA256.HashData, etc.

There is one other mechanism for producing HMACs and hashes, which is HashAlgorithmName. This type is an identifier for the hash, but does not implement the hash itself. This type is fed in to places like IncrementalHash, AsymmetricAlgorithm.{SignData,VerifyData}, etc. This is useful when libraries want to allow the caller to specify the hash algorithm used.

If you want to use this in conjunction with the hashing one-shots, you currently need to switch over the HashAlgorithmName.

byte[] hash = myHashAlgorithmName.Name switch {
    "SHA1" => SHA1.HashData(data),
    "SHA256" => SHA256.HashData(data),
    // etc
    _ => throw new ArgumentException("helpful message"),
}

This has a number of undesirable properties:

  1. With the introduction of SHA-3, the number of switch cases is now 8.
  2. The internal hashing mechanism, HashProviderDispenser, actually prefers working with names. So this switch logic is inefficient, since we start out with a name, convert it to a static type, which then just changes it to the name. So the runtime can actually provide a better implementation by going directly to the internal HashProviderDispenser.
  3. The switching over the names pays a native AOT size cost. If a library author implements this switch case, but their consumers only ever use HashAlgorithmName.SHA256, they end up rooting all of the static hash APIs, regardless of if they are used. This was identified in Rfc2898DeriveBytes.Pbkdf2 is trimming unfriendly #91181

We have our own internal implementation of this called HashOneShotHelpers. It's used dozens of times, so there is a clear need for it ourselves, others will benefit from it.

API Proposal

This proposal is identical to all of the hashing one-shots on SHA{n} and HMACSHA{n}, with a HashAlgorithmName as the first parameter.

namespace System.Security.Cryptography;

// Existing class
public static partial class CryptographicOperations {
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, byte[] key, byte[] source);
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source);
        public static int HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination);
        public static bool TryHmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);

        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, byte[] key, Stream source);
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, Stream source);
        public static int HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, Stream source, Span<byte> destination);
        public static ValueTask<byte[]> HmacDataAsync(HashAlgorithmName hashAlgorithm, byte[] key, Stream source, CancellationToken cancellationToken = default);
        public static ValueTask<int> HmacDataAsync(HashAlgorithmName hashAlgorithm, ReadOnlyMemory<byte> key, Stream source, Memory<byte> destination, CancellationToken cancellationToken = default);
        public static ValueTask<byte[]> HmacDataAsync(HashAlgorithmName hashAlgorithm, ReadOnlyMemory<byte> key, Stream source, CancellationToken cancellationToken = default);

        public static byte[] HashData(HashAlgorithmName hashAlgorithm, byte[] source);
        public static byte[] HashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source);
        public static int HashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source, Span<byte> destination);
        public static bool TryHashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);

        public static byte[] HashData(HashAlgorithmName hashAlgorithm, Stream source);
        public static int HashData(HashAlgorithmName hashAlgorithm, Stream source, Span<byte> destination);
        public static ValueTask<int> HashDataAsync(HashAlgorithmName hashAlgorithm, Stream source, Memory<byte> destination, CancellationToken cancellationToken = default);
        public static ValueTask<byte[]> HashDataAsync(HashAlgorithmName hashAlgorithm, Stream source, CancellationToken cancellationToken = default);
}

API Usage

public static void SomeLibraryMethod(HashAlgorithmName hashAlgorithm) {
    byte[] digest = CryptographicOperations.HashData(hashAlgorithm, "hash this"u8);
}

Alternative Designs

No response

Risks

No response

@vcsjones vcsjones added api-suggestion Early API idea and discussion, it is NOT ready for implementation area-System.Security labels Aug 31, 2023
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Aug 31, 2023
@ghost
Copy link

ghost commented Aug 31, 2023

Tagging subscribers to this area: @dotnet/area-system-security, @bartonjs, @vcsjones
See info in area-owners.md if you want to be subscribed.

Issue Details

Background and motivation

Over the past few releases of .NET, we've added a number of "one shots" for cryptographic hashing and HMACs (#62489, #40012, #17590).

All of these one-shots live off of their respective type. For a SHA1 one-shot, use SHA1, for HMAC-SHA256, use HMACSHA256.HashData, etc.

There is one other mechanism for producing HMACs and hashes, which is HashAlgorithmName. This type is an identifier for the hash, but does not implement the hash itself. This type is fed in to places like IncrementalHash, AsymmetricAlgorithm.{SignData,VerifyData}, etc. This is useful when libraries want to allow the caller to specify the hash algorithm used.

If you want to use this in conjunction with the hashing one-shots, you currently need to switch over the HashAlgorithmName.

byte[] hash = myHashAlgorithmName.Name switch {
    "SHA1" => SHA1.HashData(data),
    "SHA256" => SHA256.HashData(data),
    // etc
    _ => throw new ArgumentException("helpful message"),
}

This has a number of undesirable properties:

  1. With the introduction of SHA-3, the number of switch cases is now 8.
  2. The internal hashing mechanism, HashProviderDispenser, actually prefers working with names. So this switch logic is inefficient, since we start out with a name, convert it to a static type, which then just changes it to the name. So the runtime can actually provide a better implementation by going directly to the internal HashProviderDispenser.
  3. The switching over the names pays a native AOT size cost. If a library author implements this switch case, but their consumers only ever use HashAlgorithmName.SHA256, they end up rooting all of the static hash APIs, regardless of if they are used. This was identified in Rfc2898DeriveBytes.Pbkdf2 is trimming unfriendly #91181

We have our own internal implementation of this called HashOneShotHelpers. It's used dozens of times, so there is a clear need for it ourselves, others will benefit from it.

API Proposal

This proposal is identical to all of the hashing one-shots on SHA{n} and HMACSHA{n}, with a HashAlgorithmName as the first parameter.

namespace System.Security.Cryptography;

// Existing class
public static partial class CryptographicOperations {
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, byte[] key, byte[] source);
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source);
        public static int HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination);
        public static bool TryHmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);

        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, byte[] key, Stream source);
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, Stream source);
        public static int HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, Stream source, Span<byte> destination);
        public static ValueTask<byte[]> HmacDataAsync(HashAlgorithmName hashAlgorithm, byte[] key, Stream source, CancellationToken cancellationToken = default);
        public static ValueTask<int> HmacDataAsync(HashAlgorithmName hashAlgorithm, ReadOnlyMemory<byte> key, Stream source, Memory<byte> destination, CancellationToken cancellationToken = default);
        public static ValueTask<byte[]> HmacDataAsync(HashAlgorithmName hashAlgorithm, ReadOnlyMemory<byte> key, Stream source, CancellationToken cancellationToken = default);

        public static byte[] HashData(HashAlgorithmName hashAlgorithm, byte[] source);
        public static byte[] HashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source);
        public static int HashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source, Span<byte> destination);
        public static bool TryHashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);

        public static byte[] HashData(HashAlgorithmName hashAlgorithm, Stream source);
        public static int HashData(HashAlgorithmName hashAlgorithm, Stream source, Span<byte> destination);
        public static ValueTask<int> HashDataAsync(HashAlgorithmName hashAlgorithm, Stream source, Memory<byte> destination, CancellationToken cancellationToken = default);
        public static ValueTask<byte[]> HashDataAsync(HashAlgorithmName hashAlgorithm, Stream source, CancellationToken cancellationToken = default);
}

API Usage

public static void SomeLibraryMethod(HashAlgorithmName hashAlgorithm) {
    byte[] digest = CryptographicOperations.HashData(hashAlgorithm, "hash this"u8);
}

Alternative Designs

No response

Risks

No response

Author: vcsjones
Assignees: -
Labels:

api-suggestion, area-System.Security

Milestone: -

@bartonjs
Copy link
Member

There's a "why CryptographicOperations?" that goes with this, so I'll summarize an offline discussion:

What's the most obvious place for these? HMAC and HashAlgorithm, obviously.

Why are those bad? HMAC : HashAlgorithm (indirectly), so if there's HashAlgorithm.Hash(HashAlgorithmName, data), then it would also be callable as HMAC.Hash(HashAlgorithmName, data) because statics are invocable through derived types. Even worse, both of those have further derived types. SHA256.Hash(HashAlgorithmName.MD5, data) is total nonsense, as is HMACSHA512.Hash(HashAlgorithmName.SHA1, data) or HMACSHA512.Hmac(HashAlgorithmName.SHA256, key, data).

OK, those are out. So is there anywhere that accepts a HashAlgorithmName and does a hash already? Well, sure, RSA/ECDSA/DSA/ECDH... and... IncrementalHash. But some people ::mock glares at @vcsjones:: felt that it was wrong for IncrementalHash (designed explicitly to do hashing across multiple calls) to have a one-shot (designed explicitly to do things in a single call). (Yeah, OK, it wasn't a great suggestion, but it was better than HMAC/HashAlgorithm).

Since everything for the function is in its name and its state (the type is only required because of language/runtime rules), it felt sort of in the camp of ReadBigEndian or a math operation. Those end up in types like -Primitives or -Operations... and, guess what, we already have a CryptographicOperations. Huzzah.

So, does that mean CryptographicOperations would become a dumping ground for other "typeless functions" in the crypto space? Well, in one sense that's what it's for (like the fixed time equality check). In a closer context, the problem here is that the expected names were taken. Our new algorithms/concepts haven't tried to introduce type hierarchies, and generally have thought about both static/one-shot and instance/accumulator patterns at design time; which just leaves existing modelled concepts. Core crypto boils down to hashing, HMAC, symmetric encryption, and asymmetric algorithms. Hashing and HMAC are this proposal, asymmetric has cumbersome keys and needs some sort of instance to track externalized keys (e.g. HSM), so that leaves symmetric. Symmetric cryptography sometimes has externalized keys (CNG supports persisted 3DES/AES keys), and the platform underlying instances maintain state that is easier to reset than it is to build (so two operations with the same key are cheaper if they reused the instance)... so static one-shots (vs the instance one-shots we already added) are of very marginal value.

So, basically, hashing and HMAC are the only things that would "pollute" CryptographicOperations, and there's not an obvious good name for a type to hold these statics that isn't already taken by a type hierarchy... so it seems reasonable.

@bartonjs bartonjs added this to the 9.0.0 milestone Aug 31, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 31, 2023
@bartonjs bartonjs added untriaged New issue has not been triaged by the area owner api-ready-for-review API is ready for review, it is NOT ready for implementation and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Aug 31, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Aug 31, 2023
@bartonjs
Copy link
Member

Looks good as proposed.

namespace System.Security.Cryptography;

// Existing class
public static partial class CryptographicOperations {
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, byte[] key, byte[] source);
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source);
        public static int HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination);
        public static bool TryHmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);

        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, byte[] key, Stream source);
        public static byte[] HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, Stream source);
        public static int HmacData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> key, Stream source, Span<byte> destination);
        public static ValueTask<byte[]> HmacDataAsync(HashAlgorithmName hashAlgorithm, byte[] key, Stream source, CancellationToken cancellationToken = default);
        public static ValueTask<int> HmacDataAsync(HashAlgorithmName hashAlgorithm, ReadOnlyMemory<byte> key, Stream source, Memory<byte> destination, CancellationToken cancellationToken = default);
        public static ValueTask<byte[]> HmacDataAsync(HashAlgorithmName hashAlgorithm, ReadOnlyMemory<byte> key, Stream source, CancellationToken cancellationToken = default);

        public static byte[] HashData(HashAlgorithmName hashAlgorithm, byte[] source);
        public static byte[] HashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source);
        public static int HashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source, Span<byte> destination);
        public static bool TryHashData(HashAlgorithmName hashAlgorithm, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten);

        public static byte[] HashData(HashAlgorithmName hashAlgorithm, Stream source);
        public static int HashData(HashAlgorithmName hashAlgorithm, Stream source, Span<byte> destination);
        public static ValueTask<int> HashDataAsync(HashAlgorithmName hashAlgorithm, Stream source, Memory<byte> destination, CancellationToken cancellationToken = default);
        public static ValueTask<byte[]> HashDataAsync(HashAlgorithmName hashAlgorithm, Stream source, CancellationToken cancellationToken = default);
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Sep 12, 2023
@vcsjones vcsjones self-assigned this Sep 12, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 21, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Oct 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants