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

Incremental Hash, allowing to continue to hash #1968

Closed
Drawaes opened this issue Jan 21, 2020 · 7 comments · Fixed by #37936
Closed

Incremental Hash, allowing to continue to hash #1968

Drawaes opened this issue Jan 21, 2020 · 7 comments · Fixed by #37936
Labels
api-approved API was approved in API review, it can be implemented area-System.Security

Comments

@Drawaes
Copy link
Contributor

Drawaes commented Jan 21, 2020

In a number of protocols signing is based on a hash of all messages up to the signing point in the protocol. Incremental Hash is great for that because it means we don't need to hold all the current messages in the conversation in memory. However if we are required to sign at a specific point but then continue hashing for a future signing point we are stuck as we can only do a FinishAndReset

My proposal is to add a Clone or Duplicate method

public partial class IncrementalHash
{
    // Returns a new IncrementalHash in the same state
    public IncrementalHash DuplicateHash() => throw null;

    // Gets the completed hash for the already applied values,
    // but does not block AppendData from accepting more data.
    // These produce equivalent (hash1, hash2) values:
    // A)
    //   hasher.AppendData(data1);
    //   byte[] hash1 = hasher.GetCurrentHash();
    //   hasher.AppendData(data2);
    //   byte[] hash2 = hasher.GetCurrentHash();
    // B)
    //   hasher.AppendData(data1);
    //   byte[] hash1 = hasher.GetHashAndReset();
    //   hasher.AppendData(data1);
    //   hasher.AppendData(data2);
    //   byte[] hash2 = hasher.GetHashAndReset();
    // C)
    //   hasher.AppendData(data1);
    //   using (IncrementalHash tmp = hasher.DuplicateHash())
    //   {
    //       hash1 = tmp.GetHashAndReset();
    //   }
    //   hasher.AppendData(data2);
    //   hash2 = hasher.GetHashAndReset();
    public bool TryGetCurrentHash(Span<byte> destination, out int bytesWritten) => throw null;
    public byte[] GetCurrentHash() => throw null;
}

I know for a fact that both OpenSsl and BCrypt support this operation, I am unsure on osx.

Such implementation on OpenSsl would look like

            var ctx = EVP_MD_CTX_copy_ex(_ctx);

And in BCrypt something like

           var newHash = BCryptDuplicateHash(_hashHandle);

Obviously with differences due to using the .net core PAL etc

  1. Updated to put back the (Try) GetCurrentHash methods as suggested by @bartonjs
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 21, 2020
@GrabYourPitchforks
Copy link
Member

I'd honestly go with a Clone or Duplicate approach. Usability is a little more clear with a dedicated method to clone the internal state than the AndContinue proposal IMO.

@Drawaes
Copy link
Contributor Author

Drawaes commented Jan 21, 2020

I am fine with that. Have updated to reflect it

@bartonjs
Copy link
Member

This came up previously and my initial thought was (Try)GetPartialHash or (Try)GetCurrentHash. DuplicateHash is slightly more generalized, since it allows for not only partial-and-continue but also divergent hashes.

If the "Partial" (or "Current") seems self-identifying enough then we could do both; but if we want to only do one then DuplicateHash is probably the one to do (because of generalization). And, as noted, DuplicateHash might get renamed to Clone in API review, even though that word has a certain amount of smell to it 😄.

@Drawaes
Copy link
Contributor Author

Drawaes commented Jan 22, 2020

So my initial API was either of those, but actually I like you idea of both.

I 100% agree that clone is very overloaded in meaning already and I wouldn't like it as much. How about I put back the (Try)GetCurrentHash as well (we can always negotiate down to only the duplicate).

@bartonjs bartonjs added api-ready-for-review and removed untriaged New issue has not been triaged by the area owner labels Jan 22, 2020
@terrajobst
Copy link
Member

terrajobst commented Feb 11, 2020

Video

  • There don't seem to be any scenarios for DuplicateHash, so let's remove it for now. If we were to do it, we should probably name it Clone though.
  • Rest looks good as proposed.
  • We should add a HashSizeInBytes property so that callers don't have to call TryGetCurrentHash in a loop
  • While we're at it, we should also add overloads for the new TryGetCurrentHash and the existing TryGetHashAndReset to return the length instead of returning a bool.
namespace System.Security.Cryptography
{
    public partial class IncrementalHash
    {
        public bool TryGetCurrentHash(Span<byte> destination, out int bytesWritten);
        public int GetCurrentHash(Span<byte> destination);
        // public bool TryGetHashAndReset(Span<byte> destination, out int bytesWritten);
        public int GetHashAndReset(Span<byte> destination);
        public byte[] GetCurrentHash();
        public int HashLengthInBytes { get; }
    }
}

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review labels Feb 11, 2020
@Drawaes
Copy link
Contributor Author

Drawaes commented Feb 17, 2020

@terrajobst It appears that some API changes are needed on the public type HashProvider. They are in the PR

public virtual int GetCurrentHash(Span<byte> destination);
public virtual bool TryGetCurrentHash(Span<byte> destination, out int bytesWritten);

@bartonjs
Copy link
Member

HashProvider isn't public...

@bartonjs bartonjs self-assigned this Jun 10, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
@bartonjs bartonjs removed their assignment Jul 26, 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.

5 participants