coova / coova-chilli

CoovaChilli is an open-source software access controller for captive portal hotspots.
Other
518 stars 260 forks source link

radius proxy broken with bfe0e4b0 #270

Open nvx opened 8 years ago

nvx commented 8 years ago

bfe0e4b0 introduces a regression breaking the radius proxy. Replies from the radius proxy are incorrectly sent from this->fd instead of this->proxyfd in radius_resp() which results in the incorrect source port being seen by the AP, which rightly ignores the packet.

I'm not overly familiar with the codebase, so I'm not sure if this is the cleanest solution by far, but a quick hack that appears to have fixed the issue is as follows:

--- a/src/radius.c
+++ b/src/radius.c
@@ -1528,6 +1528,26 @@ int radius_pkt_send(struct radius_t *this,

 /*
+ * radius_pkt_send_proxy()
+ * Directly send a packet
+ */
+int radius_pkt_send_proxy(struct radius_t *this,
+      struct radius_packet_t *pack,
+      struct sockaddr_in *peer) {
+
+  size_t len = ntohs(pack->length);
+
+  if (sendto(this->proxyfd, pack, len, 0,(struct sockaddr *) peer,
+       sizeof(struct sockaddr_in)) < 0) {
+    syslog(LOG_ERR, "%s: sendto() failed!", strerror(errno));
+    return -1;
+  }
+
+  return 0;
+}
+
+
+/*
  * radius_req()
  * Send of a packet and place it in the retransmit queue
  */
@@ -1592,7 +1612,7 @@ int radius_resp(struct radius_t *this,
                                this->proxysecret,
                                this->proxysecretlen);

-  return radius_pkt_send(this, pack, peer);
+  return radius_pkt_send_proxy(this, pack, peer);
 }
 #endif
heruan commented 5 years ago

Not sure if I'm experiencing the same issue, but after configuring the proxy CoovaChilli correctly proxies the request but does not send the reply back.

  1. client request to CoovaChilli: works
  2. CoovaChilli proxies to RADIUS: works
  3. RADIUS replies to CoovaChilli: works
  4. CoovaChilli DOES NOT proxy the reply to the client!

@sevan @pinkra any thoughts on this?

nvx commented 5 years ago

@heruan That sounds like the issue I was having which my patch fixes.

heruan commented 5 years ago

@nvx Did you open a PR with your patch?

nvx commented 5 years ago

The breaking change seemed a little weird and I didn't quite understand it so I wanted some feedback on the patch first go figure.

That said, I've been using the patch in the issue in production for quite a while without issues.