Copilot commented on code in PR #17302:
URL: https://github.com/apache/iotdb/pull/17302#discussion_r2944981709
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/conf/CommonDescriptor.java:
##########
@@ -80,6 +80,8 @@ public void loadCommonProps(TrimProperties properties) throws
IOException {
// if using org.apache.iotdb.db.auth.authorizer.OpenIdAuthorizer,
openID_url is needed.
config.setOpenIdProviderUrl(
properties.getProperty("openID_url",
config.getOpenIdProviderUrl()).trim());
+ config.setOpenIdAudience(
+ properties.getProperty("openID_audience",
config.getOpenIdAudience()).trim());
Review Comment:
The comment still references
`org.apache.iotdb.db.auth.authorizer.OpenIdAuthorizer`, but the implementation
lives under `org.apache.iotdb.commons.auth.authorizer.OpenIdAuthorizer`. Update
this comment to avoid pointing users at a non-existent/incorrect class name.
##########
iotdb-core/node-commons/pom.xml:
##########
@@ -149,6 +149,16 @@
<groupId>io.jsonwebtoken</groupId>
<artifactId>jjwt-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-impl</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-jackson</artifactId>
+ <scope>test</scope>
Review Comment:
`OpenIdAuthorizer` is production code (in `src/main`) and requires
`jjwt-impl` at runtime (the `jjwt-api` artifact alone is not sufficient for
`Jwts.parser()`/parsing). Declaring `jjwt-impl` as `<scope>test</scope>` can
lead to runtime `ClassNotFoundException`/`UnsupportedJwtException` when OpenID
auth is enabled. Move `jjwt-impl` (and the JSON codec module you rely on) to
compile/runtime scope so it’s present in the server classpath.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/authorizer/OpenIdAuthorizer.java:
##########
@@ -161,14 +190,9 @@ public boolean login(String token, String password, final
boolean useEncryptedPa
try {
claims = validateToken(token);
} catch (JwtException e) {
- logger.error("Unable to login the user with Username (token) {}", token,
e);
+ logger.error("Unable to login the user with Username (token), {}",
e.getMessage());
Review Comment:
The updated log statement drops the exception stack trace entirely
(`e.getMessage()` only), which makes production troubleshooting hard. Consider
logging the exception itself without including the token (e.g., keep the
current message but pass `e` as the throwable, or log stack trace at DEBUG).
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/authorizer/OpenIdAuthorizer.java:
##########
@@ -237,7 +299,7 @@ public boolean isAdmin(String token) {
try {
claims = validateToken(token);
} catch (JwtException e) {
- logger.warn("Unable to validate token {}!", token, e);
+ logger.warn("Unable to validate token! {}", e.getMessage());
Review Comment:
Same as in `login(...)`: this warning now logs only `e.getMessage()` and
drops the stack trace. Consider logging the throwable (without logging the
token) so validation failures can be debugged in production.
##########
iotdb-core/node-commons/pom.xml:
##########
@@ -149,6 +149,16 @@
<groupId>io.jsonwebtoken</groupId>
<artifactId>jjwt-api</artifactId>
</dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-impl</artifactId>
+ <scope>test</scope>
+ </dependency>
+ <dependency>
+ <groupId>io.jsonwebtoken</groupId>
+ <artifactId>jjwt-jackson</artifactId>
+ <scope>test</scope>
Review Comment:
Same issue as `jjwt-impl`: `jjwt-jackson` (or another jjwt JSON serializer
module) is typically needed at runtime for claims JSON handling depending on
the jjwt version/config. With `<scope>test</scope>`, OpenID auth can fail in
production. Prefer compile/runtime scope, and then you can likely remove the
extra `maven-dependency-plugin` ignore block added only to silence “unused”
warnings.
##########
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/auth/authorizer/OpenIdAuthorizer.java:
##########
@@ -96,12 +114,23 @@ private static JSONObject getJwkFromProvider(String
providerUrl)
// Fetch Metadata
OIDCProviderMetadata providerMetadata = fetchMetadata(providerUrl);
- logger.debug("Using Provider Metadata: {}", providerMetadata);
+ Set<String> acceptedAudiences = parseAudiences(config.getOpenIdAudience());
+ if (acceptedAudiences.isEmpty()) {
+ throw new AuthException(
+ TSStatusCode.INIT_AUTH_ERROR,
+ "openID_audience must be configured when OpenIdAuthorizer is
enabled");
+ }
+
+ String issuer =
+ providerMetadata.getIssuer() == null ? null :
providerMetadata.getIssuer().getValue();
+ if (issuer == null || issuer.isEmpty()) {
+ throw new AuthException(
+ TSStatusCode.INIT_AUTH_ERROR, "OIDC provider metadata does not
contain an issuer");
+ }
try {
URL url = new URI(providerMetadata.getJWKSetURI().toString()).toURL();
- logger.debug("Using url {}", url);
- return getProviderRsaJwk(url.openStream());
+ return new ProviderContext(getProviderRsaJwk(url.openStream()), issuer,
acceptedAudiences);
Review Comment:
`getProviderRsaJwk(...)` can return `null` when the JWKS has no RSA signing
key, but `loadProviderContext` passes that through and the constructor will
then fail with a `NullPointerException`/unhelpful parse error. Add an explicit
null check after fetching the JWK and throw an `AuthException` with a clear
INIT_AUTH_ERROR message (e.g., “No RSA signing key found in JWKS”).
##########
iotdb-core/node-commons/src/test/java/org/apache/iotdb/commons/auth/authorizer/OpenIdAuthorizerTest.java:
##########
@@ -0,0 +1,261 @@
+/*
+ * 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.iotdb.commons.auth.authorizer;
+
+import org.apache.iotdb.commons.conf.CommonConfig;
+import org.apache.iotdb.commons.conf.CommonDescriptor;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.JWSAlgorithm;
+import com.nimbusds.jose.JWSHeader;
+import com.nimbusds.jose.crypto.RSASSASigner;
+import com.nimbusds.jose.jwk.KeyUse;
+import com.nimbusds.jose.jwk.RSAKey;
+import com.nimbusds.jwt.JWTClaimsSet;
+import com.nimbusds.jwt.SignedJWT;
+import com.sun.net.httpserver.HttpExchange;
+import com.sun.net.httpserver.HttpServer;
+import net.minidev.json.JSONObject;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.net.InetSocketAddress;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.PrivateKey;
+import java.security.interfaces.RSAPublicKey;
+import java.time.Instant;
+import java.util.Collections;
+import java.util.Date;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class OpenIdAuthorizerTest {
+
+ private final CommonConfig config =
CommonDescriptor.getInstance().getConfig();
+ private PrivateKey privateKey;
+ private JSONObject publicJwk;
+
+ @Before
+ public void setUp() throws Exception {
+ KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA");
+ keyPairGenerator.initialize(2048);
+ KeyPair keyPair = keyPairGenerator.generateKeyPair();
+ privateKey = keyPair.getPrivate();
+ publicJwk =
+ new JSONObject(
+ new RSAKey.Builder((RSAPublicKey) keyPair.getPublic())
+ .keyUse(KeyUse.SIGNATURE)
+ .keyID("datanode-openid-test-key")
+ .build()
+ .toJSONObject());
+ }
+
+ @After
+ public void tearDown() throws IOException {}
Review Comment:
These tests mutate the global `CommonDescriptor` singleton config (e.g.,
`setOpenIdAudience`) but `tearDown()` is empty, so the changed values can leak
into other tests and create order-dependent failures. Save the previous config
values in `setUp()` and restore them in `tearDown()` (or use a dedicated test
config instance) to keep tests isolated.
##########
iotdb-core/node-commons/src/test/java/org/apache/iotdb/commons/auth/authorizer/OpenIdAuthorizerTest.java:
##########
@@ -0,0 +1,261 @@
+/*
+ * 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.iotdb.commons.auth.authorizer;
+
+import org.apache.iotdb.commons.conf.CommonConfig;
+import org.apache.iotdb.commons.conf.CommonDescriptor;
+
+import com.nimbusds.jose.JOSEException;
+import com.nimbusds.jose.JWSAlgorithm;
+import com.nimbusds.jose.JWSHeader;
+import com.nimbusds.jose.crypto.RSASSASigner;
+import com.nimbusds.jose.jwk.KeyUse;
+import com.nimbusds.jose.jwk.RSAKey;
+import com.nimbusds.jwt.JWTClaimsSet;
+import com.nimbusds.jwt.SignedJWT;
+import com.sun.net.httpserver.HttpExchange;
+import com.sun.net.httpserver.HttpServer;
+import net.minidev.json.JSONObject;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.io.IOException;
+import java.io.OutputStream;
+import java.net.InetSocketAddress;
+import java.security.KeyPair;
+import java.security.KeyPairGenerator;
+import java.security.PrivateKey;
+import java.security.interfaces.RSAPublicKey;
+import java.time.Instant;
+import java.util.Collections;
+import java.util.Date;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class OpenIdAuthorizerTest {
+
+ private final CommonConfig config =
CommonDescriptor.getInstance().getConfig();
+ private PrivateKey privateKey;
+ private JSONObject publicJwk;
+
+ @Before
+ public void setUp() throws Exception {
+ KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA");
+ keyPairGenerator.initialize(2048);
+ KeyPair keyPair = keyPairGenerator.generateKeyPair();
+ privateKey = keyPair.getPrivate();
+ publicJwk =
+ new JSONObject(
+ new RSAKey.Builder((RSAPublicKey) keyPair.getPublic())
+ .keyUse(KeyUse.SIGNATURE)
+ .keyID("datanode-openid-test-key")
+ .build()
+ .toJSONObject());
+ }
+
+ @After
+ public void tearDown() throws IOException {}
+
+ @Test
+ public void loginWithJWT() throws Exception {
+ String jwt = createJwt(false);
+ OpenIdAuthorizer authorizer = new OpenIdAuthorizer(publicJwk);
+ assertTrue(authorizer.login(jwt, null, false));
+ }
+
+ @Test
+ public void isAdmin_hasAccess() throws Exception {
+ String jwt = createJwt(true);
+ OpenIdAuthorizer authorizer = new OpenIdAuthorizer(publicJwk);
+ assertTrue(authorizer.isAdmin(jwt));
+ }
+
+ @Test
+ public void isAdmin_noAdminClaim() throws Exception {
+ String jwt = createJwt(false);
+ OpenIdAuthorizer authorizer = new OpenIdAuthorizer(publicJwk);
+ assertFalse(authorizer.isAdmin(jwt));
+ }
+
+ @Test
+ public void testExpiredTokenRejectedByLoginAndIsAdmin() throws Exception {
+ KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA");
+ keyPairGenerator.initialize(2048);
+ KeyPair keyPair = keyPairGenerator.generateKeyPair();
+
+ JSONObject jwk =
+ new JSONObject(
+ new RSAKey.Builder((RSAPublicKey) keyPair.getPublic())
+ .privateKey(keyPair.getPrivate())
+ .keyUse(KeyUse.SIGNATURE)
+ .keyID("expired-token-test-key")
+ .build()
+ .toJSONObject());
+
+ OpenIdAuthorizer authorizer = new OpenIdAuthorizer(jwk);
+ String expiredToken =
+ createSignedToken(
+ keyPair.getPrivate(),
+ new JWTClaimsSet.Builder()
+ .subject("attacker")
+ .expirationTime(Date.from(Instant.now().minusSeconds(3600)))
+ .claim(
+ "realm_access",
+ Collections.singletonMap(
+ "roles",
+
Collections.singletonList(OpenIdAuthorizer.IOTDB_ADMIN_ROLE_NAME))));
+
+ assertFalse(authorizer.login(expiredToken, "", false));
+ assertFalse(authorizer.isAdmin(expiredToken));
+ }
+
+ @Test
+ public void testWrongIssuerRejected() throws Exception {
+ config.setOpenIdAudience("iotdb");
+ KeyPair keyPair = generateKeyPair();
+ HttpServer server = startProviderServer(keyPair);
+ String issuer = "http://127.0.0.1:"; + server.getAddress().getPort() + "/";
+
+ try {
+ OpenIdAuthorizer authorizer = new OpenIdAuthorizer(issuer);
+ String token =
+ createSignedToken(
+ keyPair.getPrivate(),
+ new JWTClaimsSet.Builder()
+ .subject("attacker")
+ .issuer("https://evil.example/issuer";)
+ .audience("iotdb")
+ .expirationTime(Date.from(Instant.now().plusSeconds(3600)))
+ .claim(
+ "realm_access",
+ Collections.singletonMap(
+ "roles",
+
Collections.singletonList(OpenIdAuthorizer.IOTDB_ADMIN_ROLE_NAME))));
+
+ assertFalse(authorizer.login(token, "", false));
+ assertFalse(authorizer.isAdmin(token));
+ } finally {
+ server.stop(0);
+ }
+ }
+
+ @Test
+ public void testWrongAudienceRejected() throws Exception {
+ config.setOpenIdAudience("iotdb");
+ KeyPair keyPair = generateKeyPair();
+ HttpServer server = startProviderServer(keyPair);
+ String issuer = "http://127.0.0.1:"; + server.getAddress().getPort() + "/";
+
+ try {
+ OpenIdAuthorizer authorizer = new OpenIdAuthorizer(issuer);
+ String token =
+ createSignedToken(
+ keyPair.getPrivate(),
+ new JWTClaimsSet.Builder()
+ .subject("attacker")
+ .issuer(issuer)
+ .audience("unrelated-client")
+ .expirationTime(Date.from(Instant.now().plusSeconds(3600)))
+ .claim(
+ "realm_access",
+ Collections.singletonMap(
+ "roles",
+
Collections.singletonList(OpenIdAuthorizer.IOTDB_ADMIN_ROLE_NAME))));
+
+ assertFalse(authorizer.login(token, "", false));
+ assertFalse(authorizer.isAdmin(token));
+ } finally {
+ server.stop(0);
+ }
+ }
+
+ private KeyPair generateKeyPair() throws Exception {
+ KeyPairGenerator keyPairGenerator = KeyPairGenerator.getInstance("RSA");
+ keyPairGenerator.initialize(2048);
+ return keyPairGenerator.generateKeyPair();
+ }
+
+ private String createSignedToken(PrivateKey privateKey, JWTClaimsSet.Builder
claimsBuilder)
+ throws JOSEException {
+ SignedJWT signedJwt = new SignedJWT(new JWSHeader(JWSAlgorithm.RS256),
claimsBuilder.build());
+ signedJwt.sign(new RSASSASigner(privateKey));
+ return signedJwt.serialize();
+ }
+
+ private HttpServer startProviderServer(KeyPair keyPair) throws Exception {
+ JSONObject publicJwk =
+ new JSONObject(
+ new RSAKey.Builder((RSAPublicKey) keyPair.getPublic())
+ .keyUse(KeyUse.SIGNATURE)
+ .keyID("openid-provider-test-key")
+ .build()
+ .toJSONObject());
+
+ HttpServer server = HttpServer.create(new InetSocketAddress("127.0.0.1",
0), 0);
+ String issuer = "http://127.0.0.1:"; + server.getAddress().getPort() + "/";
+ String metadata =
+ "{"
+ + "\"issuer\":\""
+ + issuer
+ + "\","
+ + "\"jwks_uri\":\""
+ + issuer
+ + "jwks.json\","
+ + "\"subject_types_supported\":[\"public\"],"
+ + "\"response_types_supported\":[\"code\"],"
+ + "\"id_token_signing_alg_values_supported\":[\"RS256\"]"
+ + "}";
+ String jwks = "{\"keys\":[" + publicJwk.toJSONString() + "]}";
+
+ server.createContext(
+ "/.well-known/openid-configuration", exchange -> writeJson(exchange,
metadata));
+ server.createContext("/jwks.json", exchange -> writeJson(exchange, jwks));
+ server.start();
+ return server;
+ }
+
+ private void writeJson(HttpExchange exchange, String json) throws
IOException {
+ byte[] response = json.getBytes(java.nio.charset.StandardCharsets.UTF_8);
+ exchange.getResponseHeaders().set("Content-Type", "application/json");
+ exchange.sendResponseHeaders(200, response.length);
+ try (OutputStream outputStream = exchange.getResponseBody()) {
+ outputStream.write(response);
Review Comment:
`writeJson(...)` closes the response body stream but never closes the
`HttpExchange` itself. To avoid resource leaks in the embedded `HttpServer`
tests (especially if tests are extended), explicitly call `exchange.close()` in
a `finally` block after writing the response.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
