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

WIP Add support for Ed25519, Ed448, X25519 and X448. RFC 8037 #98

Draft
wants to merge 37 commits into
base: master
Choose a base branch
from

Conversation

atombrella
Copy link
Contributor

@atombrella atombrella commented May 9, 2021

This is still pretty much WIP!

  • https://github.com/google/wycheproof Has vectors that may be interesting for edge cases.
  • Add tests for all key types.
  • Revert typo fixes for non-relevant code parts (could be moved to a separate PR)
  • Fix Mypy issues for JWKOkt
  • Reconsider if OpenSSL vendors all of the algorithms, or just a subset of them.

@atombrella
Copy link
Contributor Author

Please see issue #45

src/josepy/jwk.py Outdated Show resolved Hide resolved
@atombrella atombrella marked this pull request as draft May 12, 2021 11:22
@alexzorin alexzorin added enhancement priority: unplanned Work that we believe should be done, but does not have a higher priority. labels May 13, 2021
Copy link
Contributor Author

@atombrella atombrella left a comment

Choose a reason for hiding this comment

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

Just a few notes for myself.

src/josepy/jwk_test.py Outdated Show resolved Hide resolved
@@ -13,6 +13,10 @@
from cryptography.hazmat.primitives.asymmetric import ec
from cryptography.hazmat.primitives.asymmetric import rsa

from cryptography.hazmat.primitives.asymmetric import (
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if there are coding guidelines about grouping imports.

src/josepy/jwa_test.py Outdated Show resolved Hide resolved
constraints.txt Outdated Show resolved Hide resolved
src/josepy/jwk.py Outdated Show resolved Hide resolved
constraints.txt Outdated Show resolved Hide resolved
src/josepy/jwk.py Outdated Show resolved Hide resolved

# TODO: write the thumbprint
thumbprint = (
b'kPrK_qmxVWaYVA9wwBF6Iuo3vVzz7TxHCTwXBygrS4k'
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure what the thumbprint should be here. Some guidance would be appreciated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bmw @ohemorange Any help on this?

Copy link
Member

Choose a reason for hiding this comment

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

Does this help? If not, can you tell me more about the type of guidance you're looking for here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this is fine. I'll try to push some more tests, and changes. I'm not really sure why the thumbprint of all of the tested keys is not tested, please see JWKECTest for an example. So I think just testing one key works?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think a gist to generate them would be helpful.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see. Yes, I think testing all of them would be ideal.

I think there could be a clever design here either extending base class logic and giving each key its own test class or having a list of thumbprints and keys to test. If testing all of them proves difficult for some reason though, I wouldn't worry too much about it.

src/josepy/util.py Outdated Show resolved Hide resolved
@atombrella
Copy link
Contributor Author

@JamesTheAwesomeDude You're welcome to assist a bit in getting this in a mergeable state.

tests/jwk_test.py Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement priority: unplanned Work that we believe should be done, but does not have a higher priority.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants