Add password wrap for all versions

[TimothyMakkison]

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 taking password, memoryCost, iterations & degreeOfParallelism. The current solution is to add method overloads to Encode. I'm not sure if this is considered good practice/idiomatic and could be confusing for users.

// Default for Local, Public, Secret, Lid, Pid, Sid
Encode(PasetoKey pasetoKey, PaserkType type)
// Secret/Local PW V1-V3
Encode(PasetoKey pasetoKey, PaserkType type, string password, int iterations = 100_000) 
// Secret/Local PW V2-V4
Encode(PasetoKey pasetoKey, PaserkType type, string password, int memoryCost, int iterations, int degreeOfParallelism = 1)
// .... Add more overloads as needed ie
// For PKE Seal
Encode(PasetoKey pasetoKey, PaserkType type, byte[] key) 

[codecov-commenter]

Codecov Report

Merging #91 (7e7c252) into master (9637767) will increase coverage by 0.41%.
The diff coverage is 82.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%)	⬆️
src/Paseto/Cryptography/Internal/Argon2/Argon2.cs	53.57% <53.57%> (ø)
src/Paseto/Utils/EncodingHelper.cs	78.04% <60.00%> (-5.83%)	⬇️
.../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%)	⬇️
...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.

[daviddesmet]

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?

[TimothyMakkison]

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.

[daviddesmet]

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.

[TimothyMakkison]

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.

[TimothyMakkison]

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.

[TimothyMakkison]

• 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.
• Lowered the memory cost and iterations for Argon2id encode/decode test, should reduce test run time

[daviddesmet]

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?

[TimothyMakkison]

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.

[daviddesmet]

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?

[TimothyMakkison]

• Finished bike shedding over the bytes magnitude and settled on KiBytes to be consistent with Argon2.
• Added more summaries.
• Added a dedicated method overload for decoding password wrapped keys. I opted to throw an error when decoding non password wrapped Paserks to avoid misuse - don't know if this is a good choice.

[TimothyMakkison]

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:

• Create an intrinisics specific 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).
• Change 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?

[daviddesmet]

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 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).
• Change ChaCha20BaseIntrinsics to remove the Xor making it compatible with ProcessKeyStreamBlock. 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 with ChaCha20, 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.

[daviddesmet]

@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?

[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?

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.

Api Change

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);

[daviddesmet]

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 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.

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 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);

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 =)