Skip to content

Commit 5c43ca8

Browse files
authored
[#11409] fix(idp): Validate incompatible simple mode at startup (#11497)
### What changes were proposed in this pull request? - Add `IdpConfigurationValidator` in `org.apache.gravitino.idp.config` to validate server configuration before the built-in IdP plugin starts. - Reject startup when `gravitino.authorization.enable=true` and `gravitino.authenticators` includes `simple` (including the default value). - Invoke validation from `IdpRESTFeature.configure()` before IdP registers HTTP Basic authentication. - Add unit tests covering explicit `simple`, default authenticators, and compatible OAuth configuration. ### Why are the changes needed? Enabling built-in IdP (`gravitino.server.rest.extensionPackages = org.apache.gravitino.idp.web.rest.feature`) together with Simple authentication and authorization leads to broken Web UI login: the IdP plugin injects HTTP Basic authentication ahead of Simple, while Web v2 sends username-only Basic credentials. This change fails fast at startup with a clear error instead of leaving operators with a running server and a non-functional UI. Fix: #11409 ### Does this PR introduce _any_ user-facing change? 1. Server startup now fails with `IllegalStateException` when built-in IdP is enabled with authorization and Simple authentication (explicit or default). 2. Operators must remove `simple` from `gravitino.authenticators` ### How was this patch tested? - `./gradlew spotlessApply` - `./gradlew :plugins:idp-basic:test -PskipITs -PskipDockerTests=true --tests org.apache.gravitino.idp.config.TestIdpConfigurationValidator`
1 parent b3c6f92 commit 5c43ca8

8 files changed

Lines changed: 156 additions & 8 deletions

File tree

design-docs/gravitino-local-authentication.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -319,6 +319,9 @@ fresh Gravitino deployment.
319319
gravitino.authorization.serviceAdmins=admin1,admin2
320320
```
321321

322+
Built-in IdP is incompatible with the `simple` authenticator. When the `idp-basic` plugin is
323+
enabled, `gravitino.authenticators` must not include `simple`.
324+
322325
2. Export the initial service admin password before starting Gravitino:
323326

324327
```bash

docs/open-api/idp/idp.yaml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,10 @@ paths:
2424
tags:
2525
- IDP
2626
summary: Add built-in IDP user
27-
description: Creates a built-in IDP user with the given username and password.
27+
description: >
28+
Creates a built-in IDP user with the given username and password.
29+
Requires the `idp-basic` plugin and built-in IdP Basic authentication.
30+
`gravitino.authenticators` must not include `simple`.
2831
operationId: addIdpUser
2932
requestBody:
3033
required: true

docs/open-api/idp/openapi.yaml

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,10 @@ info:
2525
version: 1.3.0-SNAPSHOT
2626
description: |
2727
OpenAPI specification for built-in IDP user and group management APIs exposed
28-
by the `idp-basic` plugin when the `basic` authenticator is enabled.
28+
by the `idp-basic` plugin. Clients authenticate with Basic credentials
29+
validated against built-in IdP user metadata. Enable the plugin via
30+
`gravitino.server.rest.extensionPackages`; `gravitino.authenticators` must not
31+
include `simple` when IdP is enabled.
2932
3033
servers:
3134
- url: "{scheme}://{host}:{port}/{basePath}"

docs/security/how-to-authenticate.md

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,9 @@ This example shows how to enable built-in Basic authentication.
366366

367367
- Gravitino distribution package (includes the idp-basic plugin on the server classpath)
368368

369+
Built-in IdP is **incompatible** with the `simple` authenticator (the default). When the
370+
`idp-basic` plugin is enabled, `gravitino.authenticators` must not include `simple`.
371+
369372
**Configuration:**
370373

371374
Append the following to `conf/gravitino.conf`:

docs/security/how-to-use-built-in-idp.md

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,11 @@ Before you call `/api/idp/*`, ensure the following:
3131
gravitino.server.rest.extensionPackages = org.apache.gravitino.idp.web.rest.feature
3232
```
3333

34-
2. **Service admin passwords** — Built-in IDP requires every username in
34+
2. **Server authenticator** — Built-in IdP is **incompatible** with the `simple` authenticator
35+
(the default). When the `idp-basic` plugin is enabled, `gravitino.authenticators` must not
36+
include `simple`.
37+
38+
3. **Service admin passwords** — Built-in IDP requires every username in
3539
`gravitino.authorization.serviceAdmins` to have a password stored in `idp_user_meta` before you
3640
can call management APIs.
3741

@@ -61,9 +65,10 @@ Before you call `/api/idp/*`, ensure the following:
6165

6266
Set service admins in `gravitino.conf` (see also [Prerequisites](#prerequisites)):
6367

64-
| Configuration item | Description | Example |
65-
|-----------------------------------------|-------------------------------------------------------------------------------------|---------|
66-
| `gravitino.authorization.serviceAdmins` | Comma-separated service admin that can call built-in IDP management APIs | `admin` |
68+
| Configuration item | Description | Example |
69+
|-------------------------------------------|-------------------------------------------------------------------------------------|---------|
70+
| `gravitino.server.rest.extensionPackages` | Registers built-in IdP REST APIs | `org.apache.gravitino.idp.web.rest.feature` |
71+
| `gravitino.authorization.serviceAdmins` | Comma-separated service admin that can call built-in IDP management APIs | `admin` |
6772

6873
Example:
6974

plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/feature/IdpRESTFeature.java

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
import org.apache.gravitino.Config;
2727
import org.apache.gravitino.Configs;
2828
import org.apache.gravitino.GravitinoEnv;
29+
import org.apache.gravitino.auth.AuthenticatorType;
2930
import org.apache.gravitino.idp.IdpUserGroupManager;
3031
import org.apache.gravitino.idp.auth.BasicAuthenticator;
3132
import org.apache.gravitino.idp.web.rest.IdpAuthorizationFilter;
@@ -59,6 +60,7 @@ public class IdpRESTFeature implements Feature {
5960
public boolean configure(FeatureContext context) {
6061
GravitinoEnv env = GravitinoEnv.getInstance();
6162
Config config = env.config();
63+
validateConfiguration(config);
6264
registerBasicAuthenticator(config);
6365

6466
try {
@@ -76,6 +78,23 @@ public boolean configure(FeatureContext context) {
7678
return true;
7779
}
7880

81+
/**
82+
* Validates that the server configuration is compatible with the built-in IdP plugin.
83+
*
84+
* @param config The server configuration.
85+
*/
86+
static void validateConfiguration(Config config) {
87+
boolean usesSimple =
88+
config.get(Configs.AUTHENTICATORS).stream()
89+
.anyMatch(name -> AuthenticatorType.SIMPLE.name().equalsIgnoreCase(name.trim()));
90+
if (usesSimple) {
91+
LOG.error(
92+
"Built-in IdP is incompatible with Simple authentication. "
93+
+ "Remove 'simple' from gravitino.authenticators (default is simple).");
94+
System.exit(1);
95+
}
96+
}
97+
7998
private static void registerBasicAuthenticator(Config config) {
8099
List<Authenticator> authenticators = ServerAuthenticator.getInstance().authenticators();
81100
if (authenticators == null) {

plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,13 +30,15 @@
3030
import java.nio.file.Files;
3131
import java.nio.file.Path;
3232
import java.nio.file.Paths;
33+
import java.security.KeyPairGenerator;
3334
import java.util.Base64;
3435
import java.util.List;
3536
import java.util.Map;
3637
import java.util.Set;
3738
import org.apache.commons.lang3.StringUtils;
3839
import org.apache.gravitino.Configs;
3940
import org.apache.gravitino.auth.AuthConstants;
41+
import org.apache.gravitino.auth.AuthenticatorType;
4042
import org.apache.gravitino.dto.responses.ErrorConstants;
4143
import org.apache.gravitino.idp.dto.requests.AddGroupRequest;
4244
import org.apache.gravitino.idp.dto.requests.AddUserRequest;
@@ -48,6 +50,7 @@
4850
import org.apache.gravitino.integration.test.util.BaseIT;
4951
import org.apache.gravitino.integration.test.util.ITUtils;
5052
import org.apache.gravitino.json.JsonUtils;
53+
import org.apache.gravitino.server.authentication.OAuthConfig;
5154
import org.junit.jupiter.api.Assertions;
5255
import org.junit.jupiter.api.BeforeAll;
5356
import org.junit.jupiter.api.Test;
@@ -84,6 +87,11 @@ public void startIntegrationTest() throws Exception {
8487
configs.put(Configs.CACHE_ENABLED.getKey(), String.valueOf(false));
8588
configs.put(Configs.STORE_DELETE_AFTER_TIME.getKey(), String.valueOf(20 * 60 * 1000L));
8689
configs.put(Configs.SERVICE_ADMINS.getKey(), ADMIN);
90+
configs.put(Configs.AUTHENTICATORS.getKey(), AuthenticatorType.OAUTH.name().toLowerCase());
91+
configs.put(OAuthConfig.SERVICE_AUDIENCE.getKey(), "service1");
92+
configs.put(OAuthConfig.DEFAULT_SIGN_KEY.getKey(), oauthPublicSignKey());
93+
configs.put(OAuthConfig.DEFAULT_SERVER_URI.getKey(), "test");
94+
configs.put(OAuthConfig.DEFAULT_TOKEN_PATH.getKey(), "test");
8795
configs.put(
8896
Configs.REST_API_EXTENSION_PACKAGES.getKey(), IdpRESTFeature.IDP_REST_EXTENSION_PACKAGE);
8997
registerCustomConfigs(configs);
@@ -117,8 +125,8 @@ private static void ensureDeployInitialAdminPasswordInDistributionEnv() throws I
117125
@Test
118126
void testIdpAuthorization() throws Exception {
119127
Assertions.assertEquals(200, get("/version", ADMIN, ADMIN_PASSWORD).statusCode());
120-
// No Authorization: simple authenticator allows anonymous access; IdP filter rejects.
121-
assertError(403, get("/idp/users/" + USER1, null, null), ErrorConstants.FORBIDDEN_CODE);
128+
// No Authorization: OAuth rejects the request before the IdP filter runs.
129+
Assertions.assertEquals(401, get("/idp/users/" + USER1, null, null).statusCode());
122130

123131
postUser(USER2, USER_PASSWORD);
124132
assertError(
@@ -371,4 +379,10 @@ private static void assertError(int expectedStatus, HttpResponse<String> respons
371379
private static int errorCode(HttpResponse<String> response) throws Exception {
372380
return JsonUtils.objectMapper().readTree(response.body()).get("code").asInt();
373381
}
382+
383+
private static String oauthPublicSignKey() throws Exception {
384+
KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA");
385+
generator.initialize(2048);
386+
return Base64.getEncoder().encodeToString(generator.generateKeyPair().getPublic().getEncoded());
387+
}
374388
}
Lines changed: 98 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,98 @@
1+
/*
2+
* Licensed to the Apache Software Foundation (ASF) under one
3+
* or more contributor license agreements. See the NOTICE file
4+
* distributed with this work for additional information
5+
* regarding copyright ownership. The ASF licenses this file
6+
* to you under the Apache License, Version 2.0 (the
7+
* "License"); you may not use this file except in compliance
8+
* with the License. You may obtain a copy of the License at
9+
*
10+
* http://www.apache.org/licenses/LICENSE-2.0
11+
*
12+
* Unless required by applicable law or agreed to in writing,
13+
* software distributed under the License is distributed on an
14+
* "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
15+
* KIND, either express or implied. See the License for the
16+
* specific language governing permissions and limitations
17+
* under the License.
18+
*/
19+
20+
package org.apache.gravitino.idp.web.rest.feature;
21+
22+
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
23+
import static org.junit.jupiter.api.Assertions.assertEquals;
24+
import static org.junit.jupiter.api.Assertions.assertThrows;
25+
26+
import com.google.common.collect.Lists;
27+
import org.apache.gravitino.Config;
28+
import org.apache.gravitino.Configs;
29+
import org.apache.gravitino.auth.AuthenticatorType;
30+
import org.junit.jupiter.api.Test;
31+
32+
class TestIdpRESTFeature {
33+
34+
@Test
35+
void testSimpleFails() {
36+
Config config = newConfig(AuthenticatorType.SIMPLE.name().toLowerCase());
37+
38+
SystemExitException exception =
39+
assertThrows(SystemExitException.class, () -> validateWithExitGuard(config));
40+
41+
assertEquals(1, exception.status());
42+
}
43+
44+
@Test
45+
void testDefaultSimpleFails() {
46+
Config config = new Config(false) {};
47+
48+
assertThrows(SystemExitException.class, () -> validateWithExitGuard(config));
49+
}
50+
51+
@Test
52+
void testOAuthOk() {
53+
Config config = newConfig(AuthenticatorType.OAUTH.name().toLowerCase());
54+
55+
assertDoesNotThrow(() -> IdpRESTFeature.validateConfiguration(config));
56+
}
57+
58+
private static Config newConfig(String... authenticators) {
59+
Config config = new Config(false) {};
60+
config.set(Configs.AUTHENTICATORS, Lists.newArrayList(authenticators));
61+
return config;
62+
}
63+
64+
@SuppressWarnings("removal")
65+
private static void validateWithExitGuard(Config config) {
66+
SecurityManager original = System.getSecurityManager();
67+
System.setSecurityManager(
68+
new SecurityManager() {
69+
@Override
70+
public void checkExit(int status) {
71+
throw new SystemExitException(status);
72+
}
73+
74+
@Override
75+
public void checkPermission(java.security.Permission perm) {
76+
// Allow test execution.
77+
}
78+
});
79+
try {
80+
IdpRESTFeature.validateConfiguration(config);
81+
} finally {
82+
System.setSecurityManager(original);
83+
}
84+
}
85+
86+
private static final class SystemExitException extends SecurityException {
87+
private final int status;
88+
89+
private SystemExitException(int status) {
90+
super("System.exit(" + status + ")");
91+
this.status = status;
92+
}
93+
94+
private int status() {
95+
return status;
96+
}
97+
}
98+
}

0 commit comments

Comments
 (0)