Skip to content
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

Make TLS and Authorization Configuration Fields Optional #589

Open
wants to merge 12 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions crates/notary/server/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ pub struct AuthorizationProperties {
/// Switch to turn on or off auth middleware
pub enabled: bool,
/// File path of the whitelist API key csv
pub whitelist_csv_path: String,
pub whitelist_csv_path: Option<String>,
}

#[derive(Clone, Debug, Deserialize, Default)]
Expand Down Expand Up @@ -55,8 +55,8 @@ pub struct TLSProperties {
/// turned on unless TLS is handled by external setup e.g. reverse proxy,
/// cloud)
pub enabled: bool,
pub private_key_pem_path: String,
pub certificate_pem_path: String,
pub private_key_pem_path: Option<String>,
pub certificate_pem_path: Option<String>,
}

#[derive(Clone, Debug, Deserialize, Default)]
Expand Down
88 changes: 55 additions & 33 deletions crates/notary/server/src/server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,22 +59,26 @@ pub async fn run_server(config: &NotaryServerProperties) -> Result<(), NotarySer
debug!("Skipping TLS setup as it is turned off.");
None
} else {
let (tls_private_key, tls_certificates) = load_tls_key_and_cert(
&config.tls.private_key_pem_path,
&config.tls.certificate_pem_path,
)
.await?;

let mut server_config = ServerConfig::builder()
.with_safe_defaults()
.with_no_client_auth()
.with_single_cert(tls_certificates, tls_private_key)
.map_err(|err| eyre!("Failed to instantiate notary server tls config: {err}"))?;

// Set the http protocols we support
server_config.alpn_protocols = vec![b"http/1.1".to_vec()];
let tls_config = Arc::new(server_config);
Some(TlsAcceptor::from(tls_config))
if let (Some(private_key_path), Some(certificate_pem_path)) = (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the pem paths are not set when tls.enabled is true, we should throw an error — like your line 297

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we have updated pr to be based on the dev branch instead of the main branch and added validation to throw an error when TLS is enabled but PEM paths are not set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yorozunouchu i didn't see the error throwing fix yet, have you guys pushed it? right now if paths are not set it's still returning None, but we need it to throw error — so instead of doing if let Some(path)... just directly do something like path.ok_or_else(...)?

config.tls.private_key_pem_path.as_deref(),
config.tls.certificate_pem_path.as_deref(),
) {
let (tls_private_key, tls_certificates) =
load_tls_key_and_cert(private_key_path, certificate_pem_path).await?;

let mut server_config = ServerConfig::builder()
.with_safe_defaults()
.with_no_client_auth()
.with_single_cert(tls_certificates, tls_private_key)
.map_err(|err| eyre!("Failed to instantiate notary server tls config: {err}"))?;

// Set the http protocols we support
server_config.alpn_protocols = vec![b"http/1.1".to_vec()];
let tls_config = Arc::new(server_config);
Some(TlsAcceptor::from(tls_config))
} else {
None
}
};

// Load the authorization whitelist csv if it is turned on
Expand Down Expand Up @@ -289,11 +293,17 @@ fn load_authorization_whitelist(
debug!("Skipping authorization as it is turned off.");
None
} else {
// Check if whitelist_csv_path is Some and convert to &str
let whitelist_csv_path = config
.authorization
.whitelist_csv_path
.as_deref()
.ok_or_else(|| {
eyre!("Authorization whitelist csv path is not provided in the config")
})?;
// Load the csv
let whitelist_csv = parse_csv_file::<AuthorizationWhitelistRecord>(
&config.authorization.whitelist_csv_path,
)
.map_err(|err| eyre!("Failed to parse authorization whitelist csv: {:?}", err))?;
let whitelist_csv = parse_csv_file::<AuthorizationWhitelistRecord>(whitelist_csv_path)
.map_err(|err| eyre!("Failed to parse authorization whitelist csv: {:?}", err))?;
// Convert the whitelist record into hashmap for faster lookup
let whitelist_hashmap = authorization_whitelist_vec_into_hashmap(whitelist_csv);
Some(whitelist_hashmap)
Expand Down Expand Up @@ -343,10 +353,19 @@ fn watch_and_reload_authorization_whitelist(
)
.map_err(|err| eyre!("Error occured when setting up watcher for hot reload: {err}"))?;

// Check if whitelist_csv_path is Some and convert to &str
let whitelist_csv_path = config
.authorization
.whitelist_csv_path
.as_deref()
.ok_or_else(|| {
eyre!("Authorization whitelist csv path is not provided in the config")
})?;

// Start watcher to listen to any changes on the whitelist file
watcher
.watch(
Path::new(&config.authorization.whitelist_csv_path),
Path::new(whitelist_csv_path),
RecursiveMode::Recursive,
)
.map_err(|err| eyre!("Error occured when starting up watcher for hot reload: {err}"))?;
Expand Down Expand Up @@ -400,7 +419,7 @@ mod test {
let config = NotaryServerProperties {
authorization: AuthorizationProperties {
enabled: true,
whitelist_csv_path,
whitelist_csv_path: Some(whitelist_csv_path.clone()),
},
..Default::default()
};
Expand All @@ -424,16 +443,19 @@ mod test {
api_key: "unit-test-api-key".to_string(),
created_at: "unit-test-created-at".to_string(),
};
let file = OpenOptions::new()
.append(true)
.open(&config.authorization.whitelist_csv_path)
.unwrap();
let mut wtr = WriterBuilder::new()
.has_headers(false) // Set to false to avoid writing header again
.from_writer(file);
wtr.serialize(new_record).unwrap();
wtr.flush().unwrap();

if let Some(ref path) = config.authorization.whitelist_csv_path {
let file = OpenOptions::new()
.append(true)
.open(path)
.unwrap();
let mut wtr = WriterBuilder::new()
.has_headers(false) // Set to false to avoid writing header again
.from_writer(file);
wtr.serialize(new_record).unwrap();
wtr.flush().unwrap();
} else {
panic!("Whitelist CSV path should be provided in the config");
}
// Sleep to buy a bit of time for updated whitelist to be hot reloaded
tokio::time::sleep(Duration::from_millis(50)).await;

Expand All @@ -444,6 +466,6 @@ mod test {
.contains_key("unit-test-api-key"));

// Delete the cloned whitelist
std::fs::remove_file(&config.authorization.whitelist_csv_path).unwrap();
std::fs::remove_file(&whitelist_csv_path).unwrap();
}
}
6 changes: 3 additions & 3 deletions crates/notary/tests-integration/tests/notary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ fn get_server_config(port: u16, tls_enabled: bool, auth_enabled: bool) -> Notary
},
tls: TLSProperties {
enabled: tls_enabled,
private_key_pem_path: "../server/fixture/tls/notary.key".to_string(),
certificate_pem_path: "../server/fixture/tls/notary.crt".to_string(),
private_key_pem_path: Some("../server/fixture/tls/notary.key".to_string()),
certificate_pem_path: Some("../server/fixture/tls/notary.crt".to_string()),
},
notary_key: NotarySigningKeyProperties {
private_key_pem_path: "../server/fixture/notary/notary.key".to_string(),
Expand All @@ -64,7 +64,7 @@ fn get_server_config(port: u16, tls_enabled: bool, auth_enabled: bool) -> Notary
},
authorization: AuthorizationProperties {
enabled: auth_enabled,
whitelist_csv_path: "../server/fixture/auth/whitelist.csv".to_string(),
whitelist_csv_path: Some("../server/fixture/auth/whitelist.csv".to_string()),
},
}
}
Expand Down