[PATCH 1/3] storage: Maintain the tls session cache object

[PATCH 1/3] storage: Maintain the tls session cache object

[Andrew Zaborowski]

Instead of only providing methods to load and save the TLS cache from/to
file, keep the memory copy of the cache directly in storage.c since it
doesn't fit anywhere else conceptually.  storage.c now only provides a
getter for the cache object and the _sync method to be called by anyone
who makes a change to the cache object.

While the use case is for eap-tls-common.c to call these, the same cache
object could be used if l_tls is used in another part of IWD.  Any
sessions saved in relation to a specific network (ESS) should use group
names ending in the storage_get_network_file_path encoding of the
network's SSID+security type, so that storage.c can automatically drop
these cache entries if the network is being forgotten.

Since the cache object needs to be freed when exiting, use
storage_exit() for that.  However, since there was already a
storage_init() and storage_exit() pair of functions, with storage_init
called from main.c conditionally on whether systemd encryption was
enabled, rename that storage_init() to storage_key_init() and declare an
IWD_MODULE to hanadle the calling of the new storage_init() and
storage_exit().  There's no warranty that those will be called before
and after any other module's init/exit but they're not necessary for any
functionality that other modules depend on.
---
 src/main.c    |  3 +-
 src/storage.c | 92 ++++++++++++++++++++++++++++++++++++++++-----------
 src/storage.h |  7 ++--
 3 files changed, 77 insertions(+), 25 deletions(-)

diff --git a/src/main.c b/src/main.c
index a3d85535..3da4c7fe 100644
--- a/src/main.c
+++ b/src/main.c
@@ -451,7 +451,7 @@ static bool setup_system_key(void)
 		goto unmap;
 	}
 
-	r = storage_init(key, st.st_size);
+	r = storage_key_init(key, st.st_size);
 	munlock(key, st.st_size);
 
 unmap:
@@ -600,7 +600,6 @@ int main(int argc, char *argv[])
 	exit_status = l_main_run_with_signal(signal_handler, NULL);
 
 	iwd_modules_exit();
-	storage_exit();
 
 failed_storage:
 	dbus_exit();
diff --git a/src/storage.c b/src/storage.c
index b2c5ed48..68814aae 100644
--- a/src/storage.c
+++ b/src/storage.c
@@ -46,6 +46,9 @@
 
 #include "src/missing.h"
 #include "src/common.h"
+#include "src/module.h"
+#include "src/scan.h"
+#include "src/knownnetworks.h"
 #include "src/storage.h"
 #include "src/crypto.h"
 
@@ -53,12 +56,13 @@
 #define STORAGE_FILE_MODE (S_IRUSR | S_IWUSR)
 
 #define KNOWN_FREQ_FILENAME ".known_network.freq"
-#define TLS_CACHE_FILENAME ".tls-session-cache"
+#define TLS_SESSION_CACHE_FILENAME ".tls-session-cache"
 
 static char *storage_path = NULL;
 static char *storage_hotspot_path = NULL;
 static uint8_t system_key[32];
 static bool system_key_set = false;
+static struct l_settings *tls_session_cache;
 
 static int create_dirs(const char *filename)
 {
@@ -653,14 +657,40 @@ void storage_network_sync(enum security type, const char *ssid,
 	write_file(data, length, true, "%s", path);
 }
 
+static void storage_network_remove_tls_sessions(const char *path)
+{
+	_auto_(l_strv_free) char **groups = NULL;
+	char **group;
+	size_t group_len;
+	size_t path_len;
+	bool changed = false;
+
+	if (!tls_session_cache)
+		return;
+
+	groups = l_settings_get_groups(tls_session_cache);
+	path_len = strlen(path);
+
+	for (group = groups; *group; group++)
+		if ((group_len = strlen(*group)) > path_len &&
+				!memcmp(*group + group_len - path_len, path,
+					path_len)) {
+			l_settings_remove_group(tls_session_cache, *group);
+			changed = true;
+		}
+
+	if (changed)
+		storage_tls_session_cache_sync();
+}
+
 int storage_network_remove(enum security type, const char *ssid)
 {
-	char *path;
+	_auto_(l_free) char *path = storage_get_network_file_path(type, ssid);
 	int ret;
 
-	path = storage_get_network_file_path(type, ssid);
 	ret = unlink(path);
-	l_free(path);
+
+	storage_network_remove_tls_sessions(path);
 
 	return ret < 0 ? -errno : 0;
 }
@@ -702,29 +732,42 @@ void storage_known_frequencies_sync(struct l_settings *known_freqs)
 	l_free(known_freq_file_path);
 }
 
