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

aa/attester: add IBM Secure Execution driver framework #492

Merged
merged 3 commits into from
Jun 11, 2024

Conversation

huoqifeng
Copy link

@huoqifeng huoqifeng commented Feb 29, 2024

This is the initial draft to fix: #485 and the draft PR provides the API framework without a real pvattest crate yet.

Try it after adding the new Tee type Tee::Se in https://github.com/virtee/kbs-types/blob/main/src/lib.rs#L24, run:

cd guest-components
TEE_PLATFORM=se KBC=cc_kbc make

Depends on:

@Xynnn007
Copy link
Member

Xynnn007 commented Mar 1, 2024

Thanks @huoqifeng for the contribution! I'd love to review this if it is ok : D

@huoqifeng huoqifeng force-pushed the s390x-attester branch 4 times, most recently from c6a4fad to f82d20b Compare March 6, 2024 00:09
@huoqifeng huoqifeng changed the title aa/attester: add s390x z16 evidence driver framework aa/attester: add IBM Secure Execution (SE) evidence driver framework Mar 6, 2024
@huoqifeng
Copy link
Author

huoqifeng commented Mar 6, 2024

@Xynnn007 this is not fully ready for review, but I think we can start discuss the potential changes both on guest-components side and on trustee side, trustee change is tracked by confidential-containers/trustee#345.

We want to move the challenge generation from session to attestation-service and introduce a new fn generate_challenge. like in https://github.com/huoqifeng/trustee/blob/s390x-verifier/attestation-service/attestation-service/src/lib.rs#L243, we want use this challenge.extra_params in guest like https://github.com/huoqifeng/guest-components/blob/s390x-attester/attestation-agent/kbs_protocol/src/client/rcar_client.rs#L140, a sub-field in extra_params also works for us.

The reason is: for IBM Secure Execution (SE), attester need an attestation-request to generate the evidence, while the attestation-request should always be created on verifier. CC @BbolroC @steffen-eiden

@huoqifeng huoqifeng changed the title aa/attester: add IBM Secure Execution (SE) evidence driver framework aa/attester: add IBM Secure Execution evidence driver framework Mar 6, 2024
@huoqifeng huoqifeng changed the title aa/attester: add IBM Secure Execution evidence driver framework [WIP] aa/attester: add IBM Secure Execution evidence driver framework Mar 6, 2024
@huoqifeng huoqifeng force-pushed the s390x-attester branch 11 times, most recently from 8cc2716 to 718c63c Compare March 11, 2024 08:44
@huoqifeng
Copy link
Author

@Xynnn007 @fitzthum I'd like start the review process of the API change to support IBM SE. May you please help on that?

@huoqifeng huoqifeng changed the title [WIP] aa/attester: add IBM Secure Execution evidence driver framework aa/attester: add IBM Secure Execution evidence driver framework Mar 15, 2024
@huoqifeng huoqifeng marked this pull request as ready for review March 21, 2024 01:00
@huoqifeng huoqifeng changed the title aa/attester: add IBM Secure Execution evidence driver framework aa/attester: add IBM Secure Execution driver framework Mar 21, 2024
@huoqifeng
Copy link
Author

@mkulke @Xynnn007 comments addressed, thanks for all of your review! I'm curious why the CI is not health with no failure task?

@mkulke
Copy link
Contributor

mkulke commented May 30, 2024

@mkulke @Xynnn007 comments addressed, thanks for all of your review! I'm curious why the CI is not health with no failure task?

some workflows have been cancelled, seemingly for no reason. I've noticed this problem elsewhere this week, I'll re-schedule the cancelled ones, that worked so far.

Copy link
Contributor

@mkulke mkulke left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Some more nits..

attestation-agent/attester/Cargo.toml Show resolved Hide resolved
attestation-agent/attester/src/se/ibmse.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/se/ibmse.rs Outdated Show resolved Hide resolved
BbolroC added a commit to BbolroC/trustee that referenced this pull request Jun 3, 2024
We are encountering an issue on s390x when compiling AS with
`all-verifier`. The error message is as follows:

```
error: failed to run custom build command for `tss-esapi-sys v0.5.0`
```

