Repository navigation
feat!: update library as per draft-ietf-rats-corim-11 and cca-endorsements-04 specs - #21
shefali-kamal wants to merge 4 commits into
Conversation
setrofim
left a comment
There was a problem hiding this comment.
Also, please update the commit message to be compliant with conventions commits:
Update(library):are not valid type/scope; should be justfeat:(the scope is optional and can be omitted if the change is not local to a specific part).- Commit title should not end with a period.
- Commits that break compatibility should have a
!following the type/scope (before the:), and should have aBREAKING CHANGE:footer explaining the nature of the change.
| } | ||
| let result = verifier.verify(&args.scheme, evidence.as_slice(), nonce.as_deref())?; | ||
|
|
||
| debug!("ACS: {}", serde_json::to_string(&result.acs)?); |
There was a problem hiding this comment.
My bad, deleted while removing other debugging statements. Will add it back.
| based on an identifier inside the evidence. This is scheme-specific. For CCA, the instance ID | ||
| is used. Evidence claims are then extracted as ECT (environment-claims tuple) records. | ||
| - The evidence ECTs are then matched to the relations in the corim store. This results in the | ||
| ACS (appraisal claims set) -- a vector of ECT records containing evidence claims and matched |
There was a problem hiding this comment.
This is correct. -- in ASCII indicates an em-dash (rather than a hypen), which is what is intended here.
| } | ||
| } | ||
| } | ||
| for kv in self.corims.iter_key() { |
There was a problem hiding this comment.
Q: why have we dropped the duplicate detection logic?
There was a problem hiding this comment.
Now we are using Key-relation (K-ECT) to store attestation verification keys. as per draft-ydb-rats-cca-endorsements-04, each key-attest-triple must contain only one attestation verification key, so each condition key-ect will contain exactly one key corresponding to the instance and impl id.
So that is the reason we do not need to have duplicate detection logic. If a given key-item in the corim-store matches the environment, then we take first element of array (len always 1)
There was a problem hiding this comment.
as per draft-ydb-rats-cca-endorsements-04, each key-attest-triple must contain only one attestation verification key, so each condition key-ect will contain exactly one key corresponding to the instance and impl id.
Where is this validated?
Also note that this is generic logic. You cannot assume anything CCA-specific here.
| .map(|t| t.as_i128() as u64) | ||
| .unwrap_or(0); | ||
|
|
||
| let not_after = validity.not_after.as_i128() as u64; |
There was a problem hiding this comment.
I'm quasi sure that this wraps on overflow rather than generating an error.
There was a problem hiding this comment.
Thanks for pointing out. I have updated the code.
| impl KeyStore for MemKeyStore { | ||
| fn add(&mut self, kid: &[u8], key: &[u8]) -> Result<()> { | ||
| debug!("adding kid {:x?}", kid); | ||
| debug!("Key kid : \"{}\"", str::from_utf8(kid).unwrap()); |
There was a problem hiding this comment.
unsure why do we need to change this in a panic-y way?
There was a problem hiding this comment.
Also, please merge this and the following line into a single debug!, as they're logging the same operation.
There was a problem hiding this comment.
Changed and updated into single statement.
| // TODO: | ||
| // Implement realm evidence profile check here once rust-ccatoken is updated. | ||
| // This does not have any impact on end result, since all the current supported profiles by | ||
| // ccaguest and one specificed in draft-ydb-rats-cca-endorsements-04, produce same Evidence object. |
There was a problem hiding this comment.
to increase visibility, maybe translate this in an issue.
| Ok(Ect::from(ect)) | ||
| } | ||
|
|
||
| fn realm_to_ect<'a>(realm: &Realm) -> Result<Ect<'a>, Error> { |
There was a problem hiding this comment.
Question: where is the authority set?
There was a problem hiding this comment.
My bad, it was missed. I have added now. Please review latest changes.
| false | ||
| } | ||
|
|
||
| fn supports_corim(&self, corim: &Corim<'_>) -> Result<bool> { |
There was a problem hiding this comment.
it looks like we can remove Result<>
| #[arg(name = "key", short, long, action = ArgAction::Append)] | ||
| keys: Vec<String>, | ||
|
|
||
| /// Public key of Verifier/user of library in PEM format. This key is used as authority of attest/identiy key |
There was a problem hiding this comment.
| /// Public key of Verifier/user of library in PEM format. This key is used as authority of attest/identiy key | |
| /// Public key of Verifier/user of library in PEM format. This key is used as authority of attest/identity key |
| corim_store.add(&parsed_corim)?; | ||
| corim_loaded = true; | ||
| } else { | ||
| info!( |
There was a problem hiding this comment.
This should be at least a warning as you're ignoring expressly provided input.
There was a problem hiding this comment.
Thanks for suggestion. Changed to warn
| corim_store.add(&parsed_corim)?; | ||
| corim_loaded = true; | ||
| } else { | ||
| info!( |
There was a problem hiding this comment.
Again, should be at least a warning
| impl KeyStore for MemKeyStore { | ||
| fn add(&mut self, kid: &[u8], key: &[u8]) -> Result<()> { | ||
| debug!("adding kid {:x?}", kid); | ||
| debug!("Key kid : \"{}\"", str::from_utf8(kid).unwrap()); |
There was a problem hiding this comment.
Also, please merge this and the following line into a single debug!, as they're logging the same operation.
… triple parsing, as they are not supported by the Arm CCA, only supported scheme as of now by Cover - Update README and lib.rs to remove obsolete CE Series triple parsing references - update dependencies and remove unsupported CE parsing - Fix Clippy redundant-reference warning with Clippy 0.1.97 - Update anyhow to 1.0.104 to address cargo-deny advisory warning Signed-off-by: Patel, Ajay Kumar <Ajaykumar.Patel@fujitsu.com>
…ments-04 spec - Refactor ECT structures by introducing CommonEct and renaming Ect to ElementEct. Ect now refers to enum representing all ect(s) - Add key-ect support and populate key relations from key triple records - Add support for inputting verfier-key and use it as authority for for key addition ect - For unsigned corims, input verifier-key is used as authority - Refactor evidence/reference-value measurement map conversion using From trait on ElementMap - Update CM-Type handling according to CoRIM spec revision 11. Supported only 3 types of cm-type now - Add profile compatibility checks for input evidence formats - Validate CoRIMs and reject expired or unsupported profiles CoRIMs - Added Unit tests for added functionality - Update test data, with CCA required fields, and tests to ensure the complete test suite passes - Apply Rust formatting and Clippy fixes and bump dependencies to latest versions Signed-off-by: Patel, Ajay Kumar <Ajaykumar.Patel@fujitsu.com>
- Updated `ccatoken` dependency to a newer revision - Upgraded `jsonwebtoken` to version 11.1.0 and refactored `authority.rs` to handle unknown algorithms and elliptic curves more gracefully - Modified `cca/mod.rs` to accommodate changes in platform and realm claims structures - Introduced new test data for CCA claims and tokens, replacing outdated files - Added a script to rebuild CCA tokens using the new claims structure - updated test cases to use updated ccatoken - improve logging and error handling - update attestation keys and its path BREAKING CHANGE: Legacy ccatoken with realm profile as empty string not supported. Realm token to ect tranformation now checks for realm profile explicitly. Signed-off-by: Patel, Ajay Kumar <Ajaykumar.Patel@fujitsu.com>
|
|
||
| for name in "${corims[@]}"; do | ||
| echo "Rebuilding signed-corim-${name}.cbor..." | ||
| # echo "compile corim-${name}.json -o signed-corim-${name}.cbor --kid key.pub.pem --key key.priv.pem -f" |
There was a problem hiding this comment.
Remove commented out code.
| } | ||
| } | ||
| } | ||
| for kv in self.corims.iter_key() { |
There was a problem hiding this comment.
as per draft-ydb-rats-cca-endorsements-04, each key-attest-triple must contain only one attestation verification key, so each condition key-ect will contain exactly one key corresponding to the instance and impl id.
Where is this validated?
Also note that this is generic logic. You cannot assume anything CCA-specific here.
| if supported { | ||
| self.corims.add(&corim) | ||
| } else { | ||
| Ok(()) |
There was a problem hiding this comment.
This should be an error -- you should not just silently ignore unrecognised inputs.
There was a problem hiding this comment.
Also, this whole check should be moved into add_corim() above, and just call that here after parsing the bytes.
| return true; | ||
| } | ||
| } | ||
| info!("Unsupported profile \"{}\" ", corim_profile); |
There was a problem hiding this comment.
This should be a debug! -- it should be up to the caller how to handler unsupported profiles, e.g. report it to the user, return an error, etc.
| ); | ||
| appraisal.update_status_from_trust_vector(); | ||
|
|
||
| appraisal.policy_claims = |
There was a problem hiding this comment.
Why is this being removed? In the updated EAR this coresponds to verifier_claims.
| jwk::KeyAlgorithm::UNKNOWN_ALGORITHM => { | ||
| Err(Error::Custom(format!("Unknowm algorithm {}", alg))) | ||
| } | ||
| _ => todo!(), |
There was a problem hiding this comment.
Shouldn't leave todo!s on main. Return an "unsupported algorithm" error instead.
| jwk::EllipticCurve::P384 => CoseEllipticCurve::P384, | ||
| jwk::EllipticCurve::P521 => CoseEllipticCurve::P521, | ||
| jwk::EllipticCurve::Ed25519 => CoseEllipticCurve::Ed25519, | ||
| _ => todo!(), |
| Ok(result) | ||
| pub fn parse_corim<'a, 'b>( | ||
| corim: &Corim<'a>, | ||
| key: &[u8], |
There was a problem hiding this comment.
Maybe call this corim_key or something, so it's a bit clearer why this needs bot a key and a verifier_key.
| } | ||
| // Get cryptographic key for signed corim, | ||
| // for unsigned corims, use verifier's cryptographic key | ||
| let key: Vec<u8> = match corim.as_signed_ref() { |
There was a problem hiding this comment.
As with the param name below, call this corim_key or something else more descriptive than just key, as there is also a verifier_key in this context.
| fn from_key_triple_record<T>( | ||
| k: &T, | ||
| profile: &Option<ProfileTypeChoice>, | ||
| verifier: &[CryptoKeyTypeChoice], |
There was a problem hiding this comment.
Rename to verifier_authority.
…A scheme attestation logic - Updated ccatoken and ear dependencies to their latest main branch revisions - Improved error handling across the authority, corim, and verifier modules, specifically within JWK to crypto key conversions (jwk_ec_curve_to_cose) - Modified the CCA scheme implementation to accept a list of trust-anchors and properly handle single attestation key requirements - Restored ear_verifier_claims and added policy IDs to appraisals - Updated test data to match the latest EAR specification naming conventions and standardized the use of cca-token-03.cbor across all tests - Refactored variable names for better readability and reduced scheme module logging verbosity from info to debug Signed-off-by: Patel, Ajay Kumar <Ajaykumar.Patel@fujitsu.com>
|
Please could you update the |
These are the latest important changes you're missing: |
This PR introduced below changes/updates:
Remove CE and CES as per cca-endorsements-04 spec:
Update library as per draft-ietf-rats-corim-11:
Fromtrait onElementMap