improved IKE_SA uniqueness check

src/charon/sa/ike_sa.h

@@ -657,9 +657,9 @@ struct ike_sa_t {
 	 * 
 	 * @return
 	 * 						- SUCCESS if deletion is initialized
-	 * 						- INVALID_STATE, if the IKE_SA is not in 
+	 * 						- DESTROY_ME, if the IKE_SA is not in 
 	 * 						  an established state and can not be
-	 * 						  delete (but destroyed).
+	 * 						  deleted (but destroyed).
 	 */
 	status_t (*delete) (ike_sa_t *this);
 	

src/charon/sa/ike_sa_manager.c

@@ -1234,18 +1234,23 @@ static ike_sa_t* checkout_by_name(private_ike_sa_manager_t *this, char *name,
 }
 	
 /**
- * Implementation of ike_sa_manager_t.checkout_duplicate.
+ * Implementation of ike_sa_manager_t.check_uniqueness.
  */
-static ike_sa_t* checkout_duplicate(private_ike_sa_manager_t *this,
+static bool check_uniqueness(private_ike_sa_manager_t *this, ike_sa_t *ike_sa)
-									ike_sa_t *ike_sa)
 {
-	linked_list_t *list;
+	bool cancel = FALSE;
-	entry_t *entry;
+	peer_cfg_t *peer_cfg = ike_sa->get_peer_cfg(ike_sa);
+	unique_policy_t policy = peer_cfg->get_unique_policy(peer_cfg);
+	linked_list_t *list, *duplicate_ids = NULL;
 	ike_sa_id_t *duplicate_id = NULL;
-	ike_sa_t *duplicate = NULL;
 	identification_t *me, *other;
 	u_int row, segment;
-	rwlock_t *lock; 
+	rwlock_t *lock;
+	
+	if (policy == UNIQUE_NO)
+	{
+		return FALSE;
+	}
 	
 	me = ike_sa->get_my_id(ike_sa);
 	other = ike_sa->get_other_id(ike_sa);
@@ -1262,25 +1267,70 @@ static ike_sa_t* checkout_duplicate(private_ike_sa_manager_t *this,
 		if (list->find_first(list, (linked_list_match_t)connected_peers_match,
 								 (void**)&current, me, other) == SUCCESS)
 		{
-			/* we just return the first ike_sa_id we have cached for these ids */
+			/* clone the list, so we can release the lock */
-			current->sas->get_first(current->sas, (void**)&duplicate_id);
+			duplicate_ids = current->sas->clone_offset(current->sas,
+									offsetof(ike_sa_id_t, clone));
 		}
 	}
 	lock->unlock(lock);
 	
-	if (duplicate_id)
+	if (!duplicate_ids)
 	{
-		if (get_entry_by_id(this, duplicate_id, &entry, &segment) == SUCCESS)
+		return FALSE;
-		{
-			if (wait_for_entry(this, entry, segment))
-			{
-				duplicate = entry->ike_sa;
-				entry->checked_out = TRUE;
-			}
-			unlock_single_segment(this, segment);
-		}
 	}
-	return duplicate;
+	
+	enumerator_t *enumerator = duplicate_ids->create_enumerator(duplicate_ids);
+	while (enumerator->enumerate(enumerator, (void**)&duplicate_id))
+	{
+		status_t status = SUCCESS;
+		ike_sa_t *duplicate = checkout(this, duplicate_id);
+		if (!duplicate)
+		{
+			continue;
+		}
+		peer_cfg = duplicate->get_peer_cfg(duplicate);
+		if (peer_cfg && peer_cfg->equals(peer_cfg, ike_sa->get_peer_cfg(ike_sa)))
+		{
+			switch (duplicate->get_state(duplicate))
+			{
+				case IKE_ESTABLISHED:
+				case IKE_REKEYING:
+					switch (policy)
+					{
+						case UNIQUE_REPLACE:
+							DBG1(DBG_IKE, "deleting duplicate IKE_SA due"
+									" uniqueness policy");
+							status = duplicate->delete(duplicate);
+							break;
+						case UNIQUE_KEEP:
+							cancel = TRUE;
+							/* we keep the first IKE_SA and delete all
+							 * other duplicates that might exist */
+							policy = UNIQUE_REPLACE;
+							break;
+						default:
+							break;
+					}
+					break;
+				default:
+					break;
+			}
+		}
+		if (status == DESTROY_ME)
+		{
+			charon->ike_sa_manager->checkin_and_destroy(charon->ike_sa_manager,
+														duplicate);
+		}
+		else
+		{
+			charon->ike_sa_manager->checkin(charon->ike_sa_manager, duplicate);
+		}
+		/* reset thread's current IKE_SA after checkin */
+		charon->bus->set_sa(charon->bus, ike_sa);
+	}
+	enumerator->destroy(enumerator);
+	duplicate_ids->destroy_offset(duplicate_ids, offsetof(ike_sa_id_t, destroy));
+	return cancel;
 }
 
 /**
@@ -1645,7 +1695,7 @@ ike_sa_manager_t *ike_sa_manager_create()
 	this->public.checkout_by_config = (ike_sa_t*(*)(ike_sa_manager_t*,peer_cfg_t*))checkout_by_config;
 	this->public.checkout_by_id = (ike_sa_t*(*)(ike_sa_manager_t*,u_int32_t,bool))checkout_by_id;
 	this->public.checkout_by_name = (ike_sa_t*(*)(ike_sa_manager_t*,char*,bool))checkout_by_name;
-	this->public.checkout_duplicate = (ike_sa_t*(*)(ike_sa_manager_t*, ike_sa_t *ike_sa))checkout_duplicate;
+	this->public.check_uniqueness = (bool(*)(ike_sa_manager_t*, ike_sa_t *ike_sa))check_uniqueness;
 	this->public.create_enumerator = (enumerator_t*(*)(ike_sa_manager_t*))create_enumerator;
 	this->public.checkin = (void(*)(ike_sa_manager_t*,ike_sa_t*))checkin;
 	this->public.checkin_and_destroy = (void(*)(ike_sa_manager_t*,ike_sa_t*))checkin_and_destroy;

src/charon/sa/ike_sa_manager.h

@@ -1,4 +1,5 @@
 /*
+ * Copyright (C) 2008 Tobias Brunner
  * Copyright (C) 2005-2006 Martin Willi
  * Copyright (C) 2005 Jan Hutter
  * Hochschule fuer Technik Rapperswil
@@ -103,12 +104,17 @@ struct ike_sa_manager_t {
 								 	 peer_cfg_t *peer_cfg);
 	
 	/**
-	 * Check out a duplicate if ike_sa to do uniqueness tests.
+	 * Check for duplicates of the given IKE_SA.
-	 *
+	 * 
-	 * @param ike_sa			ike_sa to get a duplicate from
+	 * Measures are taken according to the uniqueness policy of the IKE_SA.
-	 * @return					checked out duplicate
+	 * The return value indicates whether duplicates have been found and if
+	 * further measures should be taken (e.g. cancelling an IKE_AUTH exchange).
+	 * 
+	 * @param ike_sa			ike_sa to check
+	 * @return					TRUE, if the given IKE_SA has duplicates and
+	 * 							should be deleted
 	 */
-	ike_sa_t* (*checkout_duplicate)(ike_sa_manager_t *this, ike_sa_t *ike_sa);
+	bool (*check_uniqueness)(ike_sa_manager_t *this, ike_sa_t *ike_sa);
 	
 	/**
 	 * Check out an IKE_SA a unique ID.

src/charon/sa/tasks/ike_auth.c

@@ -87,70 +87,6 @@ struct private_ike_auth_t {
 	bool peer_authenticated;
 };
 
-/**
- * check uniqueness and delete duplicates
- */
-static bool check_uniqueness(private_ike_auth_t *this)
-{
-	ike_sa_t *duplicate;
-	unique_policy_t policy;
-	status_t status = SUCCESS;
-	peer_cfg_t *peer_cfg;
-	bool cancel = FALSE;
-	
-	peer_cfg = this->ike_sa->get_peer_cfg(this->ike_sa);
-	policy = peer_cfg->get_unique_policy(peer_cfg);
-	if (policy == UNIQUE_NO)
-	{
-		return FALSE;
-	}
-	duplicate = charon->ike_sa_manager->checkout_duplicate(
-										charon->ike_sa_manager, this->ike_sa);
-	if (duplicate)
-	{
-		peer_cfg = duplicate->get_peer_cfg(duplicate);
-		if (peer_cfg &&
-			peer_cfg->equals(peer_cfg, this->ike_sa->get_peer_cfg(this->ike_sa)))
-		{
-			switch (duplicate->get_state(duplicate))
-			{
-				case IKE_ESTABLISHED:
-				case IKE_REKEYING:
-					switch (policy)
-					{
-						case UNIQUE_REPLACE:
-							DBG1(DBG_IKE, "deleting duplicate IKE_SA due "
-								 "uniqueness policy");
-							status = duplicate->delete(duplicate);
-							break;
-						case UNIQUE_KEEP:
-							DBG1(DBG_IKE, "cancelling IKE_SA setup due "
-								 "uniqueness policy");
-							cancel = TRUE;
-							break;
-						default:
-							break;
-					}
-					break;
-				default:
-					break;
-			}
-		}
-		if (status == DESTROY_ME)
-		{
-			charon->ike_sa_manager->checkin_and_destroy(charon->ike_sa_manager,
-														duplicate);
-		}
-		else
-		{
-			charon->ike_sa_manager->checkin(charon->ike_sa_manager, duplicate);
-		}
-	}
-	/* set threads active IKE_SA after checkin */
-	charon->bus->set_sa(charon->bus, this->ike_sa);
-	return cancel;
-}
-
 /**
  * get the authentication class of a config
  */
@@ -681,8 +617,10 @@ static status_t build_r(private_ike_auth_t *this, message_t *message)
 		return FAILED;
 	}
 	
-	if (check_uniqueness(this))
+	if (charon->ike_sa_manager->check_uniqueness(charon->ike_sa_manager,
+												 this->ike_sa))
 	{
+		DBG1(DBG_IKE, "cancelling IKE_SA setup due uniqueness policy");
 		message->add_notify(message, TRUE, AUTHENTICATION_FAILED, chunk_empty);
 		return FAILED;
 	}