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

Opensource pcs and dcap_artifact_retrieval #619

Merged
merged 4 commits into from
Sep 16, 2024

Conversation

raoulstrackx
Copy link
Contributor

No description provided.

@raoulstrackx raoulstrackx marked this pull request as draft June 28, 2024 17:57
@Taowyoo
Copy link
Collaborator

Taowyoo commented Jun 28, 2024

Just a kind reminder that each source file should contain copyright notice at top.
You could use this script to add them: https://github.com/fortanix/rustls-mbedtls-provider/blob/master/add_copyright.py

intel-sgx/pcs/Cargo.toml Outdated Show resolved Hide resolved
intel-sgx/dcap_artifact_retrieval/Cargo.toml Outdated Show resolved Hide resolved
intel-sgx/pcs/src/qe_identity.rs Show resolved Hide resolved
test_helpers, IntelProvisioningClientBuilder, PcsVersion, ProvisioningClient
};

const IAS_TEST_KEY: &str = "<redacted>"; // Primary API key of [email protected]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pass API key as GitHub secret

@raoulstrackx raoulstrackx force-pushed the raoul/opensource_dcap_artifact_tools branch from 2a88905 to 64237b4 Compare July 11, 2024 08:55
@nshyrei
Copy link
Contributor

nshyrei commented Jul 29, 2024

@Taowyoo Added licence headers, can you please review it again please?

@nshyrei nshyrei requested a review from Taowyoo July 29, 2024 13:14
@nshyrei nshyrei marked this pull request as ready for review July 29, 2024 13:24
@Taowyoo
Copy link
Collaborator

Taowyoo commented Jul 29, 2024

Hi I think DCAP artifact retrieval tool should be public(with removing some private info) as a README.md file under dcap_artifact_retrieval crate.

Taowyoo
Taowyoo previously approved these changes Jul 29, 2024
@@ -53,6 +53,9 @@ jobs:
- name: cargo test -p async-usercalls --target x86_64-fortanix-unknown-sgx --no-run
run: cargo +nightly test --verbose --locked -p async-usercalls --target x86_64-fortanix-unknown-sgx --no-run

- name: Nightly test -p dcap-artifact-retrieval --target x86_64-fortanix-unknown-sgx --no-default-features --no-run
Copy link
Collaborator

Choose a reason for hiding this comment

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

If dcap-artifact-retrieval or pcs crates are only test-able on nightly or they need special features to be compiled and run correctly, you need to disable them in test Cargo test --all --exclude sgxs-loaders at line 51.

Copy link
Contributor

@nshyrei nshyrei Jul 30, 2024

Choose a reason for hiding this comment

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

I don't understand what has to be changed on line 51. Running the line gives me the following compilation error in 3rd party crate:

error: field must have #[serde(default)] because previous field 2 has #[serde(default)]
   --> /home/nikitashyrei/.cargo/registry/src/index.crates.io-6f17d22bba15001f/aws-nitro-enclaves-cose-0.1.0/src/sign.rs:129:5
    |
129 |     ByteBuf,
    |     ^^^^^^^

error: field must have #[serde(default)] because previous field 2 has #[serde(default)]
   --> /home/nikitashyrei/.cargo/registry/src/index.crates.io-6f17d22bba15001f/aws-nitro-enclaves-cose-0.1.0/src/sign.rs:131:5
    |
131 |     ByteBuf,
    |     ^^^^^^^

error[E0277]: the trait bound `SigStructure: sign::_::_serde::Serialize` is not satisfied
   --> /home/nikitashyrei/.cargo/registry/src/index.crates.io-6f17d22bba15001f/aws-nitro-enclaves-cose-0.1.0/src/sign.rs:167:28
    |
167 |         serde_cbor::to_vec(self)
    |         ------------------ ^^^^ the trait `sign::_::_serde::Serialize` is not implemented for `SigStructure`
    |         |
    |         required by a bound introduced by this call
    |
    = help: the following other types implement trait `sign::_::_serde::Serialize`:
              &'a T
              &'a mut T
              ()
              (T0, T1)
              (T0, T1, T2)
              (T0, T1, T2, T3)
              (T0, T1, T2, T3, T4)
              (T0, T1, T2, T3, T4, T5)
            and 136 others
