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

EdDSA alg identifier might be getting deprecated #23

Closed
panva opened this issue Aug 30, 2023 · 9 comments · Fixed by #24
Closed

EdDSA alg identifier might be getting deprecated #23

panva opened this issue Aug 30, 2023 · 9 comments · Fixed by #24

Comments

@panva
Copy link
Contributor

panva commented Aug 30, 2023

See https://mailarchive.ietf.org/arch/msg/jose/3REQba16DLXxxpk8IU1pF-AiTCM/, the EdDSA alg identifier might be getting deprecated in the foreseeable future.

I believe we could remove the jwk import/export steps that mention "EdDSA" in webcrypto-secure-curves since there's always the crv member present that fully specifies the key's sub type. Or we could remove the alg member from export and incorporate the new ones into import only as an option to check for, albeit with the risk of the algorithm changing during the RFC process.

@twiss WDYT? I lean towards not having the alg member checked during import and also not exporting it, that would meant the steps are identical for all 4 algorithms in this spec.

@panva panva changed the title The JWK EdDSA alg identifier might be getting deprecated in the future The JWK EdDSA alg identifier might be getting deprecated Aug 30, 2023
@panva panva changed the title The JWK EdDSA alg identifier might be getting deprecated EdDSA JWK alg identifier might be getting deprecated Aug 30, 2023
@panva panva changed the title EdDSA JWK alg identifier might be getting deprecated EdDSA alg identifier might be getting deprecated Aug 30, 2023
@twiss
Copy link
Collaborator

twiss commented Aug 30, 2023

Hello 👋 AFAIU from the proposal and linked draft (https://www.ietf.org/archive/id/draft-jones-jose-fully-specified-algorithms-00.html), the alg member won't be removed but replaced with a more specific value, and I assume the crv member would be removed instead. So, I think removing the alg member wouldn't be conformant to this proposal nor the current spec.

I don't actually quite understand why the proposal is limited to EdDSA; to me it would seem simplest to define "alg": "Ed25519" | "Ed448" | "X25519" | "X448". I guess we could additionally (or instead) keep the crv member with the exact same values, which would bring it closer to your proposal, but we should then first discuss it on the mailing list there, rather than unilaterally changing this spec.

Edit: I guess for ECDH it's a bit more complicated because they also want to include the AES-KW algorithm, as per https://www.rfc-editor.org/rfc/rfc7518#section-4.6, so they'd need X25519+A128KW, X448+A256KW, etc. (And then it wouldn't really be obvious for us which alg to use, same as today)

@panva
Copy link
Contributor Author

panva commented Aug 30, 2023

Hello 👋 AFAIU from the proposal and linked draft (ietf.org/archive/id/draft-jones-jose-fully-specified-algorithms-00.html), the alg member won't be removed but replaced with a more specific value, and I assume the crv member would be removed instead.

There are no changes to OKP JWK key crv values. What's being deprecated is the EdDSA alg used for JWS using both Ed25519 and Ed448 and replaced with a unique identifier for each sub type.

So, I think removing the alg member wouldn't be conformant to this proposal nor the current spec.

It would, alg is not a required JWK member. The X25519/448 JWKs our spec exports/imports doesn't have alg either.

I guess for ECDH it's a bit more complicated because they also want to include the AES-KW algorithm, as per rfc-editor.org/rfc/rfc7518#section-4.6, so they'd need X25519+A128KW, X448+A256KW, etc. (And then it wouldn't really be obvious for us which alg to use, same as today)

No touch up to the ECDH JWA algorithm identifier is currently part of the discussion. Edit: I see now (Discuss the treatment of EDCH-ES and its ephemeral keys.).

@panva
Copy link
Contributor Author

panva commented Aug 30, 2023

Basically we just revert #3 and #4 and this deprecation does not affect our spec at all, even if ECDH-ES algorithm identifiers would change since we already ignore the jwk alg on import for all ECDH-able algorithms.

@twiss
Copy link
Collaborator

twiss commented Aug 30, 2023

I see, OK. I was under the impression that "crv" defined a subtype of the algorithm (i.e. defining a new algorithm would void them), but I guess it defines a subtype of the "kty" OKP; that sort of makes sense. So yeah, I think you're right; do you want to make a PR or revert the previous ones? Otherwise I can do so.

@panva
Copy link
Contributor Author

panva commented Aug 30, 2023

Please do 👍 i'll do updates to WPTs

@panva
Copy link
Contributor Author

panva commented Aug 30, 2023

No existing WPT is affected. Do you think we should add negative tests checking for this removed behaviour?

@twiss
Copy link
Collaborator

twiss commented Aug 30, 2023

Hmm.. maybe it might make sense to add a generic test checking that the fields are the same / there aren't any extra fields, instead of just checking that the alg field isn't there, specifically?

@panva
Copy link
Contributor Author

panva commented Aug 30, 2023

Not sure that I got that. To check that this update was updated in an implementation we need to check that jwk export has no alg member and that the alg member is ignored during import, ie. its value can be any string.

@twiss
Copy link
Collaborator

twiss commented Aug 30, 2023

I meant for export specifically, you could check that there aren't any additional members/fields (beyond the ones specified), rather than adding a check that alg isn't there, specifically. E.g. you could check Object.keys(jwk).length, or so.

For import, indeed you could add a test that passes a different alg value as well, or a random value, if you want to thoroughly test this 👍

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 a pull request may close this issue.

2 participants