-
Notifications
You must be signed in to change notification settings - Fork 42
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
Attestation API and integration with the PDO build #501
Attestation API and integration with the PDO build #501
Conversation
This PR is related to #498 |
This PR is also related to #500 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments after trying to build and getting the big picture:
-
doing the
Time to build: a one-liner in the common/crypto/attestation-api does the trick.
did fail when doing it directly as i was expected to have some dcap repo in/tmp'. That hurdle was crossed by doing the build in docker as in
common/crypto/attestation-api/README.md` but then failed later, see errors inline in that README.md -
building the overall docker (via
make -C docker test
) did work but i'm puzzled what was really was built from a pdo-global perspective. Logging (via docker) intopdo_services:0.3.21
i didn't really see anying in the places i would have expected (e.g.,/project/pdo/{run,tools}
nor was it obvious from looking at the CMakefile (deltas). I'm missing here a bit the big picture: do i just not see anything as this would eventually be all directly backed into the e/p-service binaries/static libraries and as this PR doesn't have any integration yet there is nothing to see? But wouldn't at least the conversion script have to be installed?
So putting it differently:
- what should and should not work right now and how am i supposed to test?
|
||
DCAP support with ITA is built by default, but requires the URL to the ITA CA root certificates (in JWK format). If the URL is not specified, the build will succeed but it won't be able to verify DCAP attestations. | ||
```bash | ||
export ITA_ROOT_CACERT_URL=<certificate url> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
where do i find that URL?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should get that when you subscribe to ITA
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should get that when you subscribe to ITA
How and where do i subscribe? I was expecting some corresponding links here ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here https://docs.trustauthority.intel.com/main/articles/howto-manage-subscriptions.html
I think the ITA part should probably stay as is since (1) ITA is not free, (2) this is under a hyperledger project.
Rather, we should focus on the DCAP-direct part, which is free and open.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here https://docs.trustauthority.intel.com/main/articles/howto-manage-subscriptions.html I think the ITA part should probably stay as is since (1) ITA is not free, (2) this is under a hyperledger project. Rather, we should focus on the DCAP-direct part, which is free and open.
Hmm, but as long as we include the code we still should document it and it's use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here https://docs.trustauthority.intel.com/main/articles/howto-manage-subscriptions.html I think the ITA part should probably stay as is since (1) ITA is not free, (2) this is under a hyperledger project. Rather, we should focus on the DCAP-direct part, which is free and open.
Hmm, but as long as we include the code we still should document it and it's use?
More importantly, above link seems for external folks only? Maybe i can log in as intel employee directly in ITA portal but that link to very carefully never have a link for the portal!!
common/crypto/attestation-api/test/verify_evidence_app_go/main.go
Outdated
Show resolved
Hide resolved
With this PR you just get the static libraries in build/cmake folder, together with all the other crypto artifacts.
Things that should work are: the build, the attestation generation (see how it is used in #502), the attestation API tests (see the related readme to set up the collaterals for testing, or to skip some tests). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not reviewing the details of the api implementation; just focused on the integration with PDO.
@@ -8,15 +8,15 @@ SET(JWT_PATCHED_FILEPATH ${CMAKE_CURRENT_SOURCE_DIR}/${JWT_PATCHED_FILENAME} PAR | |||
|
|||
ADD_CUSTOM_COMMAND( | |||
OUTPUT ${JWT_PATCHED_FILENAME} | |||
COMMAND [ ! -f ${JWT_PATCHED_FILENAME} ] && cd jwt-cpp && git apply ../jwt-cpp.patch && touch ../${JWT_PATCHED_FILENAME} || true | |||
COMMAND [ ! -f ${JWT_PATCHED_FILENAME} ] && cd jwt-cpp && patch -f -p1 < ../jwt-cpp.patch && touch ../${JWT_PATCHED_FILENAME} || true | |||
WORKING_DIRECTORY ${CMAKE_CURRENT_SOURCE_DIR} | |||
) | |||
|
|||
ADD_CUSTOM_TARGET(patch_jwt DEPENDS ${JWT_PATCHED_FILENAME}) | |||
|
|||
# TODO: devise better strategy for clean up; usually the build folder is removed so the clean is not called | |||
ADD_CUSTOM_TARGET(clean_patch_jwt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually what you did with the OUTPUT above should add the file to be cleaned automatically. have you tried "make -C build clean"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is tricky because we usually get rid of the build folder. I left a comment that a better strategy is needed for patching/unpatching
docker/pdo_services_base.dockerfile
Outdated
ARG SGXSSL=3.0_Rev1 | ||
ARG SGX=2.25 | ||
ARG OPENSSL=3.0.14 | ||
ARG SGXSSL=3.0_Rev4 | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you'll need to rebase to get rid of these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a merge pr waiting for you, Sir!
docker/pdo_services_base.dockerfile
Outdated
ENV DCAP_PRIMITIVES=/tmp/SGXDataCenterAttestationPrimitives | ||
|
||
RUN git clone https://github.com/intel/SGXDataCenterAttestationPrimitives.git ${DCAP_PRIMITIVES} \ | ||
&& cd ${DCAP_PRIMITIVES}/QuoteVerification \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not just have the clone pull the submodules for you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to clone and get the submodules for a specific tag?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, made my way now through the many files of fode and after the patch i can now run tests in oaa-properly for DCAP-direct and SIM (epid i assume is as-is from FPC where it was tested and ITA i don't have credentials (besides that apparently the service doesn't work at the moment.
Besides additional inline commes a few more notes on testing
- oaa docker test (finally) worked after patch
- tests occasionally but not deterministic reproducably failed but might be due to "underpowered" old FLC NUCs?
- make clean doesn't seem robust when switching SGX_MODE from SIM to HW; required wiping build dir completely to make work
- the test doc could be made more robust by using newer cmake patterns using
cmake -S. -B build
and thencmake --build build --target test
and alike instead ofmkdir build
,cd
,cmake',
make` as in principle also should work with other cmake generators, e.g., ninja (or put differently, your instruction will fail if somebody has the CMAKE_GENERATOR env var set.
- as code is not integrated in rest of PDO (other than build) i just (a) made sure that
make -C docker test
still works and (b) did a cursory look into the built pdo_services container to see whether the build of attestation-api was called (which it was)
* C prototype declarations for the ocalls | ||
* *******************************************************************************************************************/ | ||
|
||
#ifdef __cplusplus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These things would probably best handled in separate .h files? I'm also surprised that SDK doesn't have already these wrappers?
memcpy(p+sizeof(sgx_ql_qve_collateral_t)+c->pck_crl_issuer_chain_size+c->root_ca_crl_size+c->pck_crl_size, c->tcb_info_issuer_chain, c->tcb_info_issuer_chain_size); | ||
memcpy(p+sizeof(sgx_ql_qve_collateral_t)+c->pck_crl_issuer_chain_size+c->root_ca_crl_size+c->pck_crl_size+c->tcb_info_issuer_chain_size, c->tcb_info, c->tcb_info_size); | ||
memcpy(p+sizeof(sgx_ql_qve_collateral_t)+c->pck_crl_issuer_chain_size+c->root_ca_crl_size+c->pck_crl_size+c->tcb_info_issuer_chain_size+c->tcb_info_size, c->qe_identity_issuer_chain, c->qe_identity_issuer_chain_size); | ||
memcpy(p+sizeof(sgx_ql_qve_collateral_t)+c->pck_crl_issuer_chain_size+c->root_ca_crl_size+c->pck_crl_size+c->tcb_info_issuer_chain_size+c->tcb_info_size+c->qe_identity_issuer_chain_size, c->qe_identity, c->qe_identity_size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
uff, this p+a+b+c... construct looks very brittle (and tedious). I think a running pointer and an increment after each field would be much better ...
For the same brittleness reason i would also put the deserialization (now in verify_dcap_direct) together with the serialization in the same file (presumably as a new package in common
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, and unfortunately the dcap library does not provide that. Actually, when it returns the collateral as a uint8_t*
, that's an opaque pointer to the data structure, not to the serialized buffer :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, i'm confused why refactoring our (de)serialization code into a single place (separate package) and using the less brittle pattern size=sizof(nextfield); memcpy(moving_ptr, next_field, size); moving_ptr+=size;
has anything to do with what the dcap library provides? (of course, it would have been nice if dcap library would already have provided (de)serialization but that's besides my point?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the update addresses your comment
@@ -0,0 +1,240 @@ | |||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would name this by the attestation type rather than with ias in the name ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestion. I admit that a discussion on naming conventions can be helpful. For instance, although I made some decisions regarding DCAP (ITA/Direct), the attestation generation is one (and should have one type), while the attestation verification should have two types.
0 == attestation_type.compare(EPID_UNLINKABLE_TYPE_TAG)) | ||
{ | ||
ByteArray ba_evidence(evidence_field.begin(), evidence_field.end()); | ||
bool b = verify_ias_evidence(ba_evidence, ba_expected_statement, ba_expected_code_id); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that attestation_type is not passed to verify_ias_evidence: where/how do we verify that the evidence had the correct epid type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type check is only done at line 57. Later, either the evidence has the correct epid type, or it will fail the verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure i understand 100%. Do we not care in PDO what attestation type is used? Or put differentlly, the only thing we care is that there is some valid attestation, regardless whether it is linkable EPID, unlinkable EPID or DCAP? I thought previously it was crucial that we had linkable EPID? I would have expected that when verifying evidence we would pass an expected attestation type ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now I got your question.
There are two ways:
- as I show in the next PR (see signup.cpp line 141), we can force the contract enclave to generate (or to check that it is generating) a linkable EPID quote, which will be reflected in the MRENCLAVE. So, later, if you have the right mrenclave, the verification is correct.
- the second way is to check for the the "epidpseudonym" field in the report -- this is optional and only present for linkable signatures.
We could:
- either rely on (1),
- or add the epidpseudonym check at verification time,
- or simply leave this epidpseudonym check to the policy appraisal (which is not implemented here, and should also include the debug-disabled check among others).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, now I got your question. There are two ways:
- as I show in the next PR (see signup.cpp line 141), we can force the contract enclave to generate (or to check that it is generating) a linkable EPID quote, which will be reflected in the MRENCLAVE.
This is code outside of enclave: why would that result in different MRENCLAVE depending on attestation type?
So, later, if you have the right mrenclave, the verification is correct.
2. the second way is to check for the the "epidpseudonym" field in the report -- this is optional and only present for linkable signatures.We could:
- either rely on (1),
- or add the epidpseudonym check at verification time,
- or simply leave this epidpseudonym check to the policy appraisal (which is not implemented here, and should also include the debug-disabled check among others).
My comment was originally just starting from an API perspective and my expectation would have been that there would be an a boolean flag linkable
. But you bring up epid-pseudonym of course is a good point. but as pseudonym is apriori not known, my api proposal would have been to add the boolean flag as input and have an optional outparam which for linkable would return the pseudonym. That said, at this point with EPID not really worth changing (but then my designer head cannot resist and i was actually also looking at options for PQC-equivlants of EPID (or at least DAA). There actually would be some options and they would satisfy the IETF IAB but alas very unlikely that intel would ever pick that up unless NIST would ....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is code outside of enclave: why would that result in different MRENCLAVE depending on attestation type?
You're right that the code I linked is outside of the enclave.
However, the attestation generation (get_attestation) is inside the enclave.
So, what I meant is to have a check right there to enforce that the attestation being generated is of type epid-linkable
, as defined in the usage document.
but as pseudonym is apriori not known
Right, but the "field" is in the report and signed by IAS. So you just have to check that it is present in this case.
And, again, this can be one of the conditions in a policy appraisal -- which should be implemented anyway.
common/crypto/attestation-api/evidence/verify-dcap-direct-evidence.cpp
Outdated
Show resolved
Hide resolved
ByteArray certification_data; | ||
uint32_t certification_data_size; | ||
uint16_t certification_data_type; | ||
ByteArray collateral; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These vars seem to be used only in verify block, so why not declare them there? Makes it easier when reading code to see var definitions ...
bd6e0fe
to
12864aa
Compare
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
…d build (for later troubleshooting), (2) removes the dead Go code in the attestation lib, (3) it removes duplicate packages in the services dockerfile, (4) it add the reason for doing a double build of the DCAP primitives Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
…s not a standalone repo anymore -- thanks michael for spotting this Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
…l that from pdo docker build Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
12dcb77
to
c1cf654
Compare
Signed-off-by: Bruno Vavala <bruno.vavala@intel.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR adds the initial implementation of the attestation API (extended from Hyperledger Fabric Private Chaincode) and integrates it with the PDO build.
The attestation API is contained in
common/crypto/attestation-api
.Aside from the newly added files, this PR adds 4 modifications to PDO:
jwt-cpp
for handling jwt tokens and a json library)common/crypto/attestation-api
does the trick.