note: required by a bound in `to_vec`
   --> /home/nikitashyrei/.cargo/registry/src/index.crates.io-6f17d22bba15001f/serde_cbor-0.11.2/src/ser.rs:22:8
    |
20  | pub fn to_vec<T>(value: &T) -> Result<Vec<u8>>
    |        ------ required by a bound in this function
21  | where
22  |     T: ser::Serialize,
    |        ^^^^^^^^^^^^^^ required by this bound in `to_vec`

For more information about this error, try `rustc --explain E0277`.
error: could not compile `aws-nitro-enclaves-cose` (lib) due to 3 previous errors


Copy link
Collaborator

Choose a reason for hiding this comment

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

Did you try just cargo test the pcs and dcap-artifact-retrieval locally?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this maybe a dependency issue.
Some old crates have version upgraded when adding new dependencies, the new versions are not compatible with some code or some other dependency.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I noticed the Cargo.lock there are now two version of serde. This maybe the reason

Copy link
Contributor

Choose a reason for hiding this comment

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

@Taowyoo Can you give me a link to a discrepancy you you think is the culprit here? I don't understand what the issue is.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, pcs and dcap-artifact-retrieval works locally. But new dependencies they introduced breaks the other crates in this workspace.

The error msg above means that the type definition of SigStructure in aws-nitro-enclaves-cose-0.1.0 is not compatible with newer serde.

There are some complex dependency issues here.

  • We have a lot of old dependencies which may be not compatible with even latest stable.
  • Those old dependencies may also be not compatible with new dependencies needed by pcs and dcap-artifact-retrieval.
  • One main reason is we have some patch/fork of some core crates: serde libc.

Copy link
Collaborator

@Taowyoo Taowyoo Aug 2, 2024

Choose a reason for hiding this comment

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

I tried to fix them by change version of some crates, but failed.
So I would suggest to put pcs and dcap-artifact-retrieval to separate GitHub repo instead.
There are too much tech debts about dependency in this repo.

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the CI of new repo.
You could use https://github.com/fortanix/rustls-mbedtls-provider/ as example.

Copy link
Contributor

Choose a reason for hiding this comment

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

Splitting the repos is a question to @raoulstrackx , I am not sure that this is an intended way to do it. @Taowyoo

intel-sgx/dcap-artifact-retrieval/Cargo.toml Outdated Show resolved Hide resolved
intel-sgx/pcs/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since PCS_API_KEY is been redacted, all tests use that will fail.
If you want to keep running these tests, please checkout https://docs.github.com/en/actions/security-guides/using-secrets-in-github-actions to add secret to this repo and load it from an environment variable.
You will also need to change CI yml file.

@Taowyoo Taowyoo self-requested a review July 29, 2024 19:24
Copy link
Collaborator

@Taowyoo Taowyoo left a comment

Choose a reason for hiding this comment

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

Please:

  • ensure the tests you added in the CI are executed and passed.
  • add README.md for dcap_artifact_retrieval

.github/workflows/build.yml Outdated Show resolved Hide resolved
@raoulstrackx raoulstrackx force-pushed the raoul/opensource_dcap_artifact_tools branch 4 times, most recently from 538a834 to 7e765ef Compare September 11, 2024 09:27
@raoulstrackx raoulstrackx force-pushed the raoul/opensource_dcap_artifact_tools branch from 7e765ef to b2fa24d Compare September 11, 2024 12:53
@raoulstrackx raoulstrackx force-pushed the raoul/opensource_dcap_artifact_tools branch from b137928 to 97ee9b2 Compare September 12, 2024 16:00
@raoulstrackx raoulstrackx force-pushed the raoul/opensource_dcap_artifact_tools branch from ffb9595 to 6192cba Compare September 12, 2024 16:32
@nshyrei
Copy link
Contributor

nshyrei commented Sep 16, 2024

@Taowyoo Please review.

@Taowyoo Taowyoo added this pull request to the merge queue Sep 16, 2024
Merged via the queue into master with commit 3367ada Sep 16, 2024
1 check passed
@Taowyoo Taowyoo deleted the raoul/opensource_dcap_artifact_tools branch September 16, 2024 18:45
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.

3 participants