-
Notifications
You must be signed in to change notification settings - Fork 4
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
Add rustls-mbedpki-provider
crate
#1
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.
Except the comments I left.
I suggest including https://github.com/rustls/rustls/blob/main/.rustfmt.toml in the PR and format code before merge.
Also I think pub exposed functions and types should be documented, especially for:
- rustls-mbedpki-provider/src/client_cert_verifier.rs
- rustls-mbedpki-provider/src/server_cert_verifier.rs
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.
I also recommend to include Cargo.lock
into repo to ensure a more stable build/CI.
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.
Hmm, for lib crates is that the preferred approach? I ask because I think lib crates should work with any sem-ver-compatible version of their dependencies.
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.
I think the reason is mainly for CI. It help trace the dependencies and build error when incomparable issue happened (like GitHub's dependent bot will help check update and security vulnerability based on Cargo.toml and Cargo.lock ).
For user of this crates, they always generate a new lock file their project.
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.
I will merge this crate into workspace after the crypto provider PR is merged
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.
I see. I guess my point is: the libs should work with newer sem-ver compatible versions of their dependencies as well (which is what could happen in the CI). But I see your point. We don't want a situation where CI passes today and fails tomorrow while our code has not changed.
Also please remember to resolve the conflict by merge master to this branch (this is fine since we will squash merge this PR) |
f3a3252
to
da58a26
Compare
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.
LGTM
commit 766eb0d Author: Yuxiang Cao <[email protected]> Date: Wed Nov 1 17:03:49 2023 -0700 chore: add copyright header commit e18e768 Author: Yuxiang Cao <[email protected]> Date: Wed Nov 1 16:53:33 2023 -0700 chore: update license commit 9931669 Author: Arash Sahebolamri <[email protected]> Date: Wed Nov 1 16:09:06 2023 -0700 Add `rustls-mbedpki-provider` crate (#1) * Create ci.yml * Add rustls-mbedpki-provider crate * Formatting changes * Check for mbedpki-provider warnings in ci.yml * Implement signature verification methods of `ClientCertVerifier` and `ServerSertVerifier` * Address review comments * Add doc comments to public items --------- Co-authored-by: Arash Sahebolamri <[email protected]>
No description provided.