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

HMAC one-shot methods #40012

Closed
vcsjones opened this issue Jul 28, 2020 · 14 comments · Fixed by #53487
Closed

HMAC one-shot methods #40012

vcsjones opened this issue Jul 28, 2020 · 14 comments · Fixed by #53487
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-System.Security
Milestone

Comments

@vcsjones
Copy link
Member

vcsjones commented Jul 28, 2020

Background and Motivation

In #17590 we added one-shot methods for hashing. It might be worth considering similar methods on the HMAC classes to provide some API parity, and to offer the same benefits as one-shot hash: straight forward API, no stateful object allocation, etc.

Proposed API

namespace System.Security.Cryptography {
    public partial class HMACMD5 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA1 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA256 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA384 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA512 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }
}

The API is the same as #17590 on the appropriate classes, with the addition of a key parameter.

@vcsjones vcsjones added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Jul 28, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jul 28, 2020
@Dotnet-GitSync-Bot
Copy link
Collaborator

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@vcsjones
Copy link
Member Author

@bartonjs @krwq

@ghost
Copy link

ghost commented Jul 28, 2020

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

@bartonjs bartonjs added this to the 6.0.0 milestone Jul 28, 2020
@GrabYourPitchforks
Copy link
Member

What's wrong with using the method name HashData, same as we added on the other types? "Hashing" is still the appropriate term. It just happens to be keyed rather than unkeyed.

@vcsjones
Copy link
Member Author

What's wrong with using the method name HashData

Nothing. I just thought Hash was not exactly descriptive, but maybe I thought about it too hard. We can keep the name too.

@SteveSyfuhs
Copy link

HashData would be preferred speaking as a consumer of them. It is the method I'd logically look for first when trying to figure out how to make it work.

@vcsjones
Copy link
Member Author

vcsjones commented Aug 6, 2020

Okay, sounds like I am the outlier on wanting a different name; I have restored the name from the other proposal.

@vcsjones vcsjones closed this as completed Aug 6, 2020
@vcsjones vcsjones reopened this Aug 6, 2020
@vcsjones
Copy link
Member Author

vcsjones commented Aug 6, 2020

vcsjones closed this

Doh.

@bartonjs
Copy link
Member

My only concern with HashData(key, data) is that it doesn't necessarily follow that the first argument is the key (and, since HMAC doesn't require block-sized keys, there's not a strong feedback that you got it wrong).

As it is, one has to think about HashData(a) vs HashData(a, b) for which of a/b is the key (cognative overload with the same identifier).

Specs seem to be all over the place.

  • The HMAC RFC doesn't actually define a function syntax, and they use H(a, b) to mean H(a.Concat(b))
  • The old CBC-MAC standard (FIPS 113) also doesn't define a syntax, it just names the output (DAC, instead of MAC, even though it acknowledged they meant the same thing).
  • At least one paper uses MACa(x), which doesn't help our ordering problem.
  • The NIST glossary, via NIST SP 800-56A (rev 2), defines MAC(MacKey, MacData), which is what I (and probably all of us crypto-nerds) think of for the general application of the problem.

So, for a thing that uses a key and an algorithm over data, I think the verb is "Authenticate"... though that does imply a boolean response (to me): public static byte[] AuthenticateData(key, data).

NIST SP 800-56A does define the output of that function (after being run through a truncator, if needed) MacTag. So we could use TagData/GetTag/ComputeTag, since any of these imply (to me) that they are "tagging" something, producing "a tag"... and then I need to understand what that means.

Alternatively, MacData, though it requires we introduce "Mac" as a verb, and slightly looks like "son-of-(LCDR )Data". Y'know, little Lal MacData?

@vcsjones
Copy link
Member Author

vcsjones commented Aug 10, 2020

Oh good someone who can articulate what is in my head clearly, much appreciated.

I would personally prefer MacData, as evidence of the original proposal. On the other hand...

I (and probably all of us crypto-nerds) think of for the general application of the problem

I can be persuaded that generally, most of the time the audience may not be crypto nerds that go spelunking through NIST papers and MacData terminology is "technically correct, the best kind of correct". That combined with the platform has used the nomenclature (keyed) "hashing" for HMAC since forever, maybe HashData, or KeyedHashData might be more acceptable. (The latter is perhaps redundant since it is off of an HMAC class).

I think the verb is "Authenticate"

In the case of MACs, I tend to think of it as symmetric signing, so if I was forced to pick a verb, it would be "sign" to produce the signature, and "authenticate" would be "verify" that returns a bool. I shied away from mentioning this or considering Sign as a name since nowhere in the platform is HMAC explained as symmetric signing. (WebCrypto is one notable place that does).

I'm in favor or MacData or similar for since it feels right to me, but also in favor of HashData since I think that is closer to what people would expect and understand based on the current framework.

@bartonjs bartonjs removed the untriaged New issue has not been triaged by the area owner label Aug 28, 2020
@vcsjones
Copy link
Member Author

@bartonjs What's remaining to move this forward? It seems like the only hitch is the name of the members. Given the discussion, it seems like:

  1. HashData - it's still hashing, just not keyed hashing. This is also consistent (unsure if that is good or bad...) with the non-keyed hash methods.
  2. MacData - Alternative name to make it clear it's a mac, but perhaps redundant given the methods are off HMAC classes.
  3. AuthenticateData - We don't really use "authenticate" anywhere within Libraries to mean a cryptographic hmac. Some precedent with the AesGcm class.
  4. SignData - thought of as a "symmetric sign". Not my favorite but thought I would list it.

@GrabYourPitchforks GrabYourPitchforks added 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 May 19, 2021
@bartonjs
Copy link
Member

bartonjs commented May 19, 2021

Video

Looks good as proposed.

namespace System.Security.Cryptography {
    public partial class HMACMD5 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA1 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA256 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA384 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }

    public partial class HMACSHA512 {
        public static byte[] HashData(byte[] key, byte[] source) => throw null;
        public static byte[] HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source) => throw null;
        public static int HashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination) => throw null;
        public static bool TryHashData(ReadOnlySpan<byte> key, ReadOnlySpan<byte> source, Span<byte> destination, out int bytesWritten) => throw null;
    }
}

@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 May 19, 2021
@vcsjones vcsjones self-assigned this May 19, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 30, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 4, 2021
@teo-tsirpanis
Copy link
Contributor

Shouldn't this be added in "What's new in .NET 6 Preview 5"?

@bartonjs
Copy link
Member

bartonjs commented Jun 4, 2021

Shouldn't this be added in "What's new in .NET 6 Preview 5"?

No, Preview 5 closed for new features about a week ago, this'll be new in Preview 6.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 5, 2021
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.

7 participants