[GitHub] [guacamole-client] mike-jumper commented on a change in pull request #392: GUACAMOLE-774: Add in MD4 support for MSCHAPv1/2

[GitBox <gi...@apache.org>]

mike-jumper commented on a change in pull request #392: GUACAMOLE-774: Add in MD4 support for MSCHAPv1/2
URL: https://github.com/apache/guacamole-client/pull/392#discussion_r273619494
 
 

 ##########
 File path: extensions/guacamole-auth-radius/src/main/java/org/apache/guacamole/auth/radius/RadiusConnectionService.java
 ##########
 @@ -129,6 +133,18 @@ private RadiusAuthenticator setupRadiusAuthenticator(RadiusClient radiusClient)
         if (radAuth == null)
             throw new GuacamoleException("Could not get a valid RadiusAuthenticator for specified protocol: " + confService.getRadiusAuthProtocol());
 
+        // For MSCHAPv1/2, we need MD4 support
+        if (radAuth instanceof MSCHAPv1Authenticator
+                || radAuth instanceof MSCHAPv2Authenticator) {
 
 Review comment:
   Rather than checking `instanceof` everywhere, and thus relying on the internals of JRadius, it would be better if `getRadiusAuthProtocol()` leveraged an `enum` that could be checked independently of whatever JRadius decides to return. Such an `enum` could provide a method for obtaining a corresponding instance of `RadiusAuthenticator` given a `RadiusClient`.
   
   Rechecking `getRadiusAuthProtocol()`, it is somewhat scary that the raw string value is passed straight to JRadius. I think there should be some validation on the value on the Guacamole side, and a decoupling from the underlying RADIUS library.

----------------------------------------------------------------
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.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services