yifan-c commented on code in PR #201: URL: https://github.com/apache/cassandra-sidecar/pull/201#discussion_r1980349493
########## server/src/test/java/org/apache/cassandra/sidecar/acl/authentication/JWTAuthenticationHandlerFactoryTest.java: ########## @@ -0,0 +1,58 @@ +/* + * 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.cassandra.sidecar.acl.authentication; + +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import io.vertx.core.Vertx; +import org.apache.cassandra.sidecar.config.AccessControlConfiguration; +import org.apache.cassandra.sidecar.tasks.PeriodicTaskExecutor; + +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.Mockito.mock; + +/** + * Test for {@link JwtAuthenticationHandlerFactory} + */ +class JWTAuthenticationHandlerFactoryTest +{ + Vertx vertx = Vertx.vertx(); Review Comment: close the vertx object. On a second thought. There is no need to instantiate `vertx` object. It is not required for the sole test in the class. Can you just return a mock instance? ########## server/src/main/java/org/apache/cassandra/sidecar/acl/authentication/ReloadingJwtAuthenticationHandler.java: ########## @@ -0,0 +1,179 @@ +/* + * 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.cassandra.sidecar.acl.authentication; + +import java.util.List; +import java.util.concurrent.atomic.AtomicReference; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import io.vertx.core.AsyncResult; +import io.vertx.core.Future; +import io.vertx.core.Handler; +import io.vertx.core.Promise; +import io.vertx.core.Vertx; +import io.vertx.core.json.JsonObject; +import io.vertx.ext.auth.User; +import io.vertx.ext.auth.authentication.AuthenticationProvider; +import io.vertx.ext.auth.oauth2.OAuth2Options; +import io.vertx.ext.auth.oauth2.providers.OpenIDConnectAuth; +import io.vertx.ext.web.RoutingContext; +import io.vertx.ext.web.handler.HttpException; +import io.vertx.ext.web.handler.impl.AuthenticationHandlerImpl; +import io.vertx.ext.web.handler.impl.OAuth2AuthHandlerImpl; +import org.apache.cassandra.sidecar.common.server.utils.DurationSpec; +import org.apache.cassandra.sidecar.tasks.PeriodicTask; +import org.apache.cassandra.sidecar.tasks.PeriodicTaskExecutor; + +import static io.netty.handler.codec.http.HttpResponseStatus.SERVICE_UNAVAILABLE; +import static io.netty.handler.codec.http.HttpResponseStatus.UNAUTHORIZED; +import static org.apache.cassandra.sidecar.utils.AuthUtils.CASSANDRA_ROLES_ATTRIBUTE_NAME; +import static org.apache.cassandra.sidecar.utils.AuthUtils.CASSANDRA_ROLE_SPLITTER; + +/** + * {@link ReloadingJwtAuthenticationHandler} validates JWT token of a user. It handles periodically calling + * {@link OpenIDConnectAuth} discover to fetch latest configuration and handles reloading {@link OAuth2AuthHandlerImpl}. + * It can be chained with other {@link io.vertx.ext.web.handler.AuthenticationHandler} implementations. + */ +public class ReloadingJwtAuthenticationHandler +extends AuthenticationHandlerImpl<ReloadingJwtAuthenticationHandler.NoOpAuthenticationProvider> +{ + private static final Logger LOGGER = LoggerFactory.getLogger(ReloadingJwtAuthenticationHandler.class); + + AtomicReference<OAuth2AuthHandlerImpl> delegateHandler = new AtomicReference<>(); + + private final Vertx vertx; + private final JwtParameterExtractor jwtParameterExtractor; + private final JwtRoleProcessor roleProcessor; + + public ReloadingJwtAuthenticationHandler(Vertx vertx, + JwtParameterExtractor jwtParameterExtractor, + JwtRoleProcessor roleProcessor, + PeriodicTaskExecutor periodicTaskExecutor) + { + super(NoOpAuthenticationProvider.INSTANCE); + this.vertx = vertx; + this.jwtParameterExtractor = jwtParameterExtractor; + this.roleProcessor = roleProcessor; + + periodicTaskExecutor.schedule(new OAuth2AuthHandlerGenerateTask()); + } + + @Override + public void authenticate(RoutingContext context, Handler<AsyncResult<User>> handler) + { + OAuth2AuthHandlerImpl oAuth2AuthHandler = delegateHandler.get(); + if (oAuth2AuthHandler == null) + { + handler.handle(Future.failedFuture(new HttpException(SERVICE_UNAVAILABLE.code(), + "JWT authentication handler unavailable"))); + return; + } + + oAuth2AuthHandler.authenticate(context, authN -> { + if (authN.failed()) + { + handler.handle(Future.failedFuture(new HttpException(UNAUTHORIZED.code(), authN.cause()))); + return; + } + + User user = authN.result(); + JsonObject decodedToken = user.attributes().containsKey("accessToken") + ? user.attributes().getJsonObject("accessToken") + : user.attributes().getJsonObject("idToken"); + + if (decodedToken == null) + { + handler.handle(Future.failedFuture(new HttpException(UNAUTHORIZED.code(), + "Could not process decoded JWT token"))); + return; + } + + try + { + List<String> roles = roleProcessor.processRoles(decodedToken); + if (!roles.isEmpty()) + { + user.attributes().put(CASSANDRA_ROLES_ATTRIBUTE_NAME, String.join(CASSANDRA_ROLE_SPLITTER, roles)); + } + handler.handle(Future.succeededFuture(user)); + } + catch (Exception e) + { + LOGGER.debug("Error processing cassandra role from JWT token", e); + handler.handle(Future.failedFuture(new HttpException(UNAUTHORIZED.code(), + "Error processing cassandra role from token"))); + } + }); + } + + /** + * {@link NoOpAuthenticationProvider} is used in {@link ReloadingJwtAuthenticationHandler}. + * {@link ReloadingJwtAuthenticationHandler} delegates authenticating user to delegate handler. + * Hence it uses no op authentication provider + */ + protected static class NoOpAuthenticationProvider implements AuthenticationProvider + { + public static final NoOpAuthenticationProvider INSTANCE = new NoOpAuthenticationProvider(); + + private NoOpAuthenticationProvider() + { + } + + @Override + public void authenticate(JsonObject credentials, Handler<AsyncResult<User>> resultHandler) + { + resultHandler.handle(Future.succeededFuture()); + } + } + + /** + * Periodic task to generate {@link OAuth2AuthHandlerImpl} with refreshed configuration from + * {@link OpenIDConnectAuth} discover. + */ + private class OAuth2AuthHandlerGenerateTask implements PeriodicTask + { + public DurationSpec delay() + { + return jwtParameterExtractor.configDiscoverInterval(); + } + + public void execute(Promise<Void> promise) + { + OAuth2Options options = new OAuth2Options().setSite(jwtParameterExtractor.site()) + .setClientId(jwtParameterExtractor.clientId()); + + OpenIDConnectAuth.discover(vertx, options) + .onSuccess(oAuthProvider -> { + OAuth2AuthHandlerImpl handler = new OAuth2AuthHandlerImpl(vertx, oAuthProvider, null); + if (!jwtParameterExtractor.scopes().isEmpty()) + { + handler.withScopes(jwtParameterExtractor.scopes()); + } + delegateHandler.set(handler); + promise.complete(); + }) + .onFailure(cause -> { + LOGGER.error("Error encountered during OpenID discovery", cause); + promise.fail(cause); + }); + } + } +} Review Comment: I am a bit worried about the tight coupling between the refresh task and the authenticator. Also such periodic task is created per each `ReloadingJwtAuthenticationHandler` instance, which I do not think it is desired. Although `ReloadingJwtAuthenticationHandler` is effectively singleton, there is no strong guarantee on it. Can you move the `OAuth2AuthHandlerGenerateTask` to a dedicated class file and make it a singleton? It can register the "discover" listener/`ReloadingJwtAuthenticationHandler` and update the reference on success. ########## conf/sidecar.yaml: ########## @@ -175,6 +175,10 @@ access_control: enabled: false # Supports setting multiple authenticators, request is authenticated if it is authenticated by any of the # configured authenticators + # Out of the box, Cassandra Sidecar provides following authenticator provider factories Review Comment: Can you also add a paragraph regarding the evaluation order for authentication? According to `ChainAuthHandlerImpl`, it is the first authenticator that passes. ########## server/src/test/java/org/apache/cassandra/sidecar/acl/authentication/ReloadingJwtAuthenticationHandlerTest.java: ########## @@ -0,0 +1,60 @@ +/* + * 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.cassandra.sidecar.acl.authentication; + +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import io.vertx.core.Vertx; +import io.vertx.ext.web.RoutingContext; +import org.apache.cassandra.sidecar.tasks.PeriodicTaskExecutor; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.doNothing; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + * Test for {@link ReloadingJwtAuthenticationHandler} + */ +class ReloadingJwtAuthenticationHandlerTest +{ + Vertx vertx = Vertx.vertx(); Review Comment: close the vertx object ########## server/src/main/java/org/apache/cassandra/sidecar/acl/authentication/MutualTlsAuthenticationHandler.java: ########## @@ -54,6 +68,39 @@ public void authenticate(RoutingContext ctx, Handler<AsyncResult<User>> handler) .recover(cause -> { // converts any exception to unauthorized http exception throw new HttpException(HttpResponseStatus.UNAUTHORIZED.code(), cause); }) - .andThen(handler); + .andThen(authN-> { + if (authN.failed()) + { + handler.handle(Future.failedFuture(new HttpException(UNAUTHORIZED.code(), authN.cause()))); + return; + } + + List<String> identities = extractIdentities(authN.result()); + + if (identities.isEmpty()) + { + handler.handle(Future.failedFuture("Missing client identities")); + return; + } Review Comment: I think this case is not tested. Can you add a test case? Btw, should it fail with `HttpException` too? ########## server/src/test/java/org/apache/cassandra/sidecar/acl/authentication/JwtParameterExtractorTest.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.cassandra.sidecar.acl.authentication; + +import java.util.Arrays; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +import static org.apache.cassandra.sidecar.acl.authentication.JwtParameterExtractor.SITE_SUFFIX; +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +/** + * Test for {@link JwtParameterExtractor} + */ +class JwtParameterExtractorTest +{ + @Test + void testExtractingValidParameters() + { + JwtParameterExtractor parameterExtractor = new JwtParameterExtractor(Map.of("site", "x.com", + "client_id", "id", + "scopes_supported", "phone,email", + "config_discover_interval", "2m")); + assertThat(parameterExtractor.site()).isEqualTo("x.com"); + assertThat(parameterExtractor.clientId()).isEqualTo("id"); + assertThat(parameterExtractor.scopes()).containsAll(Arrays.asList("email", "phone")); + assertThat(parameterExtractor.configDiscoverInterval().toSeconds()).isEqualTo(120); + } + + @Test + void testSiteSuffixRemoved() + { + JwtParameterExtractor parameterExtractor = new JwtParameterExtractor(Map.of("site", "x" + SITE_SUFFIX, + "client_id", "id")); + assertThat(parameterExtractor.site()).as("Site suffix should be removed").isEqualTo("x"); + } + + @Test + void testCustomScopeSeparator() + { + JwtParameterExtractor parameterExtractor = new JwtParameterExtractor(Map.of("site", "x.com", Review Comment: hmm.. do we want to put `x.com` here? It is an existing site. Maybe `www.apache.org` instead. -- 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: pr-unsubscr...@cassandra.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org --------------------------------------------------------------------- To unsubscribe, e-mail: pr-unsubscr...@cassandra.apache.org For additional commands, e-mail: pr-h...@cassandra.apache.org
