Skip to content

Commit

Permalink
mod_oauth: Persist new access/refresh tokens to config files.
Browse files Browse the repository at this point in the history 
Originally, updated refresh tokens were only saved in memory,
which meant that whenever the module was loaded again, if the
original refresh token was no longer valid, authentication would
eventually starting failing until the tokens were updated manually.
To solve this, rudimentary API has been added to config.c to allow
updating/adding a config setting, which is used for updating the
saved refresh token and, in some cases, the access token. This
ensures that whenever the config is parsed again, we load the
latest token(s) and don't use stale tokens which may no longer work.
  • Loading branch information
@InterLinked1
InterLinked1 committed Oct 13, 2024
1 parent b4bb34a commit cf85c5d
Show file tree
Hide file tree
Showing 4 changed files with 360 additions and 14 deletions.
172 changes: 165 additions & 7 deletions bbs/config.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -298,13 +298,17 @@ int bbs_config_free(struct bbs_config *c)

#define BEGINS_SECTION(s) (*s == '[')

static struct bbs_config *config_parse(const char *name)
/*! \brief Parse a config file, and optionally write a setting to it */
static struct bbs_config *config_parse_or_write(const char *name, FILE **restrict write_fp_ptr, const char *write_section, const char *write_key, const char *write_value)
{
struct bbs_config *cfg;
struct bbs_config_section *section = NULL;
struct bbs_keyval *keyval;
FILE *fp;
char *line = NULL;
char *dupline = NULL;
const char *endl = NULL;
int has_line_ending = 0;
size_t len = 0;
ssize_t bytes_read;
char *key, *value;
Expand Down Expand Up @@ -349,12 +353,32 @@ static struct bbs_config *config_parse(const char *name)

bbs_debug(3, "Parsing config %s\n", fullname);

#define COPY_EXISTING_LINE() fprintf(*write_fp_ptr, "%s", dupline);

while ((bytes_read = getline(&line, &len, fp)) != -1) {
lineno++;
rtrim(line);
if (strlen_zero(line)) {
continue; /* Skip blank/empty lines */
continue;
}
if (write_fp_ptr) {
has_line_ending = strchr(line, '\n') ? 1 : 0; /* Whether CR or LF, it has an LF */
if (!endl) {
/* Preserve whichever line endings this file uses. */
endl = strchr(line, '\r') ? "\r\n" : "\n";
}
/* Handle previous line */
if (dupline) {
COPY_EXISTING_LINE();
free(dupline);
}
dupline = strdup(line);
if (ALLOC_FAILURE(dupline)) {
/* Exit cleanly, but fail. */
fclose(*write_fp_ptr);
*write_fp_ptr = NULL;
}
}
rtrim(line);
if (*line == '\r' || *line == '\n') {
continue; /* Skip blank/empty lines */
}
Expand Down Expand Up @@ -384,6 +408,25 @@ static struct bbs_config *config_parse(const char *name)
#ifdef DEBUG_CONFIG_PARSING
bbs_debug(7, "New section: %s\n", section_name);
#endif

if (write_fp_ptr && write_section && section && !strcmp(section->name, write_section)) {
/* Append keyval to previous section since it was not already present.
* The only downside of this logic is in a file formatted like this:
* [section]
* keyA=valA
* keyB=valB
*
* [section2]
* ...
* If we append, it will be on the line immediately before [section2],
* which is correct, but it will be AFTER the blank line.
* This issue doesn't come up when the value is being replaced, rather than added.
* There's nothing incorrect about this, it just looks weird,
* but it's not worth the added overhead to work around that to me... */
fprintf(*write_fp_ptr, "%s = %s%s", write_key, write_value, endl);
bbs_verb(5, "Adding setting '%s' in existing config\n", write_key);
}

section = calloc(1, sizeof(*section));
if (ALLOC_FAILURE(section)) {
continue;
Expand Down Expand Up @@ -416,12 +459,31 @@ static struct bbs_config *config_parse(const char *name)
*value++ = '\0';
trim(key);
trim(value);

if (!section) {
bbs_warning("Failed to process %s=%s, not in a section (%s:%d)\n", key, value, name, lineno);
continue;
}

if (write_fp_ptr && write_section) {
/* At this point, we know if this key should be updated.
* Furthermore, keys cannot have empty values, so we are replacing some value. */
if (!strcmp(key, write_key) && !strcmp(section->name, write_section)) {
char *tmp = strchr(dupline, '=');
bbs_assert_exists(tmp); /* We duplicated a string which contains '=', so it must exist */
tmp++; /* There must be a value, so tmp must be nonempty at this point */
tmp = strstr(dupline, value);
bbs_assert_exists(value);
tmp += strlen(value);
/* Copy remainder of line from tmp onwards,
* then free dupline since we already handled this line. */
fprintf(*write_fp_ptr, "%s = %s%s", key, write_value, S_IF(tmp)); /* tmp also includes the line ending */
bbs_verb(5, "Updating setting '%s' in existing config\n", write_key);
FREE(dupline); /* free and set to NULL */
write_section = NULL; /* Do not do any further replacements/additions */
}
}

keyval = calloc(1, sizeof(*keyval));
if (ALLOC_FAILURE(keyval)) {
continue;
Expand All @@ -447,18 +509,114 @@ static struct bbs_config *config_parse(const char *name)
if (line) {
free(line); /* Free only once at the end */
}
if (write_fp_ptr && dupline) {
COPY_EXISTING_LINE();
FREE(dupline);
}
if (write_fp_ptr && write_section && section && !strcmp(section->name, write_section)) {
/* If the last line did not originally end with a newline, don't add one at the end now */
fprintf(*write_fp_ptr, "%s = %s%s", write_key, write_value, has_line_ending ? endl : "");
bbs_verb(5, "Adding setting '%s' in existing config\n", write_key);
}
fclose(fp);

/* Only at the end should we insert the config into the list. */
RWLIST_WRLOCK(&configs);
RWLIST_INSERT_TAIL(&configs, cfg, entry);
RWLIST_UNLOCK(&configs);
if (!write_key) {
RWLIST_WRLOCK(&configs);
RWLIST_INSERT_TAIL(&configs, cfg, entry);
RWLIST_UNLOCK(&configs);
}

bbs_verb(5, "Parsed config %s\n", fullname);

return cfg;
}

static struct bbs_config *config_parse(const char *name)
{
return config_parse_or_write(name, NULL, NULL, NULL, NULL);
}

int bbs_config_set_keyval(const char *filename, const char *section, const char *key, const char *value)
{
FILE *oldfp, *newfp;
struct bbs_config *cfg;
size_t fsize;
struct stat st;
char tmpfile[256] = "/tmp/bbs_config_XXXXXX";

/* Unlike Asterisk, we do not parse the entire config into memory,
* modify config objects, and then serialize it back to disk.
* Instead, we just work with the INI config file directly.
* This avoids having to worry about preserving comments and formatting verbatim,
* resulting in "dumber" (less semantic/powerful) but ultimately much simpler code.
* This would be very inefficient for updating multiple key-value pairs,
* but for updating a single setting in a file, it is fairly efficient. */

/* The GNU and BSD versions of sed use different syntax,
* and we may want to either set or replace the config value
* (and may not know or care what the old value is).
* So, do a brute force copy and update/add/replace. */

oldfp = fopen(filename, "r");
if (!oldfp) {
bbs_warning("Existing config file '%s' does not exist\n", filename);
/* If config file doesn't exist, we could create it,
* but more than likely something is wrong and we should just abort. */
return -1;
}
newfp = bbs_mkftemp(tmpfile, 0660);
if (!newfp) {
fclose(oldfp);
return -1;
}

cfg = config_parse_or_write(filename, &newfp, section, key, value);

/* Finalize and cleanup */
fclose(oldfp);
if (!newfp) {
/* Failure occured, and newfp has already been closed. */
if (cfg) {
config_free(cfg);
bbs_delete_file(tmpfile);
}
return -1;
}
fflush(newfp);
fsize = (size_t) ftell(newfp);
fclose(newfp);

/* We parsed the config, but don't want to keep it.
* For one, it's now outdated, and it may be duplicating the stale version already in the linked list. */
if (!cfg) {
bbs_delete_file(tmpfile);
return -1;
}
config_free(cfg); /* Wasn't inserted into list, so don't use bbs_config_free */

/* Edge case, but check if the disk was full and we weren't actually able to write the file to disk. */
if (stat(tmpfile, &st)) {
bbs_error("Failed to stat %s: %s\n", tmpfile, strerror(errno));
bbs_delete_file(tmpfile);
return -1;
}
if ((size_t) st.st_size != fsize) {
bbs_error("File size mismatch: %lu != %lu\n", st.st_size, fsize);
bbs_delete_file(tmpfile);
return -1;
}

/* Okay, now do the atomic rename, since we are confident file truncation did not occur. */
if (rename(tmpfile, filename)) {
bbs_error("Failed to rename %s -> %s: %s\n", tmpfile, filename, strerror(errno));
bbs_delete_file(tmpfile);
return -1;
}

return 0;
}

static int __bbs_cached_config_outdated(struct bbs_config *cfg, const char *name)
{
/* Check if the config has changed since we parsed it. */
Expand Down
10 changes: 10 additions & 0 deletions include/config.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -152,6 +152,16 @@ int bbs_config_free(struct bbs_config *cfg);
/*! \brief Destroy all existing configs (used at shutdown) */
void bbs_configs_free_all(void);

/*!
* \brief Write a single particular key-value setting to a config file, adding if not present and overwriting existing value if already present
* \param filename Config fil name
* \param section Config section name in which to add or update setting
* \param key Key name of setting to add or update
* \param value Setting value to add or update
* \retval 0 on success, -1 on failure
*/
int bbs_config_set_keyval(const char *filename, const char *section, const char *key, const char *value) __attribute__((nonnull (1, 2, 3, 4)));

/*!
* \brief Check whether a config file has been updated since it was last parsed
* \param name Config filename
Expand Down
37 changes: 30 additions & 7 deletions modules/mod_oauth.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -38,12 +38,14 @@ struct oauth_client {
const char *clientid;
const char *clientsecret;
const char *posturl;
const char *filename;
char *accesstoken;
char *refreshtoken;
RWLIST_ENTRY(oauth_client) entry;
time_t tokentime;
time_t expires;
unsigned int userid;
unsigned int accesstokeninitiallyempty:1;
bbs_mutex_t lock;
char data[0];
};
Expand All @@ -66,10 +68,10 @@ static void free_client(struct oauth_client *client)

/*! \note clients must be WRLOCK'd when calling */
static int add_oauth_client(const char *name, const char *clientid, const char *clientsecret, const char *refreshtoken, const char *accesstoken,
const char *posturl, int expires, unsigned int userid)
const char *posturl, int expires, unsigned int userid, const char *filename)
{
struct oauth_client *client;
size_t namelen, idlen, secretlen, urllen;
size_t namelen, idlen, secretlen, urllen, filenamelen;
char *pos;

REQUIRE_SETTING(clientid);
Expand All @@ -96,7 +98,8 @@ static int add_oauth_client(const char *name, const char *clientid, const char *
idlen = strlen(clientid);
secretlen = !strlen_zero(clientsecret) ? strlen(clientsecret) : 0;
urllen = strlen(posturl);
client = calloc(1, sizeof(*client) + namelen + idlen + secretlen + urllen + 4); /* NULs for each of them */
filenamelen = strlen(filename);
client = calloc(1, sizeof(*client) + namelen + idlen + secretlen + urllen + filenamelen + 5); /* NULs for each of them */
if (ALLOC_FAILURE(client)) {
return -1;
}
Expand All @@ -121,6 +124,10 @@ static int add_oauth_client(const char *name, const char *clientid, const char *
strcpy(pos, posturl);
client->posturl = pos;

pos += urllen + 1;
strcpy(pos, filename);
client->filename = pos;

client->refreshtoken = strdup(refreshtoken);
if (ALLOC_FAILURE(client->refreshtoken)) {
free(client);
Expand All @@ -136,6 +143,8 @@ static int add_oauth_client(const char *name, const char *clientid, const char *
return -1;
}
client->tokentime = time(NULL); /* Assumed to be valid as of now. */
} else {
client->accesstokeninitiallyempty = 1;
}

client->expires = expires;
Expand Down Expand Up @@ -197,7 +206,6 @@ static int refresh_token(struct oauth_client *client)
}

client->tokentime = now;
REPLACE(client->accesstoken, newaccesstoken);
if (!strlen_zero(newrefreshtoken) && strcmp(newrefreshtoken, client->refreshtoken)) {
/* The authorization server MAY issue a new refresh token, in which case the client
* MUST discard the old refresh token and replace it with the new refresh token.
Expand All @@ -206,8 +214,23 @@ static int refresh_token(struct oauth_client *client)
REPLACE(client->refreshtoken, newrefreshtoken);
/* This is good as long as the BBS is running continously, but we also need to update the static configuration file,
* or we'll lose the new refresh token the next time the BBS starts (or mod_oauth is reloaded). */
/*! \todo XXX Persist the new token to the config file */
bbs_warning("OAuth refresh token has changed, but is not persisted to configuration\n");
if (bbs_config_set_keyval(client->filename, client->name, "refreshtoken", client->refreshtoken)) {
bbs_warning("OAuth refresh token has changed, but is not persisted to configuration\n");
}
}
if (!strlen_zero(newaccesstoken) && !client->accesstokeninitiallyempty && (strlen_zero(client->accesstoken) || strcmp(newaccesstoken, client->accesstoken))) {
REPLACE(client->accesstoken, newaccesstoken);
/* Also update the config with the new access token, so we don't try to use
* an old access token in the future.
* This way, if there's an access token specified in the config file,
* when we write new refresh tokens to the file, we don't leave a stale access token there
* and load that back in when it's no longer valid. */
if (bbs_config_set_keyval(client->filename, client->name, "accesstoken", client->accesstoken)) {
bbs_warning("OAuth access token has changed, but is not persisted to configuration\n");
}
} else {
/* Just update in memory, we won't pull an old access token from the config in the future since it's not specified there to start with. */
REPLACE(client->accesstoken, newaccesstoken);
}

bbs_verb(4, "Refreshed OAuth token '%s' (good for %ds)\n", client->name, expires);
Expand Down Expand Up @@ -321,7 +344,7 @@ static int load_config_file(const char *filename, unsigned int forceuserid, cons
if (match && !strcmp(bbs_config_section_name(section), match)) {
namematch = 1;
}
add_oauth_client(bbs_config_section_name(section), clientid, clientsecret, refreshtoken, accesstoken, posturl, expires, userid);
add_oauth_client(bbs_config_section_name(section), clientid, clientsecret, refreshtoken, accesstoken, posturl, expires, userid, filename);
if (forceuserid && added++ > MAX_USER_OAUTH_TOKENS) {
/* Prevent a user from loading an unbounded amount of token mappings into memory. */
bbs_warning("Maximum user OAuth token mappings exceeded, ignoring remaining mappings\n");
Expand Down
Loading

0 comments on commit cf85c5d

Please sign in to comment.