-
Notifications
You must be signed in to change notification settings - Fork 72
Add client certificates to push-attestation prototype #984
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
Conversation
This replaces the usage of `default-tls` feature from reqwest with `native-tls`, which uses OpenSSL as backend on Linux. Add 'anyhow' crate as dependency to add context information to user facing errors. This also disables the hostname verification because the certificates generated by the verifier don't have the Subject Alternative Name (SAN) set properly, making the certificates to fail hostname verification Signed-off-by: Anderson Toshiyuki Sasaki <ansasaki@redhat.com>
sergio-correia
left a comment
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, looks good to me.
| predicates = { version = "3.1.3" } | ||
| pretty_env_logger = "0.5" | ||
| reqwest = {version = "0.12", default-features = false, features = ["json", "default-tls"]} | ||
| reqwest = {version = "0.12", default-features = false, features = ["json", "native-tls"]} |
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.
Good point. I was under the impression we had to use native-tls explictly instead of default-tls, since we have the goal to use openssl on Linux, as default-tls might change its backend in the future.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
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.
In general, changes LGTM.
The only issue I see is get_https_client function seems to be too long. I would define some intermediate functions for the new code, such as:
- load_certificate
- load_client_identity
Up to you, @ansasaki . You can push directly and we can refactor it once working.
Thanks!
I think it should be possible to move this utility functions (load certificates from files and create the |
This replaces the usage of
default-tlsfeature from reqwest withnative-tls, which uses OpenSSL as backend on Linux.Add 'anyhow' crate as dependency to add context information to user facing errors.
This also disables the hostname verification because the certificates generated by the verifier don't have the Subject Alternative Name (SAN) set properly, making the certificates to fail hostname verification