New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
GUACAMOLE-1855: Allow MFA modules to be configured to bypass or enforce for specific hosts. #911
base: main
Are you sure you want to change the base?
Conversation
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? |
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! |
Also figured out that I missed the actual code for host checking on the TOTP side. Oops. |
ceb770c
to
8ef21d3
Compare
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. |
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. |
0b2260c
to
5b04f6b
Compare
Okay, I think I've got the checks working. I'm currently hitting three issues:
|
351cd10
to
850533c
Compare
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. |
I've seen this before, as well - I think it's due to the local OS' networking stack occasionally choosing the IPv6 address for |
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. |
850533c
to
8ffcdac
Compare
.../guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java
Outdated
Show resolved
Hide resolved
8ffcdac
to
360862d
Compare
guacamole-ext/src/main/java/org/apache/guacamole/net/util/IPAddressUtil.java
Outdated
Show resolved
Hide resolved
guacamole-ext/src/main/java/org/apache/guacamole/properties/IPAddressListProperty.java
Outdated
Show resolved
Hide resolved
private static final Pattern DELIMITER_PATTERN = Pattern.compile(",\\s*"); | ||
|
||
@Override | ||
public List<IPAddress> parseValue(String values) throws GuacamoleException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm on the fence about directly exposing an object from a third-party library as part of the extension API. Should we ever decide to switch to a different IP address library, this would encumber that switch.
Would Java's standard InetAddress
work? I see IPAddress
provides a toInetAddress()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the main challenge with that approach is that the list, as implemented with the IPAddress
class, is designed to take a combination of IPv4 addresses (1.2.3.4), IPv4 CIDR notation (1.2.3.0/24), IPv6 addresses, and IPv6 CIDR notation, and handle all of those possibilities in a single list. I haven't found anything conclusive, nor have I tested it, but I'm pretty sure the built-in InetAddress
class can only handle IP addresses - possibly either IPv4 or IPv6 - but not CIDR notation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mike-jumper Any further thoughts on this one?
.../guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java
Outdated
Show resolved
Hide resolved
...ole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java
Outdated
Show resolved
Hide resolved
10d6aae
to
edc1f03
Compare
72bb673
to
4715508
Compare
f286f09
to
9baad08
Compare
9baad08
to
ab78fe2
Compare
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.