Adds rng support for Fuchsia #634

benbrittain · 2018-03-05T22:34:42Z

We're considering using ring as the basis of our Rust crypto for Fuchsia. The std::rand crate already has Fuchsia OS specific support and it would be great if we could get it in ring as well.

I agree to license my contributions to each file under the terms given
at the top of each file I changed.

benbrittain · 2018-03-20T22:37:23Z

Is there anything I can do to help move this forward?

andrewtj · 2018-03-20T23:34:12Z

@cavedweller I think this comment summarises the projects current priorities.

briansmith · 2018-03-21T01:30:06Z

Thanks for the PR. I'm not excited about the new crate dependency. In my past experience with crates that have depended on fuchsia-zircon, we end up having to download the fuchsia-zircon crate even if we won't ever build for Fuchsia. Not only that, but that means we'll add bitflags and fuchsia-zircon-sys dependencies, at least, leading to the same issue.

Since we already have direct native code dependencies in ring, it seems better to me to just directlyu call the underlying function that fuchsia_zircon::cprng_draw calls, and avoid the fuchsia_zircon crate dependency completely.

benbrittain · 2018-03-21T03:27:48Z

I totally understand not wanting crate dependencies. However, I don't foresee any Fuchsia specific changes necessary beyond this, and I'm fairly certain you'll never need to download fuchsia_zircon with this change unless you are targeting Fuchsia.

You'd also need to link against the zircon library if you wanted to bring this fn definition in-tree.
cprng.rs
sys/definitions.rs
cprng_draw doc

Upsides of leaving it a crate, we might wind up changing this syscall a little bit and it's definitely easier to just bump the version.

If you definitely want the dependency moved from fuchsia_zircon to zircon I'm happy to make another commit, but that sounds like it's own set of headaches.

briansmith · 2018-03-21T03:44:53Z

and I'm fairly certain you'll never need to download fuchsia_zircon with this change unless you are targeting Fuchsia.

In another project, when I do the docker build that targets x86-64, this is what I see:

$ cargo fetch --locked
 [...]
 Downloading fuchsia-zircon-sys v0.3.3
 [...]
 Downloading fuchsia-zircon v0.3.3
 [...]
 Downloading bitflags v1.0.1
 [...]

Since cargo fetch doesn't know what platform you are going to target, it has to download all the crates even if they aren't used.

Note that ring only depends on two crates right now: libc and untrusted, neither of which has dependencies. So adding support for calling this one function on one little-used platform (albeit one I am truly excited about) by more than doubling the crate dependencies doesn't seem worth it to me, without seeing a specific issue that makes it difficult to avoid the Rust wrapper. If there is something difficult about avoiding the Rust wrapper then I'd reconsider.

briansmith · 2018-03-21T04:41:25Z

and I'm fairly certain you'll never need to download fuchsia_zircon with this change unless you are targeting Fuchsia.

In another project, when I do the docker build that targets x86-64, this is what I see:
$ cargo fetch --locked
[...]
Downloading fuchsia-zircon-sys v0.3.3
[...]
Downloading fuchsia-zircon v0.3.3
[...]
Downloading bitflags v1.0.1
[...]

OK, I see the problem is a limitation of cargo fetch. If I do cargo build without an explicit cargo fetch then these crates are not downloaded! I filed rust-lang/cargo#5216 to add a --target flag to cargo fetch and I guess I'm rewriting all my Rust Dockerfiles!

So, I don't think the additional optional crate dependencies are are problem any more. I will review the PR more carefully soon.

benbrittain · 2018-04-30T16:03:50Z

cargo can now fetch targets!
rust-lang/cargo#5349

Are there any more blockers I can help with?

briansmith

Are there any more blockers I can help with?

I got bogged down trying to set up a Fuchsia build/test environment. Could you please document:

What's the minimum build environment and configuration of Fuchsia/Zircon that one can use to build ring and run its test suite?
Can you help add Fuchsia/Zircon to ring's CI? For now it would be convenient if we could run the build & tests on Travis CI however I would be happy with doing it in Google Cloud Container Builder or perhaps some other thing (I'm looking to containerize all of ring CI in the near future.)

briansmith · 2018-04-05T19:49:01Z

src/rand.rs

