You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "adutra (via GitHub)" <gi...@apache.org> on 2026/03/20 18:27:15 UTC
[PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
adutra opened a new pull request, #15703:
URL: https://github.com/apache/iceberg/pull/15703
This is the first PR related to the "Auth Manager v2" effort. Cf. [design document].
This PR introduces the configuration layer changes (section 5.1 of the design doc). It also introduces a few class deprecations.
The most important part is the migration of configuration properties. This is handled by `ConfigMigrator`. Its logic may seem complex, that's because we need to handle some edge cases, as detailed in the design doc, section 5.6. The most complex edge case is when a session context initiates a token exchange flow, using the catalog session's parent token as the actor token (a.k.a. the "Trino use case"): this is handled in v2 by introducing a special sentinel value that will be recognized and processed at runtime (this will come in a later PR though).
Merging this PR has only one visible consequence: deprecation warnings will be printed when loading the legacy (v1) manager, and when any deprecated v1 configuration is used.
[design document]:https://docs.google.com/document/d/1Hxw-t8Maa7wZFmrlSujm7LRawKsFP3Q31tET_3aRnQU/edit
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170390428
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
Review Comment:
OK, I understand the concern. I will move everything to a single package and use package-private visibility as the default.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170291383
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Config.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.util.Map;
+import org.apache.iceberg.rest.auth.oauth2.config.BasicConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ConfigValidator;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenRefreshConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenRefreshConfig;
+import org.immutables.value.Value;
+
+/** The configuration for the OAuth2 AuthManager. */
+@Value.Immutable
+public interface OAuth2Config {
+
+ String PREFIX = "rest.auth.oauth2.";
+
+ /**
+ * The basic configuration, including token endpoint, grant type, client id and client secret.
+ * Required.
+ */
+ BasicConfig basicConfig();
Review Comment:
The composition may seem superfluous now, but it will start making sense later.
For example, when we'll integrate interactive grants, and maybe JWT client auth, we'd have more methods:
```java
BasicConfig basicConfig();
@Value.Default
default TokenRefreshConfig tokenRefreshConfig() { ... }
@Value.Default
default TokenExchangeConfig tokenExchangeConfig() { ... }
@Value.Default
default AuthorizationCodeConfig authorizationCodeConfig() { ... }
@Value.Default
default DeviceCodeConfig deviceCodeConfig() { ... }
@Value.Default
default JWTClientAuthConfig jwtClientAuthConfig() { ... }
```
etc.
OAuth2 is an evolving set of standards, so we should be prepared for that. Isolating each OAuth2 feature in its own config class helps keeping the configuration tidy.
As for `BasicConfig`: it just felt to me more natural to keep `OAuth2Config` as the umbrella configuration class, and delegate actual properties to config-section classes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3269848091
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Config.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.util.Map;
+import org.apache.iceberg.rest.auth.oauth2.config.BasicConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ConfigValidator;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenRefreshConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenRefreshConfig;
+import org.immutables.value.Value;
+
+/** The configuration for the OAuth2 AuthManager. */
+@Value.Immutable
+public interface OAuth2Config {
+
+ String PREFIX = "rest.auth.oauth2.";
+
+ /**
+ * The basic configuration, including token endpoint, grant type, client id and client secret.
+ * Required.
+ */
+ BasicConfig basicConfig();
+
+ /** The token refresh configuration. Optional. */
+ @Value.Default
+ default TokenRefreshConfig tokenRefreshConfig() {
Review Comment:
I rephrased a few method javadocs, lmk if that's better.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3632104403
##########
build.gradle:
##########
@@ -403,6 +403,13 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ // OAuth2
+ implementation(libs.nimbus.oauth2.oidc.sdk) {
Review Comment:
I'm not sure how to address the dependency issues, but this pulls in dependencies for the runtime bundles without actually using them yet. Is there an easy way to exclude these until we have an implementation that would actually be used.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3632410270
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/LegacyConfigAdaptor.java:
##########
@@ -0,0 +1,459 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.id.Audience;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.TokenTypeURI;
+import com.nimbusds.oauth2.sdk.token.TypelessAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.ResourcePaths;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.util.PropertyUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A component for adapting from legacy OAuth2 properties (from {@link OAuth2Properties}) to the new
+ * OAuth2 properties (as declared in {@link OAuth2Config}).
+ */
+@SuppressWarnings("deprecation")
+final class LegacyConfigAdaptor {
+
+ /**
+ * The default client ID to use when no client ID is provided in the legacy {@link
+ * OAuth2Properties#CREDENTIAL} property.
+ */
+ public static final ClientID DEFAULT_CLIENT_ID = new ClientID("iceberg");
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(LegacyConfigAdaptor.class);
+
+ private static final Splitter CREDENTIAL_SPLITTER = Splitter.on(":").limit(2).trimResults();
+
+ private static final Set<String> TABLE_CONFIG_ALLOW_LIST =
+ Set.of(
+ BasicConfig.TOKEN,
+ TokenExchangeConfig.SUBJECT_TOKEN,
+ TokenExchangeConfig.SUBJECT_TOKEN_TYPE,
+ TokenExchangeConfig.ACTOR_TOKEN,
+ TokenExchangeConfig.ACTOR_TOKEN_TYPE);
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_LEGACY_OPTION =
+ "Detected legacy OAuth2 property '{}', please use option{} {} instead.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_NO_CLIENT_ID =
+ "The legacy OAuth2 property 'credential' was provided, but it did not contain a client ID; assuming '{}'.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_MISSING_TOKEN_ENDPOINT =
+ "The OAuth2 configuration does not specify a token endpoint nor an issuer URL: "
+ + "the token endpoint URL will default to {}. "
+ + "This automatic fallback will be removed in a future Iceberg release. "
+ + "Please configure OAuth2 endpoints using the following properties: '{}' or '{}'. "
+ + "This warning will disappear if OAuth2 endpoints are properly configured. "
+ + "See https://github.com/apache/iceberg/issues/10537";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_RELATIVE_TOKEN_ENDPOINT =
+ "The OAuth2 token endpoint URL is a relative URL. "
+ + "It will be resolved against the catalog URI, resulting in the absolute URL: '{}'. "
+ + "This automatic fallback will be removed in a future Iceberg release. "
+ + "Please configure OAuth2 endpoints using absolute URLs.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_MERGED_CONTEXTUAL_CONFIG =
+ "The OAuth2 configuration property '{}' was not found in the context session, "
+ + "and will be inherited from the parent session. "
+ + "This automatic fallback will be removed in a future Iceberg release.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_VENDED_TOKEN =
+ "The OAuth2 configuration property '{}' was found in the table configuration "
+ + "and indicates that the catalog server vended an OAuth2 token. "
+ + "Vended OAuth2 tokens will be disallowed in a future Iceberg release.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_TABLE_CONFIG_NOT_ALLOWED =
+ "The OAuth2 configuration property '{}' is not allowed to be vended by catalog servers.";
+
+ private final BiConsumer<String, String[]> logConsumer;
+
+ LegacyConfigAdaptor() {
+ this(LOGGER);
+ }
+
+ LegacyConfigAdaptor(Logger logger) {
+ this(logger::warn);
+ }
+
+ @VisibleForTesting
+ LegacyConfigAdaptor(BiConsumer<String, String[]> logConsumer) {
+ this.logConsumer = logConsumer;
+ }
+
+ /**
+ * Migrates catalog-level properties.
+ *
+ * <p>Legacy Iceberg OAuth2 properties are migrated, and warnings are logged for each detected
+ * legacy property.
+ *
+ * <p>The migration will further check the token endpoint for the following legacy situations:
+ *
+ * <ol>
+ * <li>If no token endpoint is provided, a default one will be added to the migrated properties;
+ * its value is the catalog URI + {@link ResourcePaths#tokens()} – that is, it will point to
+ * the (deprecated) REST Catalog token endpoint.
+ * <li>If a token endpoint is provided, but is a relative path, it will be resolved against the
+ * catalog URI.
+ * </ol>
+ *
+ * In both cases, a warning will be logged.
+ *
+ * @param properties The properties to migrate
+ * @param catalogUri The catalog URI, for extended token endpoint checks
+ */
+ public OAuth2Config migrateCatalogConfig(Map<String, String> properties, String catalogUri) {
+ Map<String, String> migrated = migrateProperties(properties);
+ handleTokenEndpoint(migrated, catalogUri);
+ return OAuth2Config.from(migrated);
+ }
+
+ /**
+ * Migrates session context properties.
+ *
+ * <p>Legacy Iceberg OAuth2 properties are migrated, and warnings are logged for each detected
+ * legacy property.
+ *
+ * <p>See {@link #migrateCatalogConfig(Map, String)} for details on token endpoint checks.
+ *
+ * <p>Contextual configs inherit some properties from their parent config. This is legacy
+ * behavior, and will be removed after 1.13; a warning will be logged when this happens.
+ *
+ * @param parent The parent config
+ * @param properties The properties to migrate
+ * @param catalogUri The catalog URI, for extended token endpoint checks
+ */
+ @SuppressWarnings("CyclomaticComplexity")
+ public OAuth2Config migrateContextualConfig(
Review Comment:
```suggestion
public OAuth2Config fromContextualConfig(
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639150146
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ Duration MIN_TIMEOUT = Duration.parse("PT30S");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Optional. Either this property or {@link #TOKEN_ENDPOINT} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. If not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ *
+ * <p>Optional. Either this property or {@link #ISSUER_URL} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server.
+ *
+ * <p>Optional. Required unless a {@linkplain #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Optional. Rquired when the
+ * client is private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server.
+ *
+ * <p>Optional. If absent, no scopes are sent.
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
+
+ /**
+ * Extra parameters to include in each request to the token endpoint. This is useful for custom
+ * parameters that are not covered by the standard OAuth2 specification. Defaults to an empty map.
+ *
+ * <p>This is a prefix property, and multiple values can be set, each with a different key and
+ * value. The values must NOT be URL-encoded. Example:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.custom_param1=custom_value1
+ * rest.auth.oauth2.extra-params.custom_param2=custom_value2
+ * }</pre>
+ *
+ * For example, Auth0 requires the {@code audience} parameter to be set to the API identifier.
+ * This can be done by setting the following configuration:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.audience=https://iceberg-rest-catalog/api
+ * }</pre>
+ */
+ Map<String, String> extraRequestParameters();
+
+ /**
+ * The token acquisition timeout. Defaults to {@link #DEFAULT_TIMEOUT}. The default timeout is
+ * intentionally large, in order to accommodate for long-running flows that require human
+ * intervention (e.g. Authorization Code flow).
+ *
+ * <p>Must be a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601
+ * duration</a>.
+ */
+ @Value.Default
+ default Duration tokenAcquisitionTimeout() {
+ return DEFAULT_TIMEOUT;
+ }
+
+ /**
+ * The session cache timeout. Cached sessions will become eligible for eviction after this
+ * duration of inactivity. Defaults to 1 hour. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ *
+ * <p>This value is used for housekeeping; it does not mean that cached sessions will stop working
+ * after this time, but that the session cache will evict the session after this time of
+ * inactivity. If the context is used again, a new session will be created and cached.
+ *
+ * <p>This property can only be specified at catalog session level. It is ignored if present in
+ * other levels.
+ */
+ @Value.Default
+ default Duration sessionCacheTimeout() {
+ return DEFAULT_SESSION_CACHE_TIMEOUT;
+ }
+
+ /**
+ * The minimum timeout for token acquisition.
+ *
+ * <p>This option is not exposed as a public configuration property, and is intended for testing
+ * purposes only.
+ */
+ @Value.Default
+ default Duration minTokenAcquisitionTimeout() {
+ return MIN_TIMEOUT;
+ }
+
+ @Value.Check
+ default void validate() {
+ ConfigValidator validator = new ConfigValidator();
+
+ if (token().isEmpty()) {
+
+ validator.check(
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.contains(grantType()),
+ GRANT_TYPE,
+ "grant type must be one of: %s",
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.stream()
+ .map(GrantType::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.contains(clientAuthenticationMethod()),
+ CLIENT_AUTH,
+ "client authentication method must be one of: %s",
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.stream()
+ .map(ClientAuthenticationMethod::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ issuerUrl().isPresent() || tokenEndpoint().isPresent(),
+ List.of(ISSUER_URL, TOKEN_ENDPOINT),
+ "either issuer URL or token endpoint must be set");
+
+ validator.check(clientId().isPresent(), CLIENT_ID, "client ID must not be empty");
+
+ if (ConfigUtil.requiresClientSecret(clientAuthenticationMethod())) {
+ validator.check(
+ clientSecret().isPresent(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be empty when client authentication is '%s'",
+ clientAuthenticationMethod().getValue());
+ } else if (clientAuthenticationMethod().equals(ClientAuthenticationMethod.NONE)) {
+ validator.check(
+ clientSecret().isEmpty(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be set when client authentication is '%s'",
+ ClientAuthenticationMethod.NONE.getValue());
+ validator.check(
+ !grantType().equals(DEFAULT_GRANT_TYPE),
+ List.of(CLIENT_AUTH, GRANT_TYPE),
+ "grant type must not be '%s' when client authentication is '%s'",
Review Comment:
```suggestion
"Grant type must not be '%s' when client authentication is '%s'",
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4977910373
@danielcweeks can I get another review please? Thanks 🙏
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3391965690
##########
gradle/libs.versions.toml:
##########
@@ -169,6 +171,8 @@ lz4Java = { module = "at.yawk.lz4:lz4-java", version.ref = "lz4Java" }
microprofile-openapi-api = { module = "org.eclipse.microprofile.openapi:microprofile-openapi-api", version.ref = "microprofile-openapi-api" }
nessie-client = { module = "org.projectnessie.nessie:nessie-client", version.ref = "nessie" }
netty-buffer = { module = "io.netty:netty-buffer", version.ref = "netty-buffer" }
+nimbus-oauth2-oidc-sdk = { module = "com.nimbusds:oauth2-oidc-sdk", version.ref = "nimbus-oauth2" }
+nimbus-jose-jwt = { module = "com.nimbusds:nimbus-jose-jwt", version.ref = "nimbus-jose-jwt" }
Review Comment:
We're going to need more eyes on the library additions (@jbonofre @rdblue)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3693262313
##########
build.gradle:
##########
@@ -403,6 +403,14 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ // OAuth2
+ // TODO switch from compileOnly to implementation when oauth2 v2 is complete
Review Comment:
Strong preference to not in include `TODO`s in the codebase. Even if this is a work-in-progress, it's best to not use the codebase as a tracker.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163165149
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Manager.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import java.util.Map;
+import org.apache.iceberg.catalog.SessionCatalog.SessionContext;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthManager;
+import org.apache.iceberg.rest.auth.AuthSession;
+
+public class OAuth2Manager implements AuthManager {
Review Comment:
I'm not convinced it makes sense to introduce this now if there's no implementation.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170450324
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
+
+ /**
+ * Default access token lifespan; if the OAuth2 server returns an access token without specifying
+ * its expiration time, this value will be used.
+ *
+ * <p>Optional, defaults to {@link #DEFAULT_ACCESS_TOKEN_LIFESPAN}. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration accessTokenLifespan() {
+ return DEFAULT_ACCESS_TOKEN_LIFESPAN;
+ }
+
+ /**
+ * Refresh safety margin to use; a new token will be fetched when the current token's remaining
+ * lifespan is less than this value. Optional, defaults to {@link #DEFAULT_SAFETY_MARGIN}. Must be
+ * a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration safetyMargin() {
Review Comment:
I took note of the jitter requirement. Will include it in the actual fetch code (that will come later).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163126741
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
+
+ /**
+ * Default access token lifespan; if the OAuth2 server returns an access token without specifying
+ * its expiration time, this value will be used.
+ *
+ * <p>Optional, defaults to {@link #DEFAULT_ACCESS_TOKEN_LIFESPAN}. Must be a valid <a
Review Comment:
This doesn't appear to be optional.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3162927560
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/ConfigMigrator.java:
##########
@@ -0,0 +1,463 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.id.Audience;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.TokenTypeURI;
+import com.nimbusds.oauth2.sdk.token.TypelessAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.ResourcePaths;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.rest.auth.oauth2.ImmutableOAuth2Config;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.apache.iceberg.util.PropertyUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A component for migrating from legacy OAuth2 properties (from {@link OAuth2Properties}) to the
+ * new OAuth2 properties (as declared in {@link OAuth2Config}).
+ */
+@SuppressWarnings("deprecation")
+public final class ConfigMigrator {
Review Comment:
Minor point here, but this isn't Migrating anything since it's not updating the source based on what I see. This is more of a `ConfigAdaptor` (or maybe `LegacyConfigAdaptor`?).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3711951861
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.ofMinutes(5);
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.ofHours(1);
+
+ Duration MIN_TIMEOUT = Duration.ofSeconds(30);
Review Comment:
> Does Nimbus not have reasonable defaults?
Nimbus does not ship with an OAuth client. It only has the building blocks to create such a client.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3704937155
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigUtil.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.OptionalInt;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import javax.annotation.Nullable;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+
+/**
+ * Helper class for parsing configuration options. It also exposes useful constants for config
+ * validation.
+ */
+final class ConfigUtil {
+
+ public static final List<GrantType> SUPPORTED_GRANT_TYPES =
+ List.of(GrantType.CLIENT_CREDENTIALS, GrantType.TOKEN_EXCHANGE, GrantType.REFRESH_TOKEN);
+
+ public static final List<GrantType> SUPPORTED_INITIAL_GRANT_TYPES =
+ List.of(GrantType.CLIENT_CREDENTIALS, GrantType.TOKEN_EXCHANGE);
+
+ public static final List<ClientAuthenticationMethod> SUPPORTED_CLIENT_AUTH_METHODS =
+ List.of(
+ ClientAuthenticationMethod.NONE,
+ ClientAuthenticationMethod.CLIENT_SECRET_BASIC,
+ ClientAuthenticationMethod.CLIENT_SECRET_POST);
+
+ /**
+ * A sentinel value used to indicate that the parent session's token should be used. This is
+ * useful for the token exchange flow.
+ */
+ public static final String PARENT_TOKEN = "::parent::";
Review Comment:
For the record, you could see how it would be used here:
https://github.com/apache/iceberg/pull/15704/changes#diff-51fed95e2595a7597626d432abf3fe6cde63de001185eca1f97192652da36bb5R106-R117
Let me remove this for now. We can decide later if we really need to support this edge case.
The more it goes the more I feel that people are OK with us dropping support for all the weird edge cases in v2 anyways.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3660046711
##########
build.gradle:
##########
@@ -403,6 +403,13 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ // OAuth2
+ implementation(libs.nimbus.oauth2.oidc.sdk) {
Review Comment:
We can use `compileOnly` for now and switch to `implementation` later.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3693201155
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigValidator.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.google.errorprone.annotations.FormatMethod;
+import com.google.errorprone.annotations.FormatString;
+import java.net.URI;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.immutables.value.Value;
+
+final class ConfigValidator {
Review Comment:
The project has a standardized way of doing this using either Precondition checks or ValidationException (with the latter being used for more config validation). This is not following that model and introduces more complexity for very little added value.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170430764
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
Review Comment:
That is true, but the documentation tool I wrote will leverage the `Duration.parse()` method to extract the ISO-8601 litteral and include it in the documentation.
For example, the `accessTokenLifespan()` method will render as :
```markdown
### `rest.auth.oauth2.token-refresh.access-token-lifespan`
Default access token lifespan; if the OAuth2 server returns an access token without specifying its expiration time, this value will be used.
Optional, defaults to `PT1H`. Must be a valid [ISO-8601 duration](https://en.wikipedia.org/wiki/ISO_8601#Durations).
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3269849649
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
+
+ /**
+ * Default access token lifespan; if the OAuth2 server returns an access token without specifying
+ * its expiration time, this value will be used.
+ *
+ * <p>Optional, defaults to {@link #DEFAULT_ACCESS_TOKEN_LIFESPAN}. Must be a valid <a
Review Comment:
Rephrased.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3693282498
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.ofMinutes(5);
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.ofHours(1);
+
+ Duration MIN_TIMEOUT = Duration.ofSeconds(30);
Review Comment:
Is there really a practical reason to expose configurable timeouts like this? We're introducing more config complexity and the library defaults should be adequate for OAuth. Does Nimbus not have reasonable defaults?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3660300049
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Manager.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import java.util.Map;
+import org.apache.iceberg.catalog.SessionCatalog.SessionContext;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthManager;
+import org.apache.iceberg.rest.auth.AuthSession;
+
+public class OAuth2Manager implements AuthManager {
Review Comment:
In fact, given the direction we are taking, and since @rdblue also requested that we revert the deprecations introduced in this PR, I think we should re-center this PR around config classes exclusively.
Let's remove this `OAuth2Manager` for now, revert the deprecations and revert the changes to the `AuthManagers` class. This will considerably reduce the PR scope.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163138440
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
+
+ /**
+ * Default access token lifespan; if the OAuth2 server returns an access token without specifying
+ * its expiration time, this value will be used.
+ *
+ * <p>Optional, defaults to {@link #DEFAULT_ACCESS_TOKEN_LIFESPAN}. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration accessTokenLifespan() {
+ return DEFAULT_ACCESS_TOKEN_LIFESPAN;
+ }
+
+ /**
+ * Refresh safety margin to use; a new token will be fetched when the current token's remaining
+ * lifespan is less than this value. Optional, defaults to {@link #DEFAULT_SAFETY_MARGIN}. Must be
+ * a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration safetyMargin() {
+ return DEFAULT_SAFETY_MARGIN;
+ }
+
+ @Value.Check
+ default void validate() {
+ if (enabled()) {
+ ConfigValidator validator = new ConfigValidator();
+ validator.check(
+ accessTokenLifespan().compareTo(MIN_ACCESS_TOKEN_LIFESPAN) >= 0,
+ ACCESS_TOKEN_LIFESPAN,
+ "access token lifespan must be greater than or equal to %s",
+ MIN_ACCESS_TOKEN_LIFESPAN);
+ validator.check(
+ safetyMargin().compareTo(MIN_SAFETY_MARGIN) >= 0,
+ SAFETY_MARGIN,
+ "refresh safety margin must be greater than or equal to %s",
+ MIN_SAFETY_MARGIN);
+ validator.check(
+ safetyMargin().compareTo(accessTokenLifespan()) < 0,
+ List.of(SAFETY_MARGIN, ACCESS_TOKEN_LIFESPAN),
+ "refresh safety margin must be less than the access token lifespan");
+ validator.validate();
+ }
+ }
+
+ static ImmutableTokenRefreshConfig.Builder parse(Map<String, String> properties) {
Review Comment:
Same here, prefer `from` as opposed to `parse`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3153118459
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
Review Comment:
See my answer to your other comment.
In an ideal world, there should be no reason to pass an access token "as is" to the catalog.
You would instead either:
1. Let the catalog fetch the token for you; or
2. Exchange an external token for a new token, using the token exchange grant.
Passing a raw access token effectively beats the purpose of using OAuth2, and in most cases, the token cannot be refreshed.
This option only exists to "bridge the gap" until everyone is able to use OAuth2 optimally.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639147509
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ Duration MIN_TIMEOUT = Duration.parse("PT30S");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Optional. Either this property or {@link #TOKEN_ENDPOINT} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. If not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ *
+ * <p>Optional. Either this property or {@link #ISSUER_URL} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server.
+ *
+ * <p>Optional. Required unless a {@linkplain #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Optional. Rquired when the
+ * client is private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server.
+ *
+ * <p>Optional. If absent, no scopes are sent.
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
+
+ /**
+ * Extra parameters to include in each request to the token endpoint. This is useful for custom
+ * parameters that are not covered by the standard OAuth2 specification. Defaults to an empty map.
+ *
+ * <p>This is a prefix property, and multiple values can be set, each with a different key and
+ * value. The values must NOT be URL-encoded. Example:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.custom_param1=custom_value1
+ * rest.auth.oauth2.extra-params.custom_param2=custom_value2
+ * }</pre>
+ *
+ * For example, Auth0 requires the {@code audience} parameter to be set to the API identifier.
+ * This can be done by setting the following configuration:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.audience=https://iceberg-rest-catalog/api
+ * }</pre>
+ */
+ Map<String, String> extraRequestParameters();
+
+ /**
+ * The token acquisition timeout. Defaults to {@link #DEFAULT_TIMEOUT}. The default timeout is
+ * intentionally large, in order to accommodate for long-running flows that require human
+ * intervention (e.g. Authorization Code flow).
+ *
+ * <p>Must be a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601
+ * duration</a>.
+ */
+ @Value.Default
+ default Duration tokenAcquisitionTimeout() {
+ return DEFAULT_TIMEOUT;
+ }
+
+ /**
+ * The session cache timeout. Cached sessions will become eligible for eviction after this
+ * duration of inactivity. Defaults to 1 hour. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ *
+ * <p>This value is used for housekeeping; it does not mean that cached sessions will stop working
+ * after this time, but that the session cache will evict the session after this time of
+ * inactivity. If the context is used again, a new session will be created and cached.
+ *
+ * <p>This property can only be specified at catalog session level. It is ignored if present in
+ * other levels.
+ */
+ @Value.Default
+ default Duration sessionCacheTimeout() {
+ return DEFAULT_SESSION_CACHE_TIMEOUT;
+ }
+
+ /**
+ * The minimum timeout for token acquisition.
+ *
+ * <p>This option is not exposed as a public configuration property, and is intended for testing
+ * purposes only.
+ */
+ @Value.Default
+ default Duration minTokenAcquisitionTimeout() {
+ return MIN_TIMEOUT;
+ }
+
+ @Value.Check
+ default void validate() {
+ ConfigValidator validator = new ConfigValidator();
+
+ if (token().isEmpty()) {
+
+ validator.check(
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.contains(grantType()),
+ GRANT_TYPE,
+ "grant type must be one of: %s",
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.stream()
+ .map(GrantType::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.contains(clientAuthenticationMethod()),
+ CLIENT_AUTH,
+ "client authentication method must be one of: %s",
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.stream()
+ .map(ClientAuthenticationMethod::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ issuerUrl().isPresent() || tokenEndpoint().isPresent(),
+ List.of(ISSUER_URL, TOKEN_ENDPOINT),
+ "either issuer URL or token endpoint must be set");
+
+ validator.check(clientId().isPresent(), CLIENT_ID, "client ID must not be empty");
+
+ if (ConfigUtil.requiresClientSecret(clientAuthenticationMethod())) {
+ validator.check(
+ clientSecret().isPresent(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be empty when client authentication is '%s'",
+ clientAuthenticationMethod().getValue());
+ } else if (clientAuthenticationMethod().equals(ClientAuthenticationMethod.NONE)) {
+ validator.check(
+ clientSecret().isEmpty(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be set when client authentication is '%s'",
+ ClientAuthenticationMethod.NONE.getValue());
+ validator.check(
+ !grantType().equals(DEFAULT_GRANT_TYPE),
+ List.of(CLIENT_AUTH, GRANT_TYPE),
+ "grant type must not be '%s' when client authentication is '%s'",
+ DEFAULT_GRANT_TYPE.getValue(),
+ ClientAuthenticationMethod.NONE.getValue());
+ }
+ }
+
+ if (issuerUrl().isPresent()) {
+ validator.checkEndpoint(issuerUrl().get(), ISSUER_URL, "Issuer URL");
+ }
+
+ if (tokenEndpoint().isPresent()) {
+ validator.checkEndpoint(tokenEndpoint().get(), TOKEN_ENDPOINT, "Token endpoint");
+ }
+
+ validator.check(
+ tokenAcquisitionTimeout().compareTo(minTokenAcquisitionTimeout()) >= 0,
+ TIMEOUT,
+ "timeout must be greater than or equal to %s",
Review Comment:
```suggestion
"Timeout must be greater than or equal to %s",
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639138889
##########
core/src/main/java/org/apache/iceberg/rest/auth/AuthProperties.java:
##########
@@ -34,8 +34,12 @@ private AuthProperties() {}
"org.apache.iceberg.rest.auth.NoopAuthManager";
public static final String AUTH_MANAGER_IMPL_BASIC =
"org.apache.iceberg.rest.auth.BasicAuthManager";
- public static final String AUTH_MANAGER_IMPL_OAUTH2 =
+ static final String AUTH_MANAGER_IMPL_OAUTH2_LEGACY =
"org.apache.iceberg.rest.auth.OAuth2Manager";
+ static final String AUTH_MANAGER_IMPL_OAUTH2_NEW =
+ "org.apache.iceberg.rest.auth.oauth2.OAuth2Manager";
+ public static final String AUTH_MANAGER_IMPL_OAUTH2 =
+ AUTH_MANAGER_IMPL_OAUTH2_LEGACY; // TODO switch to new manager
Review Comment:
Strong preference to not include `TODO`, we already call it out `LEGACY/NEW`, we should drop this comment.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4336945040
This PR has been updated to incorporate the changes from #16081.
Since Nimbus is now a dependency of `iceberg-core`, the following dependencies are now visible in all runtime jars:
```
com.nimbusds:content-type:2.3
com.nimbusds:lang-tag:1.7
com.nimbusds:nimbus-jose-jwt:10.9
com.nimbusds:oauth2-oidc-sdk:11.37
net.minidev:json-smart:2.5.2
```
All `runtime-deps.txt` and `LICENSE` files were update. No `NOTICE` file requires updates, because none of the above dependencies ships with a `NOTICE` file.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170355955
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
+ * https://<keycloak-server>/realms/<realm-name>}.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #TOKEN_ENDPOINT} must be set.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. For Keycloak, this is typically {@code
+ * https://<keycloak-server>/realms/<realm-name>/protocol/openid-connect/token}.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #ISSUER_URL} must be set. In case it is not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Optional, defaults to {@link #DEFAULT_GRANT_TYPE}.
Review Comment:
While the config classes are not considered public API, their javadocs will be used later to create a documentation page in markdown format (I wrote a small tool for that).
For example, this method's javadocs will render as below:
```markdown
### `rest.auth.oauth2.grant-type`
The grant type to use when authenticating against the OAuth2 server. Valid values are:
- `client_credentials`
- `password`
- `authorization_code`
- `urn:ietf:params:oauth:grant-type:device_code`
- `urn:ietf:params:oauth:grant-type:token-exchange`
Optional, defaults to `client_credentials`.
```
So "optional" here is a way to tell the users that if they don't specify the grant type, it will default to `client_credentials`.
You will notice that all javadocs of config classes were written with this documentation goal in mind.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170436579
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
+
+ /**
+ * Default access token lifespan; if the OAuth2 server returns an access token without specifying
+ * its expiration time, this value will be used.
+ *
+ * <p>Optional, defaults to {@link #DEFAULT_ACCESS_TOKEN_LIFESPAN}. Must be a valid <a
Review Comment:
As I explained [in my other comment](https://github.com/apache/iceberg/pull/15703#discussion_r3170355955), the word "optional" here is intended for end users.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3126687830
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
Review Comment:
Taking the example of long-running Spark jobs against an Iceberg REST catalog backed by Azure Entra ID.
These jobs require per-user identity (user-delegated access), not a shared service account (client_credentials flow).
As a first step, we can have the users handle the Authorization Code + PKCE flow externally (via a CLI wrapper) and obtain an access token + refresh token pair before submitting the job.
Currently there's no way to pass that refresh token to Iceberg. The v1 OAuth2Util.refreshToken() only supports token exchange or client credentials re-auth. In the v2 config, TokenRefreshConfig controls refresh behavior but has no property to seed an external refresh token.
The flow would be:
1. External tooling handles interactive auth (PKCE, device code, etc.) before job submission
2. User configures rest.auth.oauth2.token and rest.auth.oauth2.refresh-token
3. Iceberg uses the access token until near-expiry, then calls the token endpoint with grant_type=refresh_token
4. Subsequent refreshes use the rotating refresh token from each response
Should we consider adding a config property like `rest.auth.oauth2.refresh-token` that provides an initial refresh token to RefreshTokenFlow, enabling `grant_type=refresh_token` without Iceberg needing to implement the interactive PKCE flow (which can be considered in a future update)?
We can support it in later PR as well after this, but want to understand if it already works , or if it makes sense to try supporting this case ?
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
Review Comment:
for users who obtain tokens externally (PKCE, device code), this is the only entry point. As per my below comment, if refresh-token is supported, this option may become viable as well ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3705046635
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigUtil.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.OptionalInt;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import javax.annotation.Nullable;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+
+/**
+ * Helper class for parsing configuration options. It also exposes useful constants for config
+ * validation.
+ */
+final class ConfigUtil {
+
+ public static final List<GrantType> SUPPORTED_GRANT_TYPES =
+ List.of(GrantType.CLIENT_CREDENTIALS, GrantType.TOKEN_EXCHANGE, GrantType.REFRESH_TOKEN);
+
+ public static final List<GrantType> SUPPORTED_INITIAL_GRANT_TYPES =
+ List.of(GrantType.CLIENT_CREDENTIALS, GrantType.TOKEN_EXCHANGE);
+
+ public static final List<ClientAuthenticationMethod> SUPPORTED_CLIENT_AUTH_METHODS =
+ List.of(
+ ClientAuthenticationMethod.NONE,
+ ClientAuthenticationMethod.CLIENT_SECRET_BASIC,
+ ClientAuthenticationMethod.CLIENT_SECRET_POST);
+
+ /**
+ * A sentinel value used to indicate that the parent session's token should be used. This is
+ * useful for the token exchange flow.
+ */
+ public static final String PARENT_TOKEN = "::parent::";
Review Comment:
FYI, Trino is moving away from vended token exchange. Since that was the only identified usage of this feature, I think it's fine to just drop 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-5411807361
FYI I rebased and squashed as the commit history was becoming quite large.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3693309499
##########
core/src/test/java/org/apache/iceberg/rest/auth/oauth2/TestOAuth2Config.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import java.net.URI;
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
+
+class TestOAuth2Config {
+
+ @Test
+ void testFrom() {
+ Map<String, String> properties =
+ ImmutableMap.<String, String>builder()
+ .put(BasicConfig.TOKEN_ENDPOINT, "https://example.com/token")
+ .put(BasicConfig.CLIENT_ID, "Client")
+ .put(BasicConfig.CLIENT_SECRET, "w00t")
+ .put(BasicConfig.GRANT_TYPE, GrantType.TOKEN_EXCHANGE.getValue())
+ .put(BasicConfig.SCOPE, "test")
+ .put(BasicConfig.EXTRA_PARAMS + ".key1", "value1")
+ .put(BasicConfig.EXTRA_PARAMS + ".key2", "value2")
+ .put(TokenRefreshConfig.PREFETCH, "PT20S")
+ .put(TokenExchangeConfig.SUBJECT_TOKEN, "subject-token")
+ .build();
+ OAuth2Config config = OAuth2Config.from(properties);
+ assertThat(config).isNotNull();
+ assertThat(config.basicConfig()).isNotNull();
+ assertThat(config.basicConfig().tokenEndpoint())
+ .contains(URI.create("https://example.com/token"));
+ assertThat(config.basicConfig().grantType()).isEqualTo(GrantType.TOKEN_EXCHANGE);
+ assertThat(config.basicConfig().clientId()).contains(new ClientID("Client"));
+ assertThat(config.basicConfig().clientSecret()).contains(new Secret("w00t"));
+ assertThat(config.basicConfig().scope()).contains(new Scope("test"));
+ assertThat(config.basicConfig().extraRequestParameters())
+ .isEqualTo(Map.of("key1", "value1", "key2", "value2"));
+ assertThat(config.tokenRefreshConfig()).isNotNull();
+ assertThat(config.tokenRefreshConfig().prefetch()).isEqualTo(Duration.ofSeconds(20));
+ assertThat(config.tokenExchangeConfig()).isNotNull();
+ assertThat(config.tokenExchangeConfig().subjectTokenString()).contains("subject-token");
+ }
+
+ @Test
+ void testValidate() {
Review Comment:
Similar naming: prefer something more descriptive like `invalidConfigFromProperties`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3693305440
##########
core/src/test/java/org/apache/iceberg/rest/auth/oauth2/TestOAuth2Config.java:
##########
@@ -0,0 +1,84 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatThrownBy;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import java.net.URI;
+import java.time.Duration;
+import java.util.Map;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.junit.jupiter.api.Test;
+
+class TestOAuth2Config {
+
+ @Test
+ void testFrom() {
Review Comment:
minor: we've been moving away from the `test` prefix, but I'm also thinking this is probably not a very descriptive test name. Something more like `configFromProperties` is closer to what it is doing.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3660310941
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -40,6 +40,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * @deprecated will be removed in 1.14.0, use {@link
+ * org.apache.iceberg.rest.auth.oauth2.OAuth2Manager} instead.
+ */
+@Deprecated
Review Comment:
You are right, I shouldn't have mixed deprecations and config classes in the same PR. The revised PR now contains only changes related to configuration and nothing else.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub) via issues" <is...@iceberg.apache.org>.
adutra commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-6583853861
Heads up: I'm going to close this PR in order to propose a much simpler, downscoped version of the OAuth2 manager v2.
Apologies for those who reviewed already: your effort wasn't lost though, as I incorporated all the feedback received here in the new PR. But I am aware that having the ground move under a review is frustrating, and I should have reset the scope earlier. My bad.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3403455321
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
Review Comment:
In the legacy manager there are no timeouts defined.
In v2, this timeout will determine how long we wait for tokens, regardless of the grant:
https://github.com/adutra/iceberg/blob/3316ebfe90bc3982c9bf9583a4a0dd253c063e5f/core/src/main/java/org/apache/iceberg/rest/auth/oauth2/client/OAuth2Client.java#L265-L268
In that regard, 5 minutes may even sound too much for a simple grant like `client_credentials` that either completes or fails almost immediately.
But the timeout is rather intended to protect complex, interactive grants, e.g. `authorization_code`: 5 minutes will be the maximum time the client will block waiting for the user to complete the login + consent workflow, before giving up.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3269844242
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
+ * https://<keycloak-server>/realms/<realm-name>}.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #TOKEN_ENDPOINT} must be set.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. For Keycloak, this is typically {@code
+ * https://<keycloak-server>/realms/<realm-name>/protocol/openid-connect/token}.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #ISSUER_URL} must be set. In case it is not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Optional, defaults to {@link #DEFAULT_GRANT_TYPE}.
Review Comment:
FYI I revisited some of the method javadocs and removed the word "optional" when it was misleading. Let me know if that's better.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3269841331
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
+ * https://<keycloak-server>/realms/<realm-name>}.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #TOKEN_ENDPOINT} must be set.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. For Keycloak, this is typically {@code
+ * https://<keycloak-server>/realms/<realm-name>/protocol/openid-connect/token}.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #ISSUER_URL} must be set. In case it is not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Optional, defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server. Required, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * The default is {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Required if the client is
+ * private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server. Optional,
+ * defaults to empty (no scopes).
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
Review Comment:
FYI, I updated the PR and flattened the package hierarchy, as requested. The API is now entirely package-private.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3269852575
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Manager.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import java.util.Map;
+import org.apache.iceberg.catalog.SessionCatalog.SessionContext;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthManager;
+import org.apache.iceberg.rest.auth.AuthSession;
+
+public class OAuth2Manager implements AuthManager {
Review Comment:
OK let's remove and introduce them later on.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163100823
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
Review Comment:
I don't feel like this should be necessary. If there's a refresh config, then you should refresh. If there's not, you don't. I don't see the value in providing a refresh config, but then saying you can't use 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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3162875968
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
+ * https://<keycloak-server>/realms/<realm-name>}.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #TOKEN_ENDPOINT} must be set.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. For Keycloak, this is typically {@code
+ * https://<keycloak-server>/realms/<realm-name>/protocol/openid-connect/token}.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #ISSUER_URL} must be set. In case it is not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Optional, defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server. Required, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * The default is {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Required if the client is
+ * private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server. Optional,
+ * defaults to empty (no scopes).
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
+
+ /**
+ * Extra parameters to include in each request to the token endpoint. This is useful for custom
+ * parameters that are not covered by the standard OAuth2 specification. Optional, defaults to
+ * empty.
+ *
+ * <p>This is a prefix property, and multiple values can be set, each with a different key and
+ * value. The values must NOT be URL-encoded. Example:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.custom_param1=custom_value1
+ * rest.auth.oauth2.extra-params.custom_param2=custom_value2
+ * }</pre>
+ *
+ * For example, Auth0 requires the {@code audience} parameter to be set to the API identifier.
+ * This can be done by setting the following configuration:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.audience=https://iceberg-rest-catalog/api
+ * }</pre>
+ */
+ Map<String, String> extraRequestParameters();
+
+ /**
+ * The session cache timeout. Cached sessions will become eligible for eviction after this
+ * duration of inactivity. Defaults to 1 hour. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ *
+ * <p>This value is used for housekeeping; it does not mean that cached sessions will stop working
+ * after this time, but that the session cache will evict the session after this time of
+ * inactivity. If the context is used again, a new session will be created and cached.
+ *
+ * <p>This property can only be specified at catalog session level. It is ignored if present in
+ * other levels.
+ */
+ @Value.Default
+ default Duration sessionCacheTimeout() {
+ return DEFAULT_SESSION_CACHE_TIMEOUT;
+ }
+
+ @Value.Check
+ default void validate() {
+ ConfigValidator validator = new ConfigValidator();
+
+ if (token().isEmpty()) {
+
+ validator.check(
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.contains(grantType()),
+ GRANT_TYPE,
+ "grant type must be one of: %s",
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.stream()
+ .map(GrantType::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.contains(clientAuthenticationMethod()),
+ CLIENT_AUTH,
+ "client authentication method must be one of: %s",
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.stream()
+ .map(ClientAuthenticationMethod::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ issuerUrl().isPresent() || tokenEndpoint().isPresent(),
+ List.of(ISSUER_URL, TOKEN_ENDPOINT),
+ "either issuer URL or token endpoint must be set");
+
+ validator.check(clientId().isPresent(), CLIENT_ID, "client ID must not be empty");
+
+ if (ConfigUtil.requiresClientSecret(clientAuthenticationMethod())) {
+ validator.check(
+ clientSecret().isPresent(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be empty when client authentication is '%s'",
+ clientAuthenticationMethod().getValue());
+ } else if (clientAuthenticationMethod().equals(ClientAuthenticationMethod.NONE)) {
+ validator.check(
+ clientSecret().isEmpty(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be set when client authentication is '%s'",
+ ClientAuthenticationMethod.NONE.getValue());
+ validator.check(
+ !grantType().equals(DEFAULT_GRANT_TYPE),
+ List.of(CLIENT_AUTH, GRANT_TYPE),
+ "grant type must not be '%s' when client authentication is '%s'",
+ DEFAULT_GRANT_TYPE.getValue(),
+ ClientAuthenticationMethod.NONE.getValue());
+ }
+ }
+
+ if (issuerUrl().isPresent()) {
+ validator.checkEndpoint(issuerUrl().get(), ISSUER_URL, "Issuer URL");
+ }
+
+ if (tokenEndpoint().isPresent()) {
+ validator.checkEndpoint(tokenEndpoint().get(), TOKEN_ENDPOINT, "Token endpoint");
+ }
+
+ validator.validate();
+ }
+
+ static ImmutableBasicConfig.Builder parse(Map<String, String> properties) {
Review Comment:
This isn't actually parsing, so I'd prefer `from(<properties>)` as opposed to `parse`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4346383173
A few high-level observations:
1. I think we should keep all the implementation in the same package and restrict everything to package private where possible. It's not worth losing the encapsulation to add a little bit if package structure.
2. We're leaking many of the Nimbus primitives through the interfaces. If we can keep those within the package, I'm probably ok with that, but if we expose them through public interfaces, it binds us to that implementation.
3. We use `Optional` and optional fields very inconsistently. If it's truly optional, then it should be wrapped in `Optional`.
4. I'm not convinced we should be breaking out separate configs like `BasicConfig`, `TokenRefreshConfig` into composed separate sets of interfaces. I might be able to get behind keeping refresh and exchange configs separate, but it feels like a lot of levels of indirection.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170394307
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
Review Comment:
This will end up with a potentially very large package eventually. But I guess there is no better option.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639398903
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigValidator.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.google.errorprone.annotations.FormatMethod;
+import com.google.errorprone.annotations.FormatString;
+import java.net.URI;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.immutables.value.Value;
+
+final class ConfigValidator {
Review Comment:
I'm a little concerned about the complexity we're adding here. I understand what you're trying to accomplish by trying to validate all of the properites at once and throw an exception that includes everything that's misconfigured. However, this is a big shift from the conventions used everywhere else in the project and adds a lot of overhead for understanding checks.
Looking at all of the validation logic, it seems like we could just use the standard convention of `Preconditions.check` everywhere and avoid this. What value is this really adding?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3632408364
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/LegacyConfigAdaptor.java:
##########
@@ -0,0 +1,459 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.id.Audience;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.TokenTypeURI;
+import com.nimbusds.oauth2.sdk.token.TypelessAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.ResourcePaths;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.util.PropertyUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A component for adapting from legacy OAuth2 properties (from {@link OAuth2Properties}) to the new
+ * OAuth2 properties (as declared in {@link OAuth2Config}).
+ */
+@SuppressWarnings("deprecation")
+final class LegacyConfigAdaptor {
+
+ /**
+ * The default client ID to use when no client ID is provided in the legacy {@link
+ * OAuth2Properties#CREDENTIAL} property.
+ */
+ public static final ClientID DEFAULT_CLIENT_ID = new ClientID("iceberg");
+
+ private static final Logger LOGGER = LoggerFactory.getLogger(LegacyConfigAdaptor.class);
+
+ private static final Splitter CREDENTIAL_SPLITTER = Splitter.on(":").limit(2).trimResults();
+
+ private static final Set<String> TABLE_CONFIG_ALLOW_LIST =
+ Set.of(
+ BasicConfig.TOKEN,
+ TokenExchangeConfig.SUBJECT_TOKEN,
+ TokenExchangeConfig.SUBJECT_TOKEN_TYPE,
+ TokenExchangeConfig.ACTOR_TOKEN,
+ TokenExchangeConfig.ACTOR_TOKEN_TYPE);
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_LEGACY_OPTION =
+ "Detected legacy OAuth2 property '{}', please use option{} {} instead.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_NO_CLIENT_ID =
+ "The legacy OAuth2 property 'credential' was provided, but it did not contain a client ID; assuming '{}'.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_MISSING_TOKEN_ENDPOINT =
+ "The OAuth2 configuration does not specify a token endpoint nor an issuer URL: "
+ + "the token endpoint URL will default to {}. "
+ + "This automatic fallback will be removed in a future Iceberg release. "
+ + "Please configure OAuth2 endpoints using the following properties: '{}' or '{}'. "
+ + "This warning will disappear if OAuth2 endpoints are properly configured. "
+ + "See https://github.com/apache/iceberg/issues/10537";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_RELATIVE_TOKEN_ENDPOINT =
+ "The OAuth2 token endpoint URL is a relative URL. "
+ + "It will be resolved against the catalog URI, resulting in the absolute URL: '{}'. "
+ + "This automatic fallback will be removed in a future Iceberg release. "
+ + "Please configure OAuth2 endpoints using absolute URLs.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_MERGED_CONTEXTUAL_CONFIG =
+ "The OAuth2 configuration property '{}' was not found in the context session, "
+ + "and will be inherited from the parent session. "
+ + "This automatic fallback will be removed in a future Iceberg release.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_VENDED_TOKEN =
+ "The OAuth2 configuration property '{}' was found in the table configuration "
+ + "and indicates that the catalog server vended an OAuth2 token. "
+ + "Vended OAuth2 tokens will be disallowed in a future Iceberg release.";
+
+ @VisibleForTesting
+ static final String MESSAGE_TEMPLATE_TABLE_CONFIG_NOT_ALLOWED =
+ "The OAuth2 configuration property '{}' is not allowed to be vended by catalog servers.";
+
+ private final BiConsumer<String, String[]> logConsumer;
+
+ LegacyConfigAdaptor() {
+ this(LOGGER);
+ }
+
+ LegacyConfigAdaptor(Logger logger) {
+ this(logger::warn);
+ }
+
+ @VisibleForTesting
+ LegacyConfigAdaptor(BiConsumer<String, String[]> logConsumer) {
+ this.logConsumer = logConsumer;
+ }
+
+ /**
+ * Migrates catalog-level properties.
+ *
+ * <p>Legacy Iceberg OAuth2 properties are migrated, and warnings are logged for each detected
+ * legacy property.
+ *
+ * <p>The migration will further check the token endpoint for the following legacy situations:
+ *
+ * <ol>
+ * <li>If no token endpoint is provided, a default one will be added to the migrated properties;
+ * its value is the catalog URI + {@link ResourcePaths#tokens()} – that is, it will point to
+ * the (deprecated) REST Catalog token endpoint.
+ * <li>If a token endpoint is provided, but is a relative path, it will be resolved against the
+ * catalog URI.
+ * </ol>
+ *
+ * In both cases, a warning will be logged.
+ *
+ * @param properties The properties to migrate
+ * @param catalogUri The catalog URI, for extended token endpoint checks
+ */
+ public OAuth2Config migrateCatalogConfig(Map<String, String> properties, String catalogUri) {
Review Comment:
```suggestion
public OAuth2Config fromCatalogConfig(Map<String, String> properties, String catalogUri) {
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639226852
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ Duration MIN_TIMEOUT = Duration.parse("PT30S");
Review Comment:
Any duration values we have in could should use explicit, compile-time checked APIs (e.g. `Duration.ofSeconds(30)` vs `Duration.parse("PT30S")`. It's both more readable and compile-time checked whereas parsed values are not.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3153087146
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
Review Comment:
> Currently there's no way to pass that refresh token to Iceberg.
That is a Bad Idea ™️ 😄
Refresh tokens are specific to the clients they were issued to, and are not meant to be shared. Sharing refresh tokens is not only a potential security issue, but also doesn't work in practice: most IDPs revoke the refresh token once used, so if the catalog is given a refresh token and shares it with all the FileIO instances, only the first refresh would succeed; all others would fail because the token was revoked.
Instead, the Iceberg catalog must be autonomous in terms of OAuth2 tokens: ideally, each component (the catalog, the SessionContext, the FileIO instances, etc.) should manage its token internally.
In AuthManager v2, this will eventually be possible, and the catalog itself will handle the Authorization Code flow. There will be no need for users to do the authorization flow externally.
You may ask: but what if the Authorization Code flow is handled externally (e.g. in Trino)? In this case the best alternative is to use token exchange:
1. Trino logs in the user and gets an access token for them;
2. Trino instantiates the catalog with a SessionContext configured with the user's access token and the token exchange grant;
4. The SessionContext exchanges the user token against a token for itself;
5. The SessionContext receives an exchanged access token, and a refresh token.
In this way, the catalog's SessionContext contains an independent token that can be refreshed with its own refresh token. It also doesn't interfere with FileIO instances, because the SessionContext is not shared with them.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163043102
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
+ * https://<keycloak-server>/realms/<realm-name>}.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #TOKEN_ENDPOINT} must be set.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. For Keycloak, this is typically {@code
+ * https://<keycloak-server>/realms/<realm-name>/protocol/openid-connect/token}.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #ISSUER_URL} must be set. In case it is not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Optional, defaults to {@link #DEFAULT_GRANT_TYPE}.
Review Comment:
How is this Optional? If there's a case where the grant type is unknown, we should enumerate that in the type. If that's not an option, it should be required.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3403312108
##########
core/src/main/java/org/apache/iceberg/rest/auth/AuthConfig.java:
##########
@@ -27,10 +27,13 @@
/**
* The purpose of this class is to hold OAuth configuration options for {@link
* OAuth2Util.AuthSession}.
+ *
+ * @deprecated will be removed in 1.14.0.
Review Comment:
OK but I removed the pointer because you asked me to remove the new `OAuth2Manager` for now (because the impl is empty for now).
I will reinstate the new `OAuth2Manager`, if that's OK with you. That's the only way to have a "pointer".
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "obelix74 (via GitHub)" <gi...@apache.org>.
obelix74 commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4490750104
This PR will help with some of the use cases we (Workday) face with regards to refresh tokens. How can we help to get this uptaken?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4100186390
FYI the full changes can be reviewed here for reference: #15704 .
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3660209799
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigValidator.java:
##########
@@ -0,0 +1,95 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.google.errorprone.annotations.FormatMethod;
+import com.google.errorprone.annotations.FormatString;
+import java.net.URI;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.immutables.value.Value;
+
+final class ConfigValidator {
Review Comment:
The added value is standardized error checks (e.g. when checking an endpoint URL) and standardized error messages. We can achieve the same with just `Preconditions.check` but it's going to be more brittle.
Should I remove `ConfigValidator` and replace it with `Preconditions`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4977935172
@adutra I'm out through the end of the week, but will look first thing next week.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-4953430940
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3646606803
##########
core/src/main/java/org/apache/iceberg/rest/auth/OAuth2Manager.java:
##########
@@ -40,6 +40,11 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+/**
+ * @deprecated will be removed in 1.14.0, use {@link
+ * org.apache.iceberg.rest.auth.oauth2.OAuth2Manager} instead.
+ */
+@Deprecated
Review Comment:
I don't think we should deprecate these in this PR. Before we deprecate, we need to fully replace this. Plus, this is unrelated and can go in a separate PR so this one doesn't require touching so many files. It's a best practice to keep unrelated changes in separate PRs.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-5824565388
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3156133684
##########
kafka-connect/kafka-connect-runtime/main/LICENSE:
##########
@@ -545,27 +545,34 @@ License: Apache License, Version 2.0 - https://www.apache.org/licenses/LICENSE-2
--------------------------------------------------------------------------------
-This product bundles Nimbus Lang Tag.
+This product bundles Nimbus Language Tags.
Review Comment:
Cf https://github.com/apache/iceberg/pull/16143
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3162866209
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
Review Comment:
I don't think we need a `config` package separate from the parent `oauth2`. I understand that this provides some structural separation, but it also forces all of these classes into the public, which isn't necessary.
We should move all of these up.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163106731
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
Review Comment:
Why do we need optional logic here? Seems like this would only be true or false.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163061548
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
+ * https://<keycloak-server>/realms/<realm-name>}.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #TOKEN_ENDPOINT} must be set.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. For Keycloak, this is typically {@code
+ * https://<keycloak-server>/realms/<realm-name>/protocol/openid-connect/token}.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #ISSUER_URL} must be set. In case it is not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Optional, defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server. Required, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * The default is {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Required if the client is
+ * private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server. Optional,
+ * defaults to empty (no scopes).
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
Review Comment:
It seems awkward that we're not parsing out the scopes. This is also exposing the Nimbus representation of scope, which we probably don't want to embed in the API. Ideally we'd not expose the underlying implementation throught the API surface area.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-6405495618
This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639123156
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
Review Comment:
Ok, I think I misunderstood what this timeout represented. I thought it was represented the default token timeout (assuming we don't have an explicit timeout), not the fetch timeout.
I think the issue here is that we're breaking this up and introducing things that are not being used currently, so there's a high expectation of context around what is coming next, which isn't a great approach. This is true for other values like session cache timeout, min timeout etc. Why can't we introduce those when they're used so we understand it in context of what's being added?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639200147
##########
core/src/test/java/org/apache/iceberg/rest/auth/oauth2/TestTokenRefreshConfig.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
+ *
+ * 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.iceberg.rest.auth.oauth2;
+
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.ACCESS_TOKEN_LIFESPAN;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.ENABLED;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.JITTER;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.PREFETCH;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.TOKEN_EXCHANGE_ENABLED;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class TestTokenRefreshConfig {
+
+ @ParameterizedTest
+ @MethodSource
+ @SuppressWarnings("ResultOfMethodCallIgnored")
+ void testValidate(Map<String, String> properties, List<String> expected) {
+ assertThatIllegalArgumentException()
+ .isThrownBy(() -> TokenRefreshConfig.from(properties).build())
+ .withMessage(ConfigValidator.buildDescription(expected.stream()));
+ }
+
+ static Stream<Arguments> testValidate() {
+ return Stream.of(
+ Arguments.of(
+ Map.of(ACCESS_TOKEN_LIFESPAN, "PT2S"),
+ List.of(
+ "access token lifespan must be greater than or equal to PT15S (rest.auth.oauth2.token-refresh.access-token-lifespan)",
+ "refresh prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)",
+ "jitter plus prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.jitter / rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)")),
+ Arguments.of(
+ Map.of(PREFETCH, "PT0.1S"),
+ List.of(
+ "refresh prefetch must be greater than or equal to PT10S (rest.auth.oauth2.token-refresh.prefetch)")),
+ Arguments.of(
+ Map.of(PREFETCH, "PT10M", ACCESS_TOKEN_LIFESPAN, "PT5M"),
+ List.of(
+ "refresh prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)",
+ "jitter plus prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.jitter / rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)")),
+ Arguments.of(
+ Map.of(JITTER, "PT-1S"),
+ List.of("jitter must not be negative (rest.auth.oauth2.token-refresh.jitter)")),
+ Arguments.of(
+ Map.of(JITTER, "PT1H"),
+ List.of(
+ "jitter plus prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.jitter / rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)")));
Review Comment:
Our convention is that messages should start with a capitol letter. There's a lot of new `check/validation` code being added and it's unclear to me if we're holding true to the project convention on this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3391904107
##########
core/src/main/java/org/apache/iceberg/rest/auth/AuthConfig.java:
##########
@@ -27,10 +27,13 @@
/**
* The purpose of this class is to hold OAuth configuration options for {@link
* OAuth2Util.AuthSession}.
+ *
+ * @deprecated will be removed in 1.14.0.
Review Comment:
For deprecations, we need a pointer to what to use instead.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170291383
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Config.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.util.Map;
+import org.apache.iceberg.rest.auth.oauth2.config.BasicConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ConfigValidator;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenRefreshConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenRefreshConfig;
+import org.immutables.value.Value;
+
+/** The configuration for the OAuth2 AuthManager. */
+@Value.Immutable
+public interface OAuth2Config {
+
+ String PREFIX = "rest.auth.oauth2.";
+
+ /**
+ * The basic configuration, including token endpoint, grant type, client id and client secret.
+ * Required.
+ */
+ BasicConfig basicConfig();
Review Comment:
The composition may seem superfluous now, but it will start making sense later.
For example, when we integration interactive grants, and maybe JWT client auth, we'd have more methods:
```java
BasicConfig basicConfig();
@Value.Default
default TokenRefreshConfig tokenRefreshConfig() { ... }
@Value.Default
default TokenExchangeConfig tokenExchangeConfig() { ... }
@Value.Default
default AuthorizationCodeConfig authorizationCodeConfig() { ... }
@Value.Default
default DeviceCodeConfig deviceCodeConfig() { ... }
@Value.Default
default JWTClientAuthConfig jwtClientAuthConfig() { ... }
```
etc.
OAuth2 is an evolving set of standards, so we should be prepared for that. Isolating each OAuth2 feature in its own config class helps keeping the configuration tidy.
As for `BasicConfig`: it just felt to me more natural to keep `OAuth2Config` as the umbrella configuration class, and delegate actual properties to config-section classes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3660164481
##########
core/src/test/java/org/apache/iceberg/rest/auth/oauth2/TestTokenRefreshConfig.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
+ *
+ * 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.iceberg.rest.auth.oauth2;
+
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.ACCESS_TOKEN_LIFESPAN;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.ENABLED;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.JITTER;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.PREFETCH;
+import static org.apache.iceberg.rest.auth.oauth2.TokenRefreshConfig.TOKEN_EXCHANGE_ENABLED;
+import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
+
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+class TestTokenRefreshConfig {
+
+ @ParameterizedTest
+ @MethodSource
+ @SuppressWarnings("ResultOfMethodCallIgnored")
+ void testValidate(Map<String, String> properties, List<String> expected) {
+ assertThatIllegalArgumentException()
+ .isThrownBy(() -> TokenRefreshConfig.from(properties).build())
+ .withMessage(ConfigValidator.buildDescription(expected.stream()));
+ }
+
+ static Stream<Arguments> testValidate() {
+ return Stream.of(
+ Arguments.of(
+ Map.of(ACCESS_TOKEN_LIFESPAN, "PT2S"),
+ List.of(
+ "access token lifespan must be greater than or equal to PT15S (rest.auth.oauth2.token-refresh.access-token-lifespan)",
+ "refresh prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)",
+ "jitter plus prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.jitter / rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)")),
+ Arguments.of(
+ Map.of(PREFETCH, "PT0.1S"),
+ List.of(
+ "refresh prefetch must be greater than or equal to PT10S (rest.auth.oauth2.token-refresh.prefetch)")),
+ Arguments.of(
+ Map.of(PREFETCH, "PT10M", ACCESS_TOKEN_LIFESPAN, "PT5M"),
+ List.of(
+ "refresh prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)",
+ "jitter plus prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.jitter / rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)")),
+ Arguments.of(
+ Map.of(JITTER, "PT-1S"),
+ List.of("jitter must not be negative (rest.auth.oauth2.token-refresh.jitter)")),
+ Arguments.of(
+ Map.of(JITTER, "PT1H"),
+ List.of(
+ "jitter plus prefetch must be less than the access token lifespan (rest.auth.oauth2.token-refresh.jitter / rest.auth.oauth2.token-refresh.prefetch / rest.auth.oauth2.token-refresh.access-token-lifespan)")));
Review Comment:
All messages generated by `ConfigValidator` respect that rule. Here is an example error message, where 3 validation failures are found:
```
Invalid OAuth2 configuration:
- either issuer URL or token endpoint must be set (rest.auth.oauth2.issuer-url / rest.auth.oauth2.token-endpoint)
- client ID must not be empty (rest.auth.oauth2.client-id)
- client secret must not be empty when client authentication is 'client_secret_basic' (rest.auth.oauth2.client-auth / rest.auth.oauth2.client-secret)
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "bharos (via GitHub)" <gi...@apache.org>.
bharos commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3126744632
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
Review Comment:
for users who obtain tokens externally (PKCE, device code), this is the only entry point. As per my other comment, if refresh-token is supported, this option may become viable as well ?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170410662
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
Review Comment:
But users need to tell if they want token refreshes or not. So there has to be a property for that, namely `rest.auth.oauth2.token-refresh.enabled`.
Each method in the configuration classes maps 1x1 to a user-facing configuration option.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3156161840
##########
build.gradle:
##########
@@ -395,6 +395,13 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ // OAuth2
+ implementation(libs.nimbus.oauth2.oidc.sdk) {
+ exclude group: 'net.minidev', module: 'accessors-smart'
Review Comment:
These exclusions allow to narrow the amount of transitive dependencies brought by Nimbus:
- `accessors-smart` is an optional bytecode-generation optimizer for `json-smart`; `json-smart` falls back to reflection without it. Besides `accessors-smart` also brings ASM, which is a big jar. Removing these is safe because Nimbus objects all implement `JSONAware` (i.e. they expose a `.toJSONString()` method) and thus do not need reflection in order to be serialized.
- `jcip-annotations` are compile-time thread-safety markers; they can be safely excluded.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163092738
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Config.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.util.Map;
+import org.apache.iceberg.rest.auth.oauth2.config.BasicConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ConfigValidator;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenRefreshConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenRefreshConfig;
+import org.immutables.value.Value;
+
+/** The configuration for the OAuth2 AuthManager. */
+@Value.Immutable
+public interface OAuth2Config {
+
+ String PREFIX = "rest.auth.oauth2.";
+
+ /**
+ * The basic configuration, including token endpoint, grant type, client id and client secret.
+ * Required.
+ */
+ BasicConfig basicConfig();
+
+ /** The token refresh configuration. Optional. */
+ @Value.Default
+ default TokenRefreshConfig tokenRefreshConfig() {
Review Comment:
We seem to be really inconsistent about how we're handling optional fields. This says optional but isn't wrapped as `Optional<TokenRefreshConfig>`. This is one of the places where I'd expect to see optional used.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163151206
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
+
+ /**
+ * Default access token lifespan; if the OAuth2 server returns an access token without specifying
+ * its expiration time, this value will be used.
+ *
+ * <p>Optional, defaults to {@link #DEFAULT_ACCESS_TOKEN_LIFESPAN}. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration accessTokenLifespan() {
+ return DEFAULT_ACCESS_TOKEN_LIFESPAN;
+ }
+
+ /**
+ * Refresh safety margin to use; a new token will be fetched when the current token's remaining
+ * lifespan is less than this value. Optional, defaults to {@link #DEFAULT_SAFETY_MARGIN}. Must be
+ * a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration safetyMargin() {
Review Comment:
Naming is a little awkward here. Something more like `prefechDuration` makes more sense than safety. Also, seeing the impact of this in the wild, we'll need to make sure to include reasonable jitter (refreshes across hundreds or thousands of workers can cause thundering herd problems). Not sure if this is where want that or in the actual fetch code.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3704802124
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.ofMinutes(5);
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.ofHours(1);
+
+ Duration MIN_TIMEOUT = Duration.ofSeconds(30);
Review Comment:
It's exposed as a configurable property only for testing purposes. Let me remove the configurable option, I will figure out some other way to tweak this value for tests.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3391929627
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigMigrator.java:
##########
@@ -0,0 +1,459 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.id.Audience;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.TokenTypeURI;
+import com.nimbusds.oauth2.sdk.token.TypelessAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.ResourcePaths;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.util.PropertyUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A component for migrating from legacy OAuth2 properties (from {@link OAuth2Properties}) to the
+ * new OAuth2 properties (as declared in {@link OAuth2Config}).
+ */
+@SuppressWarnings("deprecation")
+final class ConfigMigrator {
Review Comment:
I still take issue with this naming. We're not migrating, we're adapting/converting.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3391929627
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigMigrator.java:
##########
@@ -0,0 +1,459 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.id.Audience;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.TokenTypeURI;
+import com.nimbusds.oauth2.sdk.token.TypelessAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.ResourcePaths;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.util.PropertyUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A component for migrating from legacy OAuth2 properties (from {@link OAuth2Properties}) to the
+ * new OAuth2 properties (as declared in {@link OAuth2Config}).
+ */
+@SuppressWarnings("deprecation")
+final class ConfigMigrator {
Review Comment:
I still take issue with this naming. We're not migrating, we're adapting.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3632104403
##########
build.gradle:
##########
@@ -403,6 +403,13 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ // OAuth2
+ implementation(libs.nimbus.oauth2.oidc.sdk) {
Review Comment:
I'm not sure how to address the dependency issues, but this pulls in dependencies for the runtime bundles without actually using them yet. Is there an easy way to exclude these until we have an implementation that would actually be used?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639147509
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ Duration MIN_TIMEOUT = Duration.parse("PT30S");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Optional. Either this property or {@link #TOKEN_ENDPOINT} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. If not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ *
+ * <p>Optional. Either this property or {@link #ISSUER_URL} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server.
+ *
+ * <p>Optional. Required unless a {@linkplain #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Optional. Rquired when the
+ * client is private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server.
+ *
+ * <p>Optional. If absent, no scopes are sent.
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
+
+ /**
+ * Extra parameters to include in each request to the token endpoint. This is useful for custom
+ * parameters that are not covered by the standard OAuth2 specification. Defaults to an empty map.
+ *
+ * <p>This is a prefix property, and multiple values can be set, each with a different key and
+ * value. The values must NOT be URL-encoded. Example:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.custom_param1=custom_value1
+ * rest.auth.oauth2.extra-params.custom_param2=custom_value2
+ * }</pre>
+ *
+ * For example, Auth0 requires the {@code audience} parameter to be set to the API identifier.
+ * This can be done by setting the following configuration:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.audience=https://iceberg-rest-catalog/api
+ * }</pre>
+ */
+ Map<String, String> extraRequestParameters();
+
+ /**
+ * The token acquisition timeout. Defaults to {@link #DEFAULT_TIMEOUT}. The default timeout is
+ * intentionally large, in order to accommodate for long-running flows that require human
+ * intervention (e.g. Authorization Code flow).
+ *
+ * <p>Must be a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601
+ * duration</a>.
+ */
+ @Value.Default
+ default Duration tokenAcquisitionTimeout() {
+ return DEFAULT_TIMEOUT;
+ }
+
+ /**
+ * The session cache timeout. Cached sessions will become eligible for eviction after this
+ * duration of inactivity. Defaults to 1 hour. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ *
+ * <p>This value is used for housekeeping; it does not mean that cached sessions will stop working
+ * after this time, but that the session cache will evict the session after this time of
+ * inactivity. If the context is used again, a new session will be created and cached.
+ *
+ * <p>This property can only be specified at catalog session level. It is ignored if present in
+ * other levels.
+ */
+ @Value.Default
+ default Duration sessionCacheTimeout() {
+ return DEFAULT_SESSION_CACHE_TIMEOUT;
+ }
+
+ /**
+ * The minimum timeout for token acquisition.
+ *
+ * <p>This option is not exposed as a public configuration property, and is intended for testing
+ * purposes only.
+ */
+ @Value.Default
+ default Duration minTokenAcquisitionTimeout() {
+ return MIN_TIMEOUT;
+ }
+
+ @Value.Check
+ default void validate() {
+ ConfigValidator validator = new ConfigValidator();
+
+ if (token().isEmpty()) {
+
+ validator.check(
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.contains(grantType()),
+ GRANT_TYPE,
+ "grant type must be one of: %s",
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.stream()
+ .map(GrantType::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.contains(clientAuthenticationMethod()),
+ CLIENT_AUTH,
+ "client authentication method must be one of: %s",
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.stream()
+ .map(ClientAuthenticationMethod::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ issuerUrl().isPresent() || tokenEndpoint().isPresent(),
+ List.of(ISSUER_URL, TOKEN_ENDPOINT),
+ "either issuer URL or token endpoint must be set");
+
+ validator.check(clientId().isPresent(), CLIENT_ID, "client ID must not be empty");
+
+ if (ConfigUtil.requiresClientSecret(clientAuthenticationMethod())) {
+ validator.check(
+ clientSecret().isPresent(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be empty when client authentication is '%s'",
+ clientAuthenticationMethod().getValue());
+ } else if (clientAuthenticationMethod().equals(ClientAuthenticationMethod.NONE)) {
+ validator.check(
+ clientSecret().isEmpty(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be set when client authentication is '%s'",
+ ClientAuthenticationMethod.NONE.getValue());
+ validator.check(
+ !grantType().equals(DEFAULT_GRANT_TYPE),
+ List.of(CLIENT_AUTH, GRANT_TYPE),
+ "grant type must not be '%s' when client authentication is '%s'",
+ DEFAULT_GRANT_TYPE.getValue(),
+ ClientAuthenticationMethod.NONE.getValue());
+ }
+ }
+
+ if (issuerUrl().isPresent()) {
+ validator.checkEndpoint(issuerUrl().get(), ISSUER_URL, "Issuer URL");
+ }
+
+ if (tokenEndpoint().isPresent()) {
+ validator.checkEndpoint(tokenEndpoint().get(), TOKEN_ENDPOINT, "Token endpoint");
+ }
+
+ validator.check(
+ tokenAcquisitionTimeout().compareTo(minTokenAcquisitionTimeout()) >= 0,
+ TIMEOUT,
+ "timeout must be greater than or equal to %s",
Review Comment:
```suggestion
"Timeout must be greater than or equal to %s",
```
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3660117141
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Manager.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import java.util.Map;
+import org.apache.iceberg.catalog.SessionCatalog.SessionContext;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthManager;
+import org.apache.iceberg.rest.auth.AuthSession;
+
+public class OAuth2Manager implements AuthManager {
Review Comment:
I'll do the requested change, but in order for this to work we'll need to use `hiddenImpl()` in `AuthManagers` – hope that's OK.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-5095863653
@danielcweeks and @rdblue I incorporated all your feedback. The PR now is considerably reduced in size (15 files only). PTAL 🙏
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub) via issues" <is...@iceberg.apache.org>.
adutra closed pull request #15703: REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration
URL: https://github.com/apache/iceberg/pull/15703
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3632429858
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Manager.java:
##########
@@ -0,0 +1,61 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import java.util.Map;
+import org.apache.iceberg.catalog.SessionCatalog.SessionContext;
+import org.apache.iceberg.catalog.TableIdentifier;
+import org.apache.iceberg.rest.RESTClient;
+import org.apache.iceberg.rest.auth.AuthManager;
+import org.apache.iceberg.rest.auth.AuthSession;
+
+public class OAuth2Manager implements AuthManager {
Review Comment:
Is there a reason this neads to be public at this point? If you configure it, it will just throw so it may be best to just keep it package private until there's functionality that can be used.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "rdblue (via GitHub)" <gi...@apache.org>.
rdblue commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639540942
##########
build.gradle:
##########
@@ -395,6 +395,13 @@ project(':iceberg-core') {
exclude group: 'org.slf4j', module: 'slf4j-log4j12'
}
+ // OAuth2
+ implementation(libs.nimbus.oauth2.oidc.sdk) {
+ exclude group: 'net.minidev', module: 'accessors-smart'
Review Comment:
Does this need another JSON library? How larger are the dependency additions to the runtime Jars?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3639150146
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ Duration MIN_TIMEOUT = Duration.parse("PT30S");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Optional. Either this property or {@link #TOKEN_ENDPOINT} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. If not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ *
+ * <p>Optional. Either this property or {@link #ISSUER_URL} must be set, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server.
+ *
+ * <p>Optional. Required unless a {@linkplain #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * Defaults to {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Optional. Rquired when the
+ * client is private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server.
+ *
+ * <p>Optional. If absent, no scopes are sent.
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
+
+ /**
+ * Extra parameters to include in each request to the token endpoint. This is useful for custom
+ * parameters that are not covered by the standard OAuth2 specification. Defaults to an empty map.
+ *
+ * <p>This is a prefix property, and multiple values can be set, each with a different key and
+ * value. The values must NOT be URL-encoded. Example:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.custom_param1=custom_value1
+ * rest.auth.oauth2.extra-params.custom_param2=custom_value2
+ * }</pre>
+ *
+ * For example, Auth0 requires the {@code audience} parameter to be set to the API identifier.
+ * This can be done by setting the following configuration:
+ *
+ * <pre>{@code
+ * rest.auth.oauth2.extra-params.audience=https://iceberg-rest-catalog/api
+ * }</pre>
+ */
+ Map<String, String> extraRequestParameters();
+
+ /**
+ * The token acquisition timeout. Defaults to {@link #DEFAULT_TIMEOUT}. The default timeout is
+ * intentionally large, in order to accommodate for long-running flows that require human
+ * intervention (e.g. Authorization Code flow).
+ *
+ * <p>Must be a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601
+ * duration</a>.
+ */
+ @Value.Default
+ default Duration tokenAcquisitionTimeout() {
+ return DEFAULT_TIMEOUT;
+ }
+
+ /**
+ * The session cache timeout. Cached sessions will become eligible for eviction after this
+ * duration of inactivity. Defaults to 1 hour. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ *
+ * <p>This value is used for housekeeping; it does not mean that cached sessions will stop working
+ * after this time, but that the session cache will evict the session after this time of
+ * inactivity. If the context is used again, a new session will be created and cached.
+ *
+ * <p>This property can only be specified at catalog session level. It is ignored if present in
+ * other levels.
+ */
+ @Value.Default
+ default Duration sessionCacheTimeout() {
+ return DEFAULT_SESSION_CACHE_TIMEOUT;
+ }
+
+ /**
+ * The minimum timeout for token acquisition.
+ *
+ * <p>This option is not exposed as a public configuration property, and is intended for testing
+ * purposes only.
+ */
+ @Value.Default
+ default Duration minTokenAcquisitionTimeout() {
+ return MIN_TIMEOUT;
+ }
+
+ @Value.Check
+ default void validate() {
+ ConfigValidator validator = new ConfigValidator();
+
+ if (token().isEmpty()) {
+
+ validator.check(
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.contains(grantType()),
+ GRANT_TYPE,
+ "grant type must be one of: %s",
+ ConfigUtil.SUPPORTED_INITIAL_GRANT_TYPES.stream()
+ .map(GrantType::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.contains(clientAuthenticationMethod()),
+ CLIENT_AUTH,
+ "client authentication method must be one of: %s",
+ ConfigUtil.SUPPORTED_CLIENT_AUTH_METHODS.stream()
+ .map(ClientAuthenticationMethod::getValue)
+ .collect(Collectors.joining("', '", "'", "'")));
+
+ validator.check(
+ issuerUrl().isPresent() || tokenEndpoint().isPresent(),
+ List.of(ISSUER_URL, TOKEN_ENDPOINT),
+ "either issuer URL or token endpoint must be set");
+
+ validator.check(clientId().isPresent(), CLIENT_ID, "client ID must not be empty");
+
+ if (ConfigUtil.requiresClientSecret(clientAuthenticationMethod())) {
+ validator.check(
+ clientSecret().isPresent(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be empty when client authentication is '%s'",
+ clientAuthenticationMethod().getValue());
+ } else if (clientAuthenticationMethod().equals(ClientAuthenticationMethod.NONE)) {
+ validator.check(
+ clientSecret().isEmpty(),
+ List.of(CLIENT_AUTH, CLIENT_SECRET),
+ "client secret must not be set when client authentication is '%s'",
+ ClientAuthenticationMethod.NONE.getValue());
+ validator.check(
+ !grantType().equals(DEFAULT_GRANT_TYPE),
+ List.of(CLIENT_AUTH, GRANT_TYPE),
+ "grant type must not be '%s' when client authentication is '%s'",
Review Comment:
```suggestion
"Grant type must not be '%s' when client authentication is '%s'",
```
Nevermind, I see these eventually get prefixed by `Invalid configuration . . . `
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3693325358
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/ConfigUtil.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.OptionalInt;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+import javax.annotation.Nullable;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+
+/**
+ * Helper class for parsing configuration options. It also exposes useful constants for config
+ * validation.
+ */
+final class ConfigUtil {
+
+ public static final List<GrantType> SUPPORTED_GRANT_TYPES =
+ List.of(GrantType.CLIENT_CREDENTIALS, GrantType.TOKEN_EXCHANGE, GrantType.REFRESH_TOKEN);
+
+ public static final List<GrantType> SUPPORTED_INITIAL_GRANT_TYPES =
+ List.of(GrantType.CLIENT_CREDENTIALS, GrantType.TOKEN_EXCHANGE);
+
+ public static final List<ClientAuthenticationMethod> SUPPORTED_CLIENT_AUTH_METHODS =
+ List.of(
+ ClientAuthenticationMethod.NONE,
+ ClientAuthenticationMethod.CLIENT_SECRET_BASIC,
+ ClientAuthenticationMethod.CLIENT_SECRET_POST);
+
+ /**
+ * A sentinel value used to indicate that the parent session's token should be used. This is
+ * useful for the token exchange flow.
+ */
+ public static final String PARENT_TOKEN = "::parent::";
Review Comment:
This sentinel has me concerned because I don't see the context in which it would be used. At this point I would have to assume I understand and agree with how it will be used, but I don't. I'd rather leave things like this out and introduce them only when they're used so it makes sense in context of the introduced changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3391972606
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/BasicConfig.java:
##########
@@ -0,0 +1,318 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String TIMEOUT = PREFIX + "timeout";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_TIMEOUT = Duration.parse("PT5M");
Review Comment:
Are we taking this default from the previous implementation? 5 minutes seems unusually short for the default.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3162913023
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/OAuth2Config.java:
##########
@@ -0,0 +1,83 @@
+/*
+ * 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.iceberg.rest.auth.oauth2;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.util.Map;
+import org.apache.iceberg.rest.auth.oauth2.config.BasicConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ConfigValidator;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.ImmutableTokenRefreshConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenExchangeConfig;
+import org.apache.iceberg.rest.auth.oauth2.config.TokenRefreshConfig;
+import org.immutables.value.Value;
+
+/** The configuration for the OAuth2 AuthManager. */
+@Value.Immutable
+public interface OAuth2Config {
+
+ String PREFIX = "rest.auth.oauth2.";
+
+ /**
+ * The basic configuration, including token endpoint, grant type, client id and client secret.
+ * Required.
+ */
+ BasicConfig basicConfig();
Review Comment:
I'm struggling to understand the reason for the composition here as opposed to including config attributes directly in the OAuth2Config. It seems like everything in `BasicConfig` should just be part of `OAuth2Config`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163031697
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
Review Comment:
I think we should avoid using specific implementations like keycloak in the documentation. We can add site docs later for how to configure specific OAuth servers, but the javadocs should just stick to the spec/RFC definitions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163031697
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
Review Comment:
I think we should avoid using specific implementations like keycloak in the documentation. We can add docs later for how to configure specific OAuth servers, but the docs should just stick to the spec/RFC definitions.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170421477
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
Review Comment:
No, we can't have a static default here: the default is true if the initial grant is `client_credentials`, and false otherwise. This is awkward indeed, but it's a necessary precaution for people migrating from v1.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170323836
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/ConfigMigrator.java:
##########
@@ -0,0 +1,463 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.id.Audience;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.TokenTypeURI;
+import com.nimbusds.oauth2.sdk.token.TypelessAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Map.Entry;
+import java.util.Set;
+import java.util.function.BiConsumer;
+import java.util.stream.Collectors;
+import org.apache.iceberg.relocated.com.google.common.annotations.VisibleForTesting;
+import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
+import org.apache.iceberg.relocated.com.google.common.base.Splitter;
+import org.apache.iceberg.relocated.com.google.common.collect.Lists;
+import org.apache.iceberg.relocated.com.google.common.collect.Maps;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.ResourcePaths;
+import org.apache.iceberg.rest.auth.OAuth2Properties;
+import org.apache.iceberg.rest.auth.oauth2.ImmutableOAuth2Config;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.apache.iceberg.util.PropertyUtil;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+/**
+ * A component for migrating from legacy OAuth2 properties (from {@link OAuth2Properties}) to the
+ * new OAuth2 properties (as declared in {@link OAuth2Config}).
+ */
+@SuppressWarnings("deprecation")
+public final class ConfigMigrator {
Review Comment:
It takes a map of properties as its input, and outputs a map of properties semantically equivalent, but with all legacy properties migrated to their new counterparts – hence the `Migrator` suffix.
But if you still prefer `LegacyConfigAdaptor`, I'm fine with that.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3170384666
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/BasicConfig.java:
##########
@@ -0,0 +1,284 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import com.nimbusds.oauth2.sdk.Scope;
+import com.nimbusds.oauth2.sdk.auth.ClientAuthenticationMethod;
+import com.nimbusds.oauth2.sdk.auth.Secret;
+import com.nimbusds.oauth2.sdk.id.ClientID;
+import com.nimbusds.oauth2.sdk.token.AccessToken;
+import com.nimbusds.oauth2.sdk.token.BearerAccessToken;
+import java.net.URI;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.stream.Collectors;
+import org.apache.iceberg.rest.RESTUtil;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/**
+ * Basic OAuth2 properties. These properties are used to configure the basic OAuth2 options such as
+ * the issuer URL, token endpoint, client ID, and client secret.
+ */
+@Value.Immutable
+@Value.Style(redactedMask = "****")
+@SuppressWarnings({"ImmutablesStyle", "SafeLoggingPropagation"})
+public interface BasicConfig {
+
+ String PREFIX = OAuth2Config.PREFIX;
+
+ String TOKEN = PREFIX + "token";
+ String ISSUER_URL = PREFIX + "issuer-url";
+ String TOKEN_ENDPOINT = PREFIX + "token-endpoint";
+ String GRANT_TYPE = PREFIX + "grant-type";
+ String CLIENT_ID = PREFIX + "client-id";
+ String CLIENT_AUTH = PREFIX + "client-auth";
+ String CLIENT_SECRET = PREFIX + "client-secret";
+ String SCOPE = PREFIX + "scope";
+ String EXTRA_PARAMS = PREFIX + "extra-params";
+ String SESSION_CACHE_TIMEOUT = PREFIX + "session-cache.timeout";
+
+ GrantType DEFAULT_GRANT_TYPE = GrantType.CLIENT_CREDENTIALS;
+ ClientAuthenticationMethod DEFAULT_CLIENT_AUTH = ClientAuthenticationMethod.CLIENT_SECRET_BASIC;
+ Duration DEFAULT_SESSION_CACHE_TIMEOUT = Duration.parse("PT1H");
+
+ /**
+ * The initial access token to use. Optional. If this is set, the OAuth2 client will not attempt
+ * to fetch an initial token from the Authorization server, but will use this token instead.
+ *
+ * <p>This option should be avoided as in most cases, the token cannot be refreshed.
+ */
+ @Value.Redacted
+ Optional<AccessToken> token();
+
+ /**
+ * The root URL of the Authorization server, which will be used for discovering supported
+ * endpoints and their locations. For Keycloak, this is typically the realm URL: {@code
+ * https://<keycloak-server>/realms/<realm-name>}.
+ *
+ * <p>Two "well-known" paths are supported for endpoint discovery: {@code
+ * .well-known/openid-configuration} and {@code .well-known/oauth-authorization-server}. The full
+ * metadata discovery URL will be constructed by appending these paths to the issuer URL.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #TOKEN_ENDPOINT} must be set.
+ *
+ * @see <a
+ * href="https://openid.net/specs/openid-connect-discovery-1_0.html#ProviderMetadata">OpenID
+ * Connect Discovery 1.0</a>
+ * @see <a href="https://tools.ietf.org/html/rfc8414#section-5">RFC 8414 Section 5</a>
+ */
+ Optional<URI> issuerUrl();
+
+ /**
+ * URL of the OAuth2 token endpoint. For Keycloak, this is typically {@code
+ * https://<keycloak-server>/realms/<realm-name>/protocol/openid-connect/token}.
+ *
+ * <p>Unless a {@linkplain #TOKEN static token} is provided, either this property or {@link
+ * #ISSUER_URL} must be set. In case it is not set, the token endpoint will be discovered from the
+ * {@link #ISSUER_URL issuer URL}, using the OpenID Connect Discovery metadata published by the
+ * issuer.
+ */
+ Optional<URI> tokenEndpoint();
+
+ /**
+ * The grant type to use when authenticating against the OAuth2 server. Valid values are:
+ *
+ * <ul>
+ * <li>{@link GrantType#CLIENT_CREDENTIALS client_credentials}
+ * <li>{@link GrantType#TOKEN_EXCHANGE urn:ietf:params:oauth:grant-type:token-exchange}
+ * </ul>
+ *
+ * Optional, defaults to {@link #DEFAULT_GRANT_TYPE}.
+ */
+ @Value.Default
+ default GrantType grantType() {
+ return DEFAULT_GRANT_TYPE;
+ }
+
+ /**
+ * Client ID to use when authenticating against the OAuth2 server. Required, unless a {@linkplain
+ * #TOKEN static token} is provided.
+ */
+ Optional<ClientID> clientId();
+
+ /**
+ * The OAuth2 client authentication method to use. Valid values are:
+ *
+ * <ul>
+ * <li>{@link ClientAuthenticationMethod#NONE none}: the client does not authenticate itself at
+ * the token endpoint, because it is a public client with no client secret or other
+ * authentication mechanism.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_BASIC client_secret_basic}: client secret
+ * is sent in the HTTP Basic Authorization header.
+ * <li>{@link ClientAuthenticationMethod#CLIENT_SECRET_POST client_secret_post}: client secret
+ * is sent in the request body as a form parameter.
+ * </ul>
+ *
+ * The default is {@link #DEFAULT_CLIENT_AUTH}.
+ */
+ @Value.Default
+ default ClientAuthenticationMethod clientAuthenticationMethod() {
+ return DEFAULT_CLIENT_AUTH;
+ }
+
+ /**
+ * Client secret to use when authenticating against the OAuth2 server. Required if the client is
+ * private and is authenticated using the standard "client-secret" methods.
+ */
+ @Value.Redacted
+ Optional<Secret> clientSecret();
+
+ /**
+ * Space-separated list of scopes to include in each request to the OAuth2 server. Optional,
+ * defaults to empty (no scopes).
+ *
+ * <p>The scope names will not be validated by the OAuth2 client; make sure they are valid
+ * according to <a href="https://datatracker.ietf.org/doc/html/rfc6749#section-3.3">RFC 6749
+ * Section 3.3</a>.
+ */
+ Optional<Scope> scope();
Review Comment:
> not parsing out the scopes
In the Nimbus API, `Scope` can contain multiple scope tokens, e.g. `new Scope("catalog", "offline_access")`.
> This is also exposing the Nimbus representation of scope, which we probably don't want to embed in the API.
True. This intersects with your other comment about using a single package with default package-private visibility. I think that if we flatten the package hierarchy and switch to package-private methods, it's fine to expose Nimbus API, wdyt?
My fear if we forbid Nimbus API in configuration classes is that we are going to end up duplicating a few classes from the Nimbus API.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "danielcweeks (via GitHub)" <gi...@apache.org>.
danielcweeks commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3163123027
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
Review Comment:
We shouldn't rely on parsed time durations like this when there's an explicit and more readable option: `Duration.ofHours(1);`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#discussion_r3269851412
##########
core/src/main/java/org/apache/iceberg/rest/auth/oauth2/config/TokenRefreshConfig.java:
##########
@@ -0,0 +1,123 @@
+/*
+ * 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.iceberg.rest.auth.oauth2.config;
+
+import com.nimbusds.oauth2.sdk.GrantType;
+import java.time.Duration;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import org.apache.iceberg.rest.auth.oauth2.OAuth2Config;
+import org.immutables.value.Value;
+
+/** Configuration properties for the token refresh feature. */
+@Value.Immutable
+public interface TokenRefreshConfig {
+
+ String PREFIX = OAuth2Config.PREFIX + "token-refresh.";
+
+ String ENABLED = PREFIX + "enabled";
+ String TOKEN_EXCHANGE_ENABLED = PREFIX + "token-exchange-enabled";
+ String ACCESS_TOKEN_LIFESPAN = PREFIX + "access-token-lifespan";
+ String SAFETY_MARGIN = PREFIX + "safety-margin";
+
+ Duration DEFAULT_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT1H");
+ Duration DEFAULT_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ Duration MIN_ACCESS_TOKEN_LIFESPAN = Duration.parse("PT15S");
+ Duration MIN_SAFETY_MARGIN = Duration.parse("PT10S");
+
+ /**
+ * Whether to enable token refresh. If enabled, the OAuth2 client will automatically refresh its
+ * access token when it expires. If disabled, the OAuth2 client will only fetch the initial access
+ * token, but won't refresh it. Defaults to {@code true}.
+ */
+ @Value.Default
+ default boolean enabled() {
+ return true;
+ }
+
+ /**
+ * Whether to use the token exchange grant to refresh tokens.
+ *
+ * <p>When enabled, the token exchange grant will be used to refresh the access token, if no
+ * refresh token is available.
+ *
+ * <p>Optional, defaults to {@code true} if the initial grant is {@link
+ * GrantType#CLIENT_CREDENTIALS}.
+ */
+ Optional<Boolean> tokenExchangeEnabled();
+
+ /**
+ * Default access token lifespan; if the OAuth2 server returns an access token without specifying
+ * its expiration time, this value will be used.
+ *
+ * <p>Optional, defaults to {@link #DEFAULT_ACCESS_TOKEN_LIFESPAN}. Must be a valid <a
+ * href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration accessTokenLifespan() {
+ return DEFAULT_ACCESS_TOKEN_LIFESPAN;
+ }
+
+ /**
+ * Refresh safety margin to use; a new token will be fetched when the current token's remaining
+ * lifespan is less than this value. Optional, defaults to {@link #DEFAULT_SAFETY_MARGIN}. Must be
+ * a valid <a href="https://en.wikipedia.org/wiki/ISO_8601#Durations">ISO-8601 duration</a>.
+ */
+ @Value.Default
+ default Duration safetyMargin() {
Review Comment:
Renamed to `prefetch()` and added `jitter()`. PTAL.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] REST, OAuth2: OAuth2 AuthManager v2 - Part 1: Configuration [iceberg]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on PR #15703:
URL: https://github.com/apache/iceberg/pull/15703#issuecomment-5831730459
Not stale.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org