-
Notifications
You must be signed in to change notification settings - Fork 3
implementation of coserv data model #7
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
base: main
Are you sure you want to change the base?
Conversation
This commit adds the implementation of coserv data model, according to https://datatracker.ietf.org/doc/draft-ietf-rats-coserv/02/ Implementation includes unit tests for each module. The coserv testvectors from https://github.com/veraison/corim/ have also been incorporated here for testing. Note: The implementation uses data structures from corim-rs, which do not use fixed length encoding for maps everywhere. CoSERV object should be deterministically encoded, which requires maps and arrays to be fixed lenght. This causes some test cases to fail. This should be fixed by changing maps in corim-rs to fixed length (veraison/corim-rs#44). todo: signing and verification of CoSERV Signed-off-by: Dhanus M Lal <[email protected]>
paulhowardarm
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.
Many thanks for this PR - this is a huge and very welcome step forward. I really appreciate the work that has gone into this.
Overall the code looks as expected and mostly functions well. I haven't done a line-by-line review. I've been experimenting with this code alongside Veraison's CoSERV support. I've been digging into a few interop issues that this has flagged up. So far, I'm recommending one change to this PR (see my comment on the profile type). Other issues might be better addressed on the Golang/Veraison side, so I'm looking into that.
I'll try to offer more helpful review comments later, but I think it would be good to start with the profile tagging issue for now, which should be a minor change, but will help with end-to-end testing.
Apply change suggested by Paul. Changed coserv profile to untagged types. The profile can be untagged URI or untagged OID. Earlier definition used `ProfileTypeChoice' from corim-rs, which had tagged URI (32) and tagged OID (111) as variants. This commit implements `CoservProfile' enum in this crate with untagged variants. Signed-off-by: Dhanus M Lal <[email protected]>
paulhowardarm
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.
Comment on naming inconsistency - should we resolve this?
Cargo.toml
Outdated
| thiserror = "2.0.17" | ||
| corim-rs = { git = "https://github.com/veraison/corim-rs" } | ||
| chrono = "0.4.42" | ||
| cmw = { git = "https://github.com/veraison/rust-cmw" } |
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.
Please use the version published at https://crates.io/crates/cmw (0.1.0) rather than the git URL.
Signed-off-by: Dhanus M Lal <[email protected]>
Signed-off-by: Dhanus M Lal <[email protected]>
This PR adds the implementation of coserv data model, according to https://datatracker.ietf.org/doc/draft-ietf-rats-coserv/02/ Implementation includes unit tests for each module. The coserv testvectors from https://github.com/veraison/corim/ have also been incorporated here for testing. Fixes #2.
Note:
The implementation uses data structures from corim-rs, which do not use fixed length encoding for maps everywhere. CoSERV object should be deterministically encoded, which requires maps and arrays to be fixed length. This causes some test cases to fail. This should be fixed by changing maps in corim-rs to fixed length (veraison/corim-rs#44).
todo: signing and verification of CoSERV