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

Validate JWK for HmacSecretKey Class #109

Open
HamdaanAliQuatil opened this issue May 7, 2024 · 2 comments
Open

Validate JWK for HmacSecretKey Class #109

HamdaanAliQuatil opened this issue May 7, 2024 · 2 comments

Comments

@HamdaanAliQuatil
Copy link
Collaborator

Our goal is to make the API hard to use incorrectly. For the HmacSecretKey class we would want to validate the properties on importJsonWebKey against imported JWK to reduce the risk of accidentally using incorrect JWK. Below are a few options suggested by @jonasfj:

  1. Make properties on importJsonWebKey optional, and validate them when present.
  2. Make the properties required and validate with jwk.

We can try a mix for different parameters and we have a consensus of requiring the alg property to make it consistent with other JWK imports.
However, note that the RFC 7517 states that the alg property is indeed optional and this should be kept into consideration while drafting a permanent solution for webcrypto.dart.

I will post my discovery here and would love to hear opinions on the same.

@jonasfj
Copy link
Member

jonasfj commented May 8, 2024

This probably doesn't just apply to HmacSecretKey, but all places where we do importJsonWebKey.

I think on the web window.crypto.subtle.importKey always requires that you explicitly specify:

  • (a) the key (format and keyData)
  • (b) what kind of key you want to import (algorithm parameter, like HmacImportParams).
  • (c) what the key may be used for (extractable and keyUsages).

We've largely opted not to do (c), see discussion in #53.

But yes, in some cases parts of (b) could be omitted. In particular when talking about JWKs.

I guess the benefit of reading the hash from the JWK, is that it's easier to just download a JWK from somewhere and use it. Where as, the way the API currently looks you have to read the alg property of the JWKand switch onHS1, HS256, etc, to pass instances of Hash.sha1orHash.sha256in thehash` parameter. This isn't super convenient.

On the other hand, if you download a JWK from somewhere without checking to see what hash it's using, you might unintentionally be using a weaker hash than you intended. And on most use cases you probably know what kind of algorithm you expect to be using, and you shouldn't want your code to dynamically use a different hash.

So perhaps it's best to say that you can import a JWK, but you must specify what you expect the JWK to be.

And if you want to support importing any kind if JWK, then it's in convenient, and you'll have read the JWK alg, etc...

I'm actually leaning towards the current API, just because we don't necessarily want this package to provide something that is possibly too smart. And maybe it's better to be simple and a little inconvenient. And if someone really wants a smart crypto library that an import any JWK by just parsing the JWK, then maybe that someone should make a high-level package with a high-level API for this purpose.

@jonasfj
Copy link
Member

jonasfj commented May 8, 2024

As such maybe the only take away from this is:

  • Let's delete the TODO comments 🤣
  • Let's always make sure that whenever a JWK has a property, the value of said property is consistent what how we intend to use it.
    (This might especially make sense since most JWKs have lots of optional properties).

But I'm also interested in hearing other perspectives.

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

No branches or pull requests

2 participants