hsheinblatt commented on code in PR #1319:
URL: https://github.com/apache/knox/pull/1319#discussion_r3632631534
##########
gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerService.java:
##########
@@ -195,6 +202,35 @@ private synchronized void reloadRegistrySnapshot() {
registrySnapshot.set(Collections.unmodifiableMap(fresh));
} catch (Exception e) {
LOG.errorReloadingRegistrySnapshot(e.getMessage(), e);
+ throw new RuntimeException("Error reloading trusted OIDC issuer registry
snapshot", e);
}
}
+
+ /**
+ * Canonicalizes an issuer URL for registry storage and lookup by stripping
a single
+ * trailing slash, so that {@code https://issuer.example.com/} and
+ * {@code https://issuer.example.com} are treated as the same issuer. This
matches the
+ * trailing-slash stripping {@link OIDCDiscoveryHelper} already applies when
building the
+ * discovery URL. Null-safe.
+ */
+ private static String normalizeIssuerUrl(String issuerUrl) {
Review Comment:
I like the idea of ensuring that the user-entered issuer URI registered is
correct and being somewhat permissive to prevent user-input errors that might
cause confusion. However, I'm not sure it's possible.
Checked with the AI again, and this is a standard problem. What matters is
the format of the issuer in the iss JWT claim. Our registered issuer must match
it exactly, and that value comes from the IdP in use, not us. Every issuer is
different. It does seem like omitting the trailing slash is the most common
format used by the most-used IdPs, but in general, it's not a required format,
and some IdP's have the trailing slash. In particular, k8s for service account
projected tokens can include a trailing slash, depending on the config, and
some external IdP's customers might use do include it.
So when you register the issuer, you must know what format your IdP uses and
ensure that you register the correct URI. We can validate the input URL for
the known requirements, like must be 'https://', cannot contain query string
parts, and so on, but the trailing slash is not defined in the spec as required
or not allowed: https://openid.net/specs/openid-connect-discovery-1_0.html
The problem in the original PR is different: we want to use the issuer as a
base to construct the discovery URL. Adding '/.well-known/openid-configuration'
is standard, but if applied blindly in all cases can lead to a double slash,
which may fail. So to ensure the discovery lookup works for issuer urls that
end in a slash as well as those that do not, we want to normalize the base url
to have a single slash in the full discovery url. But we can't change the
stored issuer URL, since it must match whatever the IdP uses as the issuer
claim, and that may include a trailing slash.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
