Repository: apache/iceberg (language: python)

Issue:
REST: honor OAuth config sent by the server
As mandated by the spec, in case the config endpoint overrides any auth-specific property, the overridden values should be taken into account when creating the catalog auth session instance.


Discussion / hints:
Maybe the confusion is due to the `config` endpoint is mixing different semantic properties, also related to the `scope`.
For clarity, it would be better to split non authenticated config (`handshake`) from authenticated one (`config`), that's part of the "new" REST Catalog proposal.

I understand @snazy points, but related to the current implementation (as @danielcweeks said), it's confusing.

If I may, I would propose to:
1. the current impl should be improved, I think @adutra change is OK if we add scope configurable to deal with server side push
2. this change (dual config endpoints, auth and non auth) will be part of the "new" REST Catalog proposal

Thoughts ?
> If I may, I would propose to:
> 
>     1. the current impl should be improved, I think @adutra change is OK if we add scope configurable to deal with server side push
> 
>     2. this change (dual config endpoints, auth and non auth) will be part of the "new" REST Catalog proposal

+1
Not a fan of leaving `oauthServerUri` out of the scope of this PR, for the reasons @snazy explained.

Also what about `optionalOAuthParams`? For now it contains only resource and audience, but we plan to add more config properties in the future, and these would naturally fit inside `optionalOAuthParams`.

If `optionalOAuthParams` is not configurable by the server, this means that virtually _nothing_ related to auth is configurable through the server, which imho would make configuring client auth quite a complex task as more options could be added in the future.
I'm surprised by this PR because I don't think that the auth properties should be overridden by a REST service. I'm not sure about it, but it sounds like @snazy seems to agree when he says that the OAuth2 endpoint shouldn't be part of the REST spec, when @flyrain points out that you should authenticate before hitting the config endpoint, and @danielcweeks points out that credentials are specifically called out in OAuth2 as client controlled.

@adutra, what are you trying to accomplish here? Is this needed for some use case that you could, perhaps, explain?

Right now, I see the assertion that the REST spec requires this, which I don't agree with (but maybe?) and also @adutra saying things like "I think it makes sense for the client to apply all configuration received" -- is that the basis for this?

I think I need to understand the motivation. Otherwise, I think the argument that auth (including the auth endpoint) should be client-side configuration only. This could introduce a way to get the client's credential -- that is, secret -- and that's something we should be _very_ careful about.
@rdblue I have no other intention than the one of bringing the spec in line with the current implementation.

There are two areas where spec and implementation differ with respect to authentication:

1. Authentication properties from the config endpoint. The OpenAPI Spec for the config endpoint currently states:

> Catalog configuration is constructed by setting the defaults, then client-provided configuration, and finally
overrides. The final property set is then used to configure the catalog.

As we've seen, this is currently not true for many authentication properties, including `credential` and
`oauth2-server-uri`, to name a few. But it holds true for `token`.

2. Authentication properties returned in a `LoadTableResult` structure. The OpenAPI Spec for the structure currently 
states:

> The `config` map returns table-specific configuration for the table's resources [..] The following configurations
should be respected by clients: [..] `token`.

