Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (68.57%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces TLS support and configurable performance settings, enabling the server to handle encrypted traffic with a choice of crypto providers. Key additions include certificate and private key configuration, a new TlsAcceptor implementation, and integration tests for handshakes. Review feedback identifies critical bugs where non-existent methods are used for PEM loading in src/config.rs, along with suggestions to remove unused imports, fix duplicate toolchain components, and replace debugging println! calls with proper tracing.
| [toolchain] | ||
| channel = "1.90.0" | ||
| components = ["rustfmt", "clippy"] | ||
| components = ["rustfmt", "clippy", "rustfmt", "rust-src", "rust-analyzer"] |
There was a problem hiding this comment.
The rustfmt component is listed twice in the components array. Please remove the duplicate entry to keep the configuration clean.
| components = ["rustfmt", "clippy", "rustfmt", "rust-src", "rust-analyzer"] | |
| components = ["rustfmt", "clippy", "rust-src", "rust-analyzer"] |
|
/gemini review |
|
Warning Gemini is experiencing higher than usual traffic and was unable to create the review. Please try again in a few hours by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces TLS support to the server by integrating rustls and compio-tls, adding configuration options for certificates, private keys, and crypto providers. It also refactors performance settings into a dedicated configuration block. Review feedback highlights critical bugs in the TLS configuration decoding logic, specifically regarding certificate chain support and type mismatches during PEM parsing. An improvement was also suggested for the asynchronous write logic in the test suite's memory stream implementation.
| impl<S: ErrorSpan> knus::DecodeScalar<S> for TLSCertificate { | ||
| fn raw_decode( | ||
| val: &Spanned<Literal, S>, | ||
| _: &mut knus::decode::Context<S>, | ||
| ) -> Result<Self, DecodeError<S>> { | ||
| match &**val { | ||
| Literal::String(s) => { | ||
| let path = Path::new(&**s); | ||
| check_path_safety(path, "cert").map_err(|e| DecodeError::Conversion { | ||
| span: val.span().clone(), | ||
| source: e.into(), | ||
| })?; | ||
| CertificateDer::from_pem_file(path) | ||
| .map(TLSCertificate) | ||
| .map_err(|e| DecodeError::Conversion { | ||
| span: val.span().clone(), | ||
| source: Box::new(e), | ||
| }) | ||
| } | ||
| _ => Err(DecodeError::scalar_kind(Kind::String, val)), | ||
| } | ||
| } | ||
| fn type_check(type_name: &Option<Spanned<TypeName, S>>, ctx: &mut knus::decode::Context<S>) { | ||
| if let Some(typ) = type_name { | ||
| ctx.emit_error(DecodeError::TypeName { | ||
| span: typ.span().clone(), | ||
| found: Some((**typ).clone()), | ||
| expected: ExpectedType::no_type(), | ||
| rust_type: "TLSCertificate", | ||
| }); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, PartialEq)] | ||
| pub struct TLSPrivateKey(pub PrivateKeyDer<'static>); | ||
|
|
||
| impl TryFrom<PathBuf> for TLSPrivateKey { | ||
| type Error = anyhow::Error; | ||
|
|
||
| fn try_from(path: PathBuf) -> Result<Self, Self::Error> { | ||
| let data = | ||
| std::fs::read(&path).with_context(|| format!("couldn't find key file: {:?}", path))?; | ||
|
|
||
| let mut reader = &data[..]; | ||
| let key = rustls_pemfile::private_key(&mut reader) | ||
| .map_err(|e| anyhow!("PEM parsing failed: {}", e))? | ||
| .ok_or_else(|| anyhow!("key file is empty: {:?}", path))?; | ||
|
|
||
| Ok(TLSPrivateKey(key)) | ||
| } | ||
| } | ||
|
|
||
| impl FromStr for TLSPrivateKey { | ||
| type Err = anyhow::Error; | ||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| Self::try_from(PathBuf::from(s)) | ||
| } | ||
| } | ||
|
|
||
| impl From<TLSPrivateKey> for PrivateKeyDer<'static> { | ||
| fn from(value: TLSPrivateKey) -> Self { | ||
| value.0 | ||
| } | ||
| } | ||
|
|
||
| impl Clone for TLSPrivateKey { | ||
| fn clone(&self) -> Self { | ||
| TLSPrivateKey(self.0.clone_key()) | ||
| } | ||
| } | ||
|
|
||
| impl<S: ErrorSpan> knus::DecodeScalar<S> for TLSPrivateKey { | ||
| fn raw_decode( | ||
| val: &Spanned<Literal, S>, | ||
| _: &mut knus::decode::Context<S>, | ||
| ) -> Result<Self, DecodeError<S>> { | ||
| match &**val { | ||
| Literal::String(s) => { | ||
| let path = Path::new(&**s); | ||
| check_path_safety(path, "key").map_err(|e| DecodeError::Conversion { | ||
| span: val.span().clone(), | ||
| source: e.into(), | ||
| })?; | ||
| PrivateKeyDer::from_pem_file(path) | ||
| .map(TLSPrivateKey) | ||
| .map_err(|e| DecodeError::Conversion { | ||
| span: val.span().clone(), | ||
| source: e.into(), | ||
| }) | ||
| } | ||
| _ => Err(DecodeError::scalar_kind(Kind::String, val)), | ||
| } | ||
| } | ||
|
|
||
| fn type_check(type_name: &Option<Spanned<TypeName, S>>, ctx: &mut knus::decode::Context<S>) { | ||
| if let Some(typ) = type_name { | ||
| ctx.emit_error(DecodeError::TypeName { | ||
| span: typ.span().clone(), | ||
| found: Some((**typ).clone()), | ||
| expected: ExpectedType::no_type(), | ||
| rust_type: "TLSPrivateKey", | ||
| }); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The knus::DecodeScalar implementations for both TLSCertificate and TLSPrivateKey have issues that will cause compilation errors and prevent correct handling of TLS configurations.
1. TLSCertificate:
The TLSCertificate struct should hold a Vec<CertificateDer<'static>> to support certificate chains, but it's defined to hold only one. The raw_decode function attempts to use CertificateDer::from_pem_file, which returns a Vec, and then incorrectly tries to construct a TLSCertificate with it.
2. TLSPrivateKey:
Similarly, raw_decode for TLSPrivateKey uses PrivateKeyDer::from_pem_file, which also returns a Vec, and this is incompatible with the TLSPrivateKey struct that expects a single key.
Suggested Fixes:
-
Update
TLSCertificateto hold aVec:#[derive(Clone, Debug, PartialEq)] pub struct TLSCertificate(pub Vec<CertificateDer<'static>>);
And update its
FromandTryFromimplementations. -
Correct the
raw_decodeimplementations. Here's an example:For
TLSCertificate:fn raw_decode(...) -> Result<Self, DecodeError<S>> { // ... let certs = CertificateDer::from_pem_file(path).map_err(|e| ...)?; if certs.is_empty() { return Err(DecodeError::Conversion { ... }); } Ok(TLSCertificate(certs)) }
For
TLSPrivateKey:fn raw_decode(...) -> Result<Self, DecodeError<S>> { // ... let mut keys = PrivateKeyDer::from_pem_file(path).map_err(|e| ...)?; if keys.is_empty() { return Err(DecodeError::Conversion { ... }); } Ok(TLSPrivateKey(keys.remove(0))) }
-
In
src/cert.rs, change the call towith_single_certto pass the whole vector:.with_single_cert(cert.into(), key.into())
These changes are critical for the TLS feature to compile and function correctly.
| if let Err(e) = self.tx.try_send(data) { | ||
| tracing::warn!(target: "MemoryStream", "broken pipe"); | ||
| if e.is_full() { | ||
| tracing::debug!(target: "MemoryStream", "channel full, awaiting capacity"); | ||
| if self.tx.send(e.into_inner()).await.is_err() { | ||
| tracing::error!(target: "MemoryStream", "failed to send data: channel closed"); | ||
| return BufResult( | ||
| Err(std::io::Error::new( | ||
| std::io::ErrorKind::BrokenPipe, | ||
| "closed", | ||
| )), | ||
| buf, | ||
| ); | ||
| } | ||
| } else { | ||
| tracing::error!(target: "MemoryStream", "write failed: broken pipe"); | ||
| return BufResult( | ||
| Err(std::io::Error::new( | ||
| std::io::ErrorKind::BrokenPipe, | ||
| "closed", | ||
| )), | ||
| buf, | ||
| ); | ||
| } | ||
| }; |
There was a problem hiding this comment.
The write implementation for MemoryStream can be simplified. Instead of using try_send and handling the is_full case manually, you can directly use self.tx.send(data).await. This is cleaner and more idiomatic for async code.
if self.tx.send(data).await.is_err() {
tracing::error!(target: "MemoryStream", "write failed: broken pipe");
return BufResult(
Err(std::io::Error::new(
std::io::ErrorKind::BrokenPipe,
"closed",
)),
buf,
);
}1 participant
Summary
This PR introduces TLS support to the Damas server, enabling secure HTTPS communication.
Key Changes
tlsblock to the configurationconfig.kdlto specify certificate and private key paths.TLSCertificateandTLSPrivateKeytypes for robust PEM parsing and validation during configuration loading.PerformanceConfigfor performance tuning and supportring/aws-lc-rsproviders via feature flags.PerformanceConfigfor modular performance tuning and enabled switchable crypto providers via feature flagstest/tls_test.rsthat verifies the TLS handshake and data transmission using an in-memory duplex stream.Technical Notes
MemoryStreamthat satisfies compio'sAsyncRead/AsyncWritetraits to enable high-speed integration testing without actual network I/O.MemoryStreamimplementation, ensuring compatibility with compio'sIoBufrequirements.Quality Control
cargo-nextest.Related Issues