verifier: Change logic to check the attestation report version #590
Conversation
|
Do you plan on updating |
AdithyaKrishnan
force-pushed
the
main
branch
2 times, most recently
from
f00f69d to
cee4b6a
Compare
Not in this PR but might have a new PR in the future to do the same. |
There was a problem hiding this comment.
LGTM. Thanks! @AdithyaKrishnan . One thing to think about the error report
deps/verifier/src/snp/mod.rs
Outdated
| @@ -37,6 +37,10 @@ const LOADER_SPL_OID: Oid<'static> = oid!(1.3.6 .1 .4 .1 .3704 .1 .3 .1); | |||
| const KDS_CERT_SITE: &str = "https://kdsintf.amd.com"; | |||
| const KDS_VCEK: &str = "/vcek/v1"; | |||
|
|
|||
| // Attestation report versions supported | |||
There was a problem hiding this comment.
The note here looks very good to be an inline document
| // Attestation report versions supported | |
| /// Attestation report versions supported |
There was a problem hiding this comment.
Fixed. Thanks Ding!
deps/verifier/src/snp/mod.rs
Outdated
| @@ -104,7 +108,8 @@ impl Verifier for Snp { | |||
|
|
|||
| verify_report_signature(&report, &cert_chain, &self.vendor_certs)?; | |||
|
|
|||
| if report.version != 2 { | |||
| // See Trustee Issue#589 https://github.com/confidential-containers/trustee/issues/589 | |||
| if report.version < REPORT_VERSION_MIN || report.version > REPORT_VERSION_MAX { | |||
| return Err(anyhow!("Unexpected report version")); | |||
There was a problem hiding this comment.
Could we also add some context including SNP to the error information here? Because the information
ERROR kbs::http::error] Attestation failed: Verifier evaluate failed: Unexpected report version
is somehow confusing as it could be KBS protocol's error.
There was a problem hiding this comment.
Added an extra line of error log. Thanks Ding!
Fixes Issue confidential-containers#589 Change the check condition to handle multiple attestation report versions. Signed-off-by: Adithya Krishnan Kannan <[email protected]>
AdithyaKrishnan
force-pushed
the
main
branch
from
cee4b6a to
3494ea7
Compare
Addressed all issues. Please merge PR if appropriate. |
Fixes Issue #589
Change the check condition to handle multiple attestation report versions.