Add TLS to signer #357
Add TLS to signer #357
Conversation
| @@ -78,7 +81,7 @@ pub async fn handle_docker_init(config_path: PathBuf, output_dir: PathBuf) -> Re | |||
| if let Some(SignerConfig { inner: SignerType::Remote { url }, .. }) = &cb_config.signer { | |||
| url.to_string() | |||
| } else { | |||
| format!("http://cb_signer:{signer_port}") | |||
| format!("https://cb_signer:{signer_port}") | |||
There was a problem hiding this comment.
shouldn't this be http if the TslMode is insecure?
| .signer | ||
| .as_ref() | ||
| .map(|config| match &config.tls_mode { | ||
| TlsMode::Insecure => None, |
There was a problem hiding this comment.
let's a warning when this is the case, like we have already below
| module_volumes.push(Volumes::Simple(format!( | ||
| "{}:{}/{}:ro", | ||
| certs_path.join(SIGNER_TLS_CERTIFICATE_NAME).display(), | ||
| SIGNER_TLS_CERTIFICATES_PATH_DEFAULT, | ||
| SIGNER_TLS_CERTIFICATE_NAME | ||
| ))); | ||
| } |
There was a problem hiding this comment.
nit this is duplicated below, let's refactor to a get_certs_volume?
| get_env_val( | ||
| SIGNER_TLS_CERTIFICATES_PATH_ENV, | ||
| SIGNER_TLS_CERTIFICATES_PATH_DEFAULT, | ||
| ), |
There was a problem hiding this comment.
assume it's fine that we insert this even if the TslMode may be None?
There was a problem hiding this comment.
i see we're simply not loading them after, i would still avoid adding them if not needed for clarity
| fn default_tls_mode() -> TlsMode { | ||
| TlsMode::Certificate(PathBuf::from("./certs")) | ||
| } |
There was a problem hiding this comment.
should the default be None instead? as this won't work out of the box
| ) | ||
| .await | ||
| } else { | ||
| info!("NOTE: Running in insecure HTTP mode, no TLS certificates provided"); |
There was a problem hiding this comment.
| info!("NOTE: Running in insecure HTTP mode, no TLS certificates provided"); | |
| warn!("Running in insecure HTTP mode, no TLS certificates provided"); |
|
should explicitly state what TLS version is supported no? |
This is a cherry-pick of the TLS support that @ManuelBilbao added in #297. It builds on top of #354 / #353 / #356 which obviate the nonce in the
jti, but it's important to keep TLS support in as part of the audit finding as well.Note that PR hasn't been updated in a bit, so it's a good idea to get some eyes on this one to ensure it got ported over properly and doesn't need any modernization changes.
While this is in draft review I'll look at adding an
--insecurevariant to the configuration that lets the user run without TLS if they want to (better for unit testing, anyway).