[PATCH 1/2] eap-tls: Drop cached session when phase2 fails

[PATCH 1/2] eap-tls: Drop cached session when phase2 fails

[Andrew Zaborowski]

If we used TLS session resumption successfully but the overall EAP method
failed, forget the session to improve the chances that authentication
succeeds on the next attempt considering that some authenticators
strangely allow resumption but can't handle it all the way to EAP method
success.  Logically the session resumption in the TLS layers on the
server should be transparent to the EAP layers so I guess those may be
failed attempts to further optimise phase 2 when the server thinks it can
already trust the client.
---
 src/eap-tls-common.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)

diff --git a/src/eap-tls-common.c b/src/eap-tls-common.c
index 784a57ee..b909d0cc 100644
--- a/src/eap-tls-common.c
+++ b/src/eap-tls-common.c
@@ -115,6 +115,7 @@ struct eap_tls_state {
 
 	bool expecting_frag_ack:1;
 	bool tunnel_ready:1;
+	bool tls_session_resumed:1;
 
 	struct l_queue *ca_cert;
 	struct l_certchain *client_cert;
@@ -129,8 +130,10 @@ static struct l_settings *eap_tls_session_cache;
 static eap_tls_session_cache_load_func_t eap_tls_session_cache_load;
 static eap_tls_session_cache_sync_func_t eap_tls_session_cache_sync;
 
-static void __eap_tls_common_state_reset(struct eap_tls_state *eap_tls)
+static void __eap_tls_common_state_reset(struct eap_state *eap)
 {
+	struct eap_tls_state *eap_tls = eap_get_data(eap);
+
 	eap_tls->version_negotiated = EAP_TLS_VERSION_NOT_NEGOTIATED;
 	eap_tls->method_completed = false;
 	eap_tls->phase2_failed = false;
@@ -145,6 +148,27 @@ static void __eap_tls_common_state_reset(struct eap_tls_state *eap_tls)
 	if (eap_tls->tunnel)
 		l_tls_reset(eap_tls->tunnel);
 
+	/*
+	 * If we used TLS session resumption successfully but the overall
+	 * EAP method failed, forget the session to improve the chances
+	 * that authentication succeeds on the next attempt considering
+	 * that some authenticators strangely allow resumption but can't
+	 * handle it all the way to EAP method success.  Do this even if
+	 * we have no indication that the method failed but it just didn't
+	 * succeed, to handle cases like the server getting stuck and a
+	 * timout occuring at a higher layer.  The risk is that we may
+	 * occasionally flush the session data when there was only a
+	 * momentary radio issue, invalid phase2 credentials or decision
+	 * to abort.  Those are not hot paths.
+	 *
+	 * TLS errors before the ready callback are already handled in
+	 * l_tls so we only need to look at .tls_session_resumed here.
+	 */
+	if (eap_tls->tls_session_resumed && !eap_method_is_success(eap))
+		eap_tls_forget_peer(eap_get_peer_id(eap));
+
+	eap_tls->tls_session_resumed = false;
+
 	eap_tls->tx_frag_offset = 0;
 	eap_tls->tx_frag_last_len = 0;
 
@@ -187,7 +211,7 @@ bool eap_tls_common_state_reset(struct eap_state *eap)
 {
 	struct eap_tls_state *eap_tls = eap_get_data(eap);
 
-	__eap_tls_common_state_reset(eap_tls);
+	__eap_tls_common_state_reset(eap);
 
 	if (eap_tls->variant_ops->reset)
 		eap_tls->variant_ops->reset(eap_tls->variant_data);
@@ -199,7 +223,7 @@ void eap_tls_common_state_free(struct eap_state *eap)
 {
 	struct eap_tls_state *eap_tls = eap_get_data(eap);
 
-	__eap_tls_common_state_reset(eap_tls);
+	__eap_tls_common_state_reset(eap);
 
 	eap_set_data(eap, NULL);
 
@@ -244,7 +268,9 @@ static void eap_tls_tunnel_ready(const char *peer_identity, void *user_data)
 {
 	struct eap_state *eap = user_data;
 	struct eap_tls_state *eap_tls = eap_get_data(eap);
-	bool resumed = l_tls_get_session_resumed(eap_tls->tunnel);
+
+	eap_tls->tls_session_resumed =
+		l_tls_get_session_resumed(eap_tls->tunnel);
 
 	if (eap_tls->ca_cert && !peer_identity) {
 		l_error("%s: TLS did not verify AP identity",
@@ -265,7 +291,8 @@ static void eap_tls_tunnel_ready(const char *peer_identity, void *user_data)
 	if (!eap_tls->variant_ops->tunnel_ready)
 		return;
 
-	if (!eap_tls->variant_ops->tunnel_ready(eap, peer_identity, resumed))
+	if (!eap_tls->variant_ops->tunnel_ready(eap, peer_identity,
+						eap_tls->tls_session_resumed))
 		l_tls_close(eap_tls->tunnel);
 }
 
-- 
2.34.1

[PATCH 2/2] station: Add EnableEAPTLSCache bool setting

[Andrew Zaborowski]

Seeing that some authenticators can't seem to handle TLS session caching
properly, guard the EAP-TLS-based methods session caching support behind
a global [Network].EnableEAPTLSCache setting.  Defaults to false.

With the previous commit, authentication should succeed at least every
other attempt.  I'd also expect that EAP-TLS is not usually affected
because there's no phase2, unlike with EAP-PEAP/EAP-TTLS.
---
 src/station.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/src/station.c b/src/station.c
index 7f1a1e24..c92c0d96 100644
--- a/src/station.c
+++ b/src/station.c
@@ -5053,6 +5053,8 @@ static void station_known_networks_changed(enum known_networks_event event,
 
 static int station_init(void)
 {
+	bool eap_tls_cache;
+
 	station_list = l_queue_new();
 	netdev_watch = netdev_watch_add(station_netdev_watch, NULL, NULL);
 	l_dbus_register_interface(dbus_get_bus(), IWD_STATION_INTERFACE,
@@ -5103,6 +5105,11 @@ static int station_init(void)
 
 	watchlist_init(&event_watches, NULL);
 
+	if (!l_settings_get_bool(iwd_get_config(), "Network",
+				"EnableEAPTLSCache", &eap_tls_cache) ||
+			!eap_tls_cache)
+		return 0;
+
 	eap_tls_set_session_cache_ops(storage_eap_tls_cache_load,
 					storage_eap_tls_cache_sync);
 	known_networks_watch = known_networks_watch_add(
-- 
2.34.1

Re: [PATCH 1/2] eap-tls: Drop cached session when phase2 fails

[Denis Kenzior]

Hi Andrew,

On 1/27/23 17:33, Andrew Zaborowski wrote:
> If we have a TLS session cached from this attempt or a previous
> successful connection attempt but the overall EAP method fails, forget
> the session to improve the chances that authentication succeeds on the
> next attempt considering that some authenticators strangely allow
> resumption but can't handle it all the way to EAP method success.
> Logically the session resumption in the TLS layers on the server should
> be transparent to the EAP layers so I guess those may be failed
> attempts to further optimise phase 2 when the server thinks it can
> already trust the client.
> ---
>   src/eap-tls-common.c | 52 +++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 47 insertions(+), 5 deletions(-)
> 

Applied, thanks.

Regards,
-Denis

[PATCH 1/2] eap-tls: Drop cached session when phase2 fails

[Andrew Zaborowski]

If we have a TLS session cached from this attempt or a previous
successful connection attempt but the overall EAP method fails, forget
the session to improve the chances that authentication succeeds on the
next attempt considering that some authenticators strangely allow
resumption but can't handle it all the way to EAP method success.
Logically the session resumption in the TLS layers on the server should
be transparent to the EAP layers so I guess those may be failed
attempts to further optimise phase 2 when the server thinks it can
already trust the client.
---
 src/eap-tls-common.c | 52 +++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 5 deletions(-)

diff --git a/src/eap-tls-common.c b/src/eap-tls-common.c
index 784a57ee..e21e215b 100644
--- a/src/eap-tls-common.c
+++ b/src/eap-tls-common.c
@@ -115,6 +115,7 @@ struct eap_tls_state {
 
 	bool expecting_frag_ack:1;
 	bool tunnel_ready:1;
+	bool tls_session_resumed:1;
 
 	struct l_queue *ca_cert;
 	struct l_certchain *client_cert;
@@ -129,8 +130,11 @@ static struct l_settings *eap_tls_session_cache;
 static eap_tls_session_cache_load_func_t eap_tls_session_cache_load;
 static eap_tls_session_cache_sync_func_t eap_tls_session_cache_sync;
 
-static void __eap_tls_common_state_reset(struct eap_tls_state *eap_tls)
+static void __eap_tls_common_state_reset(struct eap_state *eap)
 {
+	struct eap_tls_state *eap_tls = eap_get_data(eap);
+	const char *peer_id;
+
 	eap_tls->version_negotiated = EAP_TLS_VERSION_NOT_NEGOTIATED;
 	eap_tls->method_completed = false;
 	eap_tls->phase2_failed = false;
@@ -145,6 +149,41 @@ static void __eap_tls_common_state_reset(struct eap_tls_state *eap_tls)
 	if (eap_tls->tunnel)
 		l_tls_reset(eap_tls->tunnel);
 
+	/*
+	 * Drop the TLS session cache for this peer if the overall EAP
+	 * method didn't succeed.
+	 *
+	 * Additionally if the session was cached previously, meaning
+	 * that we've had a successful authentication at least once before,
+	 * and we now used session resumption successfully and the method
+	 * failed, become suspicious of this server's TLS session
+	 * resumption support.  Some authenticators strangely allow
+	 * resumption but can't handle it all the way to EAP method
+	 * success.  This improves the chances that authentication
+	 * succeeds on the next attempt.
+	 *
+	 * Drop the cache even if we have no indication that the
+	 * method failed but it just didn't succeed, to handle cases like
+	 * the server getting stuck and a timout occuring at a higher
+	 * layer.  The risk is that we may occasionally flush the session
+	 * data when there was only a momentary radio issue, invalid
+	 * phase2 credentials or decision to abort.  Those are not hot
+	 * paths.
+	 *
+	 * Note: TLS errors before the ready callback are handled in l_tls.
+	 */
+	peer_id = eap_get_peer_id(eap);
+	if (peer_id && eap_tls_session_cache && !eap_method_is_success(eap) &&
+			l_settings_has_group(eap_tls_session_cache, peer_id)) {
+		eap_tls_forget_peer(peer_id);
+
+		if (eap_tls->tls_session_resumed)
+			l_warn("EAP: method did not finish after successful TLS"
+				" session resumption.");
+	}
+
+	eap_tls->tls_session_resumed = false;
+
 	eap_tls->tx_frag_offset = 0;
 	eap_tls->tx_frag_last_len = 0;
 
@@ -187,7 +226,7 @@ bool eap_tls_common_state_reset(struct eap_state *eap)
 {
 	struct eap_tls_state *eap_tls = eap_get_data(eap);
 
-	__eap_tls_common_state_reset(eap_tls);
+	__eap_tls_common_state_reset(eap);
 
 	if (eap_tls->variant_ops->reset)
 		eap_tls->variant_ops->reset(eap_tls->variant_data);
@@ -199,7 +238,7 @@ void eap_tls_common_state_free(struct eap_state *eap)
 {
 	struct eap_tls_state *eap_tls = eap_get_data(eap);
 
-	__eap_tls_common_state_reset(eap_tls);
+	__eap_tls_common_state_reset(eap);
 
 	eap_set_data(eap, NULL);
 
@@ -244,7 +283,9 @@ static void eap_tls_tunnel_ready(const char *peer_identity, void *user_data)
 {
 	struct eap_state *eap = user_data;
 	struct eap_tls_state *eap_tls = eap_get_data(eap);
-	bool resumed = l_tls_get_session_resumed(eap_tls->tunnel);
+
+	eap_tls->tls_session_resumed =
+		l_tls_get_session_resumed(eap_tls->tunnel);
 
 	if (eap_tls->ca_cert && !peer_identity) {
 		l_error("%s: TLS did not verify AP identity",
@@ -265,7 +306,8 @@ static void eap_tls_tunnel_ready(const char *peer_identity, void *user_data)
 	if (!eap_tls->variant_ops->tunnel_ready)
 		return;
 
-	if (!eap_tls->variant_ops->tunnel_ready(eap, peer_identity, resumed))
+	if (!eap_tls->variant_ops->tunnel_ready(eap, peer_identity,
+						eap_tls->tls_session_resumed))
 		l_tls_close(eap_tls->tunnel);
 }
 
-- 
2.34.1