Add option to verify JWT tokens in the HTTP Authorization header #72
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
tankyleo
force-pushed
the
rust-jwt-auth
branch
from
96d6359 to
d127bd1
Compare
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Seems relatively straightforward - but for my understanding: is the plan to also add the issuance part in this PR?
rust/server/src/util/config.rs
Outdated
| pub(crate) struct Config { | ||
| pub(crate) server_config: ServerConfig, | ||
| pub(crate) postgresql_config: Option<PostgreSQLConfig>, | ||
| pub(crate) postgresql_config: PostgreSQLConfig, |
There was a problem hiding this comment.
Why is this mandatory now? How will this work with the InMemoryStore or any other impl we'll add?
There was a problem hiding this comment.
We currently don't support any alternative backends, and expect if this is None. I would prefer failing earlier, with the Config member type to represent our current status (ie not Option<PostgreSQLConfig> but PostgreSQLConfig).
The error message if the postgresql_config table is not present is this one:
Failed to load configuration: TOML parse error at line 1, column 1
|
1 | [server_config]
| ^
missing field `postgresql_config`
There was a problem hiding this comment.
That being said, I'm thinking we keep this Option to open up the InMemoryStore use case later.
There was a problem hiding this comment.
That being said, I'm thinking we keep this
Optionto open up theInMemoryStoreuse case later.
Yeah, I think it would be preferable if we already know that we'll have different backends.
Do you think issuance is in-scope for VSS-server ? My immediate thinking is VSS-server should only be verifying, and not touch the private key. Issuance should be handled by something separate with different security measures that does have this privileged access to the private key. See vss channel, it seems the immediate request from users is just the ability to set the public key for verifying JWT tokens. |
|
🔔 1st Reminder Hey @tnull! This PR has been waiting for your review. |
Discussed this elsewhere: it would be good to add issuance to the |
tnull
left a comment
tnull
left a comment
There was a problem hiding this comment.
Generally looks good, some minor comments.
Do we see a chance of adding a test for the authorizer? Maybe with some fixtures?
| pub(crate) struct ServerConfig { | ||
| pub(crate) host: String, | ||
| pub(crate) port: u16, | ||
| pub(crate) rsa_pub_file_path: Option<String>, |
There was a problem hiding this comment.
Shouldn't that rather be part of a new optional JwtAuthConfig section?
| use serde::{Deserialize, Serialize}; | ||
| use std::collections::HashMap; | ||
|
|
||
| pub use jsonwebtoken::DecodingKey; |
There was a problem hiding this comment.
nit: Hmm, leaking this auth-depdenent type to main.rs is a bit strange. Wouldn't it rather make sense to have JWTAuthorizer::new be fallible and handle the teh DecodingKey::from_rsa_pem step?
| mod util; | ||
| mod vss_service; | ||
|
|
||
| use util::config::{Config, ServerConfig}; |
There was a problem hiding this comment.
nit: This likely should import be added in the above group.
| std::process::exit(1); | ||
| }, | ||
| }; | ||
| let addr: SocketAddr = match format!("{}:{}", host, port).parse() { |
There was a problem hiding this comment.
There is a lot going on here. Can we declutter this? Btw, since we're already here, might make sense to switch this to the From<(I, u16)> for SocketAddr implementation rather than allocating a string and then parsing it again.
There was a problem hiding this comment.
this is personal preference, but in the follow-up PR I consolidate the host and port setting into a single address setting, string type, ie "127.0.0.1:54234". We would then allocate the string upfront. WDYT ? Overall looking to reduce the number of lines in the config files, and the number of different settings. The address the server binds to is a single setting in my head :)
3 participants
No description provided.