Enable per namespace jwt key #861
Conversation
There was a problem hiding this comment.
I left some feedback on two big ticket items but for me the most important thing is that we at least remove all the context calls and wrap those in explicit error variants.
Some of the feedback is nits that to me remove a few of the leaky abstractions that we have here, I am okay with merging without those fixes but we should track those.
libsql-server/src/http/admin/mod.rs
Outdated
| } | ||
|
|
||
| async fn handle_post_config<M: MakeNamespace, C>( | ||
| State(app_state): State<Arc<AppState<M, C>>>, | ||
| Path(namespace): Path<String>, | ||
| Json(req): Json<HttpDatabaseConfig>, | ||
| ) -> crate::Result<()> { | ||
| if let Some(jwt_key) = req.jwt_key.as_deref() { | ||
| // Check that the jwt key is correct | ||
| parse_jwt_key(jwt_key).context("Could not parse JWT decoding key")?; |
There was a problem hiding this comment.
I think we want to avoid using context here since its going to hide the inner message if we use display. This has bitten us in the past. We should rather, create a error variant for this error case that can add this contextual message and map the error to that either within parse_jwt_key or here.
There was a problem hiding this comment.
Would you have an example of something similar that's already in the codebase, please? @LucioFranco
There was a problem hiding this comment.
In this case, it looks like we have a lot of detailed error messages here
libsql/libsql-server/src/auth.rs
Line 327 in 4db7a4c
context here.
In general, the pattern inside parse_jwt_key we should completely move away from because it tends to hide a lot of the actual errors that are useful for debugging due to the way anyhow uses debug over display. Happy to explain this more on slack but essentially we should avoid using context and instead when we want more detailed contextual errors use a new error enum that has those encoded into the variant via thiserror macro annotations.
haaawk
force-pushed
the
jwts2
branch
2 times, most recently
from
e0e9def to
e6b390c
Compare
| let config = self.db_config_store.get(); | ||
| if let Some(jwt_key) = config.jwt_key.as_deref() { | ||
| Ok(Some( | ||
| parse_jwt_key(jwt_key).context("Could not parse JWT decoding key")?, |
There was a problem hiding this comment.
It would be best if you decoded the key only once, when the namespace is loaded. As per jsonwebtoken Documentation:
All the different kind of keys we can use to decode a JWT. This key can be re-used so make sure you only initialize it once if you can for better performance.
Also, consider wrapping it in an Arc: cloning the key is not cheap.
There was a problem hiding this comment.
I was thinking about it but didn't have a good place to store the key. Let's do this in a follow up as this PR is hanging for some time now.
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
Signed-off-by: Piotr Jastrzebski <[email protected]>
No description provided.