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

Add OIDs for Edwards curves #205

Merged

Conversation

MatthiasValvekens
Copy link
Contributor

@MatthiasValvekens MatthiasValvekens commented Apr 26, 2021

This PR adds support for the OIDs defined in RFC 8410. I'm mainly interested in the EdDSA use case, but I also went ahead and added the OIDs for Edwards curve ECDH keys.

Let me know if there's anything I need to fix before this commit can be merged!

Edit: not sure why the py26 check is failing --- the test suite ran fine on my machine, and I essentially only added keys to dictionaries; I'm not seeing how that could cause issues with older Python versions... I can't view the logs, but could that be caused by the infamous TLS 1.0/1.1 deprecation issue with PyPI for Python <2.7?

@joernheissler
Copy link
Collaborator

Hi, could you please add test cases, e.g. private keys and self-signed certs that were generated with openssl?

@joernheissler
Copy link
Collaborator

not sure why the py26 check is failing --- the test suite ran fine on my machine, and I essentially only added keys to dictionaries; I'm not seeing how that could cause issues with older Python versions... I can't view the logs, but could that be caused by the infamous TLS 1.0/1.1 deprecation issue with PyPI for Python <2.7?

Can't see the logs either. It appears the circleci would like write access to all my stuff. Not going to happen.
The tests also include other packages which try to access public servers for TLS testing. So it's perfectly possible that something is incompatible.

@MatthiasValvekens
Copy link
Contributor Author

Hi, could you please add test cases, e.g. private keys and self-signed certs that were generated with openssl?

D'oh, of course. I'll take care of that. I actually have test cases that I generated for use with the code in my own project that monkeypatches these values into asn1crypto, but I forgot to add them to the PR.

@MatthiasValvekens
Copy link
Contributor Author

MatthiasValvekens commented Apr 27, 2021

Hi @joernheissler, one quick question: the public / private key fields in a PublicKeyInfo / PrivateKeyInfo for an EdDSA key should be treated as opaque bit strings / octet strings (respectively). I took this as meaning that these fields would encapsulate a tagged OctetString value, but apparently that was wrong (at least for the public parts): the key is encoded without any tags, and in particular the contents of this field aren't valid ASN.1 most of the time. See here for an example from RFC 8410 (it's a certificate with a Curve25519 ECDH key as opposed to an EdDSA one, but the effect is the same).

Is there a way to make ParsableOctetBitString and ParsableOctetString in asn1crypto deal with these things gracefully? Otherwise .native will pretty much always break on these certificates.

Since I just pass through public/private key info to the library that handles signing and validation in my project, I didn't notice this issue until now --- thanks for making me check ;)

EDIT: Aha, apparently using (OctetBitString, None) instead of OctetString seems to do the trick. PR amended with tests.

@MatthiasValvekens
Copy link
Contributor Author

I've amended the PR with some tests now. The key byte comparisons were based on output from OpenSSL.

Thank you for the review, and sorry for not catching that encoding issue with the public keys earlier!

@joernheissler
Copy link
Collaborator

Circleci complains failed to create host: Image xcode:9.0.1 is not supported.

Your PR looks good, thanks!

@joernheissler joernheissler merged commit 3c9f12d into wbond:master May 16, 2021
@MatthiasValvekens MatthiasValvekens deleted the feature/edwards-curves branch January 27, 2022 08:20
@@ -371,6 +378,8 @@ def hash_algo(self):
'sha256_ecdsa': 'sha256',
'sha384_ecdsa': 'sha384',
'sha512_ecdsa': 'sha512',
'ed25519': 'sha512',
'ed448': 'shake256',
Copy link

Choose a reason for hiding this comment

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

Late to the party - I wonder if those values are correct. Those hashes are used deeply internally and should not be passed to as a parameter to signature generation/verification functions.

I have proposed setting "raw" value here in #230 - up for discussion!

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.

3 participants