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

JWT: Enable reading a JSON Web Key Set from a file #180

Merged
merged 25 commits into from
Oct 30, 2023

Conversation

mortenmj
Copy link
Contributor

@mortenmj mortenmj commented Oct 6, 2023

This changes the JWKS configuration to be one of either inline content (as before) or a reference to a file.

To enable this, a new signature validator is added, the ForwardingSignatureValidator, which holds a pointer to some other validator that requests to ValidateSignature are forwarded to. We can replace this validator by calling Replace(). When periodically reading the content at the JWKS file path, this internal validator is updated to one containing the new JWKS content.

ForwardingSignatureValidator holds a pointer to some other SignatureValidator
and will forward ValidateSignature requests to this validator.
The validator it references can be changed by calling Replace().
This changes the jwt.proto definition to take either an inline JWKS
struct, or a message containing a file path and a refresh interval.

The intention is that when a file path and refresh interval is provided,
we create a ForwardingSignatureValidator, and periodically update its internal
SignatureValidator with content from the referenced file.

When passing inline content, behavior should remain unchanged.
When loading the JWT configuration, check if the config provides inline
JWKS content, or a reference to a file. If we get a reference to a file,
we set up a goroutine to periodically fetch the file and update a
ForwardingSignatureValidator.
pkg/jwt/configuration.go Outdated Show resolved Hide resolved
pkg/jwt/forwarding_signature_validator.go Outdated Show resolved Hide resolved
pkg/proto/configuration/jwt/jwt.proto Outdated Show resolved Hide resolved
pkg/jwt/configuration.go Outdated Show resolved Hide resolved
@mortenmj mortenmj marked this pull request as ready for review October 8, 2023 11:49
pkg/jwt/configuration.go Show resolved Hide resolved
pkg/jwt/configuration.go Outdated Show resolved Hide resolved
pkg/jwt/configuration.go Outdated Show resolved Hide resolved
pkg/jwt/configuration.go Show resolved Hide resolved
pkg/proto/configuration/jwt/jwt.proto Outdated Show resolved Hide resolved
pkg/http/server.go Outdated Show resolved Hide resolved
Copy link
Member

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is starting to look great. Final round of comments and this should be good to land. Thanks!

pkg/jwt/configuration.go Outdated Show resolved Hide resolved
pkg/jwt/configuration.go Outdated Show resolved Hide resolved
}

var jwks jose.JSONWebKeySet
err = json.NewDecoder(r).Decode(&jwks)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One interesting fact about json.NewDecoder().Decode() I got told about the other day is that it:

  • still ends up loading all JSON in memory up front. There is thus no savings in memory usage
  • permits trailing garbage, as it simply stops parsing after the top-level object is completed

With that in mind, I guess we should just io.ReadAll() it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no preference. Even if it did stream the file, these files are so small that I don't think it matters very much

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But my concern is that json.NewDecoder().Decode() permits trailing garbage. Amongst other things, it's designed to also permit reading newline delimited JSON files.

I would suggest we use io.ReadAll() here.

pkg/global/apply_configuration.go Show resolved Hide resolved
pkg/jwt/configuration.go Outdated Show resolved Hide resolved
pkg/jwt/configuration.go Outdated Show resolved Hide resolved
@mortenmj
Copy link
Contributor Author

@EdSchouten any chance we could get this merged this week?

Copy link
Member

@EdSchouten EdSchouten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey there! Sorry for letting you wait. I overlooked the fact that you made another round of changes. One small remaining request and this is good to merge. Thanks!

}

var jwks jose.JSONWebKeySet
err = json.NewDecoder(r).Decode(&jwks)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But my concern is that json.NewDecoder().Decode() permits trailing garbage. Amongst other things, it's designed to also permit reading newline delimited JSON files.

I would suggest we use io.ReadAll() here.

@mortenmj
Copy link
Contributor Author

@EdSchouten ping

@EdSchouten EdSchouten merged commit 519a894 into buildbarn:master Oct 30, 2023
1 check passed
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