Support multiple jwt keys for authentication #1374

david-mccullars · 2024-05-06T16:10:48Z

Modify the auth strategy for libsql-server to support multiple JWT keys. This will allow server administrators to more gracefully rotate keys by adding new keys without having to remove old ones simultaneously. Once all clients have been migrated to using new keys, the old one can be safely removed.

The modified auth strategy will accept a JWT if any of the public keys can successfully decrypt it. However, if none can (or all error), the last error will be returned.

As defined in RFC 1422 a *.PEM file by definition can contain multiple items, so to provide multiple JWT keys one needs only to concatenate them together. For example:

This should work whether the multiple certificates are passed via the SQLD_AUTH_JWT_KEY environment variable or the --auth-jwt-key-file argument.

jeremywrowe · 2024-05-06T16:19:07Z

libsql-server/src/auth/user_auth_strategies/jwt.rs

+ jwt_keys: &Vec<jsonwebtoken::DecodingKey>,
+ jwt: &str,
+) -> Result<Authenticated, AuthError> {
+ for (index, jwt_key) in jwt_keys.iter().enumerate() {


Should this be limited to two keys so that no iteration needs to be done and encapsulate the concept some how.

Pseudo Code:

validate_jwt(&primary_jwt_key, jwt) || validate_jwt(&secondary_jwt_key, jwt);

It's a good question. Here was my thinking:

For the (99%) key rotation use case it should only be in this state for a relatively short period of time (maybe a few hours or a day tops). Otherwise it'll be in the singleton case where we use the DecodingKeyContainer::Single to avoid iteration per Turso's suggestion.

There may be use cases that I'm not thinking about where 3 or more keys are actually desired. I couldn't come up with a good reason to artificially block this.

shopifyski · 2024-05-06T23:50:46Z

libsql-server/src/auth/user_auth_strategies/jwt.rs

@@ -7,8 +7,13 @@ use crate::{

 use super::{UserAuthContext, UserAuthStrategy};

+pub enum DecodingKeyContainer {


It won't be long until we forget why we implemented this single/multiple distinction
it is a seed of complexity -> it requires forked logic to handle
any chance we could have one more round of conversations about how much real overhead a Vec<> introduces?

I'm okay removing the complexity if everyone prefers that after looking at the enum version. Could be a classic case of premature optimization. Fwiw, I do think it should remaining somewhat intuitive that the vast majority of the time there should only be a single key and hence a motivation for this bit of weirdness.

let's let the others chime in then

I second @shopifyski here, I'd rather have a single code path, the overhead of allocating a vector once in a while is negligible

As an extra data point, I was exploring today what it would look like with just Vec<jsonwebtoken::DecodingKey> everywhere except for the DecodingKeyContainer enum. The immediate instinct was to rename all jwt_key to jwt_keys since it felt weird to do things like pub fn parse_jwt_key(data: &str) -> Result<Vec<DecodingKey>> or let key: Vec<DecodingKey> = .... However, as the thread kept on pulling I started to run into some backwards-compatibility issues, specifically with the jwt_key (singular) request variable in libsql-server/src/http/admin/mod.rs. We can certainly halt the cascade of singular-to-plural changes anywhere we like, but at some point we may have something gross like config.jwt_keys = req.jwt_key.

We could extend the config with jwt_keys, and merge jwt_key and jwt_keys as late as possible when building the decoding object. We would mark the jwt_key field as deprecated, and cleanup sometime in the future, would that contain the changes?

shopifyski · 2024-05-07T16:24:54Z

libsql-server/src/auth/user_auth_strategies/jwt.rs

+) -> Result<Authenticated, AuthError> {
+ for (index, jwt_key) in jwt_keys.iter().enumerate() {
+ let result = validate_jwt(&jwt_key, jwt);
+ if result.is_ok() || index == jwt_keys.len() - 1 {


we're swallowing individual errors
we should at least log a warning if result is Err

The problem is that those errors are expected. If I have two different keys, only one of them can decrypt the JWT, so the other one will error out. If we log them it will get very noisy without adding value.

maybe debug log then? to turn it on only when troubleshooting?

shopifyski · 2024-05-07T16:41:34Z

libsql-server/src/auth/user_auth_strategies/jwt.rs

+ }
+ }
+
+ Err(AuthError::Other)


in case validation fails, we might be interested in individual errors for each jwt
I suggest that we collect all errors an fold them into a single one, something like https://stackoverflow.com/a/61171459

When validation fails, we'll return the last error. The only reason we would ever get this generic one is when there are no JWT keys - which really shouldn't happen.

Another thought here is do we want to expose the fact that there are multiple keys to clients? That feels like an internal piece of data that really shouldn't be leaked.

shopifyski · 2024-05-07T16:53:42Z

libsql-server/src/auth/user_auth_strategies/jwt.rs

@@ -138,6 +164,23 @@ mod tests {
 (encoding_key, decoding_key)
 }

+ fn key_pairs(


maybe rename to generate_key_pairs
verb_object() naming convention is preferred for functions
key_pair() function should also be renamed
object() only names are best left for getters

I'm okay with renaming both, but as you suggest we should keep them consistent. It would be weird to have one be key_pair and the other generate_key_pairs.

libsql-server/src/auth/parsers.rs

jeremywrowe reviewed May 6, 2024

View reviewed changes

shopifyski reviewed May 6, 2024

View reviewed changes

shopifyski reviewed May 7, 2024

View reviewed changes

libsql-server/src/auth/parsers.rs Outdated Show resolved Hide resolved

shopifyski reviewed May 7, 2024

View reviewed changes

libsql-server/src/auth/parsers.rs Outdated Show resolved Hide resolved

david-mccullars force-pushed the support-multiple-jwt-keys branch 2 times, most recently from 04a8422 to b54dd01 Compare May 9, 2024 00:14

david-mccullars added 2 commits May 21, 2024 09:38

Add auth::parsers::tests::parse_jwt_key* tests

a520d2e

Support multiple JWT keys

52e2a39

david-mccullars force-pushed the support-multiple-jwt-keys branch from b54dd01 to 52e2a39 Compare May 21, 2024 14:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support multiple jwt keys for authentication #1374

Support multiple jwt keys for authentication #1374

david-mccullars commented May 6, 2024

jeremywrowe May 6, 2024 •

edited

david-mccullars May 6, 2024

shopifyski May 6, 2024 •

edited

david-mccullars May 8, 2024

shopifyski May 9, 2024

MarinPostma May 13, 2024

david-mccullars May 14, 2024

MarinPostma May 15, 2024

shopifyski May 7, 2024

david-mccullars May 8, 2024 •

edited

shopifyski May 9, 2024

shopifyski May 7, 2024

david-mccullars May 8, 2024

david-mccullars May 8, 2024

shopifyski May 7, 2024 •

edited

david-mccullars May 8, 2024 •

edited

		@@ -7,8 +7,13 @@ use crate::{

		use super::{UserAuthContext, UserAuthStrategy};

		pub enum DecodingKeyContainer {

Support multiple jwt keys for authentication #1374

Are you sure you want to change the base?

Support multiple jwt keys for authentication #1374

Conversation

david-mccullars commented May 6, 2024

jeremywrowe May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shopifyski May 6, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

david-mccullars May 8, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shopifyski May 7, 2024 • edited

Choose a reason for hiding this comment

david-mccullars May 8, 2024 • edited

Choose a reason for hiding this comment

jeremywrowe May 6, 2024 •

edited

shopifyski May 6, 2024 •

edited

david-mccullars May 8, 2024 •

edited

shopifyski May 7, 2024 •

edited

david-mccullars May 8, 2024 •

edited