Copilot commented on code in PR #4334:
URL: https://github.com/apache/solr/pull/4334#discussion_r3229737693
##########
solr/solr-ref-guide/modules/deployment-guide/pages/jwt-authentication-plugin.adoc:
##########
@@ -42,17 +42,15 @@ The simplest possible `security.json` for registering the
plugin without configu
}
----
-The plugin will by default require a valid JWT token for all traffic.
-
-If the `blockUnknown` property is set to `false` as in the above example, it
is possible to start configuring the plugin using unauthenticated REST API
calls, which is further described in section <<Editing JWT Authentication
Plugin Configuration>>.
+By default (`blockUnknown=false`), requests without a JWT are allowed through,
which makes it possible to configure the plugin using unauthenticated REST API
calls as described in <<Editing JWT Authentication Plugin Configuration>>. Set
`blockUnknown=true` to require a valid JWT for all requests.
Review Comment:
The new text claims the default is `blockUnknown=false`, but the plugin code
defaults `blockUnknown` to `true` (see `JWTAuthPlugin` initialization using
`getOrDefault(PARAM_BLOCK_UNKNOWN, true)`) and this page later also states
blocking unauthenticated requests is the default. Please update this paragraph
to reflect the actual default and keep it consistent with the rest of the doc
and runtime behavior.
This issue also appears on line 53 of the same file.
##########
solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/JWTIssuerConfig.java:
##########
@@ -441,6 +441,110 @@ public Collection<X509Certificate> getTrustedCerts() {
return this.trustedCerts;
}
+ /** Builds an SSL socket factory trusting the given certificates. */
+ static SSLSocketFactory buildSSLSocketFactory(Collection<X509Certificate>
trustedCerts) {
+ try {
+ KeyStore ks = KeyStore.getInstance(KeyStore.getDefaultType());
+ ks.load(null, null);
+ int i = 0;
+ for (X509Certificate cert : trustedCerts) {
+ ks.setCertificateEntry("trusted-cert-" + i++, cert);
+ }
+ TrustManagerFactory tmf =
+
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
+ tmf.init(ks);
+ SSLContext sslContext = SSLContext.getInstance("TLS");
+ sslContext.init(null, tmf.getTrustManagers(), null);
+ return sslContext.getSocketFactory();
+ } catch (GeneralSecurityException | IOException e) {
+ throw new SolrException(
+ SolrException.ErrorCode.SERVER_ERROR, "Failed to build custom SSL
context", e);
+ }
+ }
+
+ /**
+ * Builds a ResourceRetriever with an optional custom SSL trust store. Uses
a custom
+ * implementation when trusted certs are configured; in that case hostname
verification is also
+ * bypassed for loopback hosts. Otherwise uses the default
DefaultResourceRetriever.
+ */
+ static ResourceRetriever buildResourceRetriever(
+ Collection<X509Certificate> trustedCerts, URL url) {
+ if (trustedCerts == null) {
+ return new DefaultResourceRetriever();
+ }
+ SSLSocketFactory ssf = buildSSLSocketFactory(trustedCerts);
+ InetAddress loopback = InetAddress.getLoopbackAddress();
+ boolean disableHostnameVerification =
+ loopback.getCanonicalHostName().equals(url.getHost())
+ || loopback.getHostName().equals(url.getHost());
+ return resourceUrl -> {
+ URLConnection conn = resourceUrl.openConnection();
+ if (conn instanceof HttpsURLConnection httpsConn) {
+ httpsConn.setSSLSocketFactory(ssf);
+ if (disableHostnameVerification) {
+ httpsConn.setHostnameVerifier((hostname, session) -> true);
+ }
+ }
+ try (InputStream in = conn.getInputStream()) {
+ String content = new String(in.readAllBytes(), StandardCharsets.UTF_8);
+ return new Resource(content, conn.getContentType());
+ }
+ };
Review Comment:
`buildResourceRetriever`'s custom `ResourceRetriever` uses `URLConnection`
with no explicit connect/read timeouts (and no response size limit), unlike
`DefaultResourceRetriever`. This can cause request threads to hang indefinitely
if an IdP endpoint is slow/unresponsive. Please set reasonable timeouts on the
connection (and ideally enforce a max response size), or rework this to
leverage Nimbus's `DefaultResourceRetriever` behavior while still applying the
custom SSL socket factory / hostname verifier.
##########
solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/IssuerAwareJWSKeySelector.java:
##########
@@ -0,0 +1,218 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.solr.security.jwt;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.JWSHeader;
+import com.nimbusds.jose.KeySourceException;
+import com.nimbusds.jose.jwk.ECKey;
+import com.nimbusds.jose.jwk.JWK;
+import com.nimbusds.jose.jwk.JWKMatcher;
+import com.nimbusds.jose.jwk.JWKSelector;
+import com.nimbusds.jose.jwk.JWKSet;
+import com.nimbusds.jose.jwk.OctetSequenceKey;
+import com.nimbusds.jose.jwk.RSAKey;
+import com.nimbusds.jose.proc.JWSKeySelector;
+import com.nimbusds.jose.proc.SecurityContext;
+import java.io.IOException;
+import java.lang.invoke.MethodHandles;
+import java.security.Key;
+import java.text.ParseException;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Set;
+import javax.net.ssl.SSLHandshakeException;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * Resolves JWS signature verification keys from a set of {@link
JWTIssuerConfig} objects, which may
+ * represent any valid configuration in Solr's security.json, i.e. static list
of JWKs or keys
+ * retrieved from HTTPS JWK endpoints.
+ *
+ * <p>This implementation maintains a map of issuers, each with its own list
of {@link JWK}, and
+ * resolves the correct key from the correct issuer. The issuer is passed in
via {@link
+ * IssuerContext}.
+ *
+ * <p>If a key is not found and the issuer is backed by HTTPS JWKs, one cache
refresh is attempted
+ * before failing.
+ */
+public class IssuerAwareJWSKeySelector implements
JWSKeySelector<SecurityContext> {
+ private static final Logger log =
LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
+
+ private final Map<String, JWTIssuerConfig> issuerConfigs = new HashMap<>();
+ private final boolean requireIssuer;
+
+ /**
+ * SecurityContext subclass that carries the (unverified) issuer claim from
the JWT payload,
+ * allowing the key selector to look up the correct issuer configuration.
+ */
+ public static class IssuerContext implements SecurityContext {
+ private final String issuer;
+
+ public IssuerContext(String issuer) {
+ this.issuer = issuer;
+ }
+
+ public String getIssuer() {
+ return issuer;
+ }
+ }
+
+ /**
+ * Resolves key from a JWKs from one or more IssuerConfigs
+ *
+ * @param issuerConfigs Collection of configuration objects for the issuer(s)
+ * @param requireIssuer if true, will require 'iss' claim on jws
+ */
+ public IssuerAwareJWSKeySelector(
+ Collection<JWTIssuerConfig> issuerConfigs, boolean requireIssuer) {
+ this.requireIssuer = requireIssuer;
+ issuerConfigs.forEach(ic -> this.issuerConfigs.put(ic.getIss(), ic));
+ }
+
+ @Override
+ public List<? extends Key> selectJWSKeys(JWSHeader header, SecurityContext
context)
+ throws KeySourceException {
+ String tokenIssuer =
+ (context instanceof IssuerContext) ? ((IssuerContext)
context).getIssuer() : null;
+
+ JWTIssuerConfig issuerConfig = resolveIssuerConfig(tokenIssuer);
+
+ List<JWK> allJwks = new ArrayList<>();
+ String keysSource = "N/A";
+ try {
+ if (issuerConfig.usesHttpsJwk()) {
+ keysSource = "[" + String.join(", ", issuerConfig.getJwksUrls()) + "]";
+ for (JWTIssuerConfig.JwkSetFetcher fetcher :
issuerConfig.getHttpsJwks()) {
+ try {
+ allJwks.addAll(fetcher.getKeys());
+ } catch (SSLHandshakeException e) {
+ throw new KeySourceException(
+ "Failed to connect with "
+ + fetcher.getLocation()
+ + ", do you have the correct SSL certificate configured?",
+ e);
+ }
+ }
+ } else {
+ keysSource = "static list of keys in security.json";
+ allJwks.addAll(issuerConfig.getJsonWebKeySet().getKeys());
+ }
+ } catch (IOException | ParseException e) {
+ throw new KeySourceException(
+ String.format(
+ Locale.ROOT, "Unable to fetch JWKs from source %s: %s",
keysSource, e.getMessage()),
+ e);
+ }
+
+ JWKSelector selector = new JWKSelector(JWKMatcher.forJWSHeader(header));
+ List<JWK> matchingJwks = selector.select(new JWKSet(allJwks));
+
+ if (matchingJwks.isEmpty() && issuerConfig.usesHttpsJwk()) {
+ if (log.isDebugEnabled()) {
+ log.debug(
+ "No matching key found for JWS header {} in {} keys from {};
refreshing",
+ header,
+ allJwks.size(),
+ keysSource);
+ }
+ allJwks.clear();
+ try {
+ for (JWTIssuerConfig.JwkSetFetcher fetcher :
issuerConfig.getHttpsJwks()) {
+ fetcher.refresh();
+ allJwks.addAll(fetcher.getKeys());
+ }
Review Comment:
In the refresh path, `SSLHandshakeException` from `fetcher.refresh()` /
`fetcher.getKeys()` is not given the same helpful hint as the initial fetch
path (which explicitly suggests checking configured SSL certs). Consider
catching `SSLHandshakeException` here as well and rethrowing a
`KeySourceException` with the clearer message to keep troubleshooting
consistent.
##########
solr/modules/jwt-auth/src/java/org/apache/solr/security/jwt/JWTIssuerConfig.java:
##########
@@ -475,19 +574,12 @@ private HttpsJwks create(String url) {
SolrException.ErrorCode.SERVER_ERROR,
"Url " + url + " configured in " + PARAM_JWKS_URL + " is not a
valid URL");
Review Comment:
`HttpsJwksFactory.create()` only catches `MalformedURLException`, but
`URI.create(url)` can also throw `IllegalArgumentException` for invalid input.
That would currently bubble up as an unchecked exception instead of a clear
`SolrException` pointing to `jwksUrl`. Please catch/wrap
`IllegalArgumentException` (or validate earlier) so invalid JWKS URLs
consistently produce a helpful configuration error.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
