Introduce 419 AuthenticationTimeoutResponse on token expiration

[liamzwbao]

Description

This PR introduces the 419 AuthenticationTimeoutResponse on token expiration to conform to the REST catalog specification:

AuthenticationTimeoutResponse:
  description:
    Credentials have timed out. If possible, the client should refresh credentials and retry.
  content:
    application/json:
      schema:
        $ref: '#/components/schemas/IcebergErrorResponse'
      example: {
          "error": {
            "message": "Credentials have timed out",
            "type": "AuthenticationTimeoutException",
            "code": 419
          }
        }

Fixes #791

Testing

Covered by testTokenExpiry in the integration tests PolarisManagementServiceIntegrationTest.java

[liamzwbao]

Note to reviewers: Not sure whether this file is in the right folder, so put it close to PolarisExceptionMapper for now. Appreciate suggestions

[eric-maynard]

This sounds more like the auth call itself has timed out, can we try something like AuthenticationTokenExpirationException?

[liamzwbao]

The name AuthenticationTokenExpirationException makes sense, but AuthenticationTimeoutException follows the REST Spec in Iceberg and Polaris. I think it would be better to keep it consistent

[eric-maynard]

Same comment as above applies to the name here

[dimas-b]

419, as far as I can tell it is not an official error code. It is certainly not an "Authentication Timeout".

[liamzwbao]

right, 419 is not official so we cannot find it in the Jarkarta Response class. The code is from the Iceberg API spec, which is defined in Polaris as well

[dimas-b]

Thanks for you contribution, @liamzwbao !

I understand the intention to support existing Iceberg REST clients, but I do not think this approach is sound in the bigger authentication scope.

There were previous discussions about supporting external IdPs in Polaris. In that case it would be impossible for Polaris to provide the 419 response consistently in all cases. Therefore, I think it is preferable to keep the standard 401 response.

Also, apache/iceberg#12197 greatly expands Iceberg REST client's capabilities in the authentication area. After that PR, clients should be able to handle more complex authentication flows with OAuth2-based token refresh, so there will hopefully be no need for 419 responses.

I propose to discuss this matter on the dev mailing list to achieve clarity and consensus.

[dimas-b]

419, as far as I can tell it is not an official error code. It is certainly not an "Authentication Timeout".

[dimas-b]

I do not think 419 is a correct response for expired credentials. I believe 401 (with a challenge) is appropriate in this case.

[liamzwbao]

This is debatable but it follows the request in #791

[flyrain]

According to the Iceberg REST specification, 419 is the correct response. Polaris needs to support it, despite some reasons—of which I am not aware—that suggest otherwise. Could someone explain why this response is considered incorrect?

[dimas-b]

1. From a general security perspective, I believe is preferable to avoid disclosing unnecessary information. A response that specifically indicates the token has expired indicated that aside from the expiry date the token was valid. This can be used as a leverage in attacks.

2. There appears to be prior usage of the 419 repose code by Laravel Framework with a different meaning related to "Cross-Site Request Forgery (CSRF)" tokens.

3. 419 is generally not used by any well-known IdPs, so it will be impossible for Polaris to produce consistent response code when external IdPs are supported.

[flyrain]

Thanks for chiming in. I think this does deserve a discussion in the dev list.

I generally agree that expose whether a token is invalid isn't a good practice. This PR doesn't make Polaris worse in terms of security concern per my following analysis.

1. If third‐party IdPs don’t reveal token expiration details, then Polaris won’t either.
2. If third‐party IdPs do reveal that a token is expired, then a 419 response from Polaris would mirror that behavior, not introduce any new risk.
3. If users interact with the /token endpoint (which is for testing/POC), then yes, it may reveal expiration details—but that endpoint is not meant for production use. I believe this is the case described in Introduce 419 AuthenticationTimeoutResponse instead of using 401 UnauthorizedResponse on token expiration #791. @sungwy can correct me if I'm wrong.

So the only potential issue is scenario 2, but since the IdP is already leaking that info, Polaris doesn't make the situation worse.

[liamzwbao]

I understand the intention to support existing Iceberg REST clients, but I do not think this approach is sound in the bigger authentication scope.

There were previous discussions about supporting external IdPs in Polaris. In that case it would be impossible for Polaris to provide the 419 response consistently in all cases. Therefore, I think it is preferable to keep the standard 401 response.

Also, apache/iceberg#12197 greatly expands Iceberg REST client's capabilities in the authentication area. After that PR, clients should be able to handle more complex authentication flows with OAuth2-based token refresh, so there will hopefully be no need for 419 responses.

Thanks for the information, @dimas-b! Given this, do you think there's a better approach, or should we just wait for the completion of apache/iceberg#12197 to close #791. If it's the latter, I'll close this PR.

That said, I noticed that the token-broker config is missing in the IT tests, which affects the four tests for JWT. As a result, they aren't validating the expected behavior, since all of them are just throwing AlgorithmMismatchException instead of the intended exceptions like TokenExpiredException or InvalidClaimException. The tests still pass because they all map to the same response code 401.

How about I push a small PR to fix the config issue?

cc @flyrain

[liamzwbao]

The name AuthenticationTokenExpirationException makes sense, but AuthenticationTimeoutException follows the REST Spec in Iceberg and Polaris. I think it would be better to keep it consistent

[liamzwbao]

right, 419 is not official so we cannot find it in the Jarkarta Response class. The code is from the Iceberg API spec, which is defined in Polaris as well

[liamzwbao]

This is debatable but it follows the request in #791

[flyrain]

There were previous discussions about supporting external IdPs in Polaris. In that case it would be impossible for Polaris to provide the 419 response consistently in all cases. Therefore, I think it is preferable to keep the standard 401 response.

Also, apache/iceberg#12197 greatly expands Iceberg REST client's capabilities in the authentication area. After that PR, clients should be able to handle more complex authentication flows with OAuth2-based token refresh, so there will hopefully be no need for 419 responses.

AFAIK, apache/iceberg#12197 doesn't change the Iceberg REST spec to remove 419. Or does it? Even it does, Polaris still needs to support existing clients which expect 419. Also the serious of apache/iceberg#12197 is only for Java clients, other clients aren't covered.

That said, I'm not against keeping 401 response if there are strong reasons.

[dimas-b]

Polaris still needs to support existing clients which expect 419

I propose to discuss this need on the dev ML or on #791, but I think the dev ML is preferable.

[dimas-b]

Given the related Iceberg API discussion, would it be ok to close this PR?

https://lists.apache.org/thread/443skhqr59j3fj0ovg4tyxh9d4f4gysc

[snazy]

Given the related Iceberg API discussion, would it be ok to close this PR?

+1