From f72053d6dbd6790c42c9a8e7bbac83b840236415 Mon Sep 17 00:00:00 2001 From: Andrew Azores Date: Mon, 15 Mar 2021 15:37:10 +0000 Subject: [PATCH] Use URL-safe Base64 encoder/decoder (#419) * Use URL-safe Base64 encoder/decoder Use encoder which produces, and decoder which expects, the URL-safe Base64 alphabet variant. The decoder is also tolerant of missing padding characters at the end, which the client will not send due to their illegality in the WebSocket Subprotocol header value * Update web-client to include base64url client-side changes --- .../containerjfr/net/BasicAuthManager.java | 4 +- .../net/OpenShiftAuthManager.java | 2 +- .../AbstractAuthenticatedRequestHandler.java | 2 +- .../http/api/v2/AbstractV2RequestHandler.java | 2 +- .../net/BasicAuthManagerTest.java | 52 +++++++++++++++++++ .../InterleavedExternalTargetRequestsIT.java | 8 +-- web-client | 2 +- 7 files changed, 62 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManager.java b/src/main/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManager.java index 9aa2b69570..ca74535958 100644 --- a/src/main/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManager.java +++ b/src/main/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManager.java @@ -112,7 +112,7 @@ public Future validateHttpHeader(Supplier headerProvider) { String b64 = matcher.group(1); try { String decoded = - new String(Base64.getDecoder().decode(b64), StandardCharsets.UTF_8).trim(); + new String(Base64.getUrlDecoder().decode(b64), StandardCharsets.UTF_8).trim(); return validateToken(() -> decoded); } catch (IllegalArgumentException e) { return CompletableFuture.completedFuture(false); @@ -136,7 +136,7 @@ public Future validateWebSocketSubProtocol(Supplier subProtocol String b64 = matcher.group(1); try { String decoded = - new String(Base64.getDecoder().decode(b64), StandardCharsets.UTF_8).trim(); + new String(Base64.getUrlDecoder().decode(b64), StandardCharsets.UTF_8).trim(); return validateToken(() -> decoded); } catch (IllegalArgumentException e) { return CompletableFuture.completedFuture(false); diff --git a/src/main/java/com/redhat/rhjmc/containerjfr/net/OpenShiftAuthManager.java b/src/main/java/com/redhat/rhjmc/containerjfr/net/OpenShiftAuthManager.java index 0fa498ac12..d23033413f 100644 --- a/src/main/java/com/redhat/rhjmc/containerjfr/net/OpenShiftAuthManager.java +++ b/src/main/java/com/redhat/rhjmc/containerjfr/net/OpenShiftAuthManager.java @@ -137,7 +137,7 @@ public Future validateWebSocketSubProtocol(Supplier subProtocol String b64 = matcher.group(1); try { String decoded = - new String(Base64.getDecoder().decode(b64), StandardCharsets.UTF_8).trim(); + new String(Base64.getUrlDecoder().decode(b64), StandardCharsets.UTF_8).trim(); return validateToken(() -> decoded); } catch (IllegalArgumentException e) { return CompletableFuture.completedFuture(false); diff --git a/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/AbstractAuthenticatedRequestHandler.java b/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/AbstractAuthenticatedRequestHandler.java index d61eb47de7..9dd8342def 100644 --- a/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/AbstractAuthenticatedRequestHandler.java +++ b/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/AbstractAuthenticatedRequestHandler.java @@ -130,7 +130,7 @@ protected ConnectionDescriptor getConnectionDescriptorFromContext(RoutingContext try { c = new String( - Base64.getDecoder().decode(m.group("credentials")), + Base64.getUrlDecoder().decode(m.group("credentials")), StandardCharsets.UTF_8); } catch (IllegalArgumentException iae) { ctx.response().putHeader(JMX_AUTHENTICATE_HEADER, "Basic"); diff --git a/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/api/v2/AbstractV2RequestHandler.java b/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/api/v2/AbstractV2RequestHandler.java index 18587d266d..84942bffbb 100644 --- a/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/api/v2/AbstractV2RequestHandler.java +++ b/src/main/java/com/redhat/rhjmc/containerjfr/net/web/http/api/v2/AbstractV2RequestHandler.java @@ -146,7 +146,7 @@ protected ConnectionDescriptor getConnectionDescriptorFromParams(RequestParamete try { c = new String( - Base64.getDecoder().decode(m.group("credentials")), + Base64.getUrlDecoder().decode(m.group("credentials")), StandardCharsets.UTF_8); } catch (IllegalArgumentException iae) { params.getHeaders().set(JMX_AUTHENTICATE_HEADER, "Basic"); diff --git a/src/test/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManagerTest.java b/src/test/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManagerTest.java index 2a68879883..3d64567724 100644 --- a/src/test/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManagerTest.java +++ b/src/test/java/com/redhat/rhjmc/containerjfr/net/BasicAuthManagerTest.java @@ -308,6 +308,7 @@ void shouldPassKnownCredentials() throws Exception { Mockito.when(fs.exists(mockPath)).thenReturn(true); Mockito.when(fs.isRegularFile(mockPath)).thenReturn(true); Mockito.when(fs.isReadable(mockPath)).thenReturn(true); + // credentials: "user:pass" BufferedReader props = new BufferedReader( new StringReader( @@ -319,6 +320,56 @@ void shouldPassKnownCredentials() throws Exception { .get()); } + @Test + void shouldPassKnownCredentialsWithPadding() throws Exception { + Path mockPath = Mockito.mock(Path.class); + Mockito.when( + fs.pathOf( + System.getProperty("user.home"), + BasicAuthManager.USER_PROPERTIES_FILENAME)) + .thenReturn(mockPath); + Mockito.when(fs.exists(mockPath)).thenReturn(true); + Mockito.when(fs.isRegularFile(mockPath)).thenReturn(true); + Mockito.when(fs.isReadable(mockPath)).thenReturn(true); + // credentials: "user:pass1234" + BufferedReader props = + new BufferedReader( + new StringReader( + "user:bd94dcda26fccb4e68d6a31f9b5aac0b571ae266d822620e901ef7ebe3a11d4f")); + Mockito.when(fs.readFile(mockPath)).thenReturn(props); + Assertions.assertTrue( + mgr.validateWebSocketSubProtocol( + () -> "basic.authorization.containerjfr.dXNlcjpwYXNzMTIzNA==") + .get()); + } + + @Test + void shouldPassKnownCredentialsAndStrippedPadding() throws Exception { + Path mockPath = Mockito.mock(Path.class); + Mockito.when( + fs.pathOf( + System.getProperty("user.home"), + BasicAuthManager.USER_PROPERTIES_FILENAME)) + .thenReturn(mockPath); + Mockito.when(fs.exists(mockPath)).thenReturn(true); + Mockito.when(fs.isRegularFile(mockPath)).thenReturn(true); + Mockito.when(fs.isReadable(mockPath)).thenReturn(true); + // credentials: "user:pass1234" + BufferedReader props = + new BufferedReader( + new StringReader( + "user:bd94dcda26fccb4e68d6a31f9b5aac0b571ae266d822620e901ef7ebe3a11d4f")); + Mockito.when(fs.readFile(mockPath)).thenReturn(props); + // the subprotocol token part here should be "dXNlcjpwYXNzMTIzNA==", but the '='s are + // padding and stripped out. The decoder should treat these as optional, and the client + // is likely not to send them since they are not permitted by the WebSocket + // specification for the Sec-WebSocket-Protocol header + Assertions.assertTrue( + mgr.validateWebSocketSubProtocol( + () -> "basic.authorization.containerjfr.dXNlcjpwYXNzMTIzNA") + .get()); + } + @Test void shouldFailUnknownCredentials() throws Exception { Path mockPath = Mockito.mock(Path.class); @@ -330,6 +381,7 @@ void shouldFailUnknownCredentials() throws Exception { Mockito.when(fs.exists(mockPath)).thenReturn(true); Mockito.when(fs.isRegularFile(mockPath)).thenReturn(true); Mockito.when(fs.isReadable(mockPath)).thenReturn(true); + // credentials: "user:pass" BufferedReader props = new BufferedReader( new StringReader( diff --git a/src/test/java/itest/InterleavedExternalTargetRequestsIT.java b/src/test/java/itest/InterleavedExternalTargetRequestsIT.java index 7773078a34..b2ff1e0c78 100644 --- a/src/test/java/itest/InterleavedExternalTargetRequestsIT.java +++ b/src/test/java/itest/InterleavedExternalTargetRequestsIT.java @@ -168,7 +168,7 @@ private void createInMemoryRecordings() throws Exception { .putHeader( "X-JMX-Authorization", "Basic " - + Base64.getEncoder() + + Base64.getUrlEncoder() .encodeToString( "admin:adminpass123".getBytes())) .sendForm( @@ -198,7 +198,7 @@ private void verifyInMemoryRecordingsCreated() throws Exception { .putHeader( "X-JMX-Authorization", "Basic " - + Base64.getEncoder() + + Base64.getUrlEncoder() .encodeToString("admin:adminpass123".getBytes())) .send( ar -> { @@ -242,7 +242,7 @@ private void deleteInMemoryRecordings() throws Exception { .putHeader( "X-JMX-Authorization", "Basic " - + Base64.getEncoder() + + Base64.getUrlEncoder() .encodeToString( "admin:adminpass123".getBytes())) .sendForm( @@ -272,7 +272,7 @@ private void verifyInmemoryRecordingsDeleted() throws Exception { .putHeader( "X-JMX-Authorization", "Basic " - + Base64.getEncoder() + + Base64.getUrlEncoder() .encodeToString("admin:adminpass123".getBytes())) .send( ar -> { diff --git a/web-client b/web-client index d8198c1a49..8ccdd5b361 160000 --- a/web-client +++ b/web-client @@ -1 +1 @@ -Subproject commit d8198c1a49c40885bffea86965f169d8c54fc1e9 +Subproject commit 8ccdd5b361f10d9b3137218fb1728efe5e73ce55