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

Release v1.0.0 #23

Closed
MicahParks opened this issue Nov 13, 2021 · 9 comments
Closed

Release v1.0.0 #23

MicahParks opened this issue Nov 13, 2021 · 9 comments

Comments

@MicahParks
Copy link
Owner

MicahParks commented Nov 13, 2021

The purpose of this issue is to release v1.0.0.

Initially, I was waiting on this PR before doing a v1.0.0 release: golang-jwt/jwt#105 but it seems that the PR is stale and it, along with golang-jwt/jwt#73, will go unresolved for an undetermined amount of time.

The alternative plan, of creating the github.com/MicahParks/compatibility-keyfunc package and removing fork support from this package will likely be the long term solution. (See the v0.9.0 release.)

If you have an idea about how to improve the package before the package's externally facing API is stabilized with v1.0.0, please comment on this issue. I've included my thoughts for changes below.

If you would like to participate, please feel free to leave a comment constructive to the conversation that includes what you are in favor of and/or proposed changes you are not in favor of.

I would like for this project to be feature-complete upon the v1.0.0 release, with the exception of adding support for additional cryptographic algorithms as they become standardized and widely adopted. This isn't a hard requirement, but I have every intention to avoid a /v2 for this project.

My thoughts on changes for v1.0.0:

Breaking:

  1. Maybe rename JWKs to JWKS or JWKSet.
  • I've used JWKs as a short hand for JWK Set.
  • I've seen it both ways on the internet, and the RFC does not include a shorthand. The RFC does use JWKs as a short and for JSON Web Keys, so I'm thinking using JWKs as a shorthand for JWK Set is incorrect.
  • Nobody should be building the keyfunc.JWKs data structure through a struct literal, so the only breaking portion should be non-shorthand declarations of keyfunc.JWKs. There would also be documentation changes.

New features:

  1. Add a method to return a map[string]interface{} or maybe something similar to map[string]*jsonKey.
  • For use cases that want the cryptographic keys outside of the Keyfunc method.
  1. Explore JWK without a kid keys in the Keyfunc method.
  • The use of the kid JSON attribute in a JWK is optional: https://datatracker.ietf.org/doc/html/rfc7517#section-4.5
  • Every major identity provider, as far as I know, uses the kid JSON attribute in their JWK Sets.
  • When calling the Keyfunc method, if there is no matching kid, every key could be cycled through until there is no error.
  • This would require a change to the underlying data structures used and may require more Options fields to explicitly enable/disable the behavior.

Improvements:

  1. Parse all JWK in the JWK set upon every HTTP GET.
  • Currently JWK are not parsed until they are used the first time. After the first time, the computed result is stored in the precomputed field.
  • This defers any errors with the keys until the Keyfunc method is called, meaning the errors in parsing don't need to be reported asynchronously. It also may have a slight performance gain due to unused keys.
  • Someone pointed out to me that it's a bit more complex than just parsing all keys right away. Keys that produce errors when parsing could be ignored silently.
@MicahParks
Copy link
Owner Author

Oh, and I also plan on adding EdDSA support before v1.0.0

@AlexanderYastrebov
Copy link
Contributor

When calling the Keyfunc method, if there is no matching kid, every key could be cycled through until there is no error.

This is effectively token validation implemented inside of keyfunc. I think unless golang-jwt provides an interface to try multiple keys this is the only option. Here is the issue from the original repo dgrijalva/jwt-go#416

@AlexanderYastrebov
Copy link
Contributor

Is there a need to support multiple Options actually?

keyfunc/get.go

Line 20 in b671333

func Get(jwksURL string, options ...Options) (jwks *JWKs, err error) {

If not, I think jwksUrl could also be put into the options and the whole interface simplified to

jwks, err := keyfunc.NewJWKs(keyfunc.Options{
    url: "https://example.com",
    RefreshInterval:   1 * time.Hour,
    RefreshRateLimit:  5 * time.Minute,
    RefreshTimeout:    10 * time.Second,
    RefreshUnknownKID: true,
})

@MicahParks
Copy link
Owner Author

MicahParks commented Nov 13, 2021

Is there a need to support multiple Options actually?

keyfunc/get.go

Line 20 in b671333

func Get(jwksURL string, options ...Options) (jwks *JWKs, err error) {

If not, I think jwksUrl could also be put into the options and the whole interface simplified to

jwks, err := keyfunc.NewJWKs(keyfunc.Options{
    url: "https://example.com",
    RefreshInterval:   1 * time.Hour,
    RefreshRateLimit:  5 * time.Minute,
    RefreshTimeout:    10 * time.Second,
    RefreshUnknownKID: true,
})

There isn't. It was meant as a backwards compatible way of introducing options. It's also a pattern I've seen before with options. I'm now considering making it a non-variadic argument, which would simplify pointers like from #18 and #24.

@AlexanderYastrebov
Copy link
Contributor

AlexanderYastrebov commented Nov 13, 2021

Parse all JWK in the JWK set upon every HTTP GET.
Someone pointed out to me that it's a bit more complex than just parsing all keys right away

If the key set construction performance is a concern, it should be possible to keep track of the response data hash

keyfunc/get.go

Lines 184 to 188 in b671333

// Read the raw JWKs from the body of the response.
var jwksBytes []byte
if jwksBytes, err = ioutil.ReadAll(resp.Body); err != nil {
return err
}

and construct new keyset only when hash changes.

UPD: or just keep actual data bytes and simply compare to response body bytes

@MicahParks
Copy link
Owner Author

Is there a need to support multiple Options actually?

keyfunc/get.go

Line 20 in b671333

func Get(jwksURL string, options ...Options) (jwks *JWKs, err error) {

If not, I think jwksUrl could also be put into the options and the whole interface simplified to

jwks, err := keyfunc.NewJWKs(keyfunc.Options{
    url: "https://example.com",
    RefreshInterval:   1 * time.Hour,
    RefreshRateLimit:  5 * time.Minute,
    RefreshTimeout:    10 * time.Second,
    RefreshUnknownKID: true,
})

Simplifying the whole interface to one function with one large Options struct with many fields is interesting. I'm currently thinking that the NewJSON, NewGiven, and Get functions are a helpful distinction. But this is something I'll continue to consider.

@MicahParks
Copy link
Owner Author

I'm starting work on v1.0.0. The branch is release_candidate. Anyone is free to make a PR to it. I'll make sure any PRs containing a novel suggestion are referenced by at least one comment in this issue.

@MicahParks
Copy link
Owner Author

The PR is ready and can be viewed here: #25

  • I've renamed JWKs to JWKS.
  • Options are not variadic and mandatory for keyfunc.Get. (An empty struct is still fine.)
  • A fnv 64-bit checksum of the JWKS is used to determine if keys should be parsed after an HTTP GET.
  • EdDSA with an ed25519 curve is now supported. (ed448 is not.)
  • Remote oct key types, including HMAC, are now supported.
  • All keys are now precomputed when the raw JWKS is read.
  • There's now a function to get a map[string]interface{} for the kid to cryptographic keys found in the JWKS. It's intended to be read-only.
  • I'm currently still thinking that separate keyfunc.JWKS creation functions are helpful: Get, NewJSON, NewGiven.
  • I looked more into key selection without a kid and determined it's likely out of scope for this project.

I plan on leaving the PR up until at least the 6th of December for anyone who wishes to provide feedback or additional input.

@MicahParks
Copy link
Owner Author

Closing due to the v1.0.0 release: https://github.com/MicahParks/keyfunc/releases/tag/v1.0.0

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