Re: [PR] KAFKA-16345: Optionally URL-encode clientID and clientSecret in authorization header [kafka]

[via GitHub]

kirktrue commented on code in PR #15475:
URL: https://github.com/apache/kafka/pull/15475#discussion_r1668868527


##########
clients/src/test/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetrieverTest.java:
##########
@@ -174,33 +176,39 @@ public void testParseAccessTokenInvalidJson() {
     }
 
     @Test
-    public void testFormatAuthorizationHeader() {
-        assertAuthorizationHeader("id", "secret");
+    public void testFormatAuthorizationHeader() throws 
UnsupportedEncodingException {
+        assertAuthorizationHeader("id", "secret", false);
     }
 
     @Test
-    public void testFormatAuthorizationHeaderEncoding() {
-        // See KAFKA-14496
-        assertAuthorizationHeader("SOME_RANDOM_LONG_USER_01234", 
"9Q|0`8i~ute-n9ksjLWb\\50\"AX@UUED5E");
+    public void testFormatAuthorizationHeaderEncoding() throws 
UnsupportedEncodingException {
+        // according to RFC-7617, we need to use the *non-URL safe* base64 
encoder. See KAFKA-14496.
+        assertAuthorizationHeader("SOME_RANDOM_LONG_USER_01234", 
"9Q|0`8i~ute-n9ksjLWb\\50\"AX@UUED5E", false);
+        // according to RFC-6749 clientId & clientSecret must be urlencoded, 
see https://tools.ietf.org/html/rfc6749#section-2.3.1
+        assertAuthorizationHeader("user!@~'", "secret-(*)!", true);
     }
 
-    private void assertAuthorizationHeader(String clientId, String 
clientSecret) {
+    private void assertAuthorizationHeader(String clientId, String 
clientSecret, boolean urlencode) throws UnsupportedEncodingException {
+        String actual = 
HttpAccessTokenRetriever.formatAuthorizationHeader(clientId, clientSecret, 
urlencode);
+        if (urlencode) {
+            clientId = URLEncoder.encode(clientId, 
StandardCharsets.UTF_8.name());
+            clientSecret = URLEncoder.encode(clientSecret, 
StandardCharsets.UTF_8.name());
+        }
         String expected = "Basic " + 
Base64.getEncoder().encodeToString(Utils.utf8(clientId + ":" + clientSecret));
-        String actual = 
HttpAccessTokenRetriever.formatAuthorizationHeader(clientId, clientSecret);
         assertEquals(expected, actual, String.format("Expected the HTTP 
Authorization header generated for client ID \"%s\" and client secret \"%s\" to 
match", clientId, clientSecret));
     }
 
     @Test
     public void testFormatAuthorizationHeaderMissingValues() {
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader(null, "secret"));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("id", null));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader(null, null));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("", "secret"));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("id", ""));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("", ""));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("  ", "secret"));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("id", "  "));
-        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("  ", "  "));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader(null, "secret", false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("id", null, false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader(null, null, false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("", "secret", false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("id", "", false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("", "", false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("  ", "secret", false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("id", "  ", false));
+        assertThrows(IllegalArgumentException.class, () -> 
HttpAccessTokenRetriever.formatAuthorizationHeader("  ", "  ", false));

Review Comment:
   I'd love to see `testFormatAuthorizationHeaderMissingValues()` refactored as 
a `@ParameterizedTest`, but that's not a requirement for me.



##########
clients/src/test/java/org/apache/kafka/common/security/oauthbearer/internals/secured/HttpAccessTokenRetrieverTest.java:
##########
@@ -174,33 +176,39 @@ public void testParseAccessTokenInvalidJson() {
     }
 
     @Test
-    public void testFormatAuthorizationHeader() {
-        assertAuthorizationHeader("id", "secret");
+    public void testFormatAuthorizationHeader() throws 
UnsupportedEncodingException {
+        assertAuthorizationHeader("id", "secret", false);
     }
 
     @Test
-    public void testFormatAuthorizationHeaderEncoding() {
+    public void testFormatAuthorizationHeaderEncoding() throws 
UnsupportedEncodingException {
         // See KAFKA-14496
-        assertAuthorizationHeader("SOME_RANDOM_LONG_USER_01234", 
"9Q|0`8i~ute-n9ksjLWb\\50\"AX@UUED5E");
+        assertAuthorizationHeader("SOME_RANDOM_LONG_USER_01234", 
"9Q|0`8i~ute-n9ksjLWb\\50\"AX@UUED5E", false);
+        // See KAFKA-16345
+        assertAuthorizationHeader("user!@~'", "secret-(*)!", true);
     }
 
-    private void assertAuthorizationHeader(String clientId, String 
clientSecret) {
+    private void assertAuthorizationHeader(String clientId, String 
clientSecret, boolean urlencode) throws UnsupportedEncodingException {

Review Comment:
   I agree that it "is a bit weird" 😄
   
   What about moving the `expected` value to be a parameter to 
`assertAuthorizationHeader()` that contains the actual, pre-computed value 
instead of recalculating it inside the method? 
   
   ```diff
   - assertAuthorizationHeader("SOME_RANDOM_LONG_USER_01234", 
"9Q|0`8i~ute-n9ksjLWb\\50\"AX@UUED5E", false);
   + assertAuthorizationHeader("SOME_RANDOM_LONG_USER_01234", 
"9Q|0`8i~ute-n9ksjLWb\\50\"AX@UUED5E", false, 
"U09NRV9SQU5ET01fTE9OR19VU0VSXzAxMjM0OjlRfDBgOGl+dXRlLW45a3NqTFdiXDUwIkFYQFVVRUQ1RQ==");
   - assertAuthorizationHeader("user!@~'", "secret-(*)!", true);
   + assertAuthorizationHeader("user!@~'", "secret-(*)!", true, 
"dXNlciUyMSU0MH46c2VjcmV0LSUyOCUyQSUyOSUyMQ==");
   ```
   
   **Note**: I made a very quick attempt to generate the correct value for 
`expected` in the above code snippet for illustrative purposes.



-- 
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