smolnar82 commented on code in PR #1315: URL: https://github.com/apache/knox/pull/1315#discussion_r3630454860
########## gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerService.java: ########## @@ -0,0 +1,200 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.services.knoxidf.trustedoidcissuer; + +import org.apache.hadoop.conf.Configuration; +import org.apache.knox.gateway.config.GatewayConfig; +import org.apache.knox.gateway.database.DataSourceProvider; +import org.apache.knox.gateway.i18n.messages.MessagesFactory; +import org.apache.knox.gateway.services.ServiceLifecycleException; +import org.apache.knox.gateway.services.security.AliasService; +import org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants; + +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** + * JDBC-backed implementation of {@link TrustedOidcIssuerService}. + * <p> + * Maintains an in-memory registry snapshot as an {@link AtomicReference} to an immutable + * {@link Map}. Reads ({@link #isTrusted}, {@link #isDynamicJwks}, {@link #list}) are + * lock-free and always see a consistent snapshot. Writes ({@link #register}, + * {@link #deregister}) are synchronized: the DB is committed first, then the snapshot is + * rebuilt from a fresh SELECT to guarantee the in-memory state cannot diverge from + * persistent storage. + * <p> + * HA note: each Knox node maintains its own snapshot. A registration on node A updates + * that node's snapshot immediately; other nodes' snapshots remain stale until restart. + */ +public class JdbcTrustedOidcIssuerService implements TrustedOidcIssuerService { + + private static final TrustedOidcIssuerServiceMessages LOG = + MessagesFactory.get(TrustedOidcIssuerServiceMessages.class); + + static final String MAX_TRUSTED_ISSUERS_CONFIG = "gateway.trustedoidcissuer.max.issuers"; + private static final int DEFAULT_MAX_TRUSTED_ISSUERS = 10_000; + + private final AtomicBoolean initialized = new AtomicBoolean(false); + private final Lock initLock = new ReentrantLock(true); + + private final AtomicReference<Map<String, TrustedOidcIssuer>> registrySnapshot = + new AtomicReference<>(Collections.emptyMap()); + + private AliasService aliasService; + private TrustedOidcIssuerDatabase database; + private OIDCDiscoveryHelper discoveryHelper; + private int maxTrustedIssuers; + + @Override + public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException { + if (!initialized.get()) { + initLock.lock(); + try { + if (aliasService == null) { + throw new ServiceLifecycleException("The required AliasService reference has not been set."); + } + try { + int maxIssuers = DEFAULT_MAX_TRUSTED_ISSUERS; + long cacheTtlSecs = KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CACHE_TTL_SECS; + int connectTimeoutMs = KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CONNECT_TIMEOUT_MS; + int readTimeoutMs = KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_READ_TIMEOUT_MS; + + if (config instanceof Configuration) { + final Configuration conf = (Configuration) config; + maxIssuers = conf.getInt(MAX_TRUSTED_ISSUERS_CONFIG, DEFAULT_MAX_TRUSTED_ISSUERS); + cacheTtlSecs = conf.getLong(KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DISCOVERY_CACHE_TTL_SECS, + KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CACHE_TTL_SECS); + connectTimeoutMs = conf.getInt(KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DISCOVERY_CONNECT_TIMEOUT_MS, + KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CONNECT_TIMEOUT_MS); + readTimeoutMs = conf.getInt(KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DISCOVERY_READ_TIMEOUT_MS, + KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_READ_TIMEOUT_MS); + } + + this.maxTrustedIssuers = maxIssuers; + this.database = new TrustedOidcIssuerDatabase( + DataSourceProvider.getDataSource(config, aliasService), config.getDatabaseType()); + this.discoveryHelper = new OIDCDiscoveryHelper(this, cacheTtlSecs, + OIDCDiscoveryHelper.buildHttpClient(connectTimeoutMs, readTimeoutMs)); + reloadRegistrySnapshot(); + initialized.set(true); + } catch (ServiceLifecycleException e) { + throw e; + } catch (Exception e) { + throw new ServiceLifecycleException("Error initializing JdbcTrustedOidcIssuerService: " + e, e); + } + } finally { + initLock.unlock(); + } + } + } + + @Override + public void start() throws ServiceLifecycleException { + } + + @Override + public void stop() throws ServiceLifecycleException { + } + + public void setAliasService(AliasService aliasService) { + this.aliasService = aliasService; + } + + protected AliasService getAliasService() { + return aliasService; + } + + @Override + public boolean isTrusted(String issuerUrl) { + return registrySnapshot.get().containsKey(issuerUrl); + } + + @Override + public boolean isDynamicJwks(String issuerUrl) { + final TrustedOidcIssuer entry = registrySnapshot.get().get(issuerUrl); + return entry != null && entry.isDynamicJwks(); + } + + @Override + public Optional<String> resolveJwksUri(String issuerUrl) { + return discoveryHelper.discoverJwksUri(issuerUrl); + } + + @Override + public synchronized void register(TrustedOidcIssuer issuer) { + if (registrySnapshot.get().size() >= maxTrustedIssuers) { + throw new IllegalStateException( + "Cannot register issuer: MAX_TRUSTED_ISSUERS (" + maxTrustedIssuers + ") reached"); + } + try { + database.insert(issuer); + } catch (SQLException e) { + LOG.errorRegisteringIssuer(issuer.getIssuerUrl(), e.getMessage(), e); + throw new RuntimeException("Error registering trusted OIDC issuer: " + issuer.getIssuerUrl(), e); + } + reloadRegistrySnapshot(); + } + + @Override + public synchronized void deregister(String issuerUrl) { + try { + database.delete(issuerUrl); + } catch (SQLException e) { + LOG.errorDeregisteringIssuer(issuerUrl, e.getMessage(), e); + throw new RuntimeException("Error deregistering trusted OIDC issuer: " + issuerUrl, e); + } + reloadRegistrySnapshot(); + discoveryHelper.invalidate(issuerUrl); + } + + @Override + public void refreshJwksUri(String issuerUrl) { + if (isDynamicJwks(issuerUrl)) { + discoveryHelper.invalidate(issuerUrl); + } + } + + @Override + public List<TrustedOidcIssuer> list() { + return new ArrayList<>(registrySnapshot.get().values()); + } + + /** + * Rebuilds the registry snapshot from the current DB state. + * Called on init, after register, and after deregister. + * Synchronized on this to prevent concurrent rebuilds from interleaving with mutations. + */ + private synchronized void reloadRegistrySnapshot() { + try { + final Map<String, TrustedOidcIssuer> fresh = database.selectAll().stream() + .collect(Collectors.toMap(TrustedOidcIssuer::getIssuerUrl, Function.identity())); + registrySnapshot.set(Collections.unmodifiableMap(fresh)); + } catch (Exception e) { Review Comment: The class javadoc states the snapshot is rebuilt after each write "to guarantee the in-memory state cannot diverge from persistent storage." But: ``` public synchronized void register(TrustedOidcIssuer issuer) { ... database.insert(issuer); // committed reloadRegistrySnapshot(); // if the SELECT fails here, it's caught + logged, not rethrown } ``` If the reload `SELECT` fails after a successful insert, `register()` returns normally (caller thinks it succeeded), the row is in the DB, but i`sTrusted(url) returns false` until the next successful mutation or a restart. That's a silent divergence - exactly what the javadoc promises can't happen. Consider propagating the failure (or at least having register/deregister surface it) rather than logging and returning success. ########## gateway-util-common/src/main/java/org/apache/knox/gateway/util/knoxidf/KnoxIDFConstants.java: ########## @@ -57,4 +57,17 @@ public interface KnoxIDFConstants { String FEDERATED_OP_CONFIG_NAMES = FEDERATED_OP_CONFIG_PREFIX + "names"; String TOKEN_EXCHANGE_TOPOLOGY_NAME = "token.exchange.topology.name"; + + // TrustedOidcIssuerService gateway-level params (read from GatewayConfig / gateway-site.xml) + String TRUSTED_OIDC_ISSUER_DISCOVERY_CACHE_TTL_SECS = + "gateway.trustedoidcissuer.discovery.cache.ttl.secs"; + String TRUSTED_OIDC_ISSUER_DISCOVERY_CONNECT_TIMEOUT_MS = + "gateway.trustedoidcissuer.discovery.connect.timeout.ms"; + String TRUSTED_OIDC_ISSUER_DISCOVERY_READ_TIMEOUT_MS = + "gateway.trustedoidcissuer.discovery.read.timeout.ms"; Review Comment: They belong to `GatewayConfig.java`, just like any other gateway-level config names. ########## gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/JdbcTrustedOidcIssuerService.java: ########## @@ -0,0 +1,200 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.services.knoxidf.trustedoidcissuer; + +import org.apache.hadoop.conf.Configuration; +import org.apache.knox.gateway.config.GatewayConfig; +import org.apache.knox.gateway.database.DataSourceProvider; +import org.apache.knox.gateway.i18n.messages.MessagesFactory; +import org.apache.knox.gateway.services.ServiceLifecycleException; +import org.apache.knox.gateway.services.security.AliasService; +import org.apache.knox.gateway.util.knoxidf.KnoxIDFConstants; + +import java.sql.SQLException; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import java.util.Map; +import java.util.Optional; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; +import java.util.concurrent.locks.Lock; +import java.util.concurrent.locks.ReentrantLock; +import java.util.function.Function; +import java.util.stream.Collectors; + +/** + * JDBC-backed implementation of {@link TrustedOidcIssuerService}. + * <p> + * Maintains an in-memory registry snapshot as an {@link AtomicReference} to an immutable + * {@link Map}. Reads ({@link #isTrusted}, {@link #isDynamicJwks}, {@link #list}) are + * lock-free and always see a consistent snapshot. Writes ({@link #register}, + * {@link #deregister}) are synchronized: the DB is committed first, then the snapshot is + * rebuilt from a fresh SELECT to guarantee the in-memory state cannot diverge from + * persistent storage. + * <p> + * HA note: each Knox node maintains its own snapshot. A registration on node A updates + * that node's snapshot immediately; other nodes' snapshots remain stale until restart. + */ +public class JdbcTrustedOidcIssuerService implements TrustedOidcIssuerService { + + private static final TrustedOidcIssuerServiceMessages LOG = + MessagesFactory.get(TrustedOidcIssuerServiceMessages.class); + + static final String MAX_TRUSTED_ISSUERS_CONFIG = "gateway.trustedoidcissuer.max.issuers"; + private static final int DEFAULT_MAX_TRUSTED_ISSUERS = 10_000; + + private final AtomicBoolean initialized = new AtomicBoolean(false); + private final Lock initLock = new ReentrantLock(true); + + private final AtomicReference<Map<String, TrustedOidcIssuer>> registrySnapshot = + new AtomicReference<>(Collections.emptyMap()); + + private AliasService aliasService; + private TrustedOidcIssuerDatabase database; + private OIDCDiscoveryHelper discoveryHelper; + private int maxTrustedIssuers; + + @Override + public void init(GatewayConfig config, Map<String, String> options) throws ServiceLifecycleException { + if (!initialized.get()) { + initLock.lock(); + try { + if (aliasService == null) { + throw new ServiceLifecycleException("The required AliasService reference has not been set."); + } + try { + int maxIssuers = DEFAULT_MAX_TRUSTED_ISSUERS; + long cacheTtlSecs = KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CACHE_TTL_SECS; + int connectTimeoutMs = KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CONNECT_TIMEOUT_MS; + int readTimeoutMs = KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_READ_TIMEOUT_MS; + + if (config instanceof Configuration) { + final Configuration conf = (Configuration) config; + maxIssuers = conf.getInt(MAX_TRUSTED_ISSUERS_CONFIG, DEFAULT_MAX_TRUSTED_ISSUERS); + cacheTtlSecs = conf.getLong(KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DISCOVERY_CACHE_TTL_SECS, + KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CACHE_TTL_SECS); + connectTimeoutMs = conf.getInt(KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DISCOVERY_CONNECT_TIMEOUT_MS, + KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_CONNECT_TIMEOUT_MS); + readTimeoutMs = conf.getInt(KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DISCOVERY_READ_TIMEOUT_MS, + KnoxIDFConstants.TRUSTED_OIDC_ISSUER_DEFAULT_DISCOVERY_READ_TIMEOUT_MS); + } + + this.maxTrustedIssuers = maxIssuers; + this.database = new TrustedOidcIssuerDatabase( + DataSourceProvider.getDataSource(config, aliasService), config.getDatabaseType()); + this.discoveryHelper = new OIDCDiscoveryHelper(this, cacheTtlSecs, + OIDCDiscoveryHelper.buildHttpClient(connectTimeoutMs, readTimeoutMs)); + reloadRegistrySnapshot(); + initialized.set(true); + } catch (ServiceLifecycleException e) { + throw e; + } catch (Exception e) { + throw new ServiceLifecycleException("Error initializing JdbcTrustedOidcIssuerService: " + e, e); + } + } finally { + initLock.unlock(); + } + } + } + + @Override + public void start() throws ServiceLifecycleException { + } + + @Override + public void stop() throws ServiceLifecycleException { + } + + public void setAliasService(AliasService aliasService) { + this.aliasService = aliasService; + } + + protected AliasService getAliasService() { + return aliasService; + } + + @Override + public boolean isTrusted(String issuerUrl) { + return registrySnapshot.get().containsKey(issuerUrl); + } + + @Override + public boolean isDynamicJwks(String issuerUrl) { + final TrustedOidcIssuer entry = registrySnapshot.get().get(issuerUrl); + return entry != null && entry.isDynamicJwks(); + } Review Comment: `fetchJwksUri` strips a trailing slash before building the discovery URL (`issuerUrl.replaceAll("/$", "")`), but the registry key is the raw string. So an issuer registered as `https://idp.example.com/` won't match an `iss` claim of `https://idp.example.com` (or vice-versa), and there's no normalization at `register()` either. OIDC does require exact `iss` matching, so this may be intentional - but if so, it's worth a comment, and if not, normalizing on both write and lookup would avoid a frustrating "why isn't my trusted issuer trusted" class of bug. ########## pom.xml: ########## @@ -259,6 +259,7 @@ <mina.version>2.2.8</mina.version> <netty.version>4.1.127.Final</netty.version> <nimbus-jose-jwt.version>10.9.1</nimbus-jose-jwt.version> + <oauth2-oidc-sdk.version>11.37.2</oauth2-oidc-sdk.version> Review Comment: Please move this new attribute below to honor the alphabetical order we try to keep. Thanks! ########## gateway-server/src/main/java/org/apache/knox/gateway/services/knoxidf/trustedoidcissuer/TrustedOidcIssuerDatabase.java: ########## @@ -0,0 +1,101 @@ +/* + * 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 + * <p> + * http://www.apache.org/licenses/LICENSE-2.0 + * <p> + * 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.knox.gateway.services.knoxidf.trustedoidcissuer; + +import org.apache.knox.gateway.database.DatabaseType; +import org.apache.knox.gateway.database.KnoxDatabase; + +import javax.sql.DataSource; +import java.sql.Connection; +import java.sql.PreparedStatement; +import java.sql.ResultSet; +import java.sql.SQLException; +import java.sql.Timestamp; +import java.util.ArrayList; +import java.util.List; + +/** + * JDBC helper for the {@code TRUSTED_OIDC_ISSUERS} table. + * All SQL uses {@link PreparedStatement} with {@code ?} parameters only. + * Uses {@link ResultSet#getBoolean(String)} for the {@code dynamic_jwks} column, + * which correctly maps both BOOLEAN (standard/Derby) and NUMBER(1) (Oracle) values. + */ +class TrustedOidcIssuerDatabase extends KnoxDatabase { + + static final String TABLE_NAME = "TRUSTED_OIDC_ISSUERS"; + + private static final String INSERT_SQL = + "INSERT INTO " + TABLE_NAME + " (issuer_url, dynamic_jwks, cluster_name, registered_at, registered_by) VALUES (?, ?, ?, ?, ?)"; + private static final String DELETE_SQL = + "DELETE FROM " + TABLE_NAME + " WHERE issuer_url = ?"; + private static final String SELECT_ALL_SQL = + "SELECT issuer_url, dynamic_jwks, cluster_name, registered_at, registered_by FROM " + TABLE_NAME; + private static final String COUNT_SQL = + "SELECT COUNT(*) FROM " + TABLE_NAME; + + TrustedOidcIssuerDatabase(DataSource dataSource, String dbType) throws Exception { + super(dataSource); + final DatabaseType databaseType = DatabaseType.fromString(dbType); + createTableIfNotExists(TABLE_NAME, databaseType.trustedOidcIssuersTableSql()); + } + + void insert(TrustedOidcIssuer issuer) throws SQLException { + try (Connection connection = dataSource.getConnection(); + PreparedStatement ps = connection.prepareStatement(INSERT_SQL)) { + ps.setString(1, issuer.getIssuerUrl()); + ps.setBoolean(2, issuer.isDynamicJwks()); + ps.setString(3, issuer.getClusterName()); + ps.setTimestamp(4, Timestamp.from(issuer.getRegisteredAt())); + ps.setString(5, issuer.getRegisteredBy()); + ps.executeUpdate(); + } + } + + void delete(String issuerUrl) throws SQLException { + try (Connection connection = dataSource.getConnection(); + PreparedStatement ps = connection.prepareStatement(DELETE_SQL)) { + ps.setString(1, issuerUrl); + ps.executeUpdate(); + } + } + + List<TrustedOidcIssuer> selectAll() throws SQLException { + final List<TrustedOidcIssuer> result = new ArrayList<>(); + try (Connection connection = dataSource.getConnection(); + PreparedStatement ps = connection.prepareStatement(SELECT_ALL_SQL); + ResultSet rs = ps.executeQuery()) { + while (rs.next()) { + result.add(new TrustedOidcIssuer( + rs.getString("issuer_url"), + rs.getBoolean("dynamic_jwks"), + rs.getString("cluster_name"), + rs.getTimestamp("registered_at").toInstant(), + rs.getString("registered_by") + )); + } + } + return result; + } + + int count() throws SQLException { + try (Connection connection = dataSource.getConnection(); + PreparedStatement ps = connection.prepareStatement(COUNT_SQL); + ResultSet rs = ps.executeQuery()) { + return rs.next() ? rs.getInt(1) : 0; + } + } Review Comment: This method seems to be unused -> you may want to remove it. -- 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]
