kirktrue commented on code in PR #15475:
URL: https://github.com/apache/kafka/pull/15475#discussion_r1513681366
##########
clients/src/main/java/org/apache/kafka/common/config/SaslConfigs.java:
##########
@@ -192,6 +192,12 @@ public class SaslConfigs {
+ " be inspected for the standard OAuth \"iss\" claim and if this
value is set, the broker will match it exactly against what is in the JWT's
\"iss\" claim. If there is no"
+ " match, the broker will reject the JWT and authentication will
fail.";
+ public static final String SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE =
"sasl.oauthbearer.header.urlencode.enable";
+ public static final boolean
DEFAULT_SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE = false;
+ public static final String SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE_DOC =
"The (optional) setting to enable oauthbearer client to urlencode client_id and
client_secret in the authorization header"
+ + " in accordance with RFC6749, see
https://datatracker.ietf.org/doc/html/rfc6749#section-2.3.1 for more detail.
The default value is set to 'false' for backward compatibility";
+
+
Review Comment:
Sorry, another nit about extra whitespace.
```suggestion
```
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java:
##########
@@ -346,10 +350,17 @@ static String parseAccessToken(String responseBody)
throws IOException {
return sanitizeString("the token endpoint response's access_token JSON
attribute", accessTokenNode.textValue());
}
- static String formatAuthorizationHeader(String clientId, String
clientSecret) {
+ static String formatAuthorizationHeader(String clientId, String
clientSecret, boolean urlencode) throws
Review Comment:
Oh, I see, this is a `static` method. I don't remember why that was
necessary 🤷♂️
##########
clients/src/main/java/org/apache/kafka/common/config/SaslConfigs.java:
##########
@@ -192,6 +192,12 @@ public class SaslConfigs {
+ " be inspected for the standard OAuth \"iss\" claim and if this
value is set, the broker will match it exactly against what is in the JWT's
\"iss\" claim. If there is no"
+ " match, the broker will reject the JWT and authentication will
fail.";
+ public static final String SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE =
"sasl.oauthbearer.header.urlencode.enable";
Review Comment:
Is there a KIP associated with this Jira ticket? Unfortunately, additions of
configuration options required a KIP 😞
##########
clients/src/main/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetriever.java:
##########
@@ -346,10 +350,17 @@ static String parseAccessToken(String responseBody)
throws IOException {
return sanitizeString("the token endpoint response's access_token JSON
attribute", accessTokenNode.textValue());
}
- static String formatAuthorizationHeader(String clientId, String
clientSecret) {
+ static String formatAuthorizationHeader(String clientId, String
clientSecret, boolean urlencode) throws
Review Comment:
Instead of passing in the `urlencode` parameter here, can we use the
`urlencodeHeader` directly?
##########
clients/src/main/java/org/apache/kafka/common/config/SaslConfigs.java:
##########
@@ -192,6 +192,12 @@ public class SaslConfigs {
+ " be inspected for the standard OAuth \"iss\" claim and if this
value is set, the broker will match it exactly against what is in the JWT's
\"iss\" claim. If there is no"
+ " match, the broker will reject the JWT and authentication will
fail.";
+ public static final String SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE =
"sasl.oauthbearer.header.urlencode.enable";
+ public static final boolean
DEFAULT_SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE = false;
+ public static final String SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE_DOC =
"The (optional) setting to enable oauthbearer client to urlencode client_id and
client_secret in the authorization header"
Review Comment:
Nitpick:
```suggestion
public static final String SASL_OAUTHBEARER_HEADER_URLENCODE_ENABLE_DOC
= "The (optional) setting to enable the OAuth client to URL-encode the
client_id and client_secret in the authorization header"
```
--
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
