-
Notifications
You must be signed in to change notification settings - Fork 32
Sync JWKS URI #3009
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
base: main
Are you sure you want to change the base?
Sync JWKS URI #3009
Conversation
- So the JWKS URI federation discovery endpoint and what's in Pelican process' global federation metadata `pelican_url.FederationDiscovery` - In `discoverFederationImpl`, `fedInfo.JwksUri` will first fill with the value of the `Federation.JwkUrl` config param. If it is not set, that function will probe Discovery Endpoint to get the `jwks_uri` there. At last, if `fedInfo.JwksUri` is still empty, it will fallbacks to Director's externalUrl + "/.well-known/issuer.jwks"
|
I know you closed this, but one thing to consider -- the |
|
Thanks for the enlightenment @jhiemstrawisc. I think it would be better to reopen this PR and close the other one (#3008)? |
|
Up to you how you want to resolve things! I'd argue what we're shooting for here is:
Since we technically allow admins to overwrite discovered values with whatever they want via config, we should ask whether a configuration like that means a) "Use this different Registry internally, but keep the duplicate fed discovery metadata consistent by using the discovered registry" or does it mean b) "I want to use this Registry and also overwrite the value that gets put into federation metadata discovery. While I'm leaning towards a), I think for now it's better to do b) and present the warning you worked on in #3008 (otherwise you'll have to decouple big sections of code). What do you think? |
|
After one day of consideration, my plan is moving forward with both this PR and #3008. This PR can ensure the same keys are provided via the And #3008 can warn the Director admin for rare scenarios when they overwrite the config related to the metadata. Beside your example in 2., they can customize the |
| log.Error("Bad server configuration: fail to generate JwksUri: ", err) | ||
| jwksUri := fedInfo.JwksUri | ||
| if jwksUri == "" { | ||
| log.Error("Bad server configuration: fail to get JwksUri: ", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two comments here:
- I don't think you want to provide this
errhere because it's coming from the previousregistryUrl, err := url.Parse(registryUrlStr)and should thus always be nil - I think we should avoid using the internal variable name
JwksUrihere. Instead, can you say something likeBad server configuration: Director is missing federation keys (\"jwks_uri\")? I like this a little better because someone who looks at the metadata JSON will seejwks_uri, notJwksUri.
| ctx.JSON(http.StatusInternalServerError, server_structs.SimpleApiResp{ | ||
| Status: server_structs.RespFailed, | ||
| Msg: "Bad server configuration: JwksUri is not valid", | ||
| Msg: "Bad server configuration: unable to get JwksUri", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whatever phrasing you end up landing on for the previous comment, can you use the same phrasing between these two messages?
| if err != nil { | ||
| log.Error("Bad server configuration: fail to generate JwksUri: ", err) | ||
| jwksUri := fedInfo.JwksUri | ||
| if jwksUri == "" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I almost wonder if this should be an error that prevents the Director from starting -- if there's no jwks_uri, can the federation function at all? I'll let you decide how far you want to chase it.
This PR syncs the JWKS URI in federation discovery endpoint
/.well-known/pelican-configurationwith the JWKS URI in Pelican process' global federation metadatapelican_url.FederationDiscovery/.well-known/pelican-configurationto Director URL, even though the Discovery URL is set. This means the JWKS URI provided by Discovery URL may contain a different set of keys.But this may be intentional! Director should advertise its own key endpoint rather than potentially pointing to a different server's JWKS URI.This PR was intended to replace Periodic metadata discrepancy check for the Director #3008. But I realize maybe the good practice for a server is using its own JWKS URI, instead of pointing to another's, even though there might be discrepancy for the keys between them! We should still use a dedicated goroutine to monitor the discrepancy, as in Periodic metadata discrepancy check for the Director #3008. So I just close this PR.