-struct l_settings *storage_tls_session_cache_load(void)
+struct l_settings *storage_tls_session_cache_get(void)
 {
-	_auto_(l_settings_free) struct l_settings *cache = l_settings_new();
-	_auto_(l_free) char *tls_cache_file_path =
-		storage_get_path("%s", TLS_CACHE_FILENAME);
+	_auto_(l_free) char *cache_file_path = NULL;
 
-	if (unlikely(!l_settings_load_from_file(cache, tls_cache_file_path)))
-		return NULL;
+	if (tls_session_cache)
+		return tls_session_cache;
 
-	return l_steal_ptr(cache);
+	cache_file_path = storage_get_path("%s", TLS_SESSION_CACHE_FILENAME);
+	tls_session_cache = l_settings_new();
+
+	if (!l_settings_load_from_file(tls_session_cache, cache_file_path))
+		l_debug("No session cache loaded from %s, starting with an "
+			"empty cache", cache_file_path);
+
+	return tls_session_cache;
 }
 
-void storage_tls_session_cache_sync(struct l_settings *cache)
+void storage_tls_session_cache_sync(void)
 {
-	_auto_(l_free) char *tls_cache_file_path = NULL;
+	_auto_(l_free) char *cache_file_path = NULL;
+	_auto_(l_free) char *settings_data = NULL;
 	_auto_(l_free) char *data = NULL;
 	size_t len;
+	static const char comment[] =
+		"# External changes to this file are not tracked by IWD "
+		"and will be overwritten.\n\n";
+	static const size_t comment_len = L_ARRAY_SIZE(comment) - 1;
 
-	if (!cache)
+	if (L_WARN_ON(!tls_session_cache))
 		return;
 
-	tls_cache_file_path = storage_get_path("%s", TLS_CACHE_FILENAME);
-	data = l_settings_to_data(cache, &len);
+	cache_file_path = storage_get_path("%s", TLS_SESSION_CACHE_FILENAME);
+	settings_data = l_settings_to_data(tls_session_cache, &len);
+	data = l_malloc(comment_len + len);
+	memcpy(data, comment, comment_len);
+	memcpy(data + comment_len, settings_data, len);
 
 	/*
 	 * Note this data contains cryptographic secrets.  write_file()
@@ -732,7 +775,8 @@ void storage_tls_session_cache_sync(struct l_settings *cache)
 	 *
 	 * TODO: consider encrypting with system_key.
 	 */
-	write_file(data, len, false, "%s", tls_cache_file_path);
+	write_file(data, comment_len + len, false, "%s", cache_file_path);
+	explicit_bzero(settings_data, len);
 	explicit_bzero(data, len);
 }
 
@@ -768,7 +812,7 @@ bool storage_is_file(const char *filename)
  *	TK = HKDF-Extract(<zero>, IKM)
  *	OKM = HKDF-Expand(TK, "System Key", 32)
  */
-bool storage_init(const uint8_t *key, size_t key_len)
+bool storage_key_init(const uint8_t *key, size_t key_len)
 {
 	uint8_t tmp[32];
 
@@ -786,10 +830,20 @@ bool storage_init(const uint8_t *key, size_t key_len)
 	return system_key_set;
 }
 
-void storage_exit(void)
+static int storage_init(void)
+{
+	return 0;
+}
+
+static void storage_exit(void)
 {
 	if (system_key_set) {
 		explicit_bzero(system_key, sizeof(system_key));
 		munlock(system_key, sizeof(system_key));
 	}
+
+	l_settings_free(tls_session_cache);
+	tls_session_cache = NULL;
 }
+
+IWD_MODULE(storage, storage_init, storage_exit)
diff --git a/src/storage.h b/src/storage.h
index fe6ddbf5..de41541e 100644
--- a/src/storage.h
+++ b/src/storage.h
@@ -51,8 +51,8 @@ int storage_network_remove(enum security type, const char *ssid);
 struct l_settings *storage_known_frequencies_load(void);
 void storage_known_frequencies_sync(struct l_settings *known_freqs);
 
-struct l_settings *storage_tls_session_cache_load(void);
-void storage_tls_session_cache_sync(struct l_settings *cache);
+struct l_settings *storage_tls_session_cache_get(void);
+void storage_tls_session_cache_sync(void);
 
 int __storage_decrypt(struct l_settings *settings, const char *ssid,
 				bool *changed);
@@ -61,5 +61,4 @@ char *__storage_encrypt(const struct l_settings *settings, const char *ssid,
 bool storage_decrypt(struct l_settings *settings, const char *path,
 			const char *name);
 
-bool storage_init(const uint8_t *key, size_t key_len);
-void storage_exit(void);
+bool storage_key_init(const uint8_t *key, size_t key_len);
-- 
2.34.1

[PATCH 2/3] iwd-decrypt-profile: Update storage_init calls

[Andrew Zaborowski]

---
 tools/iwd-decrypt-profile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tools/iwd-decrypt-profile.c b/tools/iwd-decrypt-profile.c
index e71b56b4..06a9f97b 100644
--- a/tools/iwd-decrypt-profile.c
+++ b/tools/iwd-decrypt-profile.c
@@ -90,7 +90,7 @@ static bool secret_from_file(const char *file)
 	if (data == MAP_FAILED)
 		return false;
 
-	r = storage_init(data, st.st_size);
+	r = storage_key_init(data, st.st_size);
 
 	munmap(data, st.st_size);
 
@@ -168,7 +168,7 @@ int main(int argc, char *argv[])
 	}
 
 	if (pass) {
-		if (!storage_init((const uint8_t *)pass, strlen(pass)))
+		if (!storage_key_init((const uint8_t *)pass, strlen(pass)))
 			goto failed;
 	} else if (!secret_from_file(file))
 		goto failed;
-- 
2.34.1

[PATCH 3/3] eap-tls: Add session caching

[Andrew Zaborowski]

Use l_tls_set_session_cache() to enable session cache/resume in the
TLS-based EAP methods.  Sessions for all 802.1x networks are stored in
one l_settings object.

eap_{get,set}_peer_id() API is added for eapol to set the identifier of
the authenticator (or the supplicant if we're the authenticator, if
there's ever a use case for that.)  In this case that's the network's
SSID for simplicity encoded using storage_get_network_file_path.
Unfortunately this makes eapol.o depend on storage.o + common.o hence
those files are added to two unit tests that weren't linking storage.o
until now.

eap-tls-common.c can't call storage_tls_session_cache_{get,sync}()
directly because it's linked into ead and some other unit tests without
storage.o, so station.c sets pointers to the get & sync functions using
the new eap_tls_set_session_cache_ops().
---
 Makefile.am          |  9 ++++++---
 src/eap-tls-common.c | 28 ++++++++++++++++++++++++++++
 src/eap-tls-common.h |  6 ++++++
 src/eap.c            | 13 +++++++++++++
 src/eap.h            |  3 +++
 src/eapol.c          | 11 +++++++++++
 src/station.c        |  6 ++++++
 7 files changed, 73 insertions(+), 3 deletions(-)

diff --git a/Makefile.am b/Makefile.am
index d5f31a2d..b9632772 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -427,7 +427,6 @@ unit_test_eap_sim_SOURCES = unit/test-eap-sim.c \
 		src/crypto.h src/crypto.c src/simutil.h src/simutil.c \
 		src/ie.h src/ie.c \
 		src/watchlist.h src/watchlist.c \
-		src/eapol.h src/eapol.c \
 		src/eapolutil.h src/eapolutil.c \
 		src/handshake.h src/handshake.c \
 		src/eap.h src/eap.c src/eap-private.h \
@@ -497,7 +496,9 @@ unit_test_eapol_SOURCES = unit/test-eapol.c \
 				src/eap-tls-common.h src/eap-tls-common.c \
 				src/erp.h src/erp.c \
 				src/band.h src/band.c \
-				src/mschaputil.h src/mschaputil.c
+				src/mschaputil.h src/mschaputil.c \
+				src/common.h src/common.c \
+				src/storage.h src/storage.c
 unit_test_eapol_LDADD = $(ell_ldadd)
 unit_test_eapol_DEPENDENCIES = $(ell_dependencies) \
 				unit/cert-server.pem \
@@ -525,7 +526,9 @@ unit_test_wsc_SOURCES = unit/test-wsc.c src/wscutil.h src/wscutil.c \
 				src/util.h src/util.c \
 				src/erp.h src/erp.c \
 				src/band.h src/band.c \
-				src/eap-wsc.h src/eap-wsc.c
+				src/eap-wsc.h src/eap-wsc.c \
+				src/common.h src/common.c \
+				src/storage.h src/storage.c
 unit_test_wsc_LDADD = $(ell_ldadd)
 
 unit_test_eap_mschapv2_SOURCES = src/eap-mschapv2.h src/eap-mschapv2.c \
diff --git a/src/eap-tls-common.c b/src/eap-tls-common.c
index acc5b387..90198d6c 100644
--- a/src/eap-tls-common.c
+++ b/src/eap-tls-common.c
@@ -28,6 +28,7 @@
 #include <errno.h>
 #include <ell/ell.h>
 
+#include "ell/useful.h"
 #include "src/missing.h"
 #include "src/eap.h"
 #include "src/eap-private.h"
@@ -123,6 +124,9 @@ struct eap_tls_state {
 	void *variant_data;
 };
 
+static eap_tls_session_cache_get_func_t eap_tls_session_cache_get;
+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)
 {
 	eap_tls->version_negotiated = EAP_TLS_VERSION_NOT_NEGOTIATED;
@@ -571,9 +575,18 @@ static int eap_tls_handle_fragmented_request(struct eap_state *eap,
 	return r;
 }
 
+static void eap_tls_session_cache_update(void *user_data)
+{
+	if (L_WARN_ON(!eap_tls_session_cache_sync))
+		return;
+
+	eap_tls_session_cache_sync();
+}
+
 static bool eap_tls_tunnel_init(struct eap_state *eap)
 {
 	struct eap_tls_state *eap_tls = eap_get_data(eap);
+	_auto_(l_free) char *cache_group_name = NULL;
 
 	if (eap_tls->tunnel)
 		goto start;
@@ -633,6 +646,14 @@ static bool eap_tls_tunnel_init(struct eap_state *eap)
 	if (eap_tls->domain_mask)
 		l_tls_set_domain_mask(eap_tls->tunnel, eap_tls->domain_mask);
 
+	if (!eap_tls_session_cache_get)
+		goto start;
+
+	cache_group_name = l_strdup_printf("EAP-%s", eap_get_peer_id(eap));
+	l_tls_set_session_cache(eap_tls->tunnel, eap_tls_session_cache_get(),
+				cache_group_name, 24 * 3600 * L_USEC_PER_SEC, 0,
+				eap_tls_session_cache_update, NULL);
+
 start:
 	if (!l_tls_start(eap_tls->tunnel)) {
 		l_error("%s: Failed to start the TLS client",
@@ -1085,3 +1106,10 @@ void eap_tls_common_tunnel_close(struct eap_state *eap)
 
 	l_tls_close(eap_tls->tunnel);
 }
+
+void eap_tls_set_session_cache_ops(eap_tls_session_cache_get_func_t get,
+					eap_tls_session_cache_sync_func_t sync)
+{
+	eap_tls_session_cache_get = get;
+	eap_tls_session_cache_sync = sync;
+}
diff --git a/src/eap-tls-common.h b/src/eap-tls-common.h
index 174770c0..6c39ff3d 100644
--- a/src/eap-tls-common.h
+++ b/src/eap-tls-common.h
@@ -20,6 +20,9 @@
  *
  */
 
+typedef struct l_settings *(*eap_tls_session_cache_get_func_t)(void);
+typedef void (*eap_tls_session_cache_sync_func_t)(void);
+
 enum eap_tls_version {
 	EAP_TLS_VERSION_0               = 0x00,
 	EAP_TLS_VERSION_1               = 0x01,
@@ -81,3 +84,6 @@ bool eap_tls_common_tunnel_prf_get_bytes(struct eap_state *eap,
 void eap_tls_common_tunnel_send(struct eap_state *eap, const uint8_t *data,
 							size_t data_len);
 void eap_tls_common_tunnel_close(struct eap_state *eap);
+
+void eap_tls_set_session_cache_ops(eap_tls_session_cache_get_func_t get,
+					eap_tls_session_cache_sync_func_t sync);
diff --git a/src/eap.c b/src/eap.c
index 6f523f2f..981b6388 100644
--- a/src/eap.c
+++ b/src/eap.c
@@ -59,6 +59,7 @@ struct eap_state {
 	char *identity;
 	char *identity_setting;
 	bool authenticator;
+	char *peer_id;
 
 	int last_id;
 	void *method_state;
@@ -154,6 +155,7 @@ void eap_free(struct eap_state *eap)
 
 	eap_free_common(eap);
 	l_timeout_remove(eap->complete_timeout);
+	eap_set_peer_id(eap, NULL);
 
 	l_free(eap);
 }
@@ -837,6 +839,17 @@ err:
 	return false;
 }
 
+void eap_set_peer_id(struct eap_state *eap, const char *id)
+{
+	l_free(eap->peer_id);
+	eap->peer_id = l_strdup(id);
+}
+
+const char *eap_get_peer_id(struct eap_state *eap)
+{
+	return eap->peer_id;
+}
+
 void eap_set_data(struct eap_state *eap, void *data)
 {
 	eap->method_state = data;
diff --git a/src/eap.h b/src/eap.h
index f1e867f5..702caf24 100644
--- a/src/eap.h
+++ b/src/eap.h
@@ -94,6 +94,9 @@ const char *eap_get_identity(struct eap_state *eap);
 
 void eap_rx_packet(struct eap_state *eap, const uint8_t *pkt, size_t len);
 
+void eap_set_peer_id(struct eap_state *eap, const char *id);
+const char *eap_get_peer_id(struct eap_state *eap);
+
 void __eap_set_config(struct l_settings *config);
 
 int eap_init(void);
diff --git a/src/eapol.c b/src/eapol.c
index 4a1abd28..54ce253b 100644
--- a/src/eapol.c
+++ b/src/eapol.c
@@ -44,6 +44,8 @@
 #include "src/erp.h"
 #include "src/iwd.h"
 #include "src/band.h"
+#include "src/common.h"
+#include "src/storage.h"
 
 static struct l_queue *state_machines;
 static struct l_queue *preauths;
@@ -2770,6 +2772,9 @@ void eapol_register(struct eapol_sm *sm)
 bool eapol_start(struct eapol_sm *sm)
 {
 	if (sm->handshake->settings_8021x) {
+		char ssid[sm->handshake->ssid_len + 1];
+		_auto_(l_free) char *network_id = NULL;
+
 		sm->eap = eap_new(eapol_eap_msg_cb, eapol_eap_complete_cb, sm);
 
 		if (!sm->eap)
@@ -2785,6 +2790,12 @@ bool eapol_start(struct eapol_sm *sm)
 
 		eap_set_key_material_func(sm->eap, eapol_eap_results_cb);
 		eap_set_event_func(sm->eap, eapol_eap_event_cb);
+
+		memcpy(ssid, sm->handshake->ssid, sm->handshake->ssid_len);
+		ssid[sm->handshake->ssid_len] = 0;
+		network_id =
+			storage_get_network_file_path(SECURITY_8021X, ssid);
+		eap_set_peer_id(sm->eap, network_id);
 	}
 
 	sm->started = true;
diff --git a/src/station.c b/src/station.c
index eab16eff..ebd3831b 100644
--- a/src/station.c
+++ b/src/station.c
@@ -60,6 +60,9 @@
 #include "src/sysfs.h"
 #include "src/band.h"
 #include "src/ft.h"
+#include "src/eap.h"
+#include "src/eap-tls-common.h"
+#include "src/storage.h"
 
 static struct l_queue *station_list;
 static uint32_t netdev_watch;
@@ -5139,6 +5142,9 @@ static int station_init(void)
 
 	watchlist_init(&event_watches, NULL);
 
+	eap_tls_set_session_cache_ops(storage_tls_session_cache_get,
+					storage_tls_session_cache_sync);
+
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 1/3] storage: Maintain the tls session cache object

[Denis Kenzior]

Hi Andrew,

On 11/11/22 19:19, Andrew Zaborowski wrote:
> Instead of only providing methods to load and save the TLS cache from/to
> file, keep the memory copy of the cache directly in storage.c since it
> doesn't fit anywhere else conceptually.  storage.c now only provides a
> getter for the cache object and the _sync method to be called by anyone
> who makes a change to the cache object.
> 

So overall I'm fine with this approach, just two quick things:

> While the use case is for eap-tls-common.c to call these, the same cache
> object could be used if l_tls is used in another part of IWD.  Any
> sessions saved in relation to a specific network (ESS) should use group
> names ending in the storage_get_network_file_path encoding of the
> network's SSID+security type, so that storage.c can automatically drop
> these cache entries if the network is being forgotten.

It seems overkill to encode the full path into the group name.  Perhaps a 
hex-encoded SSID is enough?

> 
> Since the cache object needs to be freed when exiting, use
> storage_exit() for that.  However, since there was already a
> storage_init() and storage_exit() pair of functions, with storage_init
> called from main.c conditionally on whether systemd encryption was
> enabled, rename that storage_init() to storage_key_init() and declare an
> IWD_MODULE to hanadle the calling of the new storage_init() and
> storage_exit().  There's no warranty that those will be called before
> and after any other module's init/exit but they're not necessary for any
> functionality that other modules depend on.

While what you have proposed here works fine, I do find it a little strange that 
storage_key_init() is being called before the modules were initialized (and thus 
storage_init() was called).  There's also a strange case where storage_exit() 
would not be called in case storage_key_init() failed, unlike the behavior now 
in $HEAD.

There seems to be no need for any of this at all since storage_init does 
nothing?  It might be easier to just let storage remain special (not a module) 
as it is now.

> ---
>   src/main.c    |  3 +-
>   src/storage.c | 92 ++++++++++++++++++++++++++++++++++++++++-----------
>   src/storage.h |  7 ++--
>   3 files changed, 77 insertions(+), 25 deletions(-)
> 

<snip>

> +static void storage_network_remove_tls_sessions(const char *path)
> +{
> +	_auto_(l_strv_free) char **groups = NULL;
> +	char **group;
> +	size_t group_len;
> +	size_t path_len;
> +	bool changed = false;
> +
> +	if (!tls_session_cache)
> +		return;
> +
> +	groups = l_settings_get_groups(tls_session_cache);
> +	path_len = strlen(path);
> +
> +	for (group = groups; *group; group++)
> +		if ((group_len = strlen(*group)) > path_len &&

Hmm, >= path_len?

> +				!memcmp(*group + group_len - path_len, path,
> +					path_len)) {
> +			l_settings_remove_group(tls_session_cache, *group);
> +			changed = true;
> +		}
> +
> +	if (changed)
> +		storage_tls_session_cache_sync();
> +}
> +

Regards,
-Denis

Re: [PATCH 1/3] storage: Maintain the tls session cache object

[Andrew Zaborowski]

Hi Denis,

On Mon, 14 Nov 2022 at 21:03, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/11/22 19:19, Andrew Zaborowski wrote:
> > Any
> > sessions saved in relation to a specific network (ESS) should use group
> > names ending in the storage_get_network_file_path encoding of the
> > network's SSID+security type, so that storage.c can automatically drop
> > these cache entries if the network is being forgotten.
>
> It seems overkill to encode the full path into the group name.  Perhaps a
> hex-encoded SSID is enough?

It would be nice to include the security type, how about using the
file name without the rest of the path?

>
> >
> > Since the cache object needs to be freed when exiting, use
> > storage_exit() for that.  However, since there was already a
> > storage_init() and storage_exit() pair of functions, with storage_init
> > called from main.c conditionally on whether systemd encryption was
> > enabled, rename that storage_init() to storage_key_init() and declare an
> > IWD_MODULE to hanadle the calling of the new storage_init() and
> > storage_exit().  There's no warranty that those will be called before
> > and after any other module's init/exit but they're not necessary for any
> > functionality that other modules depend on.
>
> While what you have proposed here works fine, I do find it a little strange that
> storage_key_init() is being called before the modules were initialized (and thus
> storage_init() was called).  There's also a strange case where storage_exit()
> would not be called in case storage_key_init() failed, unlike the behavior now
> in $HEAD.

True.

>
> There seems to be no need for any of this at all since storage_init does
> nothing?

Right, it mainly only to improve the naming.

> It might be easier to just let storage remain special (not a module)
> as it is now.

Ok.

>
> > ---
> >   src/main.c    |  3 +-
> >   src/storage.c | 92 ++++++++++++++++++++++++++++++++++++++++-----------
> >   src/storage.h |  7 ++--
> >   3 files changed, 77 insertions(+), 25 deletions(-)
> >
>
> <snip>
>
> > +static void storage_network_remove_tls_sessions(const char *path)
> > +{
> > +     _auto_(l_strv_free) char **groups = NULL;
> > +     char **group;
> > +     size_t group_len;
> > +     size_t path_len;
> > +     bool changed = false;
> > +
> > +     if (!tls_session_cache)
> > +             return;
> > +
> > +     groups = l_settings_get_groups(tls_session_cache);
> > +     path_len = strlen(path);
> > +
> > +     for (group = groups; *group; group++)
> > +             if ((group_len = strlen(*group)) > path_len &&
>
> Hmm, >= path_len?

Ok, but using just the network's id as the group name wouldn't be a good idea.

Best regards

Re: [PATCH 1/3] storage: Maintain the tls session cache object

[Denis Kenzior]

Hi Andrew,

On 11/14/22 19:29, Andrew Zaborowski wrote:
> Hi Denis,
> 
> On Mon, 14 Nov 2022 at 21:03, Denis Kenzior <denkenz@gmail.com> wrote:
>> On 11/11/22 19:19, Andrew Zaborowski wrote:
>>> Any
>>> sessions saved in relation to a specific network (ESS) should use group
>>> names ending in the storage_get_network_file_path encoding of the
>>> network's SSID+security type, so that storage.c can automatically drop
>>> these cache entries if the network is being forgotten.
>>
>> It seems overkill to encode the full path into the group name.  Perhaps a
>> hex-encoded SSID is enough?
> 
> It would be nice to include the security type, how about using the
> file name without the rest of the path?
> 

OK, but I'm still failing to understand why?  TLS will only be ever relevant to 
8021x.  You even hardcode 8021x security type in the EAP changes in patch 3 :)

The file isn't really user visible either.  No normal user would be opening 
it...  In my opinion just basing things on the SSID would be enough.

However, now that I think about this approach some more, I think it will fail 
for Hotspot networks.  Since removal of these won't be detected properly. 
Actually, any removal triggered by the user won't work either since this path 
doesn't use storage_network_remove().

I think you need to hook into known_networks_remove() instead.

>>> +     for (group = groups; *group; group++)
>>> +             if ((group_len = strlen(*group)) > path_len &&
>>
>> Hmm, >= path_len?
> 
> Ok, but using just the network's id as the group name wouldn't be a good idea.
> 

Why not?  What is the additional "EAP-" prefix buying you:

+	cache_group_name = l_strdup_printf("EAP-%s", eap_get_peer_id(eap));

Which seems to translate to "EAP-/var/lib/iwd/foo.8021x"?  Is 
"/var/lib/iwd/foo.8021x" really any 'worse'?  Or just "foo"? :)

Regards,
-Denis

Re: [PATCH 1/3] storage: Maintain the tls session cache object

[Andrew Zaborowski]

On Tue, 15 Nov 2022 at 20:41, Denis Kenzior <denkenz@gmail.com> wrote:
> On 11/14/22 19:29, Andrew Zaborowski wrote:
> > On Mon, 14 Nov 2022 at 21:03, Denis Kenzior <denkenz@gmail.com> wrote:
> >> On 11/11/22 19:19, Andrew Zaborowski wrote:
> >>> Any
> >>> sessions saved in relation to a specific network (ESS) should use group
> >>> names ending in the storage_get_network_file_path encoding of the
> >>> network's SSID+security type, so that storage.c can automatically drop
> >>> these cache entries if the network is being forgotten.
> >>
> >> It seems overkill to encode the full path into the group name.  Perhaps a
> >> hex-encoded SSID is enough?
> >
> > It would be nice to include the security type, how about using the
> > file name without the rest of the path?
> >
>
> OK, but I'm still failing to understand why?

I didn't want to make this specific to 802.1x (from the commit
message: "While the use case is for eap-tls-common.c to call these,
the same cache
object could be used if l_tls is used in another part of IWD.")

>  TLS will only be ever relevant to
> 8021x.

As an example I can see things like connectivity checking needing tls
in some way.

>  You even hardcode 8021x security type in the EAP changes in patch 3 :)

That part is obviously specific to EAP, but storage.c not necessarily.

>
> The file isn't really user visible either.  No normal user would be opening
> it...  In my opinion just basing things on the SSID would be enough.

Ok.  Let's put "eap" in the filename.

>
> However, now that I think about this approach some more, I think it will fail
> for Hotspot networks.  Since removal of these won't be detected properly.

Ok, I wasn't really considering Hotspot so far.

> Actually, any removal triggered by the user won't work either since this path
> doesn't use storage_network_remove().

known_network_forget does call ops->remove which translates to
storage_network_remove for non-Hotspot networks.

>
> I think you need to hook into known_networks_remove() instead.
>
> >>> +     for (group = groups; *group; group++)
> >>> +             if ((group_len = strlen(*group)) > path_len &&
> >>
> >> Hmm, >= path_len?
> >
> > Ok, but using just the network's id as the group name wouldn't be a good idea.
> >
>
> Why not?  What is the additional "EAP-" prefix buying you:

To disambiguate between sessions saved by different places in IWD that
might use TLS, obviously this was based on the goal I mentioned above
that the cache not be limited to the EAP handshake.

Best regards

Re: [PATCH 1/3] storage: Maintain the tls session cache object

[Denis Kenzior]

Hi Andrew,

>> Actually, any removal triggered by the user won't work either since this path
>> doesn't use storage_network_remove().
> 
> known_network_forget does call ops->remove which translates to
> storage_network_remove for non-Hotspot networks.
> 

Yes, but that is not what actually removes the network.  That just generates a 
fancy 'rm /var/lib/foo.8021x' call.  The actual magic is in 
known_networks_remove() which is called by both hotspot inotify logic as well as 
known networks inotify logic.

Regards,
-Denis