search

veraison / services

Attestation verification services based on Veraison components
Apache License 2.0
26 stars 14 forks source link

fix(scheme): handle the case when no platform configuration RV is given #264

Closed thomas-fossati closed 3 months ago

thomas-fossati commented 3 months ago

Fix #263

yogeshbdeshpande commented 3 months ago

I thought the Platform Config reference value is mandatory? Isn't it?

yogeshbdeshpande commented 3 months ago

I thought the Platform Config reference value is mandatory? Isn't it?

This should be treated identical to the case of component measurements.

As this element is mandatory in the Evidence, we should mandate the setting of this, in the RV.

thomas-fossati commented 3 months ago

I thought the Platform Config reference value is mandatory? Isn't it?

This should be treated identical to the case of component measurements.

As this element is mandatory in the Evidence, we should mandate the setting of this, in the RV.

I am not sure, but we can discuss this in a separate issue.

thomas-fossati commented 3 months ago

I thought the Platform Config reference value is mandatory? Isn't it?

Currently the extractor simply ignores a missing config id: https://github.com/veraison/services/blob/4f5071848241b1370e21580f4befb359daca9853/scheme/common/cca/platform/cca_ssd_extractor.go#L20

If we want to make it mandatory (which I am not yet convinced about), we first need to check its presence on ingest. Enforcing it on verification is just too late.

Please, open a bug in veraison/corim, or on the CCA endorsements draft to discuss the design aspects.

yogeshbdeshpande commented 3 months ago

I thought the Platform Config reference value is mandatory? Isn't it?

Currently the extractor simply ignores a missing config id:

https://github.com/veraison/services/blob/4f5071848241b1370e21580f4befb359daca9853/scheme/common/cca/platform/cca_ssd_extractor.go#L20

If we want to make it mandatory (which I am not yet convinced about), we first need to check its presence on ingest. Enforcing it on verification is just too late.

Please, open a bug in veraison/corim, or on the CCA endorsements draft to discuss the design aspects.

I agree on your point that it is too late to check it, the CCA CoRIM Extractor is the right place to fix it rejecting the CoRIM which has missing this reference value. Then the existing code in services is the correct implementation which we can retain.

thomas-fossati commented 3 months ago

I agree on your point that it is too late to check it, the CCA CoRIM Extractor is the right place to fix it rejecting the CoRIM which has missing this reference value.

If we say that the config id is mandatory, which is undecided yet.

Then the existing code in services is the correct implementation which we can retain.

No, it's not.

There is a missing check there: the array is dereferenced without checking it contains at least one element.

The appraisal business logic must be robust in the face of unexpected input.

The code in this PR takes care of that.

yogeshbdeshpande commented 3 months ago

I agree on your point that it is too late to check it, the CCA CoRIM Extractor is the right place to fix it rejecting the CoRIM which has missing this reference value.

If we say that the config id is mandatory, which is undecided yet.

Then the existing code in services is the correct implementation which we can retain.

No, it's not.

There is a missing check there: the array is dereferenced without checking it contains at least one element.

The appraisal business logic must be robust in the face of unexpected input.

The code in this PR takes care of that.

ok, thanks, I will have a look in a while !

yogeshbdeshpande commented 3 months ago

ok make sense to unblock testing lets merge this and revisit the entire aspect!