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

[RFC] Change jws.Verify (and thereore jwt.Parse) for 2.0.0 #577

Closed
Tracked by #388
lestrrat opened this issue Feb 20, 2022 · 4 comments
Closed
Tracked by #388

[RFC] Change jws.Verify (and thereore jwt.Parse) for 2.0.0 #577

lestrrat opened this issue Feb 20, 2022 · 4 comments
Assignees

Comments

@lestrrat
Copy link
Collaborator

lestrrat commented Feb 20, 2022

Idea: change the basic API structure to jws.Verify([]byte, options), and handle keys by having users provide KeyProviders
If we no KeyProvider can be detected, it is an error.

basic idea

the jws verification process received multiple key providers.
A key provider may return multiple keys to be tried, for cases such as jwk.Set

for _, sig := range parsedJWS.Signatures() {
  for _, provider := range providers {
    iter := provider.FetchKey(sink, sig)
    for iter.Next() {
      alg, key, err := iter.Key()
      if payload, err := internalVerify( parsedJWS, alg, key ); err == nil {
        return payload, nil
      }
    }
  }
} 

single key

jws.Verify(payload, jws.WithKey(alg, key))

Internally, creates a StaticKeyProvider, always returns the same alg/key pair

a JWKS

jws.Verify(payload, jws.WithKeySet(ks))

Internally creaes a KeySetProvider, returns the possible keys to be tried. However, normally the kid must match, and therefore a lot of times only one key would be returned

a user-provided function

a jws.KeySink is an object where the user specifies (possible multiple) keys to be used

type KeySink interface {
  Key(jwk.Key)
}
jws.Verify(payload, jws.WithKeyProvider( jws.KeyProviderFunc( func(msg *jws.Message, sink jws.KeySink) error ) ) )

auto-verify using jku

f := jws.JWKSetFetcher(....)
jws.Verify(payload, jws.WithAutoVerify(f))

This is an odd-ball case, but it will create a JKUProvider or some such.

Other considerations

We need a way to give back the user which key was used.

var key jwk.Key 
jws.Verify(payload, jws.WithKeySet(ks), jws.WithKeyUsed(&key))

This is a fairly major change. behind the scenes, we will need to standardize on using jws.Message all the time (previously, we optimized the path for compact serialization). We will probably also need to standardize on jwk.Key in some cases (I think WithKey() can accept any raw key as well)

There are going to be issues trying to report problems from within these options.
some errors will have to be ignored, but there may be cases where users need a way to reach these ignored errors


Addendum Feb 21

  • Changed FetchKey() signature
  • Changed jws.WithVerifyAuto() signature
  • Added note about errors
@cloudkucooland
Copy link
Contributor

Looks good. Let me know when you start a 2.0 branch, I can migrate to it and start testing.

@lestrrat
Copy link
Collaborator Author

Changes on the jwt side

Most of the options will directly be translated into an equivalent jwt.WithXXX option.
However, jwt.WithKeySetProvider will have to go. This just doesn't work. See below for proposed workaround.

single key

jwt.Parse(payload, jwt.WithKey(alg, key))

a JWKS

jwt.Parse(payload, jwt.WithKeySet(set))

auto-verifying jku

f := jws.NewJWKSetFetcher(...)
jwt.Parse(payload, jwt.WithVerifyAuto(f))

KeySetProvider will go away

Looking at the timing where this is possible, it will have to be after jws.Parse is called, but before the actual verification starts -- meaning it really needs to be in jws, not jwt. So we would have to include a jwt-specific logic into jws.

Further looking into the code, adding more callbacks like this just feels like it's going to open up a can of worms for further requests to callbacks.

Therefore, I believe that this option should go away. Instead you can do this:

msg, err := jws.Parse(payload)
if err != nil {
   return err
}
tok, err := jwt.Parse(msg.Payload())
if tok.Issuer() == "my-particular-iss-of-interest" {
  jws.Verify(msg, jws.WithKeySet(set))
}

@lestrrat
Copy link
Collaborator Author

lestrrat commented Feb 22, 2022

I'm not creating a v2 branch for this yet, as I don't know if this is the approach that I will be end up taking.
But for those of you interested, there's a branch that I worked on. Feel free to test it if you are interested.

For the time being I'm done with the PoC above, and I'm going to sit on it for a while. No dates have been set for v2 yet.

@lestrrat
Copy link
Collaborator Author

lestrrat commented Mar 2, 2022

Most of this is done. closing for now

@lestrrat lestrrat closed this as completed Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants