Simplified DPD handling by using a task for a single message only

src/libcharon/sa/ikev1/task_manager_v1.c

@@ -203,12 +203,12 @@ struct private_task_manager_t {
 	/**
 	 * Sequence number for sending DPD requests
 	 */
-	u_int32_t dpd_send_seqnr;
+	u_int32_t dpd_send;
 
 	/**
 	 * Sequence number for received DPD requests
 	 */
-	u_int32_t dpd_rec_seqnr;
+	u_int32_t dpd_recv;
 };
 
 /**
@@ -736,7 +736,8 @@ static status_t process_request(private_task_manager_t *this,
 	enumerator_t *enumerator;
 	task_t *task = NULL;
 	bool send_response = FALSE;
-	bool informational = FALSE;
+	notify_payload_t *notify;
+	chunk_t data;
 
 	if (message->get_exchange_type(message) == INFORMATIONAL_V1 ||
 		this->passive_tasks->get_count(this->passive_tasks) == 0)
@@ -779,9 +780,27 @@ static status_t process_request(private_task_manager_t *this,
 				this->passive_tasks->insert_last(this->passive_tasks, task);
 				break;
 			case INFORMATIONAL_V1:
-				task = (task_t *)informational_create(this->ike_sa, NULL, this->dpd_rec_seqnr);
+				notify = message->get_notify(message, DPD_R_U_THERE);
+				if (notify)
+				{
+					data = notify->get_notification_data(notify);
+					if (this->dpd_recv == 0 && data.len == 4)
+					{	/* first DPD request, initialize counter */
+						this->dpd_recv = untoh32(data.ptr);
+					}
+					task = (task_t *)isakmp_dpd_create(this->ike_sa, FALSE,
+													   this->dpd_recv++);
+				}
+				else if (message->get_notify(message, DPD_R_U_THERE_ACK))
+				{
+					task = (task_t *)isakmp_dpd_create(this->ike_sa, TRUE,
+													   this->dpd_send - 1);
+				}
+				else
+				{
+					task = (task_t *)informational_create(this->ike_sa, NULL);
+				}
 				this->passive_tasks->insert_first(this->passive_tasks, task);
-				informational = TRUE;
 				break;
 			case TRANSACTION:
 				if (this->ike_sa->get_state(this->ike_sa) == IKE_ESTABLISHED)
@@ -812,15 +831,6 @@ static status_t process_request(private_task_manager_t *this,
 			case NEED_MORE:
 				/* processed, but task needs at least another call to build() */
 				send_response = TRUE;
-				if (informational && !this->dpd_rec_seqnr)
-				{
-					/* Update the received DPD sequence number if it the first received one */
-					if (task->get_type(task) == TASK_ISAKMP_DPD)
-					{
-						isakmp_dpd_t *isakmp_dpd = (isakmp_dpd_t *)task;
-						this->dpd_rec_seqnr = isakmp_dpd->get_dpd_seqnr(isakmp_dpd);
-					}
-				}
 				continue;
 			case ALREADY_DONE:
 				send_response = FALSE;
@@ -987,7 +997,6 @@ METHOD(task_manager_t, process_message, status_t,
 	u_int32_t hash, mid, i;
 	host_t *me, *other;
 	status_t status;
-	bool dpd_response = FALSE;
 
 	/* TODO-IKEv1: update hosts more selectively */
 	me = msg->get_destination(msg);
@@ -1015,27 +1024,7 @@ METHOD(task_manager_t, process_message, status_t,
 		}
 	}
 
-	/* DPD Acks are not sent with a same message ID as the request.*/
+	if ((mid && mid == this->initiating.mid) ||
-	if (msg->get_exchange_type(msg) == INFORMATIONAL_V1 &&
-		this->active_tasks->get_count(this->active_tasks))
-	{
-		enumerator_t *enumerator;
-		task_t *task;
-		/* In case of ongoing DPD request, let the DPD task handle all information exchanges. */
-		enumerator = this->active_tasks->create_enumerator(this->active_tasks);
-		while (enumerator->enumerate(enumerator, (void**)&task))
-		{
-			if (task->get_type(task) == TASK_ISAKMP_DPD)
-			{
-				dpd_response = TRUE;
-				break;
-			}
-		}
-		enumerator->destroy(enumerator);
-	}
-
-
-	if ((mid && mid == this->initiating.mid) || dpd_response ||
 		(this->initiating.mid == 0 &&
 		 msg->get_exchange_type(msg) == this->initiating.type &&
 		 this->active_tasks->get_count(this->active_tasks)))
@@ -1332,8 +1321,8 @@ METHOD(task_manager_t, queue_child_delete, void,
 METHOD(task_manager_t, queue_dpd, void,
 	private_task_manager_t *this)
 {
-	queue_task(this, (task_t*)isakmp_dpd_create(this->ike_sa, NULL,
+	queue_task(this, (task_t*)isakmp_dpd_create(this->ike_sa, TRUE,
-												this->dpd_send_seqnr++));
+												this->dpd_send++));
 }
 
 METHOD(task_manager_t, adopt_tasks, void,
@@ -1483,9 +1472,9 @@ task_manager_v1_t *task_manager_v1_create(ike_sa_t *ike_sa)
 		return NULL;
 	}
 
-	this->rng->get_bytes(this->rng, sizeof(this->dpd_send_seqnr),
+	this->rng->get_bytes(this->rng, sizeof(this->dpd_send),
-						 (void*)&this->dpd_send_seqnr);
+						 (void*)&this->dpd_send);
-	this->dpd_send_seqnr &= 0x7FFFFFFF;
+	this->dpd_send &= 0x7FFFFFFF;
 
 	return &this->public;
 }

src/libcharon/sa/ikev1/tasks/aggressive_mode.c

@@ -164,7 +164,7 @@ static status_t send_notify(private_aggressive_mode_t *this, notify_type_t type)
 	notify->set_spi_data(notify, spi);
 
 	this->ike_sa->queue_task(this->ike_sa,
-						(task_t*)informational_create(this->ike_sa, notify, 0));
+						(task_t*)informational_create(this->ike_sa, notify));
 	/* cancel all active/passive tasks in favour of informational */
 	return ALREADY_DONE;
 }

src/libcharon/sa/ikev1/tasks/informational.c

@@ -18,7 +18,6 @@
 #include <daemon.h>
 #include <sa/ikev1/tasks/isakmp_delete.h>
 #include <sa/ikev1/tasks/quick_delete.h>
-#include <sa/ikev1/tasks/isakmp_dpd.h>
 
 #include <encoding/payloads/delete_payload.h>
 
@@ -48,16 +47,6 @@ struct private_informational_t {
 	 * Delete subtask
 	 */
 	task_t *del;
-
-	/**
-	 * DPD subtask
-	 */
-	task_t *dpd;
-
-	/**
-	 * DPD sequence number
-	 */
-	u_int32_t dpd_seqnr;
 };
 
 METHOD(task_t, build_i, status_t,
@@ -92,15 +81,6 @@ METHOD(task_t, process_r, status_t,
 					this->ike_sa->set_condition(this->ike_sa,
 												COND_INIT_CONTACT_SEEN, TRUE);
 				}
-				else if (type == DPD_R_U_THERE)
-				{
-					DBG3(DBG_IKE, "received DPD request");
-					this->dpd = (task_t*)isakmp_dpd_create(this->ike_sa, notify, this->dpd_seqnr);
-				}
-				else if (type == DPD_R_U_THERE_ACK)
-				{
-					DBG3(DBG_IKE, "received DPD request ack");
-				}
 				else if (type < 16384)
 				{
 					DBG1(DBG_IKE, "received %N error notify",
@@ -144,11 +124,6 @@ METHOD(task_t, process_r, status_t,
 	{
 		return this->del->process(this->del, message);
 	}
-
-	if (this->dpd && status == SUCCESS)
-	{
-		return this->dpd->process(this->dpd, message);
-	}
 	return status;
 }
 
@@ -159,13 +134,6 @@ METHOD(task_t, build_r, status_t,
 	{
 		return this->del->build(this->del, message);
 	}
-
-	if (this->dpd)
-	{
-		status_t status = this->dpd->build(this->dpd, message);
-		this->dpd->destroy(this->dpd);
-		return status;
-	}
 	return FAILED;
 }
 
@@ -198,7 +166,7 @@ METHOD(task_t, destroy, void,
 /*
  * Described in header.
  */
-informational_t *informational_create(ike_sa_t *ike_sa, notify_payload_t *notify, u_int32_t dpd_seqnr)
+informational_t *informational_create(ike_sa_t *ike_sa, notify_payload_t *notify)
 {
 	private_informational_t *this;
 
@@ -212,7 +180,6 @@ informational_t *informational_create(ike_sa_t *ike_sa, notify_payload_t *notify
 		},
 		.ike_sa = ike_sa,
 		.notify = notify,
-		.dpd_seqnr = dpd_seqnr,
 	);
 
 	if (notify)

src/libcharon/sa/ikev1/tasks/informational.h

@@ -19,7 +19,7 @@
  */
 
 #ifndef INFORMATIONAL_H_
-#define informational_H_
+#define INFORMATIONAL_H_
 
 typedef struct informational_t informational_t;
 
@@ -44,9 +44,8 @@ struct informational_t {
  *
  * @param ike_sa		IKE_SA this task works for
  * @param notify		notify to send as initiator, NULL if responder
- * @param dpd_seqnr	DPD sequence number, incoming or outgoing
  * @return				task to handle by the task_manager
  */
-informational_t *informational_create(ike_sa_t *ike_sa, notify_payload_t *notify, u_int32_t dpd_seqnr);
+informational_t *informational_create(ike_sa_t *ike_sa, notify_payload_t *notify);
 
 #endif /** INFORMATIONAL_H_ @}*/

src/libcharon/sa/ikev1/tasks/isakmp_dpd.c

@@ -1,14 +1,22 @@
+/*
+ * Copyright (C) 2011 Martin Willi
+ * Copyright (C) 2011 revosec AG
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.  See <http://www.fsf.org/copyleft/gpl.txt>.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ */
+
 #include "isakmp_dpd.h"
 
-#include <encoding/payloads/notify_payload.h>
-#include <sa/ikev1/tasks/informational.h>
-
 #include <daemon.h>
-
+#include <encoding/payloads/notify_payload.h>
-#ifdef SLIPSTREAM
-/* Should be the last include */
-#include <ikev2_mem.h>
-#endif /* SLIPSTREAM */
 
 typedef struct private_isakmp_dpd_t private_isakmp_dpd_t;
 
@@ -28,9 +36,9 @@ struct private_isakmp_dpd_t {
 	u_int32_t seqnr;
 
 	/**
-	 * Notify payload, only provided for requests.
+	 * DPD initiator?
 	 */
-	notify_payload_t *notify;
+	bool initiator;
 
 	/**
 	 * IKE SA we are serving.
@@ -38,42 +46,19 @@ struct private_isakmp_dpd_t {
 	ike_sa_t *ike_sa;
 };
 
-/**
+METHOD(task_t, build, status_t,
- * Get DPD sequence number from notify payload.
+	private_isakmp_dpd_t *this, message_t *message)
- */
-static bool get_seqnr(notify_payload_t *notify, u_int32_t *seqnr)
-{
-	chunk_t chunk = notify->get_notification_data(notify);
-
-	if( chunk.ptr && chunk.len == 4)
-	{
-		u_int32_t seqnr_read = *((u_int32_t*)chunk.ptr);
-
-		*seqnr = ntohl(seqnr_read);
-
-		return TRUE;
-	}
-
-	DBG1(DBG_IKE, "no DPD seqnr received");
-
-	return FALSE;
-}
-
-/**
- * Add notify payload to message.
- */
-static void add_notify(private_isakmp_dpd_t *this, message_t *message, notify_type_t type)
 {
 	notify_payload_t *notify;
-
+	notify_type_t type;
 	ike_sa_id_t *ike_sa_id;
 	u_int64_t spi_i, spi_r;
 	u_int32_t seqnr;
 	chunk_t spi;
 
+	type = this->initiator ? DPD_R_U_THERE : DPD_R_U_THERE_ACK;
 	notify = notify_payload_create_from_protocol_and_type(NOTIFY_V1,
 														  PROTO_IKE, type);
-
 	seqnr = htonl(this->seqnr);
 	ike_sa_id = this->ike_sa->get_id(this->ike_sa);
 	spi_i = ike_sa_id->get_initiator_spi(ike_sa_id);
@@ -84,156 +69,53 @@ static void add_notify(private_isakmp_dpd_t *this, message_t *message, notify_ty
 	notify->set_notification_data(notify, chunk_from_thing(seqnr));
 
 	message->add_payload(message, (payload_t*)notify);
-}
-
-METHOD(isakmp_dpd_t, get_dpd_seqnr, u_int32_t,
-	private_isakmp_dpd_t *this)
-{
-	return this->seqnr;
-}
-
-METHOD(task_t, build_i, status_t,
-	private_isakmp_dpd_t *this, message_t *message)
-{
-	add_notify(this, message, DPD_R_U_THERE);
-
-	return NEED_MORE;
-}
-
-METHOD(task_t, build_r, status_t,
-			 private_isakmp_dpd_t *this, message_t *message)
-{
-	add_notify(this, message, DPD_R_U_THERE_ACK);
 
 	return SUCCESS;
 }
 
-METHOD(task_t, process_i, status_t,
+METHOD(task_t, process, status_t,
 	private_isakmp_dpd_t *this, message_t *message)
 {
-	enumerator_t *enumerator;
 	notify_payload_t *notify;
 	notify_type_t type;
-	payload_t *payload;
+	u_int32_t seqnr = 0;
-	task_t *info_task = NULL;
+	chunk_t chunk;
 
-	enumerator = message->create_payload_enumerator(message);
+	type = this->initiator ? DPD_R_U_THERE_ACK : DPD_R_U_THERE;
-	while (enumerator->enumerate(enumerator, &payload))
+	notify = message->get_notify(message, type);
+	if (notify)
 	{
-		switch (payload->get_type(payload))
+		chunk = notify->get_notification_data(notify);
+		if (chunk.len == 4)
 		{
-			case NOTIFY_V1:
+			seqnr = untoh32(chunk.ptr);
-				notify = (notify_payload_t*)payload;
+			if (seqnr == this->seqnr)
-				type = notify->get_notify_type(notify);
-
-				if (type == DPD_R_U_THERE_ACK)
 			{
-					u_int32_t seqnr;
+				if (!this->initiator)
-
+				{	/* queue DPD_ACK */
-					if (!get_seqnr(notify, &seqnr))
+					this->ike_sa->queue_task(this->ike_sa,
-					{
+								&isakmp_dpd_create(this->ike_sa, FALSE,
-						return FAILED;
+												   this->seqnr)->task);
 				}
-
-					if (this->seqnr != seqnr)
-					{
-						DBG1(DBG_IKE, "received DPD Ack with unexpected seqnr (%u) expect (%u)",seqnr,this->seqnr);
 				return SUCCESS;
 			}
-
-					DBG4(DBG_IKE, "received DPD Ack with seqnr (%u)",seqnr);
-
-					return SUCCESS;
-
 		}
-				else if (type == DPD_R_U_THERE)
-				{
-					u_int32_t expected = this->seqnr + 1;
-
-					if (!get_seqnr(notify, &this->seqnr))
-					{
-						return FAILED;
 	}
-
+	DBG1(DBG_IKE, "received invalid DPD sequence number %u (expected %u), "
-					if (expected != 1 && this->seqnr != expected)
+		 "ignored", seqnr, this->seqnr);
-					{
-						DBG1(DBG_IKE, "received DPD request with unexpected seqnr (%u) expect (%u)",
-							this->seqnr,expected);
-						return SUCCESS;
-					}
-
-					DBG4(DBG_IKE, "received DPD request with seqnr %u",this->seqnr);
-
-					this->public.task.build = _build_r;
-					return NEED_MORE;
-				}
-				else
-				{
-					info_task = (task_t*)informational_create(this->ike_sa, NULL, 0);
-				}
-				continue;
-
-			default:
-				continue;
-		}
-		break;
-	}
-	enumerator->destroy(enumerator);
-
-	if (info_task)
-	{
-		status_t status = info_task->process(info_task, message);
-		/* Assuming that the informational task will not need to send other replies than dpd */
-		info_task->destroy(info_task);
-		return status;
-	}
-
 	return SUCCESS;
 }
 
-METHOD(task_t, process_r, status_t,
-			 private_isakmp_dpd_t *this, message_t *message)
-{
-	u_int32_t expected = this->seqnr + 1;
-
-	if (this->notify)
-	{
-		if (!get_seqnr(this->notify, &this->seqnr))
-		{
-			return FAILED;
-		}
-
-		if (expected != 1 && this->seqnr != expected)
-		{
-			DBG1(DBG_IKE, "received DPD request with unexpected seqnr (%u) expect (%u)",
-				this->seqnr,expected);
-			return SUCCESS;
-		}
-
-		DBG4(DBG_IKE, "DPD request received with seqnr %u",this->seqnr);
-	}
-	else
-	{
-		DBG1(DBG_IKE, "no notify provided");
-		return FAILED;
-	}
-	return NEED_MORE;
-}
-
-
 METHOD(task_t, get_type, task_type_t,
 	private_isakmp_dpd_t *this)
 {
 	return TASK_ISAKMP_DPD;
 }
 
-
 METHOD(task_t, migrate, void,
 	private_isakmp_dpd_t *this, ike_sa_t *ike_sa)
 {
 	this->ike_sa = ike_sa;
-	this->seqnr = 0;
-
 }
 
 METHOD(task_t, destroy, void,
@@ -245,7 +127,8 @@ METHOD(task_t, destroy, void,
 /*
  * Described in header.
  */
-isakmp_dpd_t *isakmp_dpd_create(ike_sa_t *ike_sa, notify_payload_t *notify, u_int32_t seqnr)
+isakmp_dpd_t *isakmp_dpd_create(ike_sa_t *ike_sa, bool initiator,
+								u_int32_t seqnr)
 {
 	private_isakmp_dpd_t *this;
 
@@ -253,26 +136,16 @@ isakmp_dpd_t *isakmp_dpd_create(ike_sa_t *ike_sa, notify_payload_t *notify, u_in
 		.public = {
 			.task = {
 				.get_type = _get_type,
+				.build = _build,
+				.process = _process,
 				.migrate = _migrate,
 				.destroy = _destroy,
 			},
-			.get_dpd_seqnr = _get_dpd_seqnr,
 		},
-		.notify = notify,
 		.ike_sa = ike_sa,
 		.seqnr = seqnr,
+		.initiator = initiator,
 	);
 
-	if (!notify)
-	{
-		this->public.task.build = _build_i;
-		this->public.task.process = _process_i;
-	}
-	else
-	{
-		this->public.task.build = _build_r;
-		this->public.task.process = _process_r;
-	}
-
 	return &this->public;
 }

src/libcharon/sa/ikev1/tasks/isakmp_dpd.h

@@ -1,5 +1,25 @@
+/*
+ * Copyright (C) 2012 Martin Willi
+ * Copyright (C) 2012 revosec AG
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the
+ * Free Software Foundation; either version 2 of the License, or (at your
+ * option) any later version.  See <http://www.fsf.org/copyleft/gpl.txt>.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY
+ * or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
+ * for more details.
+ */
+
+/**
+ * @defgroup isakmp_dpd isakmp_dpd
+ * @{ @ingroup tasks
+ */
+
 #ifndef ISAKMP_DPD_H_
-#define ISAPMP_DPD_H_
+#define ISAKMP_DPD_H_
 
 typedef struct isakmp_dpd_t isakmp_dpd_t;
 
@@ -8,9 +28,7 @@ typedef struct isakmp_dpd_t isakmp_dpd_t;
 #include <sa/task.h>
 
 /**
- * Task of type isakmp_dpd, detects dead peers.
+ * IKEv1 dead peer detection task.
- *
- *
  */
 struct isakmp_dpd_t {
 
@@ -18,21 +36,17 @@ struct isakmp_dpd_t {
 	 * Implements the task_t interface
 	 */
 	task_t task;
-
-	/**
-	 * Get the received dpd seqnr.
-	 *
-	 * @return				protocol ID
-	 */
-	u_int32_t (*get_dpd_seqnr) (isakmp_dpd_t *dpd_task);
 };
 
 /**
- * Create a new isakmp_dpd task.
+ * Create a new ISAKMP_DPD task.
  *
- * @param initiator		TRUE if task is the original initiator
+ * @param ike_sa		associated IKE_SA
- * @return				isakmp_dpd task to handle by the task_manager
+ * @param initiator		TRUE if DPD initiator
+ * @param seqnr			DPD sequence number to use/expect
+ * @return				ISAKMP_DPD task to handle by the task_manager
  */
-isakmp_dpd_t *isakmp_dpd_create(ike_sa_t *ike_sa, notify_payload_t *notify, u_int32_t seqnr);
+isakmp_dpd_t *isakmp_dpd_create(ike_sa_t *ike_sa, bool initiator,
+								u_int32_t seqnr);
 
 #endif /** ISAKMP_DPD_H_ @}*/

src/libcharon/sa/ikev1/tasks/main_mode.c

@@ -176,7 +176,7 @@ static status_t send_notify(private_main_mode_t *this, notify_type_t type)
 	notify->set_spi_data(notify, spi);
 
 	this->ike_sa->queue_task(this->ike_sa,
-						(task_t*)informational_create(this->ike_sa, notify, 0));
+						(task_t*)informational_create(this->ike_sa, notify));
 	/* cancel all active/passive tasks in favour of informational */
 	return ALREADY_DONE;
 }

src/libcharon/sa/ikev1/tasks/quick_mode.c

@@ -539,7 +539,7 @@ static status_t send_notify(private_quick_mode_t *this, notify_type_t type)
 	notify->set_spi(notify, this->spi_i);
 
 	this->ike_sa->queue_task(this->ike_sa,
-						(task_t*)informational_create(this->ike_sa, notify, 0));
+						(task_t*)informational_create(this->ike_sa, notify));
 	/* cancel all active/passive tasks in favour of informational */
 	return ALREADY_DONE;
 }