Implementation of CoSERV signing and verification - #8
Conversation
thomas-fossati
left a comment
thomas-fossati
left a comment
There was a problem hiding this comment.
Looks very good to me, thanks!
I have inlined a high-level (non-blocking) comment on the verification API signature that I’d like to discuss.
|
|
||
| // check if header contains the same algorithm as the verifier | ||
| // todo: find a better way of doing this | ||
| if let Some(ref alg) = sign1.protected.header.alg { |
There was a problem hiding this comment.
This makes me wonder whether the verification API should simply not require an explicit algorithm from the caller and instead try to use whatever is present in the signed object.
There was a problem hiding this comment.
I think this comment was not addressed by the rework. The current implementation is similar to the go implementation of coserv verification in veraison/corim.
The explicit check was removed because if there is a mismatch, it would throw an error during signature verification.
But taking the algorithm from the header seems to be the better approach since it is a mandatory field according to the spec. @paulhowardarm, since you have already approved the changes, is it ok to change the verify function signature from verify_and_extract(verifier: &impl CoseVerifier, cose_alg: CoseAlgorithm, data: &[u8]) to verify_and_extract(verifier: &impl CoseVerifier, data: &[u8]).
There was a problem hiding this comment.
Sure @DhanusML no problem at all - feel free to tweak as you see fit, and we can merge as soon as you're done.
There was a problem hiding this comment.
We have done the changes and pushed them. Please check.
paulhowardarm
left a comment
paulhowardarm
left a comment
There was a problem hiding this comment.
Thank you for the PR. Looks great overall, and offers the expected functionality. I have a few observations and suggestions. There are a couple of small things I think should be changed:
- The
fix_kty()function should be renamed, and the comment should be changed (it is not a "bug work-around") - One of the comments had a copy/paste error.
I also had a question on whether the sign/verify traits could be re-used from corim-rs rather than re-defined here. Maybe you already tried to do this but there was some reason it didn't work? If so, we don't necessarily need to change it.
| }) | ||
| } | ||
|
|
||
| /// Construct [OpensslVerifier] from PEM private key |
There was a problem hiding this comment.
I think the comment should say "from JWK public key" here
| use super::{CoseAlgorithm, CoservError}; | ||
|
|
||
| /// Interface for COSE signer. | ||
| pub trait CoseSigner { |
There was a problem hiding this comment.
For the CoseSigner and CoseVerifier traits, might it be possible to import those from corim-rs rather than re-define them here? The definitions are not identical, but given @thomas-fossati 's other comment on whether the algorithm field is necessary, I wonder if that makes it easier to just re-use the bits from corim-rs? Just to avoid the duplication.
| // this function is a workaround for a bug in corim-rs: | ||
| // key type for elliptic curve keys is serialized as EC2 | ||
| // instead of EC in corim-rs | ||
| fn fix_kty(bytes: &[u8]) -> Result<Vec<u8>, CoservError> { |
There was a problem hiding this comment.
This is not a bug in corim-rs. What's actually happening here is a format conversion from JWK to COSE key. I think this function needs to be called jwk_to_cose() (or something like that). The code itself might be fine as it is (certainly for a PoC), but please check the specs to make sure that no additional conversion steps are needed.
Implement methods for signing and verification of CoSERV. Re-exports CoseSigner and CoseVerifier traits from corim-rs. Also includes implementations OpensslSigner and OpensslVerifier of these traits, hidden behind the feature flag `openssl`. These internally use corim_rs::OpensslSigner and support JWK and PEM encoded keys. Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
cargo-deny: update Cargo.lock (bytes and time) Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
77f077e to
b99ffa5
Compare
|
Review comments are addressed. @paulhowardarm @thomas-fossati Please check. |
paulhowardarm
left a comment
paulhowardarm
left a comment
There was a problem hiding this comment.
Thanks for addressing the rework comments and the CI issues. It looks good to me now. I'm still slightly puzzled as to the need for the wrappers around the OpenSslSigner and OpenSslVerifier, but that's an implementation detail I guess - I'm happy that we're now re-using the traits from corim-rs rather than defining new ones.
There is no |
|
Thanks @DhanusML - I appreciate the explanation. I'm good with that for now, and happy to approve. I think in the future we probably need to revisit the sign/verify story across all of the Rust crates to maximise consistency and re-use. But I don't want to hold up this very useful piece of work in the meantime. I suggest we merge this PR. |
make verify_and_extract uses the alg present in the COSE header instead of supplying it. Signed-off-by: Dhanus M Lal <Dhanus.MLal@fujitsu.com>
thomas-fossati
left a comment
thomas-fossati
left a comment
There was a problem hiding this comment.
Awesome! Thanks for addressing my comment.
4 participants
Implement methods for signing and verification of CoSERV. The Signer, Verifier traits and their implementations OpensslSigner and OpensslVerifier are similar to those from corim-rs and internally uses the objects from corim-rs to perform these operations. Supports JWK and PEM encoded keys with openssl feature flag.