Note that `credential`, for instance, is not listed there, and yet _it is currently honored by the client_. IOW, if the 
server sends a `credential` in the `config` map, [the client will use it to authenticate](https://github.com/apache/iceberg/blob/7caedf85954c78fb3212414b924ff29302990b2a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java#L945). There is even a test for that: `TestRESTCatalog.testTableCredential`.

All of this tends to suggest that authentication is not handled consistently across the board. The `token` property is
generally respected, but `credential` is not. The `oauth2-server-uri` property is not respected at all.

Such inconsistencies are confusing and make it hard to reason about the system. My intention was to bring the spec in
line with the current implementation by honoring `credential` and `oauth2-server-uri`, among others, from the config
endpoint.

Let me finish by addressing the security concerns you raised.

While I would generally agree with you that authentication properties should not be shared between client and resource
server, I think we are past that point. The client already happily accepts whatever `token` the resource server gives 
it, and in some cases, it accepts even the resource server's `credential` too, and uses those to authenticate. _This is 
in complete contradiction with OAuth 2.0 principles_. Moreover, this behavior is validated by tests, so it's not 
something that we can change now.

This brings me to the tacit conclusion that client and server must form a sort of trusted pair, functioning in a trusted 
environment. In that situation, I don't see why the client could not obtain all of its authentication configuration from 
the server, rather than just some portions of it.
> My intention was to bring the spec in
line with the current implementation by honoring credential and oauth2-server-uri, among others, from the config
endpoint.

I don't believe this is a safe and raises a huge security concern for me.  A client should never be in a situation where it is sending credentials or valid tokens to an server that the client did not explicitly designate.  This change allows redirecting the auth server which should expose sensitive information to the wrong party.
> This change allows redirecting the auth server which should expose sensitive information to the wrong party.

Why should a (malicious) Iceberg REST endpoint do the more complex redirect-dance, if it can get the nearly clear-text credentials due to the `/v1/oauth/tokens` route introduced by #4771? This change tries to _mitigate_ that security issue (clear text credentials) by telling the client to use the _correct_ oauth endpoint - nothing else.
> Why should a (malicious) Iceberg REST endpoint do the more complex redirect-dance, if it can get the nearly clear-text credentials due to the `/v1/oauth/tokens` route introduced by #4771? This change tries to _mitigate_ that security issue (clear text credentials) by telling the client to use the _correct_ oauth endpoint - nothing else.

@snazy Users can override the oauth server, so for example if they set the auth server to be an Okta endpoint and they have credentials to authenticate with Okta, the resulting token from the client credential flow would go to the REST server (this is ok).  However, if the REST server then redirects the client somewhere else, any subsequent operations (included additional credential flows) would send those credentials to a second party (this is not ok). 

The rest server should not be redirecting a client configured auth server.  That's not safe.  The client should be fully in control of which auth server it uses.
@adutra, thanks for noticing and pointing out the inconsistencies. I do think it's important to address those. In particular, I did not realize that the table session that was introduced recently used the `newSession` codepath that uses `credential`.

(I also understand your reading of the `config` endpoint, but I don't think that implies that configs like the OAuth2 URI should be handled that way.)

We need to address these cases, but I think that this PR goes the wrong direction: instead of allowing auth properties from the config and table routes, we actually need to be more strict. A table session should never use a credential from the catalog service.

I think it would help if we had a common understanding of the difference between a token and a credential. A credential is a pre-established ID and secret pair that is [used for authentication](https://datatracker.ietf.org/doc/html/rfc6749#section-2.3). A credential is controlled and owned by the client and is typically long-lived, like a username/password. A token is similar because it can be used for authentication (i.e. bearer auth) but tokens are short-lived and can contain a lot more information added by the service that is validated by signing the token.

In the REST protocol with OAuth2, `credential` is used for the first authentication step, which is to use the client credentials flow to exchange it for a token with the OAuth2 endpoint (now configurable). Both the endpoint and the credential are user-controlled because the credential is a long-lived secret and the endpoint must be trusted to receive that secret. The token that is returned to the client has a much shorter lifespan, which is why it is okay to share the token with the catalog routes and not only the OAuth2 endpoint.

Another reason why tokens are handled differently is that tokens can contain additional information that is signed by the service. That additional information can be used to check something once and use the result for the lifetime of the token. When designing the REST protocol, we thought that it was likely that tokens would be used for this purpose, so we built a couple of ways to exchange an auth token (from OAuth2) for a more specific or narrow token.

For example, the service could verify that the client is not coming from a blocked IP range. Checking IP ranges is expensive, so you'd want to do it once and produce a token that shows the incoming IP has been validated. As long as the token's IP claim is the same as the incoming IP, you can trust that the validation was done and not repeat the expensive check.

As you can see, there's a good reason to handle `token` and `credential` differently. Tokens must be sent to the catalog service for bearer auth and implementations are free to issue more narrow tokens in reasonable places. However, credentials should be more protected and only sent to the correct OAuth2 endpoint.

For this PR, the question is how to handle `credential`, `scope`, and `oauth2-server-uri`. For those, I think the current handling is correct.
* `credential` -- This is a pre-established shared secret. It should never be supplied by the catalog service, especially when `token` is suitable for any purpose that `credential` might be used for.
* `scope` -- I don't think this one makes sense. Why would a service tell the client to request more capabilities?
* `oauth2-server-uri` -- If the service can send a different URI, then it can tell the client to send its credential to a different endpoint. I don't think there's any value to that, given that auth needs to happen before `config` . This would just increase the surface area for attacks, which we don't want to do however unlikely they may be.

In the end, allowing these to be overridden by catalog endpoints would weaken security and introduce extra requests (keep requirements simple!). I don't think it is a good idea to move forward with this PR. That said, good job catching that `credential` from the table route would be used in a session. Can you open a PR to disallow that?
Hi @rdblue, thank you for your detailed answer. 

I am really sorry that this PR, that I thought would be a fairly consensual one, eventually cracked open a can of worms that I did not intend to open.

I'm pleased to realize that, in spite of some apparent divergences, we all seem to agree that the current handling of authentication suffers from a few loopholes – such as the `credential` passing from table routes – and that we should strive to make things more strict, not less. That is the mindset that I think we all should strive to adopt, when it comes to security.

That being said, I agree to close this PR and open another one to fix the table routes instead.

There remains, however, the broader question, mentioned a few times above, of a better separation of concerns between the conceptual OAuth 2.0 roles of "authorization" and "resource" servers. But this PR has never had the ambition of tackling that, so I'd like to request that we resume that discussion later in a more appropriate place, like our mailing list. 

Fix patch:
diff --git a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
index da40d4c3ae31..78f8e5a316a0 100644
--- a/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
+++ b/core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java
@@ -68,6 +68,7 @@
 import org.apache.iceberg.relocated.com.google.common.base.Preconditions;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableList;
 import org.apache.iceberg.relocated.com.google.common.collect.ImmutableMap;
+import org.apache.iceberg.relocated.com.google.common.collect.ImmutableSet;
 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.auth.AuthConfig;
@@ -124,6 +125,13 @@ public class RESTSessionCatalog extends BaseViewSessionCatalog
           OAuth2Properties.SAML2_TOKEN_TYPE,
           OAuth2Properties.SAML1_TOKEN_TYPE);
 
+  // Auth-related properties that are allowed to be passed to the table session
+  private static final Set<String> TABLE_SESSION_ALLOW_LIST =
+      ImmutableSet.<String>builder()
+          .add(OAuth2Properties.TOKEN)
+          .addAll(TOKEN_PREFERENCE_ORDER)
+          .build();
+
   private final Function<Map<String, String>, RESTClient> clientBuilder;
   private final BiFunction<SessionContext, Map<String, String>, FileIO> ioBuilder;
   private Cache<String, AuthSession> sessions = null;
@@ -922,7 +930,14 @@ private FileIO tableFileIO(SessionContext context, Map<String, String> config) {
   }
 
   private AuthSession tableSession(Map<String, String> tableConf, AuthSession parent) {
-    Pair<String, Supplier<AuthSession>> newSession = newSession(tableConf, tableConf, parent);
+    Map<String, String> credentials = Maps.newHashMapWithExpectedSize(tableConf.size());
+    for (String prop : tableConf.keySet()) {
+      if (TABLE_SESSION_ALLOW_LIST.contains(prop)) {
+        credentials.put(prop, tableConf.get(prop));
+      }
+    }
+
+    Pair<String, Supplier<AuthSession>> newSession = newSession(credentials, tableConf, parent);
     if (null == newSession) {
       return parent;
     }


Test patch:
diff --git a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
index 95380424e7ad..db02c5e7a8a9 100644
--- a/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
+++ b/core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java
@@ -902,9 +902,9 @@ public void testTableCredential(String oauth2ServerUri) {
     testTableAuth(
         "catalog",
         ImmutableMap.of("urn:ietf:params:oauth:token-type:id_token", "id-token"),
-        ImmutableMap.of("credential", "table-user:secret"),
+        ImmutableMap.of("credential", "table-user:secret"), // will be ignored
+        ImmutableMap.of("Authorization", "Bearer token-exchange-token:sub=id-token,act=catalog"),
         ImmutableMap.of("Authorization", "Bearer token-exchange-token:sub=id-token,act=catalog"),
-        ImmutableMap.of("Authorization", "Bearer client-credentials-token:sub=table-user"),
         oauth2ServerUri);
   }
 
@@ -1354,9 +1354,9 @@ public void testTableAuth(
             eq(expectedContextHeaders),
             any());
 
-    // if the table returned a bearer token, there will be no token request
-    if (!tableConfig.containsKey("token")) {
-      // client credentials or token exchange to get a table token
+    // if the table returned a bearer token or a credential, there will be no token request
+    if (!tableConfig.containsKey("token") && !tableConfig.containsKey("credential")) {
+      // token exchange to get a table token
       Mockito.verify(adapter, times(1))
           .execute(
               eq(HTTPMethod.POST),
@@ -1368,38 +1368,40 @@ public void testTableAuth(
               any());
     }
 
-    // automatic refresh when metadata is accessed after commit
-    Mockito.verify(adapter)
-        .execute(
-            eq(HTTPMethod.GET),
-            eq("v1/namespaces/ns/tables/table"),
-            any(),
-            any(),
-            eq(LoadTableResponse.class),
-            eq(expectedTableHeaders),
-            any());
-
-    // load table from catalog
-    Mockito.verify(adapter)
-        .execute(
-            eq(HTTPMethod.GET),
-            eq("v1/namespaces/ns/tables/table"),
-            any(),
-            any(),
-            eq(LoadTableResponse.class),
-            eq(expectedContextHeaders),
-            any());
+    if (expectedContextHeaders.equals(expectedTableHeaders)) {
+      // load table from catalog + refresh loaded table
+      Mockito.verify(adapter, times(2))
+          .execute(
+              eq(HTTPMethod.GET),
+              eq("v1/namespaces/ns/tables/table"),
+              any(),
+              any(),
+              eq(LoadTableResponse.class),
+              eq(expectedTableHeaders),
+              any());
+    } else {
+      // load table from catalog
+      Mockito.verify(adapter)
+          .execute(
+              eq(HTTPMethod.GET),
+              eq("v1/namespaces/ns/tables/table"),
+              any(),
+              any(),
+              eq(LoadTableResponse.class),
+              eq(expectedContextHeaders),
+              any());
 
-    // refresh loaded table
-    Mockito.verify(adapter)
-        .execute(
-            eq(HTTPMethod.GET),
-            eq("v1/namespaces/ns/tables/table"),
-            any(),
-            any(),
-            eq(LoadTableResponse.class),
-            eq(expectedTableHeaders),
-            any());
+      // refresh loaded table
+      Mockito.verify(adapter)
+          .execute(
+              eq(HTTPMethod.GET),
+              eq("v1/namespaces/ns/tables/table"),
+              any(),
+              any(),
+              eq(LoadTableResponse.class),
+              eq(expectedTableHeaders),
+              any());
+    }
   }
 
   @ParameterizedTest