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

Updated Paserk Local, Public & Secret and added Lid, Pid, Sid #88

Merged
merged 23 commits into from
Sep 28, 2022

Conversation

TimothyMakkison
Copy link
Contributor

  • Added/updated test vectors
  • Updated Local, Public Secret encoding / decoding to validate key sizes
  • Removed purpose parameter from Paserk Encode
  • Added Lid, Pid and Sid encoding
  • Added tests for Id encoding
  • Updated tests for Local, Public Secret encode/decode
  • Added tests to verify that Encode won't accept incompatible PaserkKey PaserkType combinations

Note that 51/52 47/48 tests pass for TypesTestVectors and TestIdVectors. The 2 fails are because V1 public key does not encode correctly. The test expects an ASN1 encoded PKCS#1 object identifier to be appended to the front of the key. Updating V1 encode/decode to append/remove MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A might fix the problem. - This seems a little hacky and I don't know enough about ASN1 to be sure so I held off.

TODO

  • Add check for valid point compression for V3 public keys
  • The tests and PaserkHelper could probably do with a refactor, I feel like I'm abusing switch expressions and generators 😆
  • Should the PasetoKey and its derivatives validate the keys they are constructed with? A length check/type check could be added to each key constructor to reject incorrect keys.

@daviddesmet
Copy link
Owner

daviddesmet commented Sep 24, 2022

Wow, that was a huge contribution! Thanks a lot!

Regarding the MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A value, I digged around and found out this:

I don't think openssl commandline program(rsa) can read the PKCS#1 format. As explained here the difference between the PKCS#1 and PKCS#8 format is the algorithm identifier. The algorithm identifier for RSA encryption is "1.2.840.113549.1.1.1" and the Base64 version of it is "MIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8A" which you can safely prefix with the Base64 of the RSA public key and change the header/footer from "BEGIN RSA PUBLIC KEY"/"END RSA PUBLIC KEY" to "BEGIN PUBLIC KEY"/"END PUBLIC KEY".

Source: command line tool to export RSA private key to RSAPublicKey

So based on that, it seems to be safe to append the algorithm identifier.

I don't know enough about ASN1 to be sure so I held off.

Don't worry, we all have been there. I've learned quite a lot while working in Paseto and NaCl.Core (the library which supports XChaCha20-Poly1305 when there wasn't a .NET implementation).

I've found this information really interesting regarding the two public key formats, you might want to check it out: https://stackoverflow.com/a/29707204

@daviddesmet
Copy link
Owner

daviddesmet commented Sep 24, 2022

When you update your PR, can you update the README file to match the supported Paserk extensions?
Might need to update the part about Paserk usage due to the removal of the purpose parameter.

@daviddesmet daviddesmet added the minor Requiring a minor version update according to semantic versioning label Sep 24, 2022
@codecov-commenter
Copy link

codecov-commenter commented Sep 24, 2022

Codecov Report

Merging #88 (bb4234a) into master (243b6a2) will decrease coverage by 0.16%.
The diff coverage is 66.98%.

@@            Coverage Diff             @@
##           master      #88      +/-   ##
==========================================
- Coverage   82.62%   82.45%   -0.17%     
==========================================
  Files         104      105       +1     
  Lines        5295     5387      +92     
  Branches      327      344      +17     
==========================================
+ Hits         4375     4442      +67     
- Misses        798      815      +17     
- Partials      122      130       +8     
Impacted Files Coverage Δ
src/Paseto/Paserk.cs 32.25% <46.51%> (+5.94%) ⬆️
src/Paseto/PaserkHelpers.cs 80.95% <80.95%> (ø)
...c/Paseto/Exceptions/PaserkNotSupportedException.cs 25.00% <0.00%> (+25.00%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@daviddesmet daviddesmet merged commit 9637767 into daviddesmet:master Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
minor Requiring a minor version update according to semantic versioning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for PASERK Type: pid Add support for PASERK Type: sid Add support for PASERK Type: lid
3 participants