Refine hashing usages #15621
Refine hashing usages #15621
Conversation
| @@ -66,6 +66,7 @@ | |||
| <PackageManagement Include="StackExchange.Redis" Version="2.7.33" /> | |||
| <PackageManagement Include="StyleCop.Analyzers" Version="1.1.118" /> | |||
| <PackageManagement Include="System.Linq.Async" Version="6.0.1" /> | |||
| <PackageManagement Include="System.IO.Hashing" Version="8.0.0" /> | |||
There was a problem hiding this comment.
Contains new hashing algorithm, official Ms library.
| @@ -165,8 +165,6 @@ private string GetHash(string queryStringTokenKey) | |||
|
|
|||
| entry.SlidingExpiration = TimeSpan.FromHours(5); | |||
|
|
|||
| using var hmac = new HMACSHA256(_hashKey); | |||
There was a problem hiding this comment.
Removing the custom instantiation, it's recommended to use the static methods.
|
|
||
| mediaTokenSettings.HashKey = new byte[64]; | ||
| rng.GetBytes(mediaTokenSettings.HashKey); | ||
| mediaTokenSettings.HashKey = RandomNumberGenerator.GetBytes(DefaultMediaTokenKeySize); |
There was a problem hiding this comment.
Use of static method
| } | ||
|
|
||
| return sb.ToString(); | ||
| var hash = new XxHash32(); |
There was a problem hiding this comment.
MD5/SHA1 should not be used for security reasons (I have been told by experts). xxHash32/64/128 are also much faster so the ones to use for non-cryptographic hashes.
| return Convert.ToHexString(hash); | ||
| foreach (var part in parts) | ||
| { | ||
| hash.Append(Encoding.UTF8.GetBytes(part)); |
There was a problem hiding this comment.
No string concatenation necessary with this one.
There was a problem hiding this comment.
Note: since you're changing the algorithm, I presume the result is only used internally and can change at any time.
If so, you may want to use the UTF-16 representation here, to avoid an intermediate string allocation.
Something like hash.Append(MemoryMarshal.AsBytes<char>(part));
There was a problem hiding this comment.
Here is a benchmark with UTF8 vs Marshal.AsBytes, with and without the HEX encoding. Was wondering is the encoding would cancel out the benefits, still good. A little slower however, probably because of the extra hashing work.
| Method | Mean | Error | StdDev | Gen0 | Allocated |
|------------- |----------:|----------:|---------:|-------:|----------:|
| UTF8 | 77.37 ns | 39.491 ns | 2.165 ns | 0.0348 | 328 B |
| UTF8ToHex | 82.21 ns | 51.756 ns | 2.837 ns | 0.0408 | 384 B |
| Marshal | 104.60 ns | 25.270 ns | 1.385 ns | 0.0196 | 184 B |
| MarshalToHex | 91.35 ns | 7.423 ns | 0.407 ns | 0.0254 | 240 B |
sebastienros
force-pushed
the
sebros/hmac
branch
from
5bc032b
to
da775f2
Compare
|
@kevinchalet if you have any question or comment |
| return Convert.ToHexString(hash); | ||
| foreach (var part in parts) | ||
| { | ||
| hash.Append(Encoding.UTF8.GetBytes(part)); |
There was a problem hiding this comment.
Note: since you're changing the algorithm, I presume the result is only used internally and can change at any time.
If so, you may want to use the UTF-16 representation here, to avoid an intermediate string allocation.
Something like hash.Append(MemoryMarshal.AsBytes<char>(part));
No description provided.