Opened 8 months ago

Closed 4 months ago

#24660 closed enhancement (implemented)

Wrap our PRNG interface(s) in Rust with appropriate traits

Reported by: isis Owned by: isis
Priority: Low Milestone: Tor: 0.3.4.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: rust, tor-crypto, rng, roadmap, 034-roadmap-master, 034-triage-20180328, 034-included-20180328
Cc: chelseakomlo, catalyst, isis, nickm, teor Actual Points:
Parent ID: Points: 2
Reviewer: nickm Sponsor: SponsorQ

Description

Similar to #24659, we should provide a way to wrap our C code for getting randomness in Rust, while implementing the appropriate traits (from rand) so that we're able to switch in Rust implementations if we want later.

This is also blocking #23886.

Child Tickets

Change History (21)

comment:1 Changed 8 months ago by isis

Sponsor: Sponsor3-can

(Not sure if this is more Sponsor8 or Sponsor3 or something else.)

comment:2 Changed 8 months ago by chelseakomlo

Cc: chelseakomlo added

comment:3 Changed 8 months ago by budabudimir

I'd like to solve this one, as my first contribution.

comment:4 Changed 8 months ago by nickm

Cool! Please ask if you have any questions. There is some possibly helpful documentation in the doc/HACKING subdirectory of the Tor repository, and there are frequently helpful people on the #tor-dev IRC channel on OFTC.

comment:5 Changed 7 months ago by nickm

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

Deferring various "new"-status enhancement tickets to 0.3.4

comment:6 Changed 5 months ago by isis

Marked #25508 as a potential duplicate of this ticket.

comment:7 Changed 5 months ago by isis

Cc: catalyst isis nickm teor added
Keywords: rng roadmap added
Sponsor: Sponsor3-canSponsorQ

Closed #25508 as a duplicate and transferred sponsor/tags/cc here.

comment:8 Changed 5 months ago by isis

Owner: set to isis
Points: 2
Status: newaccepted

So it looks like the correct traits to implement are those in rand::rand_core (documentation) and then we get the automatic implementation of rand::Rng for free.

Assigning to myself since this is blocking #25381, #23886, and #24988, possibly among others, it's in the March/April 2018 roadmap, and I can probably do it in an afternoon?

comment:9 Changed 5 months ago by nickm

Keywords: 034-roadmap-master added

comment:10 Changed 5 months ago by nickm

Keywords: 034-triage-20180328 added

comment:11 Changed 5 months ago by nickm

Keywords: 034-included-20180328 added

comment:12 Changed 4 months ago by isis

I've made an upstream issue asking about the release/stabilisation of the rand_core crate: https://github.com/rust-lang-nursery/rand/issues/386

comment:13 Changed 4 months ago by isis

Status: acceptedneeds_review

There's an implementation in my bug24660 branch (Travis mostly passes, just not the cargo offline mode). The added dependency for rand_core is in the master branch of my tor-rust-dependencies repo, but from https://github.com/rust-lang-nursery/rand/issues/386#issuecomment-380489759 it looks like we should wait a few days to vendor rand_core-0.1.0.

comment:14 Changed 4 months ago by asn

Reviewer: nickm

comment:15 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

I've added some comments and questions to the branch on github. Let's discuss more there. Thanks, Isis!

comment:16 in reply to:  15 Changed 4 months ago by isis

Replying to nickm:

I've added some comments and questions to the branch on github. Let's discuss more there. Thanks, Isis!


Replied to the comments on https://github.com/isislovecruft/tor/commit/5a984badc59927063d9682ffd8cc6f52fc15a44b and made a bunch of fixup commits (Travis fails because we need to pull in the appropriate tor-rust-dependencies changes). A squashed version is in my bug24660_r1 branch.

The requisite tor-rust-dependencies changes are in my add/rand_core branch and add/rand branch.

comment:17 Changed 4 months ago by isis

Status: needs_revisionneeds_review

comment:18 Changed 4 months ago by nickm

Status: needs_reviewneeds_revision

Lgtm, except that I instead of exposing only crypto_strongest_rand() as an Rng, I think we should either expose only crypto_rand(), or expose both. Once that's done I think this is merge_ready.

Edited to add: see comment on the pull request for more info.

Last edited 4 months ago by nickm (previous) (diff)

comment:19 Changed 4 months ago by teor

Priority: MediumLow

This feature is low-priority for 0.3.4, because merging PrivCount in Tor has been delayed until 0.3.5.

comment:20 in reply to:  18 Changed 4 months ago by isis

Status: needs_revisionmerge_ready

Replying to nickm:

Lgtm, except that I instead of exposing only crypto_strongest_rand() as an Rng, I think we should either expose only crypto_rand(), or expose both. Once that's done I think this is merge_ready.

Edited to add: see comment on the pull request for more info.


Okay, I've exposed both in commit 94dcd38a14 in my bug24660_r1 branch. The TorRng struct calls crypto_rand() and the TorStrongestRng struct calls crypto_strongest_rand(). Also I've changed the pure-Rust RNG used when we compile tests to be rand::ChaChaRng because that doesn't require compiling the rand crate with the --features="std" (since that makes it pull in a bunch of dependencies).

Merge ready?

comment:21 Changed 4 months ago by nickm

Resolution: implemented
Status: merge_readyclosed

LGTM on review. Merging, updating git submodules.

Note: See TracTickets for help on using tickets.