| Message ID | 6f9819204cd32b37ad98c3d24c18469d1d7db3ea.1612720013.git.ps@pks.im |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | File-backed external password store | expand |
On Sun, Feb 07, 2021 at 06:48:36PM +0100, Patrick Steinhardt wrote: > It's currently not easily possible to separate configuration of an > interface and credentials. This makes it hard to distribute > configuration across a set of nodes which use wpa_supplicant without > also having to store credentials in the same file. While this can be > solved via scripting, having a native way to achieve this would be > preferable. > > Turns out there already is a framework to have external password > storages. It only has a single "test" backend though, which is kind of > an in-memory store which gets initialized with all passwords up front. > This isn't really suitable for above usecase: the backend cannot be > initialized as part of the central configuration given that it needs the > credentials, and we want to avoid scripting. > > This commit thus extends the infrastructure to implement a new backend, > which instead uses a simple configuration file containing key-value > pairs. The file follows the format which wpa_supplicant.conf(5) uses: > empty lines and comments are ignored, while passwords can be specified > with simple `password-name=password-value` assignments. This sounds like a reasonable addition. > diff --git a/src/utils/ext_password_file.c b/src/utils/ext_password_file.c > +#include "common.h" > +#include "ext_password_i.h" > +#include "utils/config.h" I'd prefer to list the src/*/*.h before wpa_supplicant/*.h in include statements. > +static void * ext_password_file_init(const char *params) > +{ > + struct ext_password_file_data *data; > + > + if (!params) { > + wpa_printf(MSG_ERROR, "EXT PW FILE: no path given"); > + return NULL; > + } > + > + data = os_zalloc(sizeof(*data)); > + if (data == NULL) > + return NULL; > + data->path = os_strdup(params); > + > + return data; That os_strdup() might fail. It would be better check for that here and return NULL instead of assuming everything is fine and later crashing due to NULL pointer dereferencing.. > +static void ext_password_file_deinit(void *ctx) > +{ > + struct ext_password_file_data *data = ctx; > + > + str_clear_free(data->path); str_clear_free() sounds a bit heavy for a path name, but well, if that contains some secure information.. However: > +static struct wpabuf * ext_password_file_get(void *ctx, const char *name) > +{ > + struct ext_password_file_data *data = ctx; > + struct wpabuf *password = NULL; > + char buf[512], *pos; This buf[] is used to read the actual passwords, so it would be more useful to explicitly clear that memory after use here.. And probably not the best design to use wpa_printf(MSG_ERROR, "stuff with the raw line from the password file") to get passwords exposed in debug logs and/or stdout. Maybe just print the line number without any of the payload. > + while (wpa_config_get_line(buf, sizeof(buf), f, &line, &pos)) { > +done: > + fclose(f); > + return password; In other words, forced_memzero(buf, sizeof(buf)) before returning from the function. > diff --git a/wpa_supplicant/Makefile b/wpa_supplicant/Makefile And also similar changes for wpa_supplicant/Android.mk.
On Sun, Feb 14, 2021 at 12:18:22AM +0200, Jouni Malinen wrote: > On Sun, Feb 07, 2021 at 06:48:36PM +0100, Patrick Steinhardt wrote: [snip] > > +static void ext_password_file_deinit(void *ctx) > > +{ > > + struct ext_password_file_data *data = ctx; > > + > > + str_clear_free(data->path); > > str_clear_free() sounds a bit heavy for a path name, but well, if that > contains some secure information.. However: Right, that could be a simple `os_free` call. I'll leave it as-is for now, but I'm happy to change it. > > +static struct wpabuf * ext_password_file_get(void *ctx, const char *name) > > +{ > > + struct ext_password_file_data *data = ctx; > > + struct wpabuf *password = NULL; > > + char buf[512], *pos; > > This buf[] is used to read the actual passwords, so it would be more > useful to explicitly clear that memory after use here.. And probably not > the best design to use wpa_printf(MSG_ERROR, "stuff with the raw line > from the password file") to get passwords exposed in debug logs and/or > stdout. Maybe just print the line number without any of the payload. > > > + while (wpa_config_get_line(buf, sizeof(buf), f, &line, &pos)) { > > +done: > > + fclose(f); > > + return password; > > In other words, forced_memzero(buf, sizeof(buf)) before returning from > the function. Yup, makes sense. Patrick > > diff --git a/wpa_supplicant/Makefile b/wpa_supplicant/Makefile > > And also similar changes for wpa_supplicant/Android.mk. > > -- > Jouni Malinen PGP id EFC895FA
diff --git a/src/utils/ext_password.c b/src/utils/ext_password.c index 5615bd72a..cbf92de8c 100644 --- a/src/utils/ext_password.c +++ b/src/utils/ext_password.c @@ -20,6 +20,9 @@ static const struct ext_password_backend *backends[] = { #ifdef CONFIG_EXT_PASSWORD_TEST &ext_password_test, #endif /* CONFIG_EXT_PASSWORD_TEST */ +#ifdef CONFIG_EXT_PASSWORD_FILE + &ext_password_file, +#endif /* CONFIG_EXT_PASSWORD_FILE */ NULL }; diff --git a/src/utils/ext_password_file.c b/src/utils/ext_password_file.c new file mode 100644 index 000000000..9bb0cb823 --- /dev/null +++ b/src/utils/ext_password_file.c @@ -0,0 +1,129 @@ +/* + * External backend for file-backed passwords + * Copyright (c) 2021, Patrick Steinhardt <ps@pks.im> + * + * This software may be distributed under the terms of the BSD license. + * See README for more details. + */ + +#include "includes.h" + +#include "common.h" +#include "ext_password_i.h" +#include "utils/config.h" + + +/** + * Data structure for the file-backed password backend. + */ +struct ext_password_file_data { + char *path; /* path of the password file */ +}; + + +/** + * ext_password_file_init - Initialize file-backed password backend + * @params: Parameters passed by the user. + * Returns: Pointer to the initialized backend. + * + * This function initializes a new file-backed password backend. The user is + * expected to initialize this backend with the parameters being the path of + * the file that contains the passwords. + */ +static void * ext_password_file_init(const char *params) +{ + struct ext_password_file_data *data; + + if (!params) { + wpa_printf(MSG_ERROR, "EXT PW FILE: no path given"); + return NULL; + } + + data = os_zalloc(sizeof(*data)); + if (data == NULL) + return NULL; + data->path = os_strdup(params); + + return data; +} + + +/** + * ext_password_file_deinit - Deinitialize file-backed password backend + * @param ctx: The file-backed password backend + * + * This function frees all data associated with the file-backed password + * backend. + */ +static void ext_password_file_deinit(void *ctx) +{ + struct ext_password_file_data *data = ctx; + + str_clear_free(data->path); + os_free(data); +} + +/** + * ext_password_file_get - Retrieve password from the file-backed password backend + * @param ctx: The file-backed password backend + * @param name: Name of the password to retrieve + * Returns: Buffer containing the password if one was found or %NULL. + * + * This function tries to find a password identified by name in the password + * file. The password is expected to be stored in `NAME=PASSWORD` format. + * Comments and empty lines in the file are ignored. Invalid lines will cause + * an error message, but will not cause the function to fail. + */ +static struct wpabuf * ext_password_file_get(void *ctx, const char *name) +{ + struct ext_password_file_data *data = ctx; + struct wpabuf *password = NULL; + char buf[512], *pos; + int line = 0; + FILE *f; + + f = fopen(data->path, "r"); + if (f == NULL) { + wpa_printf(MSG_ERROR, "EXT PW FILE: could not open file '%s': %s", + data->path, strerror(errno)); + return NULL; + } + + wpa_printf(MSG_DEBUG, "EXT PW FILE: get(%s)", name); + + while (wpa_config_get_line(buf, sizeof(buf), f, &line, &pos)) { + char *sep = os_strchr(pos, '='); + if (!sep) { + wpa_printf(MSG_ERROR, "Invalid password line %d: '%s'.", + line, buf); + continue; + } + + if (!sep[1]) { + wpa_printf(MSG_ERROR, "No password for line %d: '%s'.", + line, buf); + continue; + + } + + if (os_strncmp(name, pos, sep - pos)) + continue; + + password = wpabuf_alloc_copy(sep + 1, os_strlen(sep + 1)); + goto done; + } + + wpa_printf(MSG_ERROR, "Password for '%s' was not found.", name); + +done: + fclose(f); + return password; +} + + +const struct ext_password_backend ext_password_file = { + .name = "file", + .init = ext_password_file_init, + .deinit = ext_password_file_deinit, + .get = ext_password_file_get, +}; diff --git a/src/utils/ext_password_i.h b/src/utils/ext_password_i.h index 948eaf542..872ccd195 100644 --- a/src/utils/ext_password_i.h +++ b/src/utils/ext_password_i.h @@ -26,4 +26,8 @@ struct wpabuf * ext_password_alloc(size_t len); extern const struct ext_password_backend ext_password_test; #endif /* CONFIG_EXT_PASSWORD_TEST */ +#ifdef CONFIG_EXT_PASSWORD_FILE +extern const struct ext_password_backend ext_password_file; +#endif /* CONFIG_EXT_PASSWORD_FILE */ + #endif /* EXT_PASSWORD_I_H */ diff --git a/wpa_supplicant/Makefile b/wpa_supplicant/Makefile index 6a92aaace..a59003ef2 100644 --- a/wpa_supplicant/Makefile +++ b/wpa_supplicant/Makefile @@ -1752,6 +1752,12 @@ CFLAGS += -DCONFIG_EXT_PASSWORD_TEST NEED_EXT_PASSWORD=y endif +ifdef CONFIG_EXT_PASSWORD_FILE +OBJS += ../src/utils/ext_password_file.o +CFLAGS += -DCONFIG_EXT_PASSWORD_FILE +NEED_EXT_PASSWORD=y +endif + ifdef NEED_EXT_PASSWORD OBJS += ../src/utils/ext_password.o CFLAGS += -DCONFIG_EXT_PASSWORD
It's currently not easily possible to separate configuration of an interface and credentials. This makes it hard to distribute configuration across a set of nodes which use wpa_supplicant without also having to store credentials in the same file. While this can be solved via scripting, having a native way to achieve this would be preferable. Turns out there already is a framework to have external password storages. It only has a single "test" backend though, which is kind of an in-memory store which gets initialized with all passwords up front. This isn't really suitable for above usecase: the backend cannot be initialized as part of the central configuration given that it needs the credentials, and we want to avoid scripting. This commit thus extends the infrastructure to implement a new backend, which instead uses a simple configuration file containing key-value pairs. The file follows the format which wpa_supplicant.conf(5) uses: empty lines and comments are ignored, while passwords can be specified with simple `password-name=password-value` assignments. With this new backend, splitting up credentials and configuration becomes trivial: # /etc/wpa_supplicant/wpa_supplicant.conf ext_password_backend=file:/etc/wpa_supplicant/psk.conf network={ ssid="foobar" psk=ext:foobar } # /etc/wpa_supplicant/psk.conf foobar=ecdabff9c80632ec6fcffc4a8875e95d45cf93376d3b99da6881298853dc686b Alternative approaches would be to support including other configuration files in the main configuration, such that common configuration and network declarations including credentials are split up into separate files. But the implementation would probably have been more complex compared to reusing the already-existing framework for external password backends. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- src/utils/ext_password.c | 3 + src/utils/ext_password_file.c | 129 ++++++++++++++++++++++++++++++++++ src/utils/ext_password_i.h | 4 ++ wpa_supplicant/Makefile | 6 ++ 4 files changed, 142 insertions(+) create mode 100644 src/utils/ext_password_file.c