Skip to content

Adapt Hashes.HMACSHA256() so that it uses native hashes when they are…#965

Closed
JVanloofsvelt wants to merge 4 commits intoMetacoSA:masterfrom
JVanloofsvelt:feature/use-native-hash-with-BC-HMac
Closed

Adapt Hashes.HMACSHA256() so that it uses native hashes when they are…#965
JVanloofsvelt wants to merge 4 commits intoMetacoSA:masterfrom
JVanloofsvelt:feature/use-native-hash-with-BC-HMac

Conversation

@JVanloofsvelt
Copy link
Copy Markdown
Contributor

… present, even if native HMAC is not.

If there is no native HMACSHA256, but there is native SHA256, then use Bouncy Castle's HMAC implementation in combination with the native Sha256 digest (instead of Bouncy Castle's SHA256 implementation).

@JVanloofsvelt
Copy link
Copy Markdown
Contributor Author

Using following compilation symbols to get a build that works in Blazor wasm:
NO_NATIVE_HMACSHA512;NO_NATIVESHA1;NO_NATIVE_RFC2898_HMACSHA512;NOCUSTOMSSLVALIDATION;NO_NATIVERIPEMD160;NETCORE;HAS_SPAN;NO_BC

The gist being that you need to add NO_NATIVE_HMACSHA512 and NO_NATIVE_RFC2898_HMACSHA512.

Comment thread NBitcoin/Crypto/Hashes.cs Outdated
#if !NONATIVEHASH
using System.Security.Cryptography;
using NBitcoin.Crypto.digests; // use managed SHA implementations
using NBitcoin.BouncyCastle.Crypto.Parameters; // we'll need this to create HMACs based o the managed SHAs.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

introduced 1 warning here: The using directive for 'NBitcoin.BouncyCastle.Crypto.Parameters' appeared previously in this namespace

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll fix it

@JVanloofsvelt
Copy link
Copy Markdown
Contributor Author

JVanloofsvelt commented Jan 31, 2021

I have no idea why the CI tests timed out on .NET 4.6

@NicolasDorier
Copy link
Copy Markdown
Collaborator

Remove changes on ECPrivKey.cs I fixed properly on 58307cc


public int DoFinal(byte[] output, int outOff)
{
var hash = nativeSha256.ComputeHash(input, 0, input.Length);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you instead rely on NBitcoin.Secp256k1.SHA256 class which properly implement DoFinal and BlockUpdate and is widely tested and memory efficient?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I'll take care of it tonight.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NicolasDorier I found the NBitcoin.Secpk1.SHA class, but it doesn't look like it implements the IDigest interface, nor the BlockUpdate/DoFinal methods.

Copy link
Copy Markdown
Collaborator

@NicolasDorier NicolasDorier Feb 5, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not saying to use it instead of ManagedSha256Digest, but to use it rather than that nativeSha256

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @NicolasDorier, I understand now. There is no NBitcoin.Secp512k1.SHA512 equivalent in the Secpk1 project though (to go along with the SHA256). Besides, wouldn't it be great if NBitcoin could rely on the framework's builtins?

It's not clear to me if DoFinal() and BlockUpdate() are BouncyCastle-specific interface methods, or if they are a more wider known concept. I'm not sure how to implement them correctly. I was hoping someone here would review it, and fill in the gaps for me.

Comment thread NBitcoin/Crypto/Hashes.cs Outdated

public static byte[] HMACSHA256(byte[] key, byte[] data)
{
var mac = new NBitcoin.BouncyCastle.Crypto.Macs.HMac(new ManagedSha256Digest());
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you dispose all the ManagedSha256Digest you instanciate?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made another update to dispose these.

@knocte
Copy link
Copy Markdown
Contributor

knocte commented Feb 2, 2021

There are conflicts, you need to rebase.

@JVanloofsvelt JVanloofsvelt force-pushed the feature/use-native-hash-with-BC-HMac branch from e9f4bce to 4d84380 Compare February 3, 2021 20:36
… present, even if native HMAC is not.

If there is no native HMACSHA256, but there is native SHA256, then use Bouncy Castle's HMAC implementation in combination with the native Sha256 digest (instead of Bouncy Castle's SHA256 implementation).
Add guards (compilation symbols) to conditionally add using statements according to the target framework.
@JVanloofsvelt JVanloofsvelt force-pushed the feature/use-native-hash-with-BC-HMac branch from 4d84380 to 8b1e49d Compare February 3, 2021 21:19
@JVanloofsvelt
Copy link
Copy Markdown
Contributor Author

@knocte I rebased

@NicolasDorier
Copy link
Copy Markdown
Collaborator

#965 (comment)

@NicolasDorier
Copy link
Copy Markdown
Collaborator

Ping @JVanloofsvelt

@NicolasDorier NicolasDorier force-pushed the master branch 2 times, most recently from 725a14b to bbcaf18 Compare January 16, 2025 14:38
@JVanloofsvelt JVanloofsvelt closed this by deleting the head repository Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants