GUACAMOLE-1855: Allow MFA modules to be configured to bypass or enforce for specific hosts.

[necouchman]

This pull request implements the feature proposed in Jira for GUACAMOLE-1855 - the Duo and TOTP modules can be configured to either be bypassed for users logging in from specific hosts, or can be configured to be explicitly required for users logging in from specific hosts, while other hosts are bypassed.

This allows situations where users connecting from the Internet ought to be required to complete MFA, but users logging in from behind a VPN or on a corporate network may not need to perform the extra authentication step.

[mike-jumper]

Looks pretty neat. What's the intended behavior for the case that an address matches subnets/addresses within both the enforce list and the bypass list?

[necouchman]

What's the intended behavior for the case that an address matches subnets/addresses within both the enforce list and the bypass list?

That's a good point - probably should give some more intentional thought to that. As it is currently written, as soon as an address is found in the bypass list, the user verification function will return, bypassing MFA authentication, and the enforcement list will never be processed.

I'm tempted to re-arrange things a bit where enforcement would actually "take precedence", and that a host in both lists would always be enforced. That seems like the more security-conscious way to go.

I'm open to suggestions, opinions, or discussion, though!

[necouchman]

Also figured out that I missed the actual code for host checking on the TOTP side. Oops.

[necouchman]

Okay, I've restructured things a bit such that having hosts listed in the enforce list should override anything listed in the bypass list. I need to do some testing before it's ready, so I'll mark this as a draft for the moment - in the meantime if you see anything else amiss, please let me know.

[mike-jumper]

Sounds good! And I agree regarding giving enforcement priority over bypass. Better for a mistake in configuration to result in excessive enforcement of MFA than unexpected bypassing of MFA.

[necouchman]

Okay, I think I've got the checks working. I'm currently hitting three issues:

• Build is impacted by the odd Bouncycastle version update. Once that's fixed the build will work properly.
• If I try to use Tomcat with its default IPv4 + IPv6 dual-stack enabled, the request.getRemoteAddr() call periodically flips between the actual IPv4 client IP and an IPv6 loopback IP (0:0:0:0:0:0:0:0:1 - or some number of 0s). I was able to pretty well remove this by adding -Djava.net.preferIPv4Stack=true to CATALINA_OPTS in the Tomcat startup, but ideally this would be able to reliably handle both IPv4 and IPv6. If anyone has any hints for that, I'm open.
• I had to make some unusual changes to the IPAddress detection code because I couldn't loop through a List of IPAddress types - I'd get an error about how IPv4Address couldn't be cast to IPAddress. What's even more weird is that I converted everything to IPAddressString data types, and I can't even do a for (IPAddressString ipAddr : ipAddrList) - I get the same error about being unable to cast it. Again, if anyone has hints, I'm open to them.

[necouchman]

Alrighty then - this should be ready for review, now. I've tested with the TOTP module and it appears to work as expected, at least with IPv4 addresses. I still have a bit of an issue with the IPv4 + IPv6 config and the client address flipping between a real IPv4 address and IPv6 loopback, but that seems to be more of a Nginx + Tomcat issue than it is anything to do with this.

[mike-jumper]

I still have a bit of an issue with the IPv4 + IPv6 config and the client address flipping between a real IPv4 address and IPv6 loopback, but that seems to be more of a Nginx + Tomcat issue than it is anything to do with this.

I've seen this before, as well - I think it's due to the local OS' networking stack occasionally choosing the IPv6 address for localhost instead of IPv4, but the default RemoteIpValve regex only matches IPv4 addresses.

[necouchman]

I've seen this before, as well - I think it's due to the local OS' networking stack occasionally choosing the IPv6 address for localhost instead of IPv4, but the default RemoteIpValve regex only matches IPv4 addresses.

Ah, okay - and, now that you mention it, it seems like something we've discussed before. Maybe I'll put in a Jira issue and update the manual page with that information.

[WilliamHorner]

I was having a look at this pull request because this functionality will be very useful. But I am hitting two compile errors:

[ERROR] ..../guacamole-client/extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java:[178,20] error: variable credentials is already defined in method verifyAuthenticatedUser(AuthenticatedUser)
[ERROR] ..../guacamole-client/extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java:[179,27] error: variable request is already defined in method verifyAuthenticatedUser(AuthenticatedUser)

Im not sure if this is my mishandling of git, but if I look at the file in the pull request I can see the two definitions there too.
I think it is probably just as simple as removing the second set of definitions? ... or fixing my understanding of git.

[necouchman]

I was having a look at this pull request because this functionality will be very useful. But I am hitting two compile errors:

[ERROR] ..../guacamole-client/extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java:[178,20] error: variable credentials is already defined in method verifyAuthenticatedUser(AuthenticatedUser) [ERROR] ..../guacamole-client/extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java:[179,27] error: variable request is already defined in method verifyAuthenticatedUser(AuthenticatedUser)

Im not sure if this is my mishandling of git, but if I look at the file in the pull request I can see the two definitions there too. I think it is probably just as simple as removing the second set of definitions? ... or fixing my understanding of git.

It should be fixed, now - apparently I didn't verify that it compiled after the last round of changes I made. Oops.