Open TimothyMakkison opened 2 years ago
Merging #91 (7e7c252) into master (9637767) will increase coverage by
0.41%
. The diff coverage is82.76%
.
@@ Coverage Diff @@
## master #91 +/- ##
==========================================
+ Coverage 82.45% 82.87% +0.41%
==========================================
Files 105 119 +14
Lines 5387 5937 +550
Branches 344 413 +69
==========================================
+ Hits 4442 4920 +478
- Misses 815 853 +38
- Partials 130 164 +34
Impacted Files | Coverage Δ | |
---|---|---|
src/Paseto/Paserk.cs | 36.80% <34.54%> (+4.54%) |
:arrow_up: |
src/Paseto/Cryptography/Internal/Argon2/Argon2.cs | 53.57% <53.57%> (ø) |
|
src/Paseto/Utils/EncodingHelper.cs | 78.04% <60.00%> (-5.83%) |
:arrow_down: |
.../Paseto/Cryptography/Internal/Argon2/Argon2Lane.cs | 71.42% <71.42%> (ø) |
|
...graphy/Internal/Argon2/LittleEndianActiveStream.cs | 78.16% <78.16%> (ø) |
|
src/Paseto/PaserkHelpers.cs | 79.52% <78.46%> (-1.43%) |
:arrow_down: |
...eto/Cryptography/Internal/Argon2/SpanExtensions.cs | 83.33% <83.33%> (ø) |
|
.../Paseto/Cryptography/Internal/Argon2/Argon2Core.cs | 87.67% <87.67%> (ø) |
|
src/Paseto/PaserkOperations/Pbkw.cs | 98.42% <98.42%> (ø) |
|
src/Paseto/Cryptography/Internal/Argon2/Argon2d.cs | 100.00% <100.00%> (ø) |
|
... and 9 more |
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.
Yay! You are on fire with the contributions! I really appreciate it! One little request: Could you resolve the conflicts so I can look at the updated changes from your previous merged PR?
Conflicts should be resolved 🤞 and I've added some documentation to explain the different encoding operations.
I'm a bit concerned with the Argon2id encode/decode, running all tests used to take me 40 secs but now its upwards of 4 mins. This might just be my computer though.
Conflicts should be resolved 🤞 and I've added some documentation to explain the different encoding operations.
Thanks for the update and for adding the documentation. I'm gonna take a look at the PR locally since it is failing in the macOS tests.
I'm a bit concerned with the Argon2id encode/decode, running all tests used to take me 40 secs but now its upwards of 4 mins. This might just be my computer though.
Yeah, looking at the numbers from the CI, it's taking longer than it used to. So, don't think is related to your computer. I'm not familiar with Argon2id so not sure if it is something expected.
Looks like the problem might be caused by the difference in Argon2id implementations. PBKW takes the arguments password
, memory cost
, time cost
and parallelism degree
. The php algorithm uses memory cost
as the "Maximum memory (in kibibytes) that may be used to compute the Argon2 hash.". In Konscious.Argon2id the property MemorySize
defines "The number of 1kB memory blocks to use while processing the hash".
So by setting the MemorySize
to the maximum memory usage, a large amount of memory is used causing the minute long run times.
I've also made mislabeled the parameter memoryCost
as being "The number of 1kB memory blocks", when its treated as being in bytes.
Don't know if my earlier comment is right but looking at TestPwVectors
in PaserkTests.cs
// Decode paserk to verify decoding works
var decoded = Paserk.Decode(test.Paserk, test.Password);
decoded.Key.Span.ToArray().Should().BeEquivalentTo(pasetoKey.Key.ToArray());
I'm not 100% sure how the test is passing because the test passes the field "memlimit": 67108864 (which I assumed is in KiB) into Argon2IdDecrypt as memoryCost
which is then divided by 1024 (converting it into MiB form). This should then return an invalid hash resulting in an invalid authentication tag but instead passes the test. Presumably memlimit is in bytes and my assumption was wrong, but then I don't know how the php implementation works as that takes KiB.
Tl Dr: I need to fix my KiB conversions, I still don't know why it's so slow.
Sorry for looking at the PR until now, I was a bit overwhelmed by work.
Changed Paserk.ArgonEncode to take bytes instead of KiB. Not sure about this change as most Argon2id implementations measure memoryCost in KiB, on the other hand Paserk might specify that this should be in bytes. Might revert this change.
I haven't looked into Argon yet, so I will trust your judgement on that.
Lowered the memory cost and iterations for Argon2id encode/decode test, should reduce test run time
I see the numbers changed, thanks for addressing those. =)
Are you OK with all the changes done in the PR in order to merge it or do you want to keep working on it?
Sorry for looking at the PR until now, I was a bit overwhelmed by work.
No worries, I'm in no rush, this pr could probably do with more time as it is.
Are you OK with all the changes done in the PR in order to merge it or do you want to keep working on it?
I'll update this commit later, it's a bit of a mess, I'd like to refactor a couple of things especially now that I've got PIE working. Might just flip a coin to resolve the byte units problem.
I've been playing around with Avx2/Simd and think Argon2/Blake could be faster if we used some optimisations from saucecontrols Blake2Fast. Going to hack a proof of concept and see if Konscious.Secrurity would be interested.
I've been playing around with Avx2/Simd and think Argon2/Blake could be faster if we used some optimisations from saucecontrols Blake2Fast. Going to hack a proof of concept and see if Konscious.Secrurity would be interested.
That sounds great, I've been postponing the Hardware Intrinsics topic for a while but it is still on my radar for NaCl.Core, there was a PR over there but it didn't make it into main due to some tests that broke. Maybe if you got some spare time and you are willing to, you could take also a look at it?
That sounds great, I've been postponing the Hardware Intrinsics topic for a while but it is still on my radar for NaCl.Core, there was a PR over there but it didn't make it into main due to some tests that broke. Maybe if you got some spare time and you are willing to, you could take also a look at it?
Thanks for showing me this pr, it has some really clever usage of unpacking and parallelizing multiple shuffles, a bit beyond me tbh, I'm still very new to Avx
It looks like @macaba bypassed ProcessKeyStreamBlock
instead Xoring the message in ChaCha20BaseIntrinsics.ChaCha20
with some unpack wizardry. Further, for byte sizes >= 256 or 512 they calculate multiple sections of the key stream simultaneously.
I can think of two options I want to experiment with:
ProcessKeyStreamBlock
method that calculates the ciphertext using the current ChaCha20BaseIntrinsics
with m = 000000...
, because the message is zeros the returned ciphertext is also the keystream. This is a little hacky but keeps the optimizations intact for normal encryption/decryption with a slight performance hit when generating a mac (having to xor again).ChaCha20BaseIntrinsics
to remove the Xor making it compatible with ProcessKeyStreamBlock
. This may lose some of the performance benefits but is much cleaner.Looks like Salsa support has been added since the original pr, iirc Salsa and ChaCha use the same shuffling method but have different starting states layouts. I reckon BaseIntrinsics.Salsa20
could be created, sharing a lot of code with ChaCha20
, just with different startings states.
Finally you mentioned COMPlus_EnableAVX
to enabled and disabled Avx for testing. Do you know if there is a way to run tests with and without this set?
I just left a minor comment regarding the documentation side, would be nice to add it to the README so people may know how to use those new password wrapper ones. What would you think?
I can think of two options I want to experiment with:
- Create an intrinisics specific
ProcessKeyStreamBlock
method that calculates the ciphertext using the currentChaCha20BaseIntrinsics
withm = 000000...
, because the message is zeros the returned ciphertext is also the keystream. This is a little hacky but keeps the optimizations intact for normal encryption/decryption with a slight performance hit when generating a mac (having to xor again).- Change
ChaCha20BaseIntrinsics
to remove the Xor making it compatible withProcessKeyStreamBlock
. This may lose some of the performance benefits but is much cleaner.
I much prefer the cleaner one unless you come with something else.
Looks like Salsa support has been added since the original pr, iirc Salsa and ChaCha use the same shuffling method but have different starting states layouts. I reckon
BaseIntrinsics.Salsa20
could be created, sharing a lot of code withChaCha20
, just with different startings states.
XSalsa20 was added too, I haven't promoted those as stable yet since I feel we need more tests over there. My main plan as I mentioned in the other PR was to include XSalsa20Poly1305.
Finally you mentioned
COMPlus_EnableAVX
to enabled and disabled Avx for testing. Do you know if there is a way to run tests with and without this set?
Aetting COMPlus_EnableAvx
and COMPlus_EnableSse
to 0
results in Avx.IsSupported and Sse.IsSupported getting false value.
Wonder if we can have a cleaner way to execute with intrinsics on and off without running the tests twice with the variable set and unset.
@TimothyMakkison
I merged your other PR and as a consequence, this PR would need to be updated, can you kindly update the PR so I can merge your changes? Once merged I think we can ship it as a new Nuget version, unless you want to add something else?
I merged your other PR and as a consequence, this PR would need to be updated, can you kindly update the PR so I can merge your changes? Once merged I think we can ship it as a new Nuget version, unless you want to add something else?
Sorry this is yet another PR I've abandoned 😅. I'll make this PR compatible.
we can ship it as a new Nuget version
ID is not correctly supported. I misread the Paserk section on pid
, lid
and sid
and haven't fully implemented it. I only added it to public
, local
and secret
paserks when all valid paserk strings should be convertible to a paserk id. I should fix this before shipping.
want to add something else?
When I added password wrapping I also added PIE which wraps local and secret keys using a symmetric key. I'll see if I can add that.
I'd been meaning to suggest changing the Paserk.Encode
and Paserk.Decode
function. It currently uses PaserkType
to choose the operation and method overloads to add additional arguments for operations. This creates several functions which are only compatible with certain operations causing confusion. In addition, this is not extendible and uses a mess of switch expressions to parse the enum.
Instead I suggest we use an interface to implement each operation with Encode
,Decode
methods. The API would appear to be the same; with the selected class performing the logic instead of an internal switch statement.
Paserk.Encode
and Paserk.Decode
could be removed or kept only calling the relevant IPaserk function. Alternatively they could enforce the Versions/KeyTypes by comparing the IPaserk.SupportsKeyTypes/SupportedVersions
to the arguments.
interface IPaserk
{
string Encode(PasetoKey key);
PasetoKey Decode(string paserk);
// Declares supported verions/types, `Paserk.Encode` and `Paserk.Decode` could enforce this.
// Helps the user know if the operation is supported.
// PasetoKey[] SupportsKeyTypes { get; }
// ProtocolVersion[] SupportedVersions { get; }
// Could require a ToId implementation.
// string ToId(string paserk)
}
public static class PaserkType
{
public static readonly IPaserk Public => new Public();
public static readonly IPaserk Local => new Local();
// Some operations require additional arguments.
// Static function (or constructor) that takes the arguments and returns the constructed type.
public static IPaserk LocalPw(string password) => new LocalPw(password);
public static IPaserk Seal(AsymmetricPrivateKey key) => new Seal(key);
}
var paserk = PaserkType.Public.Encode(myKey);
var newKey = PaserkType.LocalPw("myPassword").Decode(myPaserk);
Sorry this is yet another PR I've abandoned 😅. I'll make this PR compatible.
No worries, you can work on it when you have free time. I've been busy also and haven't got that much free time.
ID is not correctly supported. I misread the Paserk section on
pid
,lid
andsid
and haven't fully implemented it. I only added it topublic
,local
andsecret
paserks when all valid paserk strings should be convertible to a paserk id. I should fix this before shipping.
Sure, please let me know when you feel ready to merge. I think I've seen you can mark PRs as a draft.
When I added password wrapping I also added PIE which wraps local and secret keys using a symmetric key. I'll see if I can add that.
That sounds great! Thank you!
Api Change
I'd been meaning to suggest changing the
Paserk.Encode
andPaserk.Decode
function. It currently usesPaserkType
to choose the operation and method overloads to add additional arguments for operations. This creates several functions which are only compatible with certain operations causing confusion. In addition, this is not extendible and uses a mess of switch expressions to parse the enum.Instead I suggest we use an interface to implement each operation with
Encode
,Decode
methods. The API would appear to be the same; with the selected class performing the logic instead of an internal switch statement.
Paserk.Encode
andPaserk.Decode
could be removed or kept only calling the relevant IPaserk function. Alternatively they could enforce the Versions/KeyTypes by comparing theIPaserk.SupportsKeyTypes/SupportedVersions
to the arguments.interface IPaserk { string Encode(PasetoKey key); PasetoKey Decode(string paserk); // Declares supported verions/types, `Paserk.Encode` and `Paserk.Decode` could enforce this. // Helps the user know if the operation is supported. // PasetoKey[] SupportsKeyTypes { get; } // ProtocolVersion[] SupportedVersions { get; } // Could require a ToId implementation. // string ToId(string paserk) }
public static class PaserkType { public static readonly IPaserk Public => new Public(); public static readonly IPaserk Local => new Local(); // Some operations require additional arguments. // Static function (or constructor) that takes the arguments and returns the constructed type. public static IPaserk LocalPw(string password) => new LocalPw(password); public static IPaserk Seal(AsymmetricPrivateKey key) => new Seal(key); }
var paserk = PaserkType.Public.Encode(myKey); var newKey = PaserkType.LocalPw("myPassword").Decode(myPaserk);
Yeah, when I first created that little helper I didn't measure how the API would evolve. I like what you are suggesting, you can go ahead with it.
And once again, thanks a lot for your contributions =)
V2-V4 requires Argon2id, I've added the relevant files and tests from Konscious.Security.Cryptography and updated the README to reflect this.
V1-V3 and V2-V4 each require additional parameters, with V1-V3 taking a
password
&iterations
and V2-V4 takingpassword
,memoryCost
,iterations
°reeOfParallelism
. The current solution is to add method overloads toEncode
. I'm not sure if this is considered good practice/idiomatic and could be confusing for users.