mike-jumper commented on code in PR #911:
URL: https://github.com/apache/guacamole-client/pull/911#discussion_r1457957253
##########
extensions/guacamole-auth-duo/src/main/java/org/apache/guacamole/auth/duo/UserVerificationService.java:
##########
@@ -71,10 +75,38 @@ public void verifyAuthenticatedUser(AuthenticatedUser
authenticatedUser)
// Pull the original HTTP request used to authenticate
Credentials credentials = authenticatedUser.getCredentials();
HttpServletRequest request = credentials.getRequest();
+ IPAddress clientAddr = new
IPAddressString(request.getRemoteAddr()).getAddress();
// Ignore anonymous users
if
(authenticatedUser.getIdentifier().equals(AuthenticatedUser.ANONYMOUS_IDENTIFIER))
return;
+
+ // Pull address lists to check from configuration. Note that the
enforce
+ // list will override the bypass list, which means that, if the client
+ // address happens to be in both lists, Duo MFA will be enforced.
+ List<IPAddress> bypassAddresses = confService.getBypassHosts();
+ List<IPAddress> enforceAddresses = confService.getEnforceHosts();
+
+ // Check if the bypass list contains the client address, and set the
+ // enforce flag to the opposite.
+ boolean enforceHost =
!(IPAddressUtil.addressListContains(bypassAddresses, clientAddr));
+
+ // Only continue processing if the list is not empty
+ if (enforceAddresses != null && !enforceAddresses.isEmpty()) {
Review Comment:
`getEnforceHosts()` cannot return `null`. If the property is omitted,
`getEnforceHosts()` returns an empty list.
##########
guacamole-ext/src/main/java/org/apache/guacamole/net/util/IPAddressUtil.java:
##########
@@ -0,0 +1,62 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.net.util;
+
+import inet.ipaddr.IPAddress;
+import java.util.List;
+
+/**
+ * A class for utility functions dealing with IP addresses and lists.
+ */
+public class IPAddressUtil {
+
+ /**
+ * Return true if the provided address list contains the client address,
+ * or false if no match is found.
+ *
+ * @param addrList
+ * The address list to check for matches.
+ *
+ * @param ipAddr
+ * The client address to look for in the list.
+ *
+ * @return
+ * True if the client address is in the provided list, otherwise
+ * false.
+ */
+ public static boolean addressListContains(List<IPAddress> addrList,
IPAddress ipAddr) {
+
+ // If either is null, return false
+ if (ipAddr == null || addrList == null)
+ return false;
+
+ for (IPAddress ipEntry : addrList)
+
+ // If version matches and entry contains it, return true
+ if (ipEntry.getIPVersion().equals(ipAddr.getIPVersion())
+ && ipEntry.contains(ipAddr))
+ return true;
+
+ // No match, so return false
+ return false;
+
+ }
Review Comment:
I think this would have a better life as either a static convenience
function of `IPAddressListProperty`, or as a `contains()` instance method of
`IPAddressListProperty` that pulls the list from itself automatically.
##########
guacamole-ext/src/main/java/org/apache/guacamole/properties/IPAddressListProperty.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import inet.ipaddr.IPAddress;
+import inet.ipaddr.IPAddressString;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.regex.Pattern;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+
+/**
+ * A GuacamoleProperty implementation that parses a String for a
comma-separated
+ * list of IP addresses and/or IP subnets, both IPv4 and IPv6, and returns the
+ * list of those valid IP addresses/subnets.
+ */
+public abstract class IPAddressListProperty implements GuacamoleProperty<List
<IPAddress>> {
+
+ /**
+ * A pattern which matches against the delimiters between values. This is
+ * currently simply a comma and any following whitespace. Parts of the
+ * input string which match this pattern will not be included in the parsed
+ * result.
+ */
+ private static final Pattern DELIMITER_PATTERN = Pattern.compile(",\\s*");
+
+ @Override
+ public List<IPAddress> parseValue(String values) throws GuacamoleException
{
Review Comment:
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()`.
##########
extensions/guacamole-auth-totp/src/main/java/org/apache/guacamole/auth/totp/user/UserVerificationService.java:
##########
@@ -288,6 +292,45 @@ private boolean totpDisabled(UserContext context,
public void verifyIdentity(UserContext context,
AuthenticatedUser authenticatedUser) throws GuacamoleException {
+ // Pull the original HTTP request used to authenticate
+ Credentials credentials = authenticatedUser.getCredentials();
+ HttpServletRequest request = credentials.getRequest();
+
+ // Get the current client address
+ IPAddress clientAddr = new
IPAddressString(request.getRemoteAddr()).getAddress();
+
+ // Ignore anonymous users
+ if
(authenticatedUser.getIdentifier().equals(AuthenticatedUser.ANONYMOUS_IDENTIFIER))
+ return;
+
+ // Pull address lists to check from configuration. Note that the
enforce
+ // list will override the bypass list, which means that, if the client
+ // address happens to be in both lists, Duo MFA will be enforced.
+ List<IPAddress> bypassAddresses = confService.getBypassHosts();
+ List<IPAddress> enforceAddresses = confService.getEnforceHosts();
+
+ // Check the bypass list for the client address, and set the enforce
+ // flag to the opposite.
+ boolean enforceHost =
!(IPAddressUtil.addressListContains(bypassAddresses, clientAddr));
+
+ // Only continue processing if the list is not empty
+ if (enforceAddresses != null && !enforceAddresses.isEmpty()) {
Review Comment:
Same here: `getEnforceHosts()` cannot return `null`.
##########
guacamole-ext/src/main/java/org/apache/guacamole/properties/IPAddressListProperty.java:
##########
@@ -0,0 +1,81 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.guacamole.properties;
+
+import inet.ipaddr.IPAddress;
+import inet.ipaddr.IPAddressString;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+import java.util.regex.Pattern;
+import org.apache.guacamole.GuacamoleException;
+import org.apache.guacamole.GuacamoleServerException;
+
+/**
+ * A GuacamoleProperty implementation that parses a String for a
comma-separated
+ * list of IP addresses and/or IP subnets, both IPv4 and IPv6, and returns the
+ * list of those valid IP addresses/subnets.
+ */
+public abstract class IPAddressListProperty implements GuacamoleProperty<List
<IPAddress>> {
Review Comment:
Stray space here between `List` and `<IPAddress>`.
--
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.
To unsubscribe, e-mail: dev-unsubscr...@guacamole.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
