Support certificate chains #13
Conversation
| return Err(TLSError::General(format!("server certificates do not match ours"))); | ||
| } else { | ||
| tracing::trace!("Confirmed certificate match"); | ||
| } |
There was a problem hiding this comment.
We don't strictly need this message, but I kind of like having it for sanity checking.
There was a problem hiding this comment.
Thanks for the PR!
If I understand correctly, this doesn't do normal PKI verification just supports multiple certificates where those must be also in a file. I think the idea is acceptable&useful but there's at least one bug: the lengths of chains are not compared in verify_server_cert, so the comparison can return true even for different lengths - very likely a vulnerability.
| if presented_certs[0].0 != self.cert { | ||
| return Err(TLSError::General(format!("server certificates doesn't match ours"))); | ||
|
|
||
| for (c, p) in self.certs.iter().zip(presented_certs.iter()) { |
There was a problem hiding this comment.
Lengths should be compared before the loop and return error if they aren't equal. Please mention both lengths in error message.
src/lib.rs
Outdated
| |error| InternalConnectError::ParseCert { file: path.into(), error }); | ||
|
|
||
| if certs.len() != 1 { | ||
| return Err(InternalConnectError::InvalidCertCount { file: path.into(), count: certs.len(), }); | ||
| tracing::debug!("Certificate count is not 1 (Actual Count: {})", certs.len()); |
There was a problem hiding this comment.
Unconditional debug with message "Loaded certificate chain with {} certificates" (or similar) would be better since we don't treat 1 as special.
Could you also remove InvalidCertCount from InternalConnectError? It's not used anymore.
src/lib.rs
Outdated
| Ok(ServerCertVerified::assertion()) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Nit: please don't remove newlines at the end of file.
src/lib.rs
Outdated
| @@ -129,6 +130,10 @@ async fn load_macaroon(path: impl AsRef<Path> + Into<PathBuf>) -> Result<String, | |||
| /// | |||
| /// If you have a motivating use case for use of direct data feel free to open an issue and | |||
| /// explain. | |||
| #[tracing::instrument( | |||
| name = "Connecting to LND", | |||
| skip(address, cert_file, macaroon_file), | |||
There was a problem hiding this comment.
These shouldn't be sensitive if that's the reason why you skip them. (macaroon_file is just a path and on a correctly configured system it's only accessible by authorized users)
There was a problem hiding this comment.
I was actually skipping them because they did not implement debug as they were coming in, rather than them being sensitive. =)
Cargo.toml
Outdated
| @@ -22,6 +22,7 @@ webpki = "0.21.3" | |||
| rustls-pemfile = "1.0.0" | |||
| hex = "0.4.3" | |||
| tokio = { version = "1.7.1", features = ["fs"] } | |||
| tracing = { version = "0.1", features = ["log"] } | |||
There was a problem hiding this comment.
Do you happen to know how much this prolongs compile times? If yes would it be worth it to make it optional?
There was a problem hiding this comment.
I don't know. And I'm not exactly certain how to make it optional. =/
I'm actually 100% happy to pull the tracing out of here. It seems like something it should have but me just shoving it in to do some small changes is probably not ideal. It would be better if it were done everywhere and consistently.
There was a problem hiding this comment.
Thinking about it, tracing has MSRV 1.49 which could become annoying since I want MSRV 1.48 so yeah, making it optional in a separate PR would be the best appraoch.
Making it optional should be very easy, just set optional = true in Cargo.toml and then use `#[cfg_attr(feature = "tracing", tracing::instrument(...))] for attributes and
#[cfg(feature = "tracing")]
{
tracing::trace!("message");
}for logs in the code.
|
Thanks for the super quick feedback. Here's another pass. Again - super happy to pull all the tracing stuff if you'd prefer. |
|
Looks good! Could you please squash the commits? Or if you like make it two commits, one adding tracing, the other doing the rest. |
|
Thanks - I added tracing before the actual changes because the logs were kind of intertwined with the cert checks. |
|
Thanks a lot! |
chore: add minimal CI
This is a partial solution to #11, but does not address #1 at all.
I'm having a bit of an issue with root certs, but don't think it should be far off.