Move verification of response packets up to a level where it makes sense.

Replace the user_dispatch_flag on connections with
conn_user_dispatch_p().

Remove the 'original' member from packet and instead have an upper
layer verify.

Rename packet valid_flag --> received_flag to reflect that we don't
verify.

Move _close_conn() --> conn_close().

Move packet flags into a single unsigned int, for portability.

(_read_packet): Don't verify packet.

(rs_conn_receive_packet): Don't touch PKT_OUT if there isn't a packet.

(rs_conn_receive_packet): Verify packet using packet_verify_response().

6 files changed, 125 insertions, 90 deletions

diff --git a/lib/conn.c b/lib/conn.c
index 14693ae..028302c 100644
--- a/lib/conn.c
+++ b/lib/conn.c
@@ -12,10 +12,35 @@
 #include <event2/bufferevent.h>
 #include <radsec/radsec.h>
 #include <radsec/radsec-impl.h>
+#include "conn.h"
 #include "event.h"
+#include "packet.h"
 #include "tcp.h"
 
 int
+conn_close (struct rs_connection **connp)
+{
+  int r;
+  assert (connp);
+  assert (*connp);
+  r = rs_conn_destroy (*connp);
+  if (!r)
+    *connp = NULL;
+  return r;
+}
+
+int
+conn_user_dispatch_p (const struct rs_connection *conn)
+{
+  assert (conn);
+
+  return (conn->callbacks.connected_cb ||
+	  conn->callbacks.disconnected_cb ||
+	  conn->callbacks.received_cb ||
+	  conn->callbacks.sent_cb);
+}
+
+int
 rs_conn_create (struct rs_context *ctx, struct rs_connection **conn,
 		const char *config)
 {
@@ -158,7 +183,6 @@ void
 rs_conn_set_callbacks (struct rs_connection *conn, struct rs_conn_callbacks *cb)
 {
   assert (conn);
-  conn->user_dispatch_flag = 1;
   memcpy (&conn->callbacks, cb, sizeof (conn->callbacks));
 }
 
@@ -166,7 +190,6 @@ void
 rs_conn_del_callbacks (struct rs_connection *conn)
 {
   assert (conn);
-  conn->user_dispatch_flag = 0;
   memset (&conn->callbacks, 0, sizeof (conn->callbacks));
 }
 
@@ -202,7 +225,9 @@ _rcb (struct rs_packet *packet, void *user_data)
 {
   struct rs_packet *pkt = (struct rs_packet *) user_data;
   assert (pkt);
-  pkt->valid_flag = 1;
+  assert (pkt->conn);
+
+  pkt->flags |= rs_packet_received_flag;
   if (pkt->conn->bev)
     bufferevent_disable (pkt->conn->bev, EV_WRITE|EV_READ);
   else
@@ -216,17 +241,18 @@ _rcb (struct rs_packet *packet, void *user_data)
    For any other use of libradsec, a the received_cb callback should
    be registered in the callbacks member of struct rs_connection.
 
-   On successful reception, verification and decoding of a RADIUS
-   message, PKT_OUT will upon return point at a pointer to a struct
-   rs_packet containing the message.
+   On successful reception of a RADIUS message it will be verified
+   against REQ_MSG, if !NULL.
 
-   If anything goes wrong or if the read times out (TODO: explain),
-   PKT_OUT will point at the NULL pointer and one or more errors are
-   pushed on the connection (available through rs_err_conn_pop()).  */
+   If PKT_OUT is !NULL it will upon return point at a pointer to a
+   struct rs_packet containing the message.
 
+   If anything goes wrong or if the read times out (TODO: explain),
+   PKT_OUT will not be changed and one or more errors are pushed on
+   the connection (available through rs_err_conn_pop()).  */
 int
 rs_conn_receive_packet (struct rs_connection *conn,
-		        struct rs_packet *request,
+		        struct rs_packet *req_msg,
 		        struct rs_packet **pkt_out)
 {
   int err = 0;
@@ -234,13 +260,11 @@ rs_conn_receive_packet (struct rs_connection *conn,
 
   assert (conn);
   assert (conn->realm);
-  assert (!conn->user_dispatch_flag); /* Dispatching mode only.  */
+  assert (!conn_user_dispatch_p (conn)); /* Dispatching mode only.  */
 
-  if (rs_packet_create (conn, pkt_out))
+  if (rs_packet_create (conn, &pkt))
     return -1;
-  pkt = *pkt_out;
   pkt->conn = conn;
-  pkt->original = request;
 
   assert (conn->evb);
   assert (conn->bev);
@@ -249,6 +273,8 @@ rs_conn_receive_packet (struct rs_connection *conn,
 
   conn->callbacks.received_cb = _rcb;
   conn->user_data = pkt;
+  pkt->flags &= ~rs_packet_received_flag;
+
   if (conn->bev)
     {
       bufferevent_setwatermark (conn->bev, EV_READ, RS_HEADER_LEN, 0);
@@ -264,7 +290,7 @@ rs_conn_receive_packet (struct rs_connection *conn,
 				    evutil_gai_strerror (err));
     }
 
-  /* Dispatch.  */
+
   rs_debug (("%s: entering event loop\n", __func__));
   err = event_base_dispatch (conn->evb);
   conn->callbacks.received_cb = NULL;
@@ -274,13 +300,16 @@ rs_conn_receive_packet (struct rs_connection *conn,
 				evutil_gai_strerror (err));
   rs_debug (("%s: event loop done\n", __func__));
 
-  if (!pkt->valid_flag)
-    return -1;
-
-#if defined (DEBUG)
-      rs_dump_packet (pkt);
-#endif
+  if ((pkt->flags & rs_packet_received_flag) == 0
+      || (req_msg
+	  && packet_verify_response (pkt->conn, pkt, req_msg) != RSE_OK))
+    {
+      assert (rs_err_conn_peek_code (pkt->conn));
+      return -1;
+    }
 
-  pkt->original = NULL;		/* FIXME: Why?  */
+  if (pkt_out)
+    *pkt_out = pkt;
   return RSE_OK;
 }
+
diff --git a/lib/include/radsec/radsec-impl.h b/lib/include/radsec/radsec-impl.h
index f8904ac..e790ccf 100644
--- a/lib/include/radsec/radsec-impl.h
+++ b/lib/include/radsec/radsec-impl.h
@@ -81,7 +81,6 @@ struct rs_connection {
     int fd;			/* Socket.  */
     int tryagain;		/* For server failover.  */
     int nextid;			/* Next RADIUS packet identifier.  */
-    int user_dispatch_flag : 1;	/* User does the dispatching.  */
     /* TCP transport specifics.  */
     struct bufferevent *bev;	/* Buffer event.  */
     /* UDP transport specifics.  */
@@ -95,14 +94,17 @@ struct rs_connection {
 #endif
 };
 
+enum rs_packet_flags {
+    rs_packet_hdr_read_flag,
+    rs_packet_received_flag,
+    rs_packet_sent_flag,
+};
+
 struct rs_packet {
     struct rs_connection *conn;
-    char hdr_read_flag;
-    uint8_t hdr[4];
+    unsigned int flags;
+    uint8_t hdr[RS_HEADER_LEN];
     RADIUS_PACKET *rpkt;
-    struct rs_packet *original;
-    char valid_flag;
-    char written_flag;
     struct rs_packet *next;	/* Used for UDP output queue.  */
 };
 
diff --git a/lib/packet.c b/lib/packet.c
index 6ba9fd3..799234f 100644
--- a/lib/packet.c
+++ b/lib/packet.c
@@ -9,6 +9,7 @@
 #include <event2/bufferevent.h>
 #include <radsec/radsec.h>
 #include <radsec/radsec-impl.h>
+#include "conn.h"
 #include "debug.h"
 #include "packet.h"
 
@@ -18,15 +19,47 @@
 #include <event2/buffer.h>
 #endif
 
-/* Badly named helper function for preparing a RADIUS message and
-   queue it.  FIXME: Rename.  */
+int
+packet_verify_response (struct rs_connection *conn,
+			struct rs_packet *response,
+			struct rs_packet *request)
+{
+  assert (conn);
+  assert (conn->active_peer);
+  assert (conn->active_peer->secret);
+  assert (response);
+  assert (response->rpkt);
+  assert (request);
+  assert (request->rpkt);
+
+  /* Verify header and message authenticator.  */
+  if (rad_verify (response->rpkt, request->rpkt, conn->active_peer->secret))
+    {
+      conn_close (&conn);
+      return rs_err_conn_push_fl (conn, RSE_FR, __FILE__, __LINE__,
+				  "rad_verify: %s", fr_strerror ());
+    }
+
+  /* Decode and decrypt.  */
+  if (rad_decode (response->rpkt, request->rpkt, conn->active_peer->secret))
+    {
+      conn_close (&conn);
+      return rs_err_conn_push_fl (conn, RSE_FR, __FILE__, __LINE__,
+				  "rad_decode: %s", fr_strerror ());
+    }
+
+  return RSE_OK;
+}
+
+
+/* Badly named function for preparing a RADIUS message and queue it.
+   FIXME: Rename.  */
 int
 packet_do_send (struct rs_packet *pkt)
 {
   VALUE_PAIR *vp = NULL;
 
   assert (pkt->rpkt);
-  assert (!pkt->original);
 
   /* Add Message-Authenticator, RFC 2869.  */
   /* FIXME: Make Message-Authenticator optional?  */
diff --git a/lib/packet.h b/lib/packet.h
index 7053329..edff9de 100644
--- a/lib/packet.h
+++ b/lib/packet.h
@@ -2,3 +2,6 @@
    See the file COPYING for licensing information.  */
 
 int packet_do_send (struct rs_packet *pkt);
+int packet_verify_response (struct rs_connection *conn,
+			    struct rs_packet *response,
+			    struct rs_packet *request);
diff --git a/lib/send.c b/lib/send.c
index 871d657..cc7fd71 100644
--- a/lib/send.c
+++ b/lib/send.c
@@ -14,9 +14,16 @@
 #include "packet.h"
 #include "event.h"
 #include "peer.h"
+#include "conn.h"
 #include "tcp.h"
 #include "udp.h"
 
+/* RFC 5080 2.2.1.  Retransmission Behavior */
+#define IRT 2
+#define MRC 5
+#define MRT 16
+#define MRD 30
+
 static int
 _conn_open (struct rs_connection *conn, struct rs_packet *pkt)
 {
@@ -62,7 +69,7 @@ _wcb (void *user_data)
 {
   struct rs_packet *pkt = (struct rs_packet *) user_data;
   assert (pkt);
-  pkt->written_flag = 1;
+  pkt->flags |= rs_packet_sent_flag;
   if (pkt->conn->bev)
     bufferevent_disable (pkt->conn->bev, EV_WRITE|EV_READ);
   else
@@ -100,27 +107,27 @@ rs_packet_send (struct rs_packet *pkt, void *user_data)
     {
       err = event_add (conn->wev, NULL);
       if (err < 0)
-	return rs_err_conn_push_fl (pkt->conn, RSE_EVENT, __FILE__, __LINE__,
+	return rs_err_conn_push_fl (conn, RSE_EVENT, __FILE__, __LINE__,
 				    "event_add: %s",
 				    evutil_gai_strerror (err));
     }
 
   /* Do dispatch, unless the user wants to do it herself.  */
-  if (!conn->user_dispatch_flag)
+  if (!conn_user_dispatch_p (conn))
     {
       conn->callbacks.sent_cb = _wcb;
       conn->user_data = pkt;
       rs_debug (("%s: entering event loop\n", __func__));
       err = event_base_dispatch (conn->evb);
       if (err < 0)
-	return rs_err_conn_push_fl (pkt->conn, RSE_EVENT, __FILE__, __LINE__,
+	return rs_err_conn_push_fl (conn, RSE_EVENT, __FILE__, __LINE__,
 				    "event_base_dispatch: %s",
 				    evutil_gai_strerror (err));
       rs_debug (("%s: event loop done\n", __func__));
       conn->callbacks.sent_cb = NULL;
       conn->user_data = NULL;
 
-      if (!pkt->written_flag)
+      if ((pkt->flags & rs_packet_sent_flag) == 0)
 	return -1;
     }
 
diff --git a/lib/tcp.c b/lib/tcp.c
index acee94b..1801d62 100644
--- a/lib/tcp.c
+++ b/lib/tcp.c
@@ -16,6 +16,7 @@
 #include <radsec/radsec-impl.h>
 #include "tcp.h"
 #include "packet.h"
+#include "conn.h"
 #include "debug.h"
 #include "event.h"
 
@@ -41,18 +42,6 @@ _conn_timeout_cb (int fd, short event, void *data)
     }
 }
 
-static int
-_close_conn (struct rs_connection **connp)
-{
-  int r;
-  assert (connp);
-  assert (*connp);
-  r = rs_conn_destroy (*connp);
-  if (!r)
-    *connp = NULL;
-  return r;
-}
-
 /* Read one RADIUS packet header.  Return !0 on error.  A return value
    of 0 means that we need more data.  */
 static int
@@ -63,11 +52,11 @@ _read_header (struct rs_packet *pkt)
   n = bufferevent_read (pkt->conn->bev, pkt->hdr, RS_HEADER_LEN);
   if (n == RS_HEADER_LEN)
     {
-      pkt->hdr_read_flag = 1;
+      pkt->flags |= rs_packet_hdr_read_flag;
       pkt->rpkt->data_len = (pkt->hdr[2] << 8) + pkt->hdr[3];
       if (pkt->rpkt->data_len < 20 || pkt->rpkt->data_len > 4096)
 	{
-	  _close_conn (&pkt->conn);
+	  conn_close (&pkt->conn);
 	  return rs_err_conn_push (pkt->conn, RSE_INVALID_PKT,
 				   "invalid packet length: %d",
 				   pkt->rpkt->data_len);
@@ -75,7 +64,7 @@ _read_header (struct rs_packet *pkt)
       pkt->rpkt->data = rs_malloc (pkt->conn->ctx, pkt->rpkt->data_len);
       if (!pkt->rpkt->data)
 	{
-	  _close_conn (&pkt->conn);
+	  conn_close (&pkt->conn);
 	  return rs_err_conn_push_fl (pkt->conn, RSE_NOMEM, __FILE__, __LINE__,
 				      NULL);
 	}
@@ -91,7 +80,7 @@ _read_header (struct rs_packet *pkt)
     }
   else	    /* Error: libevent gave us less than the low watermark. */
     {
-      _close_conn (&pkt->conn);
+      conn_close (&pkt->conn);
       return rs_err_conn_push_fl (pkt->conn, RSE_INTERNAL, __FILE__, __LINE__,
 				  "got %d octets reading header", n);
     }
@@ -117,7 +106,7 @@ _read_packet (struct rs_packet *pkt)
     {
       bufferevent_disable (pkt->conn->bev, EV_READ);
       rs_debug (("%s: complete packet read\n", __func__));
-      pkt->hdr_read_flag = 0;
+      pkt->flags &= ~rs_packet_hdr_read_flag;
       memset (pkt->hdr, 0, sizeof(*pkt->hdr));
 
       /* Checks done by rad_packet_ok:
@@ -125,39 +114,13 @@ _read_packet (struct rs_packet *pkt)
 	 - invalid code field
 	 - attribute lengths >= 2
 	 - attribute sizes adding up correctly  */
-      if (!rad_packet_ok (pkt->rpkt, 0) != 0)
+      if (!rad_packet_ok (pkt->rpkt, 0))
 	{
-	  _close_conn (&pkt->conn);
+	  conn_close (&pkt->conn);
 	  return rs_err_conn_push_fl (pkt->conn, RSE_FR, __FILE__, __LINE__,
 				      "invalid packet: %s", fr_strerror ());
 	}
 
-      /* TODO: Verify that reception of an unsolicited response packet
-	 results in connection being closed.  */
-
-      /* If we have a request to match this response against, verify
-	 and decode the response.  */
-      if (pkt->original)
-	{
-	  /* Verify header and message authenticator.  */
-	  if (rad_verify (pkt->rpkt, pkt->original->rpkt,
-			  pkt->conn->active_peer->secret))
-	    {
-	      _close_conn (&pkt->conn);
-	      return rs_err_conn_push_fl (pkt->conn, RSE_FR, __FILE__, __LINE__,
-					  "rad_verify: %s", fr_strerror ());
-	    }
-
-	  /* Decode and decrypt.  */
-	  if (rad_decode (pkt->rpkt, pkt->original->rpkt,
-			  pkt->conn->active_peer->secret))
-	    {
-	      _close_conn (&pkt->conn);
-	      return rs_err_conn_push_fl (pkt->conn, RSE_FR, __FILE__, __LINE__,
-					  "rad_decode: %s", fr_strerror ());
-	    }
-	}
-
 #if defined (DEBUG)
       /* Find out what happens if there's data left in the buffer.  */
       {
@@ -169,8 +132,8 @@ _read_packet (struct rs_packet *pkt)
       }
 #endif
 
-      /* Hand over message to user, changes ownership of pkt.  Don't
-	 touch it afterwards -- it might have been freed.  */
+      /* Hand over message to user.  This changes ownership of pkt.
+	 Don't touch it afterwards -- it might have been freed.  */
       if (pkt->conn->callbacks.received_cb)
 	pkt->conn->callbacks.received_cb (pkt, pkt->conn->user_data);
     }
@@ -183,14 +146,12 @@ _read_packet (struct rs_packet *pkt)
   return 0;
 }
 
-/* Read callback for TCP.
+/* The read callback for TCP.
 
    Read exactly one RADIUS message from BEV and store it in struct
-   rs_packet passed in CTX (hereby called 'pkt').
-
-   Verify the received packet against pkt->original, if !NULL.
+   rs_packet passed in USER_DATA.
 
-   Inform upper layer about successful reception of valid RADIUS
+   Inform upper layer about successful reception of received RADIUS
    message by invoking conn->callbacks.recevied_cb(), if !NULL.  */
 void
 tcp_read_cb (struct bufferevent *bev, void *user_data)
@@ -204,9 +165,9 @@ tcp_read_cb (struct bufferevent *bev, void *user_data)
   pkt->rpkt->sockfd = pkt->conn->fd;
   pkt->rpkt->vps = NULL;
 
-  if (!pkt->hdr_read_flag)
+  if ((pkt->flags & rs_packet_hdr_read_flag) == 0)
     if (_read_header (pkt))
-      return;
+      return;			/* Error.  */
   _read_packet (pkt);
 }