[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165853678 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -155,9 +158,21 @@ public AuthenticatedUser authenticateUser(Credentials credentials) // This is a response to a previous challenge, authenticate with that. else { try { +String stateString = request.getParameter(RadiusStateField.PARAMETER_NAME); +if (stateString == null) { +logger.warn("Expected state parameter was not present in challenge/response."); +throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); +} + +byte[] stateBytes = DatatypeConverter.parseHexBinary(stateString); radPack = radiusService.sendChallengeResponse(credentials.getUsername(), - challengeResponse, - request.getParameter(RadiusStateField.PARAMETER_NAME)); + challengeResponse, + 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); --- End diff -- Whoa, not my best job of speling. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user mike-jumper commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165853668 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -155,9 +158,21 @@ public AuthenticatedUser authenticateUser(Credentials credentials) // This is a response to a previous challenge, authenticate with that. else { try { +String stateString = request.getParameter(RadiusStateField.PARAMETER_NAME); +if (stateString == null) { +logger.warn("Expected state parameter was not present in challenge/response."); +throw new GuacamoleInvalidCredentialsException("Authentication error.", CredentialsInfo.USERNAME_PASSWORD); +} + +byte[] stateBytes = DatatypeConverter.parseHexBinary(stateString); radPack = radiusService.sendChallengeResponse(credentials.getUsername(), - challengeResponse, - request.getParameter(RadiusStateField.PARAMETER_NAME)); + challengeResponse, + stateBytes); +} +catch (IllegalArgumentException e) { +logger.warn("Illegal hexadecimal value while parsing RADIUS state string.", e.getMessage()); --- End diff -- If you are going to include `e.getMessage()` within the log message, you will need to use the `{}` placeholder (analogous to `%s` of `printf`). See: https://github.com/apache/guacamole-client/blob/9b5483edc24d88a05075343c76377057de09f9e8/extensions/guacamole-auth-openid/src/main/java/org/apache/guacamole/auth/openid/token/TokenValidationService.java#L127 ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165833343 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java --- @@ -282,15 +282,15 @@ public RadiusPacket authenticate(String username, String secret, String state) * @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()) { logger.error("Challenge/response to RADIUS requires a username."); return null; } -if (state == null || state.isEmpty()) { +if (state == null || state.length < 1) { --- End diff -- Okay, should be done in a slightly more acceptable fashion, now. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r16589 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials credentials) // 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)); --- End diff -- Okay, removed the extra verbosity, handled the case where state field is null, and the `IllegalArgumentException`. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user mike-jumper commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165833178 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials credentials) // 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)); --- End diff -- Very true. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165833167 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials credentials) // 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)); --- End diff -- At least I was consistent. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user necouchman commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165833158 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -97,7 +99,7 @@ private CredentialsInfo getRadiusChallenge(RadiusPacket challengePacket) { // 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()); --- End diff -- Haha. Well, that was silly. Removed. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user mike-jumper commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165832815 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -97,7 +99,7 @@ private CredentialsInfo getRadiusChallenge(RadiusPacket challengePacket) { // 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()); --- End diff -- Yowza. My knee-jerk reaction was to suggest that you import `javax.xml.bind.DatatypeConverter`, but I see you've actually already done that. ;) This should be switched over to just `DatatypeConverter.printHexBinary(...)`. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user mike-jumper commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165832875 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/AuthenticationProviderService.java --- @@ -155,9 +157,10 @@ public AuthenticatedUser authenticateUser(Credentials credentials) // 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)); --- End diff -- I'd like to also mention that this will likely throw a `NullPointerException` if the state field is not present in the request. At best, the behavior of [`parseHexBinary()`](https://docs.oracle.com/javase/7/docs/api/javax/xml/bind/DatatypeConverter.html#parseHexBinary(java.lang.String)) is not defined for the case where its sole parameter is `null`. The potential lack of the parameter in the request should probably be dealt with. ---
[GitHub] guacamole-client pull request #242: GUACAMOLE-197: Convert state to Hex stri...
Github user mike-jumper commented on a diff in the pull request: https://github.com/apache/guacamole-client/pull/242#discussion_r165833012 --- Diff: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java --- @@ -282,15 +282,15 @@ public RadiusPacket authenticate(String username, String secret, String state) * @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()) { logger.error("Challenge/response to RADIUS requires a username."); return null; } -if (state == null || state.isEmpty()) { +if (state == null || state.length < 1) { --- End diff -- Testing whether the array length is less than one is an odd way to check whether the array is empty. There is exactly one length value which represents an empty array, and while that value is indeed less than one, it'd make more sense to test against that value directly. ---