VLANs, accept_mac_file, and RADIUS

Message ID CAEcV5PpSYCYYXmpwVfMYu1xg2m4Z0cYQ8a4wDsn5nQD5V-Wy3w@mail.gmail.com
State New
Headers show
Series
  • VLANs, accept_mac_file, and RADIUS
Related show

Commit Message

Nils Nieuwejaar Jan. 26, 2018, 8:01 p.m.
Hi,

In my environment, I am using WPA-EAP to authenticate users, but I want the
VLAN assignment to come from the accept_mac_file rather than the RADIUS
server. I'm happy to get into my reasons for that if you care, but I don't
think it really matters.

The documentation in the hostapd.conf file says that the dynamic_vlan
variable is used to control whether VLAN assignments are accepted from a
RADIUS server. The implication seems to be that a static VLAN assignment
will come from the accept_mac_file if dynamic_vlan is set to 0, and a
dynamic assignment will come from the RADIUS server if dynamic_vlan is set
to 1. Instead, I'm seeing that the static settings from the
accept_mac_file are ignored if dynamic_vlan is set to 0, but used if
dynamic_vlan is set to 1. If dynamic_vlan is set to 1 and the RADIUS
server does not provide a VLAN, then the accept_mac_file assignment is
overridden and the STA is assigned to the default non-VLANed interface.

If my understanding of the expected behavior is correct, then I believe the
problem is in ap_sta_set_vlan(). That routine checks the dynamic_vlan
setting, but has no way of determining whether the incoming vlan_desc is
static (i.e., from accept_mac_file) or dynamic (i.e., from a RADIUS
server).

I've attached a patch that gets hostapd working as I believe it's meant to,
and updates the documentation to make the implicit behavior explicit.

The functional changes are:

- hostapd_allowed_address() will always extract the vlan_id from the
accept_macs file. It will not update the vlan_id from the RADIUS cache
if dynamic_vlan is DISABLED.

- hostapd_acl_recv_radius() will not update the cached vlan_id if
dynamic_vlan is DISABLED.

- ieee802_1x_receive_auth() will not update the vlan_id if dynamic_vlan
is DISABLED.

More cosmetic:

Most of the delta is just moving code out of ieee802_1x_receive_auth() into a
new ieee802_1x_update_vlan() routine. While I initially did this because the
new DISABLED check introduced excessive indentation, it has the added
advantage of eliminating the vlan_description allocation and os_memset() call
for all DYNAMIC_VLAN_DISABLED configs.

I've done a couple rounds of review offline with Michael Braun (who has
done much of the work in this part of the code) and incorporated his
feedback.

Nils

 		/* This sta is lacking its own vif */

Patch

diff --git a/hostapd/hostapd.conf b/hostapd/hostapd.conf
index 73e8fc39f..5401a3831 100644
--- a/hostapd/hostapd.conf
+++ b/hostapd/hostapd.conf
@@ -1104,7 +1104,7 @@  own_ip_addr=127.0.0.1
 # Tunnel-Medium-Type (value 6 = IEEE 802), Tunnel-Private-Group-ID (value
 # VLANID as a string). Optionally, the local MAC ACL list (accept_mac_file) can
 # be used to set static client MAC address to VLAN ID mapping.
-# 0 = disabled (default)
+# 0 = disabled (default); only VLAN IDs from accept_mac_file will be used
 # 1 = option; use default interface if RADIUS server does not include VLAN ID
 # 2 = required; reject authentication if RADIUS server does not include VLAN ID
 #dynamic_vlan=0
