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

std.crypto: add DER parser and RSA #19771

Closed
wants to merge 1 commit into from

Conversation

clickingbuttons
Copy link
Contributor

@clickingbuttons clickingbuttons commented Apr 26, 2024

  • Add secure DER parser.
    • Use in ECDSA.
      • Add comments to Wycheproof tests.
      • Move Wycheproof test suite to external file.
    • Use in Ed25519.
    • Not public.
    • Ready for use in Certificate, which will be done in another PR for ease of review.
  • Add std.crypto.rsa.
    • Modulus_bits up to 4096.
    • Any hash function for PSS or PKCS1v1.5 signatures.
    • Use in Certificate and tls.Client.

- Add secure DER parser.
	- Use in ECDSA.
		- Add comments to Wycheproof tests.
		- Move Wycheproof test suite to external file.
	- Use in Ed25519.
	- Not public.
- Add std.crypto.rsa.
	- Modulus_bits up to 4096.
	- Any hash function for signatures.
	- Use it in Certificate and tls.Client.
// TODO: compare hash values instead of emsa values
const expected = try emsaEncode(hash, em);

if (!std.mem.eql(u8, expected, em)) return error.Inconsistent;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure if this needs to be constant time or not. It's not mentioned anywhere, so I guessed it's okay to use the fast version.

if (!expected_zero.isZero()) return error.KeyMismatch;

// TODO: check that d * e is one mod p-1 and mod q-1. Note d and e were bound
// const de = secret.private_exponent.mul(public.public_exponent);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Need some help here. Can also just remove this incorrect commented code for now.

@jedisct1
Copy link
Contributor

jedisct1 commented Apr 26, 2024

Once again, that should be split into more focused PRs.

Maybe starting with a PR that introduces a crypto.serialization.asn1 namespace that exposes generic DER/PEM parsing (and plan for generation as well) and OIDs.

crypto.serialization can include other codecs, especially constant-time base64 codecs that are required for PEM-encoded secrets.

And a PR that extracts the current RSA code from Certificate.zig and move it to a dedicated file. Without completely rewriting it, nor the rest at that point. Then, incremental improvements can be made to it in order to address known compatibility issues with certificates chains.

Once the serialization toolkit is done, the DER and PEM parser can be used everywhere, from ecdsa to Bundle.zig. Then, Wycheproof integration can be added.

Then, other improvements to rsa.zig can be made. In order to support blinding and operations on private key, we need to add things such as modular inversion to ff. So, individual PRs to implement these prerequisites. Then we need to make sure we can serialize to DER/PEM. Then we'll have everything to replace rsa.zig with a complete implementation, without question marks.

@clickingbuttons
Copy link
Contributor Author

clickingbuttons commented Apr 26, 2024

Sure, let's be surgical. I've split this up into issues:

Edit: As for blinding, I'm not sure that's required to make a public RSA module. Feel free to correct me in the issue.

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.

2 participants