-
Notifications
You must be signed in to change notification settings - Fork 487
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
[oidc-discovery-provider] Fix keys url #5690
base: main
Are you sure you want to change the base?
Changes from all commits
bfbdeb3
442a1c8
6b33c7f
29bdd98
63d7981
f35e79b
6f5e7e9
88e33bb
295ccc2
8800dba
219ac45
b88a3ae
9bcf2dd
d18d37d
79865b8
851dc3a
dc97122
d191da0
bc0b68e
6d07e62
8c7972b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,23 +25,39 @@ type Handler struct { | |
setKeyUse bool | ||
log logrus.FieldLogger | ||
jwtIssuer string | ||
jwksURI string | ||
Comment on lines
27
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could store these 2 at start up as *url.URL to avoid parsing at runtime. |
||
serverPathPrefix string | ||
|
||
http.Handler | ||
} | ||
|
||
func NewHandler(log logrus.FieldLogger, domainPolicy DomainPolicy, source JWKSSource, allowInsecureScheme bool, setKeyUse bool, jwtIssuer string) *Handler { | ||
func NewHandler(log logrus.FieldLogger, domainPolicy DomainPolicy, source JWKSSource, allowInsecureScheme bool, setKeyUse bool, jwtIssuer string, jwksURI string, serverPathPrefix string) *Handler { | ||
if serverPathPrefix == "" { | ||
serverPathPrefix = "/" | ||
} | ||
h := &Handler{ | ||
domainPolicy: domainPolicy, | ||
source: source, | ||
allowInsecureScheme: allowInsecureScheme, | ||
setKeyUse: setKeyUse, | ||
log: log, | ||
jwtIssuer: jwtIssuer, | ||
jwksURI: jwksURI, | ||
serverPathPrefix: serverPathPrefix, | ||
} | ||
|
||
mux := http.NewServeMux() | ||
mux.Handle("/.well-known/openid-configuration", handlers.ProxyHeaders(http.HandlerFunc(h.serveWellKnown))) | ||
mux.Handle("/keys", http.HandlerFunc(h.serveKeys)) | ||
wkPath, err := url.JoinPath(serverPathPrefix, "/.well-known/openid-configuration") | ||
if err != nil { | ||
return nil | ||
} | ||
jwksPath, err := url.JoinPath(serverPathPrefix, "/keys") | ||
if err != nil { | ||
return nil | ||
} | ||
|
||
mux.Handle(wkPath, handlers.ProxyHeaders(http.HandlerFunc(h.serveWellKnown))) | ||
mux.Handle(jwksPath, http.HandlerFunc(h.serveKeys)) | ||
|
||
h.Handler = mux | ||
return h | ||
|
@@ -56,6 +72,7 @@ func (h *Handler) serveWellKnown(w http.ResponseWriter, r *http.Request) { | |
var host string | ||
var path string | ||
var urlScheme string | ||
var jwksURI url.URL | ||
if h.jwtIssuer != "" { | ||
jwtIssuerURL, _ := url.Parse(h.jwtIssuer) | ||
host = jwtIssuerURL.Host | ||
|
@@ -68,8 +85,27 @@ func (h *Handler) serveWellKnown(w http.ResponseWriter, r *http.Request) { | |
urlScheme = "http" | ||
} | ||
} | ||
if h.jwksURI != "" { | ||
tmpURI, _ := url.Parse(h.jwksURI) | ||
jwksURI = *tmpURI | ||
} else { | ||
tmpURLScheme := "https" | ||
if h.allowInsecureScheme && r.TLS == nil && r.URL.Scheme != "https" { | ||
tmpURLScheme = "http" | ||
} | ||
keysPath, err := url.JoinPath(h.serverPathPrefix, "keys") | ||
if err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe |
||
return | ||
} | ||
jwksURI = url.URL{ | ||
Scheme: tmpURLScheme, | ||
Host: r.Host, | ||
Path: keysPath, | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Would it be possible to simplify this a bit. I'm thinking something like:
Minimizes dependencies between the parts of the two URLs and makes it a bit easier to read, at least in my opinion. |
||
|
||
if err := h.verifyHost(host); err != nil { | ||
if err := h.verifyHost(r.Host); err != nil { | ||
http.Error(w, err.Error(), http.StatusBadRequest) | ||
return | ||
} | ||
|
@@ -80,8 +116,6 @@ func (h *Handler) serveWellKnown(w http.ResponseWriter, r *http.Request) { | |
Path: path, | ||
} | ||
|
||
jwksURI := issuerURL.JoinPath("keys") | ||
|
||
doc := struct { | ||
Issuer string `json:"issuer"` | ||
JWKSURI string `json:"jwks_uri"` | ||
|
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.
Could we follow the more verbose error checking done for JWTIssuer? It gives a bit more detail about the way the uri is invalid..