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

docs: missing kwargs for Jwk.generate_for_kty method #38

Merged
merged 2 commits into from
Jan 2, 2025

Conversation

vinicius-oa
Copy link
Contributor

I followed the usage.md and got a ValueError because crv and alg kwargs cannot be both None.

@guillp
Copy link
Owner

guillp commented Jan 2, 2025

Thanks for the PR! Once again, I suck at documentation ^^

Actually, I would consider Jwk.generate_for_kty() as deprecated.
Instead of generating keys based on their key type, you should generate them based on their intended algorithm. So use Jwk.generate(alg="<alg_of_your_choice>") so that jwskate can automatically determine the key type and some extra parameters based on the intended alg_of_your_choice. The extra benefit is that you don't need to specify the alg everytime you use the key.

One of the only alg for which the curve cannot be guessed automatically is ECDH-ES+A128KW since it can be used with keys on any curve. But if you run Jwk.generate(alg="ECDH-ES+A128KW") you will get a UserWarning like this:

UserWarning: No Curve identifier (crv) specified when generating an Elliptic Curve Jwk for Key Management. Curve 'P-256' is used by default. You should explicitly pass a 'crv' parameter to select the appropriate Curve and avoid this warning.

private_jwk = Jwk.generate(alg="ECDH-ES+A128KW", crv="P-256")
public_jwk = private_jwk.public_jwk()

jwe = JweCompact.encrypt(plaintext, public_jwk, enc="A128GCM")

Feel free to edit your PR, or I can update the doc myself otherwise :)
If you spot other things to improve in the docs, feel free to open other PRs or issues.

@vinicius-oa
Copy link
Contributor Author

Thanks for the PR! Once again, I suck at documentation ^^

Actually, I would consider Jwk.generate_for_kty() as deprecated. Instead of generating keys based on their key type, you should generate them based on their intended algorithm. So use Jwk.generate(alg="<alg_of_your_choice>") so that jwskate can automatically determine the key type and some extra parameters based on the intended alg_of_your_choice. The extra benefit is that you don't need to specify the alg everytime you use the key.

One of the only alg for which the curve cannot be guessed automatically is ECDH-ES+A128KW since it can be used with keys on any curve. But if you run Jwk.generate(alg="ECDH-ES+A128KW") you will get a UserWarning like this:

UserWarning: No Curve identifier (crv) specified when generating an Elliptic Curve Jwk for Key Management. Curve 'P-256' is used by default. You should explicitly pass a 'crv' parameter to select the appropriate Curve and avoid this warning.

private_jwk = Jwk.generate(alg="ECDH-ES+A128KW", crv="P-256")
public_jwk = private_jwk.public_jwk()

jwe = JweCompact.encrypt(plaintext, public_jwk, enc="A128GCM")

Feel free to edit your PR, or I can update the doc myself otherwise :) If you spot other things to improve in the docs, feel free to open other PRs or issues.

usage.md updated along others examples using the deprecated generate_for_kty. I've also put a DeprecationWarning linking to your comment in this PR.

@guillp guillp merged commit fb3de01 into guillp:main Jan 2, 2025
@guillp
Copy link
Owner

guillp commented Jan 2, 2025

Excellent, thank you!

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