From 769a34f511d3d00d5a34f8f6816521fdd8f847c6 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sat, 3 Feb 2018 15:45:38 -0500 Subject: [PATCH 1/7] GUACAMOLE-197: Convert state to Hex string to avoid encoding issues. --- .../auth/radius/AuthenticationProviderService.java | 7 +++++-- .../guacamole/auth/radius/RadiusConnectionService.java | 8 ++++---- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java index ae9f6bfd7..d5005b227 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java @@ -21,8 +21,10 @@ package org.apache.guacamole.auth.radius; import com.google.inject.Inject; import com.google.inject.Provider; +import java.nio.charset.Charset; import java.util.Arrays; import javax.servlet.http.HttpServletRequest; +import javax.xml.bind.DatatypeConverter; import org.apache.guacamole.auth.radius.user.AuthenticatedUser; import org.apache.guacamole.auth.radius.form.RadiusChallengeResponseField; import org.apache.guacamole.auth.radius.form.RadiusStateField; @@ -97,7 +99,7 @@ public class AuthenticationProviderService { // We have the required attributes - convert to strings and then generate the additional login box/field String replyMsg = replyAttr.toString(); - String radiusState = new String(stateAttr.getValue().getBytes()); + String radiusState = javax.xml.bind.DatatypeConverter.printHexBinary(stateAttr.getValue().getBytes()); Field radiusResponseField = new RadiusChallengeResponseField(replyMsg); Field radiusStateField = new RadiusStateField(radiusState); @@ -155,9 +157,10 @@ public class AuthenticationProviderService { // This is a response to a previous challenge, authenticate with that. else { try { + byte[] stateBytes = javax.xml.bind.DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME)); radPack = radiusService.sendChallengeResponse(credentials.getUsername(), challengeResponse, - request.getParameter(RadiusStateField.PARAMETER_NAME)); + stateBytes); } catch (GuacamoleException e) { logger.error("Cannot configure RADIUS server: {}", e.getMessage()); diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java index 43c0b2d8a..8355557e8 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java @@ -187,7 +187,7 @@ public class RadiusConnectionService { * @throws GuacamoleException * If an error occurs while talking to the server. */ - public RadiusPacket authenticate(String username, String secret, String state) + public RadiusPacket authenticate(String username, String secret, byte[] state) throws GuacamoleException { // If a username wasn't passed, we quit @@ -219,7 +219,7 @@ public class RadiusConnectionService { try { AttributeList radAttrs = new AttributeList(); radAttrs.add(new Attr_UserName(username)); - if (state != null && !state.isEmpty()) + if (state != null && state.length > 0) radAttrs.add(new Attr_State(state)); radAttrs.add(new Attr_UserPassword(secret)); radAttrs.add(new Attr_CleartextPassword(secret)); @@ -282,7 +282,7 @@ public class RadiusConnectionService { * @throws GuacamoleException * If an error is encountered trying to talk to the RADIUS server. */ - public RadiusPacket sendChallengeResponse(String username, String response, String state) + public RadiusPacket sendChallengeResponse(String username, String response, byte[] state) throws GuacamoleException { if (username == null || username.isEmpty()) { @@ -290,7 +290,7 @@ public class RadiusConnectionService { return null; } - if (state == null || state.isEmpty()) { + if (state == null || state.length < 1) { logger.error("Challenge/response to RADIUS requires a prior state."); return null; } From 77b4665c5cbed27c0d6e590cc58fc7080bb3dfca Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sat, 3 Feb 2018 23:11:22 -0500 Subject: [PATCH 2/7] GUACAMOLE-197: Remove unnecessarily precise method calls. --- .../guacamole/auth/radius/AuthenticationProviderService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java index d5005b227..1ff84c9ef 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java @@ -99,7 +99,7 @@ public class AuthenticationProviderService { // We have the required attributes - convert to strings and then generate the additional login box/field String replyMsg = replyAttr.toString(); - String radiusState = javax.xml.bind.DatatypeConverter.printHexBinary(stateAttr.getValue().getBytes()); + String radiusState = DatatypeConverter.printHexBinary(stateAttr.getValue().getBytes()); Field radiusResponseField = new RadiusChallengeResponseField(replyMsg); Field radiusStateField = new RadiusStateField(radiusState); @@ -157,7 +157,7 @@ public class AuthenticationProviderService { // This is a response to a previous challenge, authenticate with that. else { try { - byte[] stateBytes = javax.xml.bind.DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME)); + byte[] stateBytes = DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME)); radPack = radiusService.sendChallengeResponse(credentials.getUsername(), challengeResponse, stateBytes); From 41dd9fc60dfe27c8e067abb9c2c797c2af49f965 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sat, 3 Feb 2018 23:17:30 -0500 Subject: [PATCH 3/7] GUACAMOLE-197: Deal with null state field.. --- .../auth/radius/AuthenticationProviderService.java | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java index 1ff84c9ef..bee0433fd 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java @@ -157,10 +157,17 @@ public class AuthenticationProviderService { // This is a response to a previous challenge, authenticate with that. else { try { - byte[] stateBytes = DatatypeConverter.parseHexBinary(request.getParameter(RadiusStateField.PARAMETER_NAME)); + String stateString = request.getParameter(RadiusStateField.PARAMETER_NAME); + if (stateString == null) { + logger.error("Could not retrieve RADIUS state."); + logger.debug("Received null value while retrieving RADIUS state parameter."); + throws new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); + } + + byte[] stateBytes = DatatypeConverter.parseHexBinary(stateString); radPack = radiusService.sendChallengeResponse(credentials.getUsername(), - challengeResponse, - stateBytes); + challengeResponse, + stateBytes); } catch (GuacamoleException e) { logger.error("Cannot configure RADIUS server: {}", e.getMessage()); From 0729fa09cdd41eb497bb4542bce0be40b11442c6 Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sat, 3 Feb 2018 23:23:27 -0500 Subject: [PATCH 4/7] GUACAMOLE-197: Handle IllegalArgumentException when parsing state string. --- .../auth/radius/AuthenticationProviderService.java | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java index bee0433fd..2418c8ee5 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java @@ -21,6 +21,7 @@ package org.apache.guacamole.auth.radius; import com.google.inject.Inject; import com.google.inject.Provider; +import java.lang.IllegalArgumentException; import java.nio.charset.Charset; import java.util.Arrays; import javax.servlet.http.HttpServletRequest; @@ -161,7 +162,7 @@ public class AuthenticationProviderService { if (stateString == null) { logger.error("Could not retrieve RADIUS state."); logger.debug("Received null value while retrieving RADIUS state parameter."); - throws new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); + throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); } byte[] stateBytes = DatatypeConverter.parseHexBinary(stateString); @@ -169,6 +170,11 @@ public class AuthenticationProviderService { challengeResponse, stateBytes); } + catch (IllegalArgumentException e) { + logger.error("Illegal argument while parsing RADIUS state string.", e.getMessage()); + logger.debug("Illegal argument found while parsing the RADIUS state string.", e); + throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); + } catch (GuacamoleException e) { logger.error("Cannot configure RADIUS server: {}", e.getMessage()); logger.debug("Error configuring RADIUS server.", e); From ef6d0c6a5386e70cbca242886d17953eeaa2591c Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sat, 3 Feb 2018 23:24:25 -0500 Subject: [PATCH 5/7] GUACAMOLE-197: Check for empty byte array in more sane manner. --- .../apache/guacamole/auth/radius/RadiusConnectionService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java index 8355557e8..ec82a63ee 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java @@ -290,7 +290,7 @@ public class RadiusConnectionService { return null; } - if (state == null || state.length < 1) { + if (state == null || state.length == 0) { logger.error("Challenge/response to RADIUS requires a prior state."); return null; } From b9a68804709bcb71d46b3ba8ee312821fbd705dd Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sun, 4 Feb 2018 09:31:47 -0500 Subject: [PATCH 6/7] GUACAMOLE-197: Brush up log messages and change levels for invalid state information. --- .../auth/radius/AuthenticationProviderService.java | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java index 2418c8ee5..630173438 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java @@ -160,8 +160,7 @@ public class AuthenticationProviderService { try { String stateString = request.getParameter(RadiusStateField.PARAMETER_NAME); if (stateString == null) { - logger.error("Could not retrieve RADIUS state."); - logger.debug("Received null value while retrieving RADIUS state parameter."); + logger.warn("Expected state parameter was not present in challenge/response."); throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); } @@ -171,8 +170,8 @@ public class AuthenticationProviderService { stateBytes); } catch (IllegalArgumentException e) { - logger.error("Illegal argument while parsing RADIUS state string.", e.getMessage()); - logger.debug("Illegal argument found while parsing the RADIUS state string.", e); + logger.warn("Illegal hexadecimal value while parsing RADIUS state string.", e.getMessage()); + logger.debug("Encountered exception while attepmting to perse the hexidecimanl state value.", e); throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); } catch (GuacamoleException e) { From 4195ac6d19c42e25137b26cf81f09af7344beada Mon Sep 17 00:00:00 2001 From: Nick Couchman Date: Sun, 4 Feb 2018 15:56:59 -0500 Subject: [PATCH 7/7] GUACAMOLE-197: Fix errors that happen when you try to code before coffee. --- .../guacamole/auth/radius/AuthenticationProviderService.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java index 630173438..852eb7206 100644 --- a/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java +++ b/extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java @@ -170,8 +170,8 @@ public class AuthenticationProviderService { stateBytes); } catch (IllegalArgumentException e) { - logger.warn("Illegal hexadecimal value while parsing RADIUS state string.", e.getMessage()); - logger.debug("Encountered exception while attepmting to perse the hexidecimanl state value.", e); + logger.warn("Illegal hexadecimal value while parsing RADIUS state string: {}", e.getMessage()); + logger.debug("Encountered exception while attempting to parse the hexidecimal state value.", e); throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); } catch (GuacamoleException e) {