lianetm commented on code in PR #19622:
URL: https://github.com/apache/kafka/pull/19622#discussion_r2076162526
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/OAuthBearerValidatorCallbackHandler.java:
##########
@@ -135,13 +136,19 @@ public void configure(Map<String, ?> configs, String
saslMechanism, List<AppConf
new
RefCountingVerificationKeyResolver(VerificationKeyResolverFactory.create(configs,
saslMechanism, moduleOptions)));
}
- AccessTokenValidator accessTokenValidator =
AccessTokenValidatorFactory.create(configs, saslMechanism,
verificationKeyResolver);
- init(verificationKeyResolver, accessTokenValidator);
+ JwtValidator jwtValidator = new DefaultJwtValidator(configs,
saslMechanism, verificationKeyResolver);
+ init(verificationKeyResolver, jwtValidator);
}
- public void init(CloseableVerificationKeyResolver verificationKeyResolver,
AccessTokenValidator accessTokenValidator) {
+ public void init(CloseableVerificationKeyResolver verificationKeyResolver,
JwtValidator jwtValidator) {
this.verificationKeyResolver = verificationKeyResolver;
- this.accessTokenValidator = accessTokenValidator;
+ this.jwtValidator = jwtValidator;
+
+ try {
+ this.jwtValidator.init();
+ } catch (IOException e) {
+ throw new KafkaException("The OAuth validator configuration
encountered an error when initializing the JwtValidator", e);
Review Comment:
this reads a bit weird, doesn't it? Should it be "The OAuth validator
callback..."??
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/ClientJwtValidator.java:
##########
@@ -123,6 +124,16 @@ else if (scopeRaw instanceof Collection)
issuedAt);
}
+ @Override
+ public void init() throws IOException {
+ // Do nothing...
+ }
Review Comment:
as above, needed?
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/FileJwtRetriever.java:
##########
@@ -23,19 +23,19 @@
import java.nio.file.Path;
/**
- * <code>FileTokenRetriever</code> is an {@link AccessTokenRetriever} that
will load the contents,
+ * <code>FileJwtRetriever</code> is an {@link JwtRetriever} that will load the
contents,
Review Comment:
```suggestion
* <code>FileJwtRetriever</code> is a {@link JwtRetriever} that will load
the contents,
```
(and are we missing something there? "contents of a file")
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/DefaultJwtValidator.java:
##########
@@ -0,0 +1,104 @@
+/*
+ * 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.kafka.common.security.oauthbearer.internals.secured;
+
+import org.apache.kafka.common.security.oauthbearer.OAuthBearerToken;
+import org.apache.kafka.common.utils.Utils;
+
+import org.jose4j.keys.resolvers.VerificationKeyResolver;
+
+import java.io.IOException;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
+import java.util.Set;
+
+import static
org.apache.kafka.common.config.SaslConfigs.SASL_OAUTHBEARER_CLOCK_SKEW_SECONDS;
+import static
org.apache.kafka.common.config.SaslConfigs.SASL_OAUTHBEARER_EXPECTED_AUDIENCE;
+import static
org.apache.kafka.common.config.SaslConfigs.SASL_OAUTHBEARER_EXPECTED_ISSUER;
+import static
org.apache.kafka.common.config.SaslConfigs.SASL_OAUTHBEARER_SCOPE_CLAIM_NAME;
+import static
org.apache.kafka.common.config.SaslConfigs.SASL_OAUTHBEARER_SUB_CLAIM_NAME;
+
+public class DefaultJwtValidator implements JwtValidator {
Review Comment:
a java doc here would be helpful I expect? This seems to default to broker
or client validators based on the VerificationKeyResolver, expected to be used
for LoginCallbacks (client) or ValidatorCallbacks (broker), is my understanding
correct?
##########
clients/src/test/java/org/apache/kafka/common/security/oauthbearer/internals/secured/JwtValidatorFactoryTest.java:
##########
@@ -19,39 +19,42 @@
import org.apache.kafka.common.KafkaException;
import
org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginCallbackHandler;
+import org.apache.kafka.common.security.oauthbearer.OAuthBearerLoginModule;
import org.junit.jupiter.api.Test;
import java.io.IOException;
import java.util.Map;
-public class AccessTokenValidatorFactoryTest extends OAuthBearerTest {
+public class JwtValidatorFactoryTest extends OAuthBearerTest {
Review Comment:
both test in this class seem to be testing the handler, so shouldn't they
belong to `OAuthBearerLoginCallbackHandlerTest`?
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpJwtRetriever.java:
##########
@@ -49,22 +49,22 @@
import javax.net.ssl.SSLSocketFactory;
/**
- * <code>HttpAccessTokenRetriever</code> is an {@link AccessTokenRetriever}
that will
+ * <code>HttpJwtRetriever</code> is an {@link JwtRetriever} that will
Review Comment:
```suggestion
* <code>HttpJwtRetriever</code> is a {@link JwtRetriever} that will
```
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/JwtValidator.java:
##########
@@ -40,13 +42,12 @@
* <li><a
href="https://datatracker.ietf.org/doc/html/draft-ietf-oauth-access-token-jwt";>RFC
6750, Section 2.1</a></li>
* </ul>
*
- * @see LoginAccessTokenValidator A basic AccessTokenValidator used by
client-side login
- * authentication
- * @see ValidatorAccessTokenValidator A more robust AccessTokenValidator that
is used on the broker
- * to validate the token's contents and
verify the signature
+ * @see ClientJwtValidator A basic JwtValidator used by client-side login
authentication
+ * @see BrokerJwtValidator A more robust JwtValidator that is used on the
broker to validate the token's
+ * contents and verify the signature
*/
-public interface AccessTokenValidator {
+public interface JwtValidator extends Initable, Closeable {
Review Comment:
with this we should update the java doc for the `Initiable` interface (reads
that it's for `initialization of the retriever` but now it's also for the
validator (I would probably just remove the "what" it initializes, it's really
a generic interface)
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/BrokerJwtValidator.java:
##########
@@ -190,6 +191,16 @@ else if (scopeRaw instanceof Collection)
issuedAt);
}
+ @Override
+ public void init() throws IOException {
+ // Do nothing...
+ }
Review Comment:
do we need this? it has a default empty implementation so I would expect we
don't (if it's that we expect we'll need it in the future let's just add it
when needed I would say)
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/ClientJwtValidator.java:
##########
@@ -50,9 +51,9 @@
* </ol>
Review Comment:
nit: on the line above, seems inconsistent that `scope` and `subject` do not
have the code tags as the other claims, intentional?
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpJwtRetriever.java:
##########
@@ -49,22 +49,22 @@
import javax.net.ssl.SSLSocketFactory;
/**
- * <code>HttpAccessTokenRetriever</code> is an {@link AccessTokenRetriever}
that will
+ * <code>HttpJwtRetriever</code> is an {@link JwtRetriever} that will
* communicate with an OAuth/OIDC provider directly via HTTP to post client
credentials
* ({@link OAuthBearerLoginCallbackHandler#CLIENT_ID_CONFIG}/{@link
OAuthBearerLoginCallbackHandler#CLIENT_SECRET_CONFIG})
* to a publicized token endpoint URL
* ({@link SaslConfigs#SASL_OAUTHBEARER_TOKEN_ENDPOINT_URL}).
*
- * @see AccessTokenRetriever
+ * @see JwtRetriever
* @see OAuthBearerLoginCallbackHandler#CLIENT_ID_CONFIG
* @see OAuthBearerLoginCallbackHandler#CLIENT_SECRET_CONFIG
* @see OAuthBearerLoginCallbackHandler#SCOPE_CONFIG
* @see SaslConfigs#SASL_OAUTHBEARER_TOKEN_ENDPOINT_URL
Review Comment:
all these are already linked in this same java doc. Do we get anything from
adding them again as `@see`? (the scope_config is the only one not duplicated,
relevant?)
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/DefaultJwtRetriever.java:
##########
@@ -96,7 +106,7 @@ public static AccessTokenRetriever create(Map<String, ?>
configs,
* <p/>
*
* This utility method ensures that we have a non-{@code null} value to
use in the
- * {@link HttpAccessTokenRetriever} constructor.
+ * {@link HttpJwtRetriever} constructor.
*/
static boolean validateUrlencodeHeader(ConfigurationUtils
configurationUtils) {
Boolean urlencodeHeader =
configurationUtils.validateBoolean(SASL_OAUTHBEARER_HEADER_URLENCODE, false);
Review Comment:
this `validateBoolean` only validates isRequired, so seems useless if we
pass `false` (not required). Is there a reason why we need this or `.get` could
do?
--
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: jira-unsubscr...@kafka.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
