Reorder filters and let RealmIdResolver throw UnresolvableRealmException #848

adutra · 2025-01-22T17:14:10Z

Fixes #819.

Summary of changes:

Vert.x route filters were converted to JAX-RS ContainerRequestFilter
New RealmIdFilter introduced
RealmIdResolver now can throw UnresolvableRealmException => maps to HTTP 404
Added tests in PolarisApplicationIntegrationTest
Fixed default realm name in tests
Priorities were revisited, new order of execution is shown below:

Filter	Order	Priority
`RealmIdFilter`	1	900
`QuarkusLoggingMDCFilter`	2	901
`QuarkusTracingFilter`	3	902
`PolarisPrincipalAuthenticatorFilter`	4	1000
`PolarisPrincipalRolesProviderFilter`	5	1001
`RateLimiterFilter`	6	5000

adutra · 2025-01-24T10:31:25Z

This is now ready for review.

dimas-b · 2025-01-24T21:22:29Z

integration-tests/src/main/java/org/apache/polaris/service/it/env/PolarisApiEndpoints.java

+   * The header name for the realm ID. Tests must make sure that Polaris is configured with this
+   * header name.
+   */
+  public static String REALM_HEADER = "Polaris-Realm";


Would you mind moving this to a (new) method on PolarisServerManager or Server so that it is possible to customize?

dimas-b · 2025-01-24T21:25:50Z

...ests/src/main/java/org/apache/polaris/service/it/test/PolarisApplicationIntegrationTest.java