+    use error;
+
+    pub fn fill(dest: &mut [u8]) -> Result<(), error::Unspecified> {
+        for s in dest.chunks_mut(fuchsia_zircon::sys::ZX_CPRNG_DRAW_MAX_LEN) {


Let's simplify this code by removing this outer loop and avoid using chunks_mut().

We can't yet remove the outer loop. The current syscall guarantees on fuchsia for CPRNG_DRAW don't guarantee a full read, because you might be kicked out of the kernel space.

We have an active issue addressing this, and it will be changed.

We can't yet remove the outer loop. The current syscall guarantees on fuchsia for CPRNG_DRAW don't guarantee a full read, because you might be kicked out of the kernel space.

Isn't that handled by the inner loop? I agree we need to keep the inner loop, but the outer loop seems unnecessary. In particular, consider this case:

The buffer to fill is 4/3 * ZX_CPRNG_DRAW_MAX_LEN bytes long.

Each call to fuchsia_zircon::cprng_draw gives us 2/3 of ZX_CPRNG_DRAW_MAX_LEN bytes.

In such a scenerio, without the outer loop, 2 calls to fuchsia_zircon::cprng_draw are needed. But, with the outer loop, 3 calls to fuchsia_zircon::cprng_draw are needed.

briansmith · 2018-05-01T19:33:10Z

In your commit message, please indicate who is making the contribution (e.g. "Contributed on behalf of Google, Inc."). Also please add the note that appears here to the commit message: https://github.com/briansmith/ring#contributing.

briansmith · 2018-07-23T17:16:20Z

I'm going to close this PR because, looking at the recent BoringSSL PRs, this is out-of-date w.r.t. the current Fuchsia API.

If anybody has a solution for adding Fuchsia to CI (Travis CI or otherwise), I'd love to see it.

This is another attempt for ring to support fuchsia, by directly calling `zx_cprng_draw` to generate random byte strings. This avoids having to pull in an extra dependency (which briansmith#634 did). With this change, all the ring tests pass on fuchsia. Closes briansmith#428

This is another attempt for ring to support fuchsia, by directly calling `zx_cprng_draw` to generate random byte strings. This avoids having to pull in an extra dependency (which briansmith#634 did). With this change, all the ring tests pass on fuchsia. I agree to license my contributions to each file under the terms given at the top of each file I changed. Closes briansmith#428

This is an alternative implementation to briansmith#634 that doesn't require pulling in the `fuchsia-zircon` crate. Also, another problem that we had in briansmith#634 is that zx_cprng_draw required a loop, that is no longer necessary in the latest fuchsia version.

This is another attempt for ring to support fuchsia, by directly calling `zx_cprng_draw` to generate random byte strings. This avoids having to pull in an extra dependency (which briansmith#634 did). With this change, all the ring tests pass on fuchsia. I agree to license my contributions to each file under the terms given at the top of each file I changed. Closes briansmith#428

This is another attempt for ring to support fuchsia, by directly calling `zx_cprng_draw` to generate random byte strings. This avoids having to pull in an extra dependency (which #634 did). With this change, all the ring tests pass on fuchsia. I agree to license my contributions to each file under the terms given at the top of each file I changed. Closes #428

Adds rng support for Fuchsia

7568558

briansmith reviewed May 1, 2018

View reviewed changes

briansmith closed this Jul 23, 2018

erickt mentioned this pull request Jan 18, 2019

Add support for fuchsia's CPRNG source #765

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds rng support for Fuchsia #634

Adds rng support for Fuchsia #634

benbrittain commented Mar 5, 2018 •

edited

Loading

benbrittain commented Mar 20, 2018

andrewtj commented Mar 20, 2018

briansmith commented Mar 21, 2018

benbrittain commented Mar 21, 2018

briansmith commented Mar 21, 2018

briansmith commented Mar 21, 2018 •

edited

Loading

benbrittain commented Apr 30, 2018

briansmith left a comment

briansmith Apr 5, 2018

benbrittain May 1, 2018

benbrittain May 1, 2018

briansmith May 1, 2018

briansmith commented May 1, 2018

briansmith commented Jul 23, 2018

Adds rng support for Fuchsia #634

Adds rng support for Fuchsia #634

Conversation

benbrittain commented Mar 5, 2018 • edited Loading

benbrittain commented Mar 20, 2018

andrewtj commented Mar 20, 2018

briansmith commented Mar 21, 2018

benbrittain commented Mar 21, 2018

briansmith commented Mar 21, 2018

briansmith commented Mar 21, 2018 • edited Loading

benbrittain commented Apr 30, 2018

briansmith left a comment

Choose a reason for hiding this comment

briansmith Apr 5, 2018

Choose a reason for hiding this comment

benbrittain May 1, 2018

Choose a reason for hiding this comment

benbrittain May 1, 2018

Choose a reason for hiding this comment

briansmith May 1, 2018

Choose a reason for hiding this comment

briansmith commented May 1, 2018

briansmith commented Jul 23, 2018

benbrittain commented Mar 5, 2018 •

edited

Loading

briansmith commented Mar 21, 2018 •

edited

Loading