Skip to content
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

Updating CCF PDO/TP Documentation with details about recent API changes used to set PDO contract enclave attestation policy. #475

Merged

Conversation

prakashngit
Copy link
Contributor

The API changes are part of PR 467 (https://github.com/hyperledger-labs/private-data-objects/pull/467/files#). This PR updates the subsection CCF TP TEE attestation verification policy contained within ledgers/ccf/README.md to reflect the new APIs introduced in PR 467.

@prakashngit prakashngit marked this pull request as ready for review March 4, 2024 22:17
ledgers/ccf/README.md Outdated Show resolved Hide resolved
ledgers/ccf/README.md Outdated Show resolved Hide resolved
@cmickeyb cmickeyb force-pushed the Prakash.PDOTP_Documentation_Update branch from 40ef5f2 to fee1eb1 Compare March 6, 2024 22:00
Copy link
Member

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @prakashngit ! I echo Bruno's and Mic's comments about separating the API description from the API usage, and adding details for newcomers. I will add more detailed comments in issue #264, but a separate question that we might want to address in this PR is: how does the intended user of this documentation actually interact with the TP? It sounds like its via the scripts, rather than the APIs directly. In that case, talking about the APIs first may not be the right level of abstraction for the user. I think it would be helpful to add a line or two describing the scripts first before you go on to talk about the APIs these scripts call.

gets to specify expected MREnclave, basename and IAS public key via the member-rpc.
- The policy (including expected value of MREnclave) can be changed anytime by the CCF Governance consortium, subject to voting rules of the consortium.
## CCF TP TEE attestation verification policy
CCF TP provides two APIs to be used by the CCF Governance consortium to register attestation verification policy that must be satisfied by PDO contract enclaves.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Two suggestions: 1) I would explain the TP acronym the first time it's used. 2) If you have a link to some documentation or website for the "CCF Governance consortium", that would be a helpful addition.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you @marcelamelara for the feedback. 1. TP full form has already been provided earlier in the same document. 2. I have added a link to the CCF Governance consortium document.


1. The first API `set_attestation_check_flag` is invoked as part of the TP start up scripts to specify whether PDO runs in SGX `HW` mode or SGX `SIM` mode. The flag can be set only once. There is no default value for the flag, and hence must be set explicitly before the TP can accept any `register_encalve` transactions.

2. The second API `set_expected_sgx_measurements` is used whenever the `set_attestation_check_flag` specifies that PDO runs in SGX `HW` mode. In this case, the second API is used to the specify expected `MREnclave` value, and additionally `basename` and the `ias_public_key`. Note that PDO currently supports SGX `HW mode` with EPID attestation. The expected SGX measurements can be updated via the second API, subject to voting rules of the consortium.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one more REQUIREMENT... line length 80.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cmickeyb thanks, pruned lines to length 80