+      assertThat(response.getStatus()).isEqualTo(Status.OK.getStatusCode());
+      Catalogs roles = response.readEntity(Catalogs.class);
+      if ("POLARIS".equals(realmId)) {
+        assertThat(roles.getCatalogs()).extracting(Catalog::getName).contains(internalCatalogName);


nit: maybe make internalCatalogName a parameter to this test?

dimas-b · 2025-01-24T21:35:01Z

service/common/src/main/java/org/apache/polaris/service/context/RealmIdFilter.java

+    try {
+      realmId = resolveRealmContext(rc);
+    } catch (NotAuthorizedException e) {
+      rc.abortWith(Response.status(Status.UNAUTHORIZED).build());


I'm not sure about 401 in this context. I believe it generally means your credentials are not valid, which is not applicable to failures in realm resolution.

I'm thinking that a 404 may fit better. For example, GH gives 404 on accessing repositories (i.e. realms) to which the user has no access.

Makes sense, i will update the PR title and description accordingly.

quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/QuarkusProducers.java

dimas-b · 2025-01-24T21:44:08Z

service/common/src/main/java/org/apache/polaris/service/context/DefaultRealmIdResolver.java

      String requestURL, String method, String path, Map<String, String> headers) {

    String realm;

    if (headers.containsKey(configuration.headerName())) {
      realm = headers.get(configuration.headerName());
      if (!configuration.realms().contains(realm)) {
-        throw new IllegalArgumentException("Unknown realm: " + realm);
+        throw new NotAuthorizedException("Unknown realm: " + realm);
      }
    } else {
      realm = configuration.defaultRealm();


I think it could be valuable to cover this with an explicit config flag. If realms are used in a particular deployment, it is probably not wise to allow potentially malicious clients to try sending requests at the default realm without knowing its ID.

I completely agree, I think that having a default realm is a potential security issue.

dimas-b

Nice improvement to request filters! Thanks, @adutra !

As far as I can tell this change is backward compatible at the REST API / HTTP level.

snazy · 2025-01-27T17:43:00Z

integration-tests/src/main/java/org/apache/polaris/service/it/env/PolarisApiEndpoints.java

@@ -49,4 +49,8 @@ public URI managementApiEndpoint() {
  public String realm() {
    return realm;
  }
+
+  public String realmHeader() {


Maybe rename to realmHeaderName() + realmId() (latter for consistency w/ Server)?

snazy · 2025-01-27T17:44:27Z

integration-tests/src/main/java/org/apache/polaris/service/it/env/Server.java

@@ -36,4 +39,8 @@ public interface Server extends AutoCloseable {
  URI baseUri();

  ClientPrincipal adminCredentials();
+
+  default String realmHeader() {


Suggested change

default String realmHeader() {

default String realmHeaderName() {

snazy · 2025-01-27T17:50:10Z

service/common/src/main/java/org/apache/polaris/service/context/RealmContextConfiguration.java

+   * <p>Note: this is actually only enforced when using {@link DefaultRealmIdResolver}. When using
+   * {@link TestRealmIdResolver}, this setting is ignored.


Suggested change

* <p>Note: this is actually only enforced when using {@link DefaultRealmIdResolver}. When using

* {@link TestRealmIdResolver}, this setting is ignored.

* <p>Note: this setting is only enforced in production setups.

snazy · 2025-01-27T17:50:31Z

service/common/src/main/java/org/apache/polaris/service/context/RealmContextConfiguration.java

+   * <p>Note: this is actually only enforced when using {@link DefaultRealmIdResolver}. When using
+   * {@link TestRealmIdResolver}, this setting is ignored.
+   */
+  boolean requireHeader();


Should this be true by default?

Defaulting to true would be a behaviour change at the REST/HTTP level. I'd prefer to keep old behaviour in this PR and change defaults later.

Yeah changing this to true would break downstreams. But we can mention this in the "Configuring for production" docs.

snazy · 2025-01-27T17:53:03Z

service/common/src/main/java/org/apache/polaris/service/context/RealmIdFilter.java

+            rc.getHeaders().entrySet().stream()
+                .collect(
+                    HashMap::new,
+                    (m, e) -> m.put(e.getKey(), e.getValue().getFirst()),
+                    Map::putAll));


Wonder if it makes more sense to change RealmIdResolver to expect a Function<String, String> rather than a "full blown map" (and use request::getHeader here).

dimas-b · 2025-01-28T03:59:10Z

@collado-mike : any concerns from your POV?

adutra force-pushed the filter-order branch 2 times, most recently from e542f5c to 9c45494 Compare January 22, 2025 17:50

adutra mentioned this pull request Jan 23, 2025

Rethink order of JAX-RS filters in Polaris #819

Open

adutra force-pushed the filter-order branch from 9c45494 to 73ef947 Compare January 24, 2025 10:28

adutra changed the title ~~[WIP] Reorder filters and let RealmIdResolver throw NotAuthorized~~ Reorder filters and let RealmIdResolver throw NotAuthorized Jan 24, 2025

adutra marked this pull request as ready for review January 24, 2025 10:29

adutra requested review from ashvina, dennishuo, dimas-b, eric-maynard, jackye1995, jbonofre, vvcephei, collado-mike, snazy, RussellSpitzer, takidau, flyrain and ebyhr as code owners January 24, 2025 10:29

dimas-b reviewed Jan 24, 2025

View reviewed changes

adutra added 4 commits January 27, 2025 13:47

Reorder filters and let RealmIdResolver throw NotAuthorized

352c536

review

31af1c5

return 404 instead of 401

dc96578

optionally require realm header

9e932b2

adutra force-pushed the filter-order branch from 73ef947 to 9e932b2 Compare January 27, 2025 13:32

adutra changed the title ~~Reorder filters and let RealmIdResolver throw NotAuthorized~~ Reorder filters and let RealmIdResolver throw UnresolvableRealmException Jan 27, 2025

dimas-b approved these changes Jan 27, 2025

View reviewed changes

snazy reviewed Jan 27, 2025

View reviewed changes

make realmId() a default method like realmHeader()

bc9452d

adutra requested a review from MonkeyCanCode as a code owner January 27, 2025 17:58

adutra added 2 commits January 27, 2025 19:03

review

cded71a

change signature of resolveRealmId

65c0cce

snazy approved these changes Jan 27, 2025

View reviewed changes

change docs

b3daca9

dimas-b approved these changes Jan 27, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorder filters and let RealmIdResolver throw UnresolvableRealmException #848

Reorder filters and let RealmIdResolver throw UnresolvableRealmException #848

adutra commented Jan 22, 2025 •

edited

Loading

adutra commented Jan 24, 2025

dimas-b Jan 24, 2025

dimas-b Jan 24, 2025

dimas-b Jan 24, 2025

adutra Jan 27, 2025

dimas-b Jan 24, 2025

adutra Jan 27, 2025

dimas-b left a comment

snazy Jan 27, 2025

snazy Jan 27, 2025

snazy Jan 27, 2025

snazy Jan 27, 2025

dimas-b Jan 27, 2025

snazy Jan 27, 2025

adutra Jan 27, 2025

snazy Jan 27, 2025

dimas-b commented Jan 28, 2025

	default String realmHeader() {
	default String realmHeaderName() {

		* <p>Note: this is actually only enforced when using {@link DefaultRealmIdResolver}. When using
		* {@link TestRealmIdResolver}, this setting is ignored.

	* <p>Note: this is actually only enforced when using {@link DefaultRealmIdResolver}. When using
	* {@link TestRealmIdResolver}, this setting is ignored.
	* <p>Note: this setting is only enforced in production setups.

Reorder filters and let RealmIdResolver throw UnresolvableRealmException #848

Are you sure you want to change the base?

Reorder filters and let RealmIdResolver throw UnresolvableRealmException #848

Conversation

adutra commented Jan 22, 2025 • edited Loading

adutra commented Jan 24, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dimas-b commented Jan 28, 2025

adutra commented Jan 22, 2025 •

edited

Loading