Fix panic when querying an extended report without certs

[heavenboy8]

Related to #122 discussed with @tylerfanelli

Is is possible to review and merge please?

Thanks

[heavenboy8]

@larrydewey @tylerfanelli any changes required regarding on the comments from the original issue #122 ?

[tylerfanelli]

@heavenboy8 this looks fine to me. @larrydewey you had some concerns on #122, do you approve of this solution?

[DGonzalezVillal]

@tylerfanelli, @larrydewey and I believe that an Error was more appropriate that a None/empty return. We think that if you are using the extended attestation report you are expecting some certificates to be there and if there are none, then you should be somehow warned. If you just want the attestation report you could just use the get_report(). Unless you think this type of error handling should be handled on the tooling side and not in the back-end api. We are open to hear your thoughts,

[heavenboy8]

@DGonzalezVillal the strategy in my client code is:

• Ask for an extended report. Get the report and the certs
• If the certs are not provided (which is the case on GCP) , then query AMD endpoints to get them

If the extended report function returns an error, in that case, then, I need to ask for a simple report and then query the AMD endpoints. It's quite a pity to fetch the simple report since the previous call got it but just returned an error (without the report). It's a useless ioctl call then.

Another strategy could have been to first detect if the certs are provided. If so, ask for an extended report, otherwise ask for a simple report. But I guess we don't know that before asking for an extended report...

Tell me what you'd rather, I will make the changes. All solutions are easy to implement. One is suboptimal for me, the other one is may be cleaner on a rust langage perspective.

[tylerfanelli]

@tylerfanelli, @larrydewey and I believe that an Error was more appropriate that a None/empty return. We think that if you are using the extended attestation report you are expecting some certificates to be there and if there are none, then you should be somehow warned. If you just want the attestation report you could just use the get_report(). Unless you think this type of error handling should be handled on the tooling side and not in the back-end api. We are open to hear your thoughts,

I strongly disagree with this. For each of our APIs, they only fail if there is a problem with the underlying ioctl. Why should we diverge from this now with one API?

[tylerfanelli]

@DGonzalezVillal the strategy in my client code is:

• Ask for an extended report. Get the report and the certs
• If the certs are not provided (which is the case on GCP) , then query AMD endpoints to get them

If the extended report function returns an error, in that case, then, I need to ask for a simple report and then query the AMD endpoints. It's quite a pity to fetch the simple report since the previous call got it but just returned an error (without the report). It's a useless ioctl call then.

Exactly. The behavior itself is fine (and successful). Error handling should be left for the case in which there's an actual error.

[tylerfanelli]

We think that if you are using the extended attestation report you are expecting some certificates to be there and if there are none, then you should be somehow warned.

Isn't the None case warning enough? Your client code should see that the certs are None, and behave accordingly. What do we gain from an Error that None doesn't give us?

[DGonzalezVillal]

@tylerfanelli Our train of thought is that if you're calling an extended attestation report you are expecting to get back both the report and the certificates. If you're getting an empty cert buffer then the host machine is in the wrong state and the extended report should fail. The extended report ioctl might be successful, but that only means that your request was successful, not so much that the machines are in the right state. Then it also adds the question of why even have a regular and extended attestation report as 2 separate commands, if they're now both essentially doing the same thing?

I also understand your point of view that the API should focus on the lower-level communication and the errors we bubble are strictly firmware, then let the user handle what the library returns however they want. But you do see how it is a little confusing as a user? If I'm requesting an extended report expecting certificates, the command works successfully, but I get an empty cert buffer, we don't necessarily know if it's that the host has not cached the certs or if there is some other issue.

If you think this is the best way to handle empty certs, we can go with it. I just want to voice where we are coming from. If we merge this PR we will also have to bump release, since it does break API since we are now returning an option.

[tylerfanelli]

@tylerfanelli Our train of thought is that if you're calling an extended attestation report you are expecting to get back both the report and the certificates. If you're getting an empty cert buffer then the host machine is in the wrong state and the extended report should fail.

I don't feel that this assumption is correct. Take @heavenboy8's example that they mentioned above. They query if the certs are found locally; if not, they fetch from KDS.

The extended report ioctl might be successful, but that only means that your request was successful, not so much that the machines are in the right state.

I'm not too sure on this assumption either. You could be querying if the certs are there if a VM guest without prior knowledge of the system.

Then it also adds the question of why even have a regular and extended attestation report as 2 separate commands, if they're now both essentially doing the same thing?

I think that's a legitimate question, and if we would like to remove get_report, then we could do so. get_ext_report does have the same functionality.

I also understand your point of view that the API should focus on the lower-level communication and the errors we bubble are strictly firmware, then let the user handle what the library returns however they want. But you do see how it is a little confusing as a user? If I'm requesting an extended report expecting certificates, the command works successfully, but I get an empty cert buffer, we don't necessarily know if it's that the host has not cached the certs or if there is some other issue.

Right, but what if there actually are no certs cached? Then if we fail, we do not know if it's just because there are no certs or another issue.

Returning None would tell a user "The command was successful, and there are no certificates currently cached".

If you think this is the best way to handle empty certs, we can go with it. I just want to voice where we are coming from. If we merge this PR we will also have to bump release, since it does break API since we are now returning an option.

Understood. Maybe we should cut a release before this is merged. We could then continue our other features/fixes that will also break the API (gmem comes to mind).

[DGonzalezVillal]

@tylerfanelli Cool, I think then the None should work. Then we should handle the empty certs from tooling side. I also think leaving the regular get_report() should be fine, that way people don't have to handle certificates if they don't want to. I agree we should probably cut a release before merging this although.

[heavenboy8]

Do you plan to merge this PR soon? How can I help @tylerfanelli ?
Thanks again

[tylerfanelli]

@heavenboy8 Apologies for the delay.

@DGonzalezVillal @larrydewey Should we merge this and update/increment the major version?

[DGonzalezVillal]

I'm about to submit a PR for the ID-BLOCK calculation. Do you want to wait for that to be merged in too. Or we can merge this now and release the new version this week.

[tylerfanelli]

@heavenboy8 Merged. We are going to wait a bit for another release. Do you mind using the upstream version for the time being?

[heavenboy8]

@heavenboy8 Merged. We are going to wait a bit for another release. Do you mind using the upstream version for the time being?

Thanks. I will do that indeed