used to set PDO contract enclave attestation policy. The API changes are
part of PR 467 (https://github.com/hyperledger-labs/private-data-objects/pull/467/files#)

This PR updates the subsection `CCF TP TEE attestation verification policy` contained
within ledgers/ccf/README.md to reflect the new APIs introduced in PR 467.

Signed-off-by: Prakash Narayana Moorthy <[email protected]>
Signed-off-by: Prakash Narayana Moorthy <[email protected]>
Copy link
Member

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates!

attestation verification, and while running in SGX HW mode, the eservice submits
IAS attestation report to the TP as part of contract enclave
registration with TP. To help the TP verify the IAS attestation report, the TP
must be programmed with expected `MREnclave`, enclave `basename` and `ias_public_key`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple points worth clarifying: Which of these expected values is unique per contract enclave? What is the actual policy, do these three values need to match for a given contract enclave? And what does the TP do if one/any of these values doesn't match the expectation?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for the questions. While these are important questions to document, these must be covered as part of PDO/documentation on "how PDO enables HW mode". The TP is one piece of the whole "PDO HW mode flow". A lot of these questions are coming up because we lack documentation on "how PDO supports EPID attestation". I suggest that an issue be created on this topic of the need for extra documentation. otherwise, these are out of scope for this PR.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these must be covered as part of PDO/documentation on "how PDO enables HW mode"

Not sure what you mean here. PDO does not enable HW mode. Rather, PDO uses SGX enclaves. The SIM mode is only for whoever wants to experiment on non-SGX platforms.

The TP is one piece of the whole "PDO HW mode flow"

I don't agree with this. The TP is an architectural component of PDO, and it's actually used in SIM mode too.

we lack documentation on "how PDO supports EPID attestation". I suggest that an issue be created on this topic of the need for extra documentation. otherwise, these are out of scope for this PR.

As I interpret Marcela's question, the focus is on the values and the policy. About the values, we would have mrenclave and manufacturer certificate even if we used DCAP, so only the basename turns out to have a specific meaning in EPID. Fortunately, we kept things simple, and this makes the answer simple -- I'll add that separately.

Copy link
Member

@marcelamelara marcelamelara Mar 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My questions are related to EPID attestation verification, but more fundamentally about assumptions-- assumptions about how the system will be used and assumptions about what the reader does and doesn't need to know. This document is quite long and I think the audience is still mostly us, as in, we are documenting this to record our design decisions etc. Does a newcomer user really need all of these details?

Many assumptions also aren't documented explicitly, making this document pretty difficult to digest at times. I don't think addressing this should be part of this PR, but it would be good for us to take a step back, put ourselves in the position of a newcomer, and create separate "quickstart" documentation that covers only the most fundamental information and keep this as a longer "deep dive" document.

Comment on lines 235 to 236
Further, the CCF TP governance consortium is permitted to change the
values of these parameters, subject to TP consoritum governance rules.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this desirable behavior? This sentence seems to be implying that the governance consortium can change the expected MRENCLAVE, base name, or IAS key for an existing registered enclave. Why wouldn't a change in these values result in a new enclave registration? That is, a new MRENCLAVE, for all intents and purposes, represents a distinct enclave. Otherwise, wouldn't a verifier with an outdated policy fail upon such changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you. same answer as above.

for PDO contract enclaves' attestation when eservices attempt registering
PDO enclaves with TP. The CCF TP governance consortium
(see https://microsoft.github.io/CCF/release/4.x/governance/index.html)
gets to set the flag after the TP is started. The flag can be set only once.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This flag is set only once per TP instance? This might be helpful to note.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you, yes, only once. again part of policy, must be covered as part of the documentation mentioned above.

ledgers/ccf/README.md Outdated Show resolved Hide resolved
ledgers/ccf/README.md Outdated Show resolved Hide resolved
Comment on lines +238 to +240
The TP provides two APIs `set_attestation_check_flag` and `set_expected_sgx_measurements`
to program the various values required to implement the above attestation
verification policy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This description is very good, and it says "to program the values".
Below, however, the description suggests that these APIs are already/only used in scripts so nobody should care about "programming" anything. So it is not clear who this description is for.

I think we have 3 levels of abstraction here:

  1. the APIs themselves as implemented in the TP.
  2. the convenience scripts for (just) calling these APIs from the client side (i.e., the python scripts)
  3. additional convenience scripts which embed these calls at the right time during a deployment -- namely when the the CCF network starts up and when the eservice starts up

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the documentation only talks about APIs provided by PDO TP that are useful to implement PDO's attestation policy. How these APIs are used by PDO clients is out of scope of this PR. you can either use the scripts that PDO provides, or write your own scripts.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can either use the scripts that PDO provides

Precisely, and the scripts are available, and they are even installed ready to be used.
So, how can they be out of scope (particularly (2) which is the TP client)?

ledgers/ccf/README.md Outdated Show resolved Hide resolved
ledgers/ccf/README.md Outdated Show resolved Hide resolved
Co-authored-by: Bruno Vavala <[email protected]>
Signed-off-by: prakashngit <[email protected]>
prakashngit and others added 2 commits March 12, 2024 10:38
Co-authored-by: Bruno Vavala <[email protected]>
Signed-off-by: prakashngit <[email protected]>
Co-authored-by: Bruno Vavala <[email protected]>
Signed-off-by: prakashngit <[email protected]>
Comment on lines +238 to +240
The TP provides two APIs `set_attestation_check_flag` and `set_expected_sgx_measurements`
to program the various values required to implement the above attestation
verification policy.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can either use the scripts that PDO provides

Precisely, and the scripts are available, and they are even installed ready to be used.
So, how can they be out of scope (particularly (2) which is the TP client)?

attestation verification, and while running in SGX HW mode, the eservice submits
IAS attestation report to the TP as part of contract enclave
registration with TP. To help the TP verify the IAS attestation report, the TP
must be programmed with expected `MREnclave`, enclave `basename` and `ias_public_key`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these must be covered as part of PDO/documentation on "how PDO enables HW mode"

Not sure what you mean here. PDO does not enable HW mode. Rather, PDO uses SGX enclaves. The SIM mode is only for whoever wants to experiment on non-SGX platforms.

The TP is one piece of the whole "PDO HW mode flow"

I don't agree with this. The TP is an architectural component of PDO, and it's actually used in SIM mode too.

we lack documentation on "how PDO supports EPID attestation". I suggest that an issue be created on this topic of the need for extra documentation. otherwise, these are out of scope for this PR.

As I interpret Marcela's question, the focus is on the values and the policy. About the values, we would have mrenclave and manufacturer certificate even if we used DCAP, so only the basename turns out to have a specific meaning in EPID. Fortunately, we kept things simple, and this makes the answer simple -- I'll add that separately.

ledgers/ccf/README.md Outdated Show resolved Hide resolved
Co-authored-by: Bruno Vavala <[email protected]>
Signed-off-by: prakashngit <[email protected]>
Copy link
Member

@marcelamelara marcelamelara left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates @prakashngit ! I'll accept the changes since I think we're good enough to close this out, but as I mentioned in a comment, it would be good for us to take a step back, put ourselves in the position of a new user, and create separate "quickstart" documentation that covers only the most fundamental information someone needs to get started, and keep this as a longer "deep dive" documentation.

@bvavala bvavala merged commit e91a785 into hyperledger-labs:main Mar 14, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants