-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: dev
Are you sure you want to change the base?
Make TLS and Authorization Configuration Fields Optional #589
Conversation
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.
Thanks for the PR guys! Few comments, and also
- base your PR on dev instead of main
- remember to follow the guideline here
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.
Remember to base your PR on dev instead of main~
crates/notary/server/src/server.rs
Outdated
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)) = ( |
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.
if the pem paths are not set when tls.enabled
is true, we should throw an error — like your line 297
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.
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.
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.
@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(...)?
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev #589 +/- ##
==========================================
+ Coverage 54.48% 54.50% +0.02%
==========================================
Files 193 193
Lines 20618 20633 +15
==========================================
+ Hits 11234 11247 +13
- Misses 9384 9386 +2 ☔ View full report in Codecov by Sentry. |
@yorozunouchu the CI fails because of clippy and formatting; can you guys follow the guidelines here https://github.com/tlsnotary/tlsn/blob/dev/CONTRIBUTING.md#linting and https://github.com/tlsnotary/tlsn/blob/dev/CONTRIBUTING.md#style |
This pull request fixes #565 where the TLSNotary server fails to start when specific configuration fields (
private_key_pem_path
,certificate_pem_path
, andwhitelist_csv_path
) are missing, even when the corresponding features are disabled.Please review and provide feedback!