-
Notifications
You must be signed in to change notification settings - Fork 19
Add option to verify JWT tokens in the HTTP Authorization header #72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -20,11 +20,14 @@ use hyper_util::rt::TokioIo; | |
| use crate::vss_service::VssService; | ||
| use api::auth::{Authorizer, NoopAuthorizer}; | ||
| use api::kv_store::KvStore; | ||
| use auth_impls::{DecodingKey, JWTAuthorizer}; | ||
| use impls::postgres_store::{Certificate, PostgresPlaintextBackend, PostgresTlsBackend}; | ||
| use std::sync::Arc; | ||
|
|
||
| pub(crate) mod util; | ||
| pub(crate) mod vss_service; | ||
| mod util; | ||
| mod vss_service; | ||
|
|
||
| use util::config::{Config, ServerConfig}; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: This likely should import be added in the above group. |
||
|
|
||
| fn main() { | ||
| let args: Vec<String> = std::env::args().collect(); | ||
|
|
@@ -33,22 +36,21 @@ fn main() { | |
| std::process::exit(1); | ||
| } | ||
|
|
||
| let config = match util::config::load_config(&args[1]) { | ||
| Ok(cfg) => cfg, | ||
| Err(e) => { | ||
| eprintln!("Failed to load configuration: {}", e); | ||
| std::process::exit(1); | ||
| }, | ||
| }; | ||
|
|
||
| let addr: SocketAddr = | ||
| match format!("{}:{}", config.server_config.host, config.server_config.port).parse() { | ||
| Ok(addr) => addr, | ||
| let Config { server_config: ServerConfig { host, port, rsa_pub_file_path }, postgresql_config } = | ||
| match util::config::load_config(&args[1]) { | ||
| Ok(cfg) => cfg, | ||
| Err(e) => { | ||
| eprintln!("Invalid host/port configuration: {}", e); | ||
| eprintln!("Failed to load configuration: {}", e); | ||
| std::process::exit(1); | ||
| }, | ||
| }; | ||
| let addr: SocketAddr = match format!("{}:{}", host, port).parse() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah probably makes sense to make it one. |
||
| Ok(addr) => addr, | ||
| Err(e) => { | ||
| eprintln!("Invalid host/port configuration: {}", e); | ||
| std::process::exit(1); | ||
| }, | ||
| }; | ||
|
|
||
| let runtime = match tokio::runtime::Builder::new_multi_thread().enable_all().build() { | ||
| Ok(runtime) => Arc::new(runtime), | ||
|
|
@@ -66,9 +68,27 @@ fn main() { | |
| std::process::exit(-1); | ||
| }, | ||
| }; | ||
| let authorizer: Arc<dyn Authorizer> = Arc::new(NoopAuthorizer {}); | ||
| let postgresql_config = | ||
| config.postgresql_config.expect("PostgreSQLConfig must be defined in config file."); | ||
|
|
||
| let authorizer: Arc<dyn Authorizer> = if let Some(file_path) = rsa_pub_file_path { | ||
| let rsa_pub_file = match std::fs::read(file_path) { | ||
| Ok(pem) => pem, | ||
| Err(e) => { | ||
| println!("Failed to read RSA public key file: {}", e); | ||
| std::process::exit(-1); | ||
| }, | ||
| }; | ||
| let rsa_public_key = match DecodingKey::from_rsa_pem(&rsa_pub_file) { | ||
| Ok(pem) => pem, | ||
| Err(e) => { | ||
| println!("Failed to parse RSA public key file: {}", e); | ||
| std::process::exit(-1); | ||
| }, | ||
| }; | ||
| Arc::new(JWTAuthorizer::new(rsa_public_key).await) | ||
| } else { | ||
| Arc::new(NoopAuthorizer {}) | ||
| }; | ||
|
|
||
| let endpoint = postgresql_config.to_postgresql_endpoint(); | ||
| let db_name = postgresql_config.database; | ||
| let store: Arc<dyn KvStore> = if let Some(tls_config) = postgresql_config.tls { | ||
|
|
@@ -109,6 +129,7 @@ fn main() { | |
| Arc::new(postgres_plaintext_backend) | ||
| }; | ||
| println!("Connected to PostgreSQL backend with DSN: {}/{}", endpoint, db_name); | ||
|
|
||
| let rest_svc_listener = | ||
| TcpListener::bind(&addr).await.expect("Failed to bind listening port"); | ||
| println!("Listening for incoming connections on {}", addr); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,13 +3,14 @@ use serde::Deserialize; | |
| #[derive(Deserialize)] | ||
| pub(crate) struct Config { | ||
| pub(crate) server_config: ServerConfig, | ||
| pub(crate) postgresql_config: Option<PostgreSQLConfig>, | ||
| pub(crate) postgresql_config: PostgreSQLConfig, | ||
|
||
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
| pub(crate) struct ServerConfig { | ||
| pub(crate) host: String, | ||
| pub(crate) port: u16, | ||
| pub(crate) rsa_pub_file_path: Option<String>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't that rather be part of a new optional There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please make it also possible to use an env var, since it's easier to use that on a k8s environment |
||
| } | ||
|
|
||
| #[derive(Deserialize)] | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Hmm, leaking this auth-depdenent type to
main.rsis a bit strange. Wouldn't it rather make sense to haveJWTAuthorizer::newbe fallible and handle the tehDecodingKey::from_rsa_pemstep?