diff --git a/design-docs/gravitino-local-authentication.md b/design-docs/gravitino-local-authentication.md index bc90a39ea96..bfb1d7ea3fc 100644 --- a/design-docs/gravitino-local-authentication.md +++ b/design-docs/gravitino-local-authentication.md @@ -319,6 +319,9 @@ fresh Gravitino deployment. gravitino.authorization.serviceAdmins=admin1,admin2 ``` + Built-in IdP is incompatible with the `simple` authenticator. When the `idp-basic` plugin is + enabled, `gravitino.authenticators` must not include `simple`. + 2. Export the initial service admin password before starting Gravitino: ```bash diff --git a/docs/open-api/idp/idp.yaml b/docs/open-api/idp/idp.yaml index 0aea6d75db3..6b233bbd511 100644 --- a/docs/open-api/idp/idp.yaml +++ b/docs/open-api/idp/idp.yaml @@ -24,7 +24,10 @@ paths: tags: - IDP summary: Add built-in IDP user - description: Creates a built-in IDP user with the given username and password. + description: > + Creates a built-in IDP user with the given username and password. + Requires the `idp-basic` plugin and built-in IdP Basic authentication. + `gravitino.authenticators` must not include `simple`. operationId: addIdpUser requestBody: required: true diff --git a/docs/open-api/idp/openapi.yaml b/docs/open-api/idp/openapi.yaml index 76e064083f0..23000ef4f9e 100644 --- a/docs/open-api/idp/openapi.yaml +++ b/docs/open-api/idp/openapi.yaml @@ -25,7 +25,10 @@ info: version: 1.3.0-SNAPSHOT description: | OpenAPI specification for built-in IDP user and group management APIs exposed - by the `idp-basic` plugin when the `basic` authenticator is enabled. + by the `idp-basic` plugin. Clients authenticate with Basic credentials + validated against built-in IdP user metadata. Enable the plugin via + `gravitino.server.rest.extensionPackages`; `gravitino.authenticators` must not + include `simple` when IdP is enabled. servers: - url: "{scheme}://{host}:{port}/{basePath}" diff --git a/docs/security/how-to-authenticate.md b/docs/security/how-to-authenticate.md index 053821fb070..5389d4a7346 100644 --- a/docs/security/how-to-authenticate.md +++ b/docs/security/how-to-authenticate.md @@ -366,6 +366,9 @@ This example shows how to enable built-in Basic authentication. - Gravitino distribution package (includes the idp-basic plugin on the server classpath) +Built-in IdP is **incompatible** with the `simple` authenticator (the default). When the +`idp-basic` plugin is enabled, `gravitino.authenticators` must not include `simple`. + **Configuration:** Append the following to `conf/gravitino.conf`: diff --git a/docs/security/how-to-use-built-in-idp.md b/docs/security/how-to-use-built-in-idp.md index ef18b44eaff..c9c00f05aa2 100644 --- a/docs/security/how-to-use-built-in-idp.md +++ b/docs/security/how-to-use-built-in-idp.md @@ -31,7 +31,11 @@ Before you call `/api/idp/*`, ensure the following: gravitino.server.rest.extensionPackages = org.apache.gravitino.idp.web.rest.feature ``` -2. **Service admin passwords** — Built-in IDP requires every username in +2. **Server authenticator** — Built-in IdP is **incompatible** with the `simple` authenticator + (the default). When the `idp-basic` plugin is enabled, `gravitino.authenticators` must not + include `simple`. + +3. **Service admin passwords** — Built-in IDP requires every username in `gravitino.authorization.serviceAdmins` to have a password stored in `idp_user_meta` before you can call management APIs. @@ -61,9 +65,10 @@ Before you call `/api/idp/*`, ensure the following: Set service admins in `gravitino.conf` (see also [Prerequisites](#prerequisites)): -| Configuration item | Description | Example | -|-----------------------------------------|-------------------------------------------------------------------------------------|---------| -| `gravitino.authorization.serviceAdmins` | Comma-separated service admin that can call built-in IDP management APIs | `admin` | +| Configuration item | Description | Example | +|-------------------------------------------|-------------------------------------------------------------------------------------|---------| +| `gravitino.server.rest.extensionPackages` | Registers built-in IdP REST APIs | `org.apache.gravitino.idp.web.rest.feature` | +| `gravitino.authorization.serviceAdmins` | Comma-separated service admin that can call built-in IDP management APIs | `admin` | Example: diff --git a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/feature/IdpRESTFeature.java b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/feature/IdpRESTFeature.java index 939c4d8406c..668b5d84a3b 100644 --- a/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/feature/IdpRESTFeature.java +++ b/plugins/idp-basic/src/main/java/org/apache/gravitino/idp/web/rest/feature/IdpRESTFeature.java @@ -26,6 +26,7 @@ import org.apache.gravitino.Config; import org.apache.gravitino.Configs; import org.apache.gravitino.GravitinoEnv; +import org.apache.gravitino.auth.AuthenticatorType; import org.apache.gravitino.idp.IdpUserGroupManager; import org.apache.gravitino.idp.auth.BasicAuthenticator; import org.apache.gravitino.idp.web.rest.IdpAuthorizationFilter; @@ -59,6 +60,7 @@ public class IdpRESTFeature implements Feature { public boolean configure(FeatureContext context) { GravitinoEnv env = GravitinoEnv.getInstance(); Config config = env.config(); + validateConfiguration(config); registerBasicAuthenticator(config); try { @@ -76,6 +78,23 @@ public boolean configure(FeatureContext context) { return true; } + /** + * Validates that the server configuration is compatible with the built-in IdP plugin. + * + * @param config The server configuration. + */ + static void validateConfiguration(Config config) { + boolean usesSimple = + config.get(Configs.AUTHENTICATORS).stream() + .anyMatch(name -> AuthenticatorType.SIMPLE.name().equalsIgnoreCase(name.trim())); + if (usesSimple) { + LOG.error( + "Built-in IdP is incompatible with Simple authentication. " + + "Remove 'simple' from gravitino.authenticators (default is simple)."); + System.exit(1); + } + } + private static void registerBasicAuthenticator(Config config) { List authenticators = ServerAuthenticator.getInstance().authenticators(); if (authenticators == null) { diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java index a732f3a5a1a..6bf9cbc75ee 100644 --- a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/integration/test/IdpRESTApiIT.java @@ -30,6 +30,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.Paths; +import java.security.KeyPairGenerator; import java.util.Base64; import java.util.List; import java.util.Map; @@ -37,6 +38,7 @@ import org.apache.commons.lang3.StringUtils; import org.apache.gravitino.Configs; import org.apache.gravitino.auth.AuthConstants; +import org.apache.gravitino.auth.AuthenticatorType; import org.apache.gravitino.dto.responses.ErrorConstants; import org.apache.gravitino.idp.dto.requests.AddGroupRequest; import org.apache.gravitino.idp.dto.requests.AddUserRequest; @@ -48,6 +50,7 @@ import org.apache.gravitino.integration.test.util.BaseIT; import org.apache.gravitino.integration.test.util.ITUtils; import org.apache.gravitino.json.JsonUtils; +import org.apache.gravitino.server.authentication.OAuthConfig; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeAll; import org.junit.jupiter.api.Test; @@ -84,6 +87,11 @@ public void startIntegrationTest() throws Exception { configs.put(Configs.CACHE_ENABLED.getKey(), String.valueOf(false)); configs.put(Configs.STORE_DELETE_AFTER_TIME.getKey(), String.valueOf(20 * 60 * 1000L)); configs.put(Configs.SERVICE_ADMINS.getKey(), ADMIN); + configs.put(Configs.AUTHENTICATORS.getKey(), AuthenticatorType.OAUTH.name().toLowerCase()); + configs.put(OAuthConfig.SERVICE_AUDIENCE.getKey(), "service1"); + configs.put(OAuthConfig.DEFAULT_SIGN_KEY.getKey(), oauthPublicSignKey()); + configs.put(OAuthConfig.DEFAULT_SERVER_URI.getKey(), "test"); + configs.put(OAuthConfig.DEFAULT_TOKEN_PATH.getKey(), "test"); configs.put( Configs.REST_API_EXTENSION_PACKAGES.getKey(), IdpRESTFeature.IDP_REST_EXTENSION_PACKAGE); registerCustomConfigs(configs); @@ -117,8 +125,8 @@ private static void ensureDeployInitialAdminPasswordInDistributionEnv() throws I @Test void testIdpAuthorization() throws Exception { Assertions.assertEquals(200, get("/version", ADMIN, ADMIN_PASSWORD).statusCode()); - // No Authorization: simple authenticator allows anonymous access; IdP filter rejects. - assertError(403, get("/idp/users/" + USER1, null, null), ErrorConstants.FORBIDDEN_CODE); + // No Authorization: OAuth rejects the request before the IdP filter runs. + Assertions.assertEquals(401, get("/idp/users/" + USER1, null, null).statusCode()); postUser(USER2, USER_PASSWORD); assertError( @@ -371,4 +379,10 @@ private static void assertError(int expectedStatus, HttpResponse respons private static int errorCode(HttpResponse response) throws Exception { return JsonUtils.objectMapper().readTree(response.body()).get("code").asInt(); } + + private static String oauthPublicSignKey() throws Exception { + KeyPairGenerator generator = KeyPairGenerator.getInstance("RSA"); + generator.initialize(2048); + return Base64.getEncoder().encodeToString(generator.generateKeyPair().getPublic().getEncoded()); + } } diff --git a/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/feature/TestIdpRESTFeature.java b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/feature/TestIdpRESTFeature.java new file mode 100644 index 00000000000..72153617979 --- /dev/null +++ b/plugins/idp-basic/src/test/java/org/apache/gravitino/idp/web/rest/feature/TestIdpRESTFeature.java @@ -0,0 +1,98 @@ +/* + * 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.gravitino.idp.web.rest.feature; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import com.google.common.collect.Lists; +import org.apache.gravitino.Config; +import org.apache.gravitino.Configs; +import org.apache.gravitino.auth.AuthenticatorType; +import org.junit.jupiter.api.Test; + +class TestIdpRESTFeature { + + @Test + void testSimpleFails() { + Config config = newConfig(AuthenticatorType.SIMPLE.name().toLowerCase()); + + SystemExitException exception = + assertThrows(SystemExitException.class, () -> validateWithExitGuard(config)); + + assertEquals(1, exception.status()); + } + + @Test + void testDefaultSimpleFails() { + Config config = new Config(false) {}; + + assertThrows(SystemExitException.class, () -> validateWithExitGuard(config)); + } + + @Test + void testOAuthOk() { + Config config = newConfig(AuthenticatorType.OAUTH.name().toLowerCase()); + + assertDoesNotThrow(() -> IdpRESTFeature.validateConfiguration(config)); + } + + private static Config newConfig(String... authenticators) { + Config config = new Config(false) {}; + config.set(Configs.AUTHENTICATORS, Lists.newArrayList(authenticators)); + return config; + } + + @SuppressWarnings("removal") + private static void validateWithExitGuard(Config config) { + SecurityManager original = System.getSecurityManager(); + System.setSecurityManager( + new SecurityManager() { + @Override + public void checkExit(int status) { + throw new SystemExitException(status); + } + + @Override + public void checkPermission(java.security.Permission perm) { + // Allow test execution. + } + }); + try { + IdpRESTFeature.validateConfiguration(config); + } finally { + System.setSecurityManager(original); + } + } + + private static final class SystemExitException extends SecurityException { + private final int status; + + private SystemExitException(int status) { + super("System.exit(" + status + ")"); + this.status = status; + } + + private int status() { + return status; + } + } +}