diff --git a/src/ap/ieee802_11_auth.c b/src/ap/ieee802_11_auth.c
index 3308398d1..c294c67cc 100644
--- a/src/ap/ieee802_11_auth.c
+++ b/src/ap/ieee802_11_auth.c
@@ -281,6 +281,9 @@  int hostapd_allowed_address(struct hostapd_data
*hapd, const u8 *addr,
 #else /* CONFIG_NO_RADIUS */
 		struct hostapd_acl_query_data *query;

+		if (hapd->conf->ssid.dynamic_vlan == DYNAMIC_VLAN_DISABLED)
+			vlan_id = NULL;
+
 		/* Check whether ACL cache has an entry for this station */
 		res = hostapd_acl_cache_get(hapd, addr, session_timeout,
 					    acct_interim_interval, vlan_id, psk,
@@ -566,12 +569,14 @@  hostapd_acl_recv_radius(struct radius_msg *msg,
struct radius_msg *req,
 			cache->acct_interim_interval = 0;
 		}

-		notempty = &cache->vlan_id.notempty;
-		untagged = &cache->vlan_id.untagged;
-		tagged = cache->vlan_id.tagged;
-		*notempty = !!radius_msg_get_vlanid(msg, untagged,
-						    MAX_NUM_TAGGED_VLAN,
-						    tagged);
+		if (hapd->conf->ssid.dynamic_vlan != DYNAMIC_VLAN_DISABLED) {
+			notempty = &cache->vlan_id.notempty;
+			untagged = &cache->vlan_id.untagged;
+			tagged = cache->vlan_id.tagged;
+			*notempty = !!radius_msg_get_vlanid(msg, untagged,
+							    MAX_NUM_TAGGED_VLAN,
+							    tagged);
+		}

 		decode_tunnel_passwords(hapd, shared_secret, shared_secret_len,
 					msg, req, cache);
diff --git a/src/ap/ieee802_1x.c b/src/ap/ieee802_1x.c
index 793d381ed..ffe539a4d 100644
--- a/src/ap/ieee802_1x.c
+++ b/src/ap/ieee802_1x.c
@@ -1673,6 +1673,49 @@  ieee802_1x_search_radius_identifier(struct
hostapd_data *hapd, u8 identifier)
 }


+#ifndef CONFIG_NO_VLAN
+static int
+ieee802_1x_update_vlan(struct radius_msg *msg,
+		       struct hostapd_data *hapd,
+		       struct sta_info *sta)
+{
+	struct vlan_description vlan_desc;
+	int *untagged, *tagged, *notempty;
+
+	os_memset(&vlan_desc, 0, sizeof(vlan_desc));
+	notempty = &vlan_desc.notempty;
+	untagged = &vlan_desc.untagged;
+	tagged = vlan_desc.tagged;
+	*notempty = !!radius_msg_get_vlanid(msg, untagged, MAX_NUM_TAGGED_VLAN,
+					    tagged);
+
+	if (vlan_desc.notempty &&
+	    !hostapd_vlan_valid(hapd->conf->vlan, &vlan_desc)) {
+		sta->eapol_sm->authFail = TRUE;
+		hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_RADIUS,
+			       HOSTAPD_LEVEL_INFO,
+			       "Invalid VLAN %d%s received from RADIUS server",
+			       vlan_desc.untagged,
+			       vlan_desc.tagged[0] ? "+" : "");
+		os_memset(&vlan_desc, 0, sizeof(vlan_desc));
+		ap_sta_set_vlan(hapd, sta, &vlan_desc);
+		return -1;
+	}
+
+	if (hapd->conf->ssid.dynamic_vlan == DYNAMIC_VLAN_REQUIRED &&
+	    !vlan_desc.notempty) {
+		sta->eapol_sm->authFail = TRUE;
+		hostapd_logger(hapd, sta->addr, HOSTAPD_MODULE_IEEE8021X,
+			       HOSTAPD_LEVEL_INFO, "authentication server did "
+			       "not include required VLAN ID in Access-Accept");
+		return -1;
+	}
+
+	return (ap_sta_set_vlan(hapd, sta, &vlan_desc));
+}
+#endif /* CONFIG_NO_VLAN */
+
+
 /**
  * ieee802_1x_receive_auth - Process RADIUS frames from Authentication Server
  * @msg: RADIUS response message
@@ -1694,12 +1737,6 @@  ieee802_1x_receive_auth(struct radius_msg *msg,
struct radius_msg *req,
 	struct eapol_state_machine *sm;
 	int override_eapReq = 0;
 	struct radius_hdr *hdr = radius_msg_get_hdr(msg);
-	struct vlan_description vlan_desc;
-#ifndef CONFIG_NO_VLAN
-	int *untagged, *tagged, *notempty;
-#endif /* CONFIG_NO_VLAN */
-
-	os_memset(&vlan_desc, 0, sizeof(vlan_desc));

 	sm = ieee802_1x_search_radius_identifier(hapd, hdr->identifier);
 	if (sm == NULL) {
@@ -1764,56 +1801,21 @@  ieee802_1x_receive_auth(struct radius_msg
*msg, struct radius_msg *req,
 	switch (hdr->code) {
 	case RADIUS_CODE_ACCESS_ACCEPT:
 #ifndef CONFIG_NO_VLAN
-		if (hapd->conf->ssid.dynamic_vlan != DYNAMIC_VLAN_DISABLED) {
-			notempty = &vlan_desc.notempty;
-			untagged = &vlan_desc.untagged;
-			tagged = vlan_desc.tagged;
-			*notempty = !!radius_msg_get_vlanid(msg, untagged,
-							    MAX_NUM_TAGGED_VLAN,
-							    tagged);
-		}
-
-		if (vlan_desc.notempty &&
-		    !hostapd_vlan_valid(hapd->conf->vlan, &vlan_desc)) {
-			sta->eapol_sm->authFail = TRUE;
-			hostapd_logger(hapd, sta->addr,
-				       HOSTAPD_MODULE_RADIUS,
-				       HOSTAPD_LEVEL_INFO,
-				       "Invalid VLAN %d%s received from RADIUS server",
-				       vlan_desc.untagged,
-				       vlan_desc.tagged[0] ? "+" : "");
-			os_memset(&vlan_desc, 0, sizeof(vlan_desc));
-			ap_sta_set_vlan(hapd, sta, &vlan_desc);
-			break;
-		}
-
-		if (hapd->conf->ssid.dynamic_vlan == DYNAMIC_VLAN_REQUIRED &&
-		    !vlan_desc.notempty) {
-			sta->eapol_sm->authFail = TRUE;
-			hostapd_logger(hapd, sta->addr,
-				       HOSTAPD_MODULE_IEEE8021X,
-				       HOSTAPD_LEVEL_INFO, "authentication "
-				       "server did not include required VLAN "
-				       "ID in Access-Accept");
-			break;
-		}
-#endif /* CONFIG_NO_VLAN */
-
-		if (ap_sta_set_vlan(hapd, sta, &vlan_desc) < 0)
+		if (hapd->conf->ssid.dynamic_vlan != DYNAMIC_VLAN_DISABLED &&
+		    ieee802_1x_update_vlan(msg, hapd, sta) < 0)
 			break;

-#ifndef CONFIG_NO_VLAN
 		if (sta->vlan_id > 0) {
 			hostapd_logger(hapd, sta->addr,
 				       HOSTAPD_MODULE_RADIUS,
 				       HOSTAPD_LEVEL_INFO,
 				       "VLAN ID %d", sta->vlan_id);
 		}
-#endif /* CONFIG_NO_VLAN */

 		if ((sta->flags & WLAN_STA_ASSOC) &&
 		    ap_sta_bind_vlan(hapd, sta) < 0)
 			break;
+#endif /* CONFIG_NO_VLAN */

 		sta->session_timeout_set = !!session_timeout_set;
 		sta->session_timeout = session_timeout;
diff --git a/src/ap/sta_info.c b/src/ap/sta_info.c
index d4f00d1f9..b9d4b49e4 100644
--- a/src/ap/sta_info.c
+++ b/src/ap/sta_info.c
@@ -888,9 +888,6 @@  int ap_sta_set_vlan(struct hostapd_data *hapd,
struct sta_info *sta,
 	struct hostapd_vlan *vlan = NULL, *wildcard_vlan = NULL;
 	int old_vlan_id, vlan_id = 0, ret = 0;

-	if (hapd->conf->ssid.dynamic_vlan == DYNAMIC_VLAN_DISABLED)
-		vlan_desc = NULL;
-
 	/* Check if there is something to do */
 	if (hapd->conf->ssid.per_sta_vif && !sta->vlan_id) {