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.

Sign up for GitHub

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

Jump to bottom

Add support for x86_64-fortanix-unknown-sgx target #670

Merged
merged 1 commit into from
Jan 8, 2019
Merged

Add support for x86_64-fortanix-unknown-sgx target #670

merged 1 commit into from
Jan 8, 2019

Conversation

akash-fortanix
Copy link
Contributor

The x86_64-fortanix-unknown-sgx recently gained Tier 3 support upstream.

The introduced dependency 'rdrand' has 'rand_core' as dependency from crates.io. Any checkout of 'rand' has 'rand_core' as path dependency. For 'rdrand' to fetch dependency from the same source as 'rand', a patch for source crates.io for 'rand_core' dependency is required. This is achieved by the added patch section in Cargo.toml file.

@akash-fortanix
Copy link
Contributor Author

Travis build failures are not caused by this PR.

@dhardy
Copy link
Member

dhardy commented Dec 25, 2018

Not surprising considering we have no testing on SGX: is it possible to add CI testing?

The patch presumably fixes local testing; I guess it's irrelevant once deployed on crates.io.

Now, why are we depending on RDRAND to provide our randomness?

nagisa
nagisa reviewed Dec 25, 2018
View reviewed changes
src/rngs/os.rs Outdated Show resolved Hide resolved
@nagisa
Copy link
Contributor

nagisa commented Dec 25, 2018

NB: Not affiliated with fortanix and am only peripherally aware of sgx.

sgx is a sort of a enclave implementation within which interactions with the world outside the enclave are limited (in either direction). While the Intel’s SDK provides a function to obtain randomness (sgx_read_rand), from what I could gather fortanix is not relying on the libraries provided by Intel (which itself most likely uses the rdrand instruction behind the covers). These factors limit the sources of randomness obtainable from within the enclave greatly (probably to just rdrand and rdseed).

@akash-fortanix akash-fortanix mentioned this pull request Dec 26, 2018
Closed
@akash-fortanix
Copy link
Contributor Author

Not surprising considering we have no testing on SGX: is it possible to add CI testing?

No, not until following requirements are met:

  1. needs sgx-enabled hardware
  2. libtest crate is not yet supported on x86_64-fortanix-unknown-sgx
  3. x86_64-fortanix-unknown-sgx is a tier 3 target, so no binaries for std are distributed

The patch presumably fixes local testing; I guess it's irrelevant once deployed on crates.io.

Cargo respects patch section only from the root crate Cargo.toml, so yeah it's required for local testing but gets ignored while building downstream crates.

Now, why are we depending on RDRAND to provide our randomness?

RDRAND is the only source of randomness on this platform.

@jethrogb
Copy link

No, not until following requirements are met:

  1. needs sgx-enabled hardware
  2. libtest crate is not yet supported on x86_64-fortanix-unknown-sgx
  3. x86_64-fortanix-unknown-sgx is a tier 3 target, so no binaries for std are distributed

NB. Building the tests can be done easily once item no. 3 has been resolved. Fulfilling the other two requirements is needed to run the tests.

@dhardy
Copy link
Member

dhardy commented Dec 26, 2018

Okay, that answers my questions.

Note that this conflicts with #643 which I would like to merge first.

@akash-fortanix
Copy link
Contributor Author

Note that this conflicts with #643 which I would like to merge first.

Sure, i can revisit this once you've merged #643

@dhardy
Copy link
Member

dhardy commented Dec 29, 2018

You can "rebase" this on master now.

@akash-fortanix akash-fortanix force-pushed the sgx-target branch 2 times, most recently from 29d46da to 97c19ec Compare January 1, 2019 05:41
@akash-fortanix
Copy link
Contributor Author

You can "rebase" this on master now.

Done.

@akash-fortanix
Copy link
Contributor Author

Thank you. Do you have a release planned for sometime soon? I need releases for 0.4.x and 0.6.x

@dhardy
Copy link
Member

dhardy commented Jan 3, 2019

We will hopefully release 0.6.2 tomorrow.

This PR targets the master branch; if you wish to have support for 0.4 you will have to make a PR against the 0.4 branch. Since that one doesn't get updated much, you might as well update the version number and changelog at the same time; assume a release date of the following day.

@akash-fortanix akash-fortanix mentioned this pull request Jan 6, 2019
Merged
@jethrogb
Copy link

jethrogb commented Jan 8, 2019

@dhardy looks like you did release, but this PR was not included?

@dhardy dhardy merged commit afc9d9a into rust-random:master Jan 8, 2019
@dhardy
Copy link
Member

dhardy commented Jan 8, 2019

The introduced dependency 'rdrand' has 'rand_core' as dependency from crates.io. Any checkout of 'rand' has 'rand_core' as path dependency. For 'rdrand' to fetch dependency from the same source as 'rand', a patch for source crates.io for 'rand_core' dependency is required. This is achieved by the added patch section in Cargo.toml file.

This is actually a problem, because I need to remove this section to allow publishing (Cargo won't publish if there is a patch section), then use --allow-dirty to work-around git status not being clean. But I believe you are correct, it is needed with the current set-up. We deliberately chose to put rand_pcg into the same repo to avoid the same problem. However, there are only two other solutions, so we may just have to stick with it:

  1. Put rdrand in the same repository (but I doubt @nagisa would be happy about this and I don't think we'd want to anyway)
  2. Move rand-core out to a new repo (worse for management and docs)

@jethrogb
Copy link

jethrogb commented Jan 8, 2019

This is actually a problem, because I need to remove this section to allow publishing (Cargo won't publish if there is a patch section),

Hmm, I think this is a bug in Cargo/crates.io. Since the patch section has no effect when the crate is used as a dependency, I don't see a reason why it wouldn't be allowed when publishing.

@jethrogb jethrogb mentioned this pull request Jan 10, 2019
Merged
@jethrogb
Copy link

Filed rust-lang/cargo#6535

bors added a commit to rust-lang/cargo that referenced this pull request Feb 12, 2019
@bors
Auto merge of #6535 - jethrogb:patch-section-check, r=alexcrichton
8fbabe3 
Having a [patch] section when publishing is not an error

It is not necessary to throw an error when your `Cargo.toml` has a `[patch]` section.

1. Cargo will normalize your `Cargo.toml` while packaging, which will remove the `[patch]` section.
2. Even if the `[patch]` section were to somehow get to crates.io, it would be ignored when the crate is used since only the top-level crate's `[patch]` section is used.

The current behavior is quite annoying for crate maintainers who need a permanent `[patch]` section, for example, [`rand`](rust-random/rand#670 (comment)). This is the situation that leads to rand needing a permanent `[patch]` section:

![image](https://user-images.githubusercontent.com/1132307/50951760-54cf3a00-14d4-11e9-8064-a7def2ed402f.png)

Here, `rand` and `rand_core` are in the same repository, and `rdrand` is in a separate repository. When building `rand` as the top-level crate (e.g. in CI), the `rand->rand_core` path dependency will be used. This is of course a different crate than the crates.io rand_core used by `rdrand`. Therefore, when building `rand` as a top-level crate, you will *always* need to patch `crates-io.rand` to the path dependency. When building `rand` as a dependency, there are no issues, as the crates.io crates are used everywhere.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nagisa nagisa nagisa left review comments

@dhardy dhardy dhardy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@akash-fortanix @dhardy @nagisa @jethrogb