A platform-specific verifier (e.g., `se-verifier`) is used here.
(confidential-containers/guest-components#492)

Although we can easily configure the verifier using `--features`,
this approach lacks flexibility when the crate is selectively called
from outside (e.g., kbs) based on `target_arch`.

The optimal solution would be to open up room for configuring the
verifier at a `dependencies` level rather than a `features` level.
This commit aims to remove `all-verifier` from the default feature
set and configure it differently for s390x.

Signed-off-by: Hyounggyu Choi <Hyounggyu.Choi@ibm.com>
@huoqifeng huoqifeng force-pushed the s390x-attester branch 2 times, most recently from c7b4d93 to ebfbab8 Compare June 3, 2024 22:56
@huoqifeng
Copy link
Author

@huoqifeng
Copy link
Author

@mkulke @Xynnn007 thank you very much for reviewing the PR and gave so many useful suggestions! thanks for your approval.

* aa/attester: add IBM Secure Execution evidence driver framework

Signed-off-by: Qi Feng Huo <huoqif@cn.ibm.com>

* aa/attester: IBM SE use nonce to pass attestation request

Signed-off-by: Qi Feng Huo <huoqif@cn.ibm.com>

---------

Signed-off-by: Qi Feng Huo <huoqif@cn.ibm.com>
@huoqifeng
Copy link
Author

Squashed commits.

Copy link
Member

@Xynnn007 Xynnn007 left a comment

Choose a reason for hiding this comment

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

Looks good. Thanks @huoqifeng .

attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
Signed-off-by: Qi Feng Huo <huoqif@cn.ibm.com>
Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

Looks pretty good. A couple minor comments.

attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
Copy link

@steffen-eiden steffen-eiden left a comment

Choose a reason for hiding this comment

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

s390 architecture stuff looks good.
You still have some unnecessary clones.

attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
attestation-agent/attester/src/se/mod.rs Outdated Show resolved Hide resolved
@mkulke
Copy link
Contributor

mkulke commented Jun 7, 2024

@huoqifeng I noticed the CI will not alert about code that doesn't compile in the se-attester module. I suppose due to the target-toggle. Do you plan to add CI for s390? If not, maybe we should revisit that toggle, otherwise the code will break unnoticed w/ future refactorings.

Signed-off-by: Qi Feng Huo <huoqif@cn.ibm.com>
@huoqifeng
Copy link
Author

huoqifeng commented Jun 7, 2024

@huoqifeng I noticed the CI will not alert about code that doesn't compile in the se-attester module. I suppose due to the target-toggle. Do you plan to add CI for s390? If not, maybe we should revisit that toggle, otherwise the code will break unnoticed w/ future refactorings.

I know @BbolroC is working on this, maybe, we bring back the change in lib.rs to enable CI for se? I just noticed it also.

Copy link
Member

@fitzthum fitzthum left a comment

Choose a reason for hiding this comment

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

LGTM

Some kind of test coverage would be good. Maybe that is a follow-on?

BbolroC added a commit to BbolroC/cc-guest-components that referenced this pull request Jun 10, 2024
This commit make the existing build/test for attestation agent running
on s390x. We will enable `cargo test` after an image for kbs is ready.
(confidential-containers/trustee#383)

The build option for attestor is not configured here, but a new attester
for s390x will be added by confidential-containers#492.
BbolroC added a commit to BbolroC/cc-guest-components that referenced this pull request Jun 10, 2024
This commit make the existing build/test for attestation agent running
on s390x. We will enable `cargo test` after an image for kbs is ready.
(confidential-containers/trustee#383)

The build option for attestor is not configured here, but a new attester
for s390x will be added by confidential-containers#492.

Signed-off-by: Hyounggyu Choi <Hyounggyu.Choi@ibm.com>
@BbolroC
Copy link
Member

BbolroC commented Jun 10, 2024

For reviewers, you can find CI enablement for s390x at #576 . Thanks! 😉

@Xynnn007 Xynnn007 merged commit c543f20 into confidential-containers:main Jun 11, 2024
15 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.

AA | Add IBM Secure Execution driver for attester
6 participants