| Message ID | 20221112011929.785388-1-andrew.zaborowski@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] storage: Maintain the tls session cache object | expand |
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
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
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
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
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
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);