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

Use SecRandomCopyBytes on macOS #398

Closed
wants to merge 1 commit into from
Closed

Use SecRandomCopyBytes on macOS #398

wants to merge 1 commit into from

Conversation

dirk
Copy link
Contributor

@dirk dirk commented Jan 1, 2017

Uses the SecRandomCopyBytes function from the Security.framework on macOS and iOS.

Fixes #149.

@coveralls
Copy link

coveralls commented Jan 1, 2017 •
edited
Loading

Coverage Status

Coverage decreased (-0.05%) to 93.315% when pulling 7e436c27837b46d91745b3992f9c89cbb62ef18a on dirk:dirk-macos-use-secrandomcopybytes into d420473 on briansmith:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.05%) to 93.315% when pulling 7e436c27837b46d91745b3992f9c89cbb62ef18a on dirk:dirk-macos-use-secrandomcopybytes into d420473 on briansmith:master.

briansmith
briansmith requested changes Jan 3, 2017
View reviewed changes
Copy link
Owner

@briansmith briansmith left a comment

Choose a reason for hiding this comment

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

Thanks for doing this!

src/rand.rs
@@ -195,6 +205,21 @@ mod sysrand_or_urandom {
}
}

#[cfg(any(target_os = "macos", target_os = "ios"))]
mod security {
Copy link
Owner

Choose a reason for hiding this comment

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

I suggest using the name darwin instead, based on https://bugs.webkit.org/show_bug.cgi?id=148439#c14 and https://trac.webkit.org/browser/trunk/Source/WTF/wtf/spi/darwin/CommonCryptoSPI.h?rev=205488#L29.

src/rand.rs
@@ -227,6 +252,15 @@ extern {
fn GFp_sysrand_chunk(buf: *mut u8, len: c::size_t) -> c::long;
}

#[cfg(any(target_os = "macos", target_os = "ios"))]
#[link(name = "Security", kind = "framework")]
extern {
Copy link
Owner

Choose a reason for hiding this comment

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

These externs should be in the darwin sub-module unless that doesn't work (if not, why?).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@briansmith: I tried putting them in the sub-module, but doing so required another use super::super::c in the sub-module, which seemed to go against the prevailing style in this project. Would you prefer the sub-mobile-with-a-use approach?

Copy link
Owner

Choose a reason for hiding this comment

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

Instead of use super::super::c I think you can just use c;

src/rand.rs
static kSecRandomDefault: *const u8;

// `rnd` should be a `SecRandomRef`.
fn SecRandomCopyBytes(rnd: *const u8, count: c::size_t, bytes: *mut u8) -> c::int;
Copy link
Owner

Choose a reason for hiding this comment

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

Let's change rnd to rnd: &SecRandomRef.
I suggest changing to comment to "rnd must be kSecRandomDefault."

src/rand.rs
#[cfg(any(target_os = "macos", target_os = "ios"))]
#[link(name = "Security", kind = "framework")]
extern {
static kSecRandomDefault: *const u8;
Copy link
Owner

Choose a reason for hiding this comment

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

static kSecRandomDefault: &'static SecRandomRef;

src/rand.rs
0 => Ok(()),
_ => Err(error::Unspecified),
}
}
Copy link
Owner

Choose a reason for hiding this comment

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

Let's do this:

enum SecRandomRef {}

Then put the extern block here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@briansmith: So I was originally doing something like this:

#[repr(c)]
struct SecRandomRef {}

But the compiler complained about empty struct layout (for good reason). I'm guessing enum—as you suggested—is the proper way to do this?

(Just satisfying my curiosity here. 🙃 )

@briansmith
Copy link
Owner

I'm guessing enum—as you suggested—is the proper way to do this?

It is the least bad way we have for now. I use this enum pattern for BIGNUM and BN_MONT_CTX in ring::rsa::bigint.

@frewsxcv frewsxcv mentioned this pull request Jan 4, 2017
Closed
@dirk
Copy link
Contributor Author

dirk commented Jan 5, 2017

@briansmith: Comments addressed! 😄

@coveralls
Copy link

coveralls commented Jan 5, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 93.47% when pulling 400c09f1a8e2ba12c0e4e98adeebe33fa9c4baf3 on dirk:dirk-macos-use-secrandomcopybytes into 368279f on briansmith:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 93.47% when pulling 400c09f1a8e2ba12c0e4e98adeebe33fa9c4baf3 on dirk:dirk-macos-use-secrandomcopybytes into 368279f on briansmith:master.

@briansmith briansmith added this to the 0.6.3 milestone Jan 14, 2017
@briansmith
Copy link
Owner

briansmith commented Jan 14, 2017 •
edited
Loading

Great, thanks!

I rebased and squashed all your commits and put them in my own dirk-macos-use-secrandomcopybytes branch: https://github.com/briansmith/ring/commits/dirk-macos-use-secrandomcopybytes. I also reformatted it to the 80-column limit we use in ring.

Please amend the commit message to add the contributor's statement from https://github.com/briansmith/ring#contributing, and I'll check this in ASAP.

Thanks!

@dirk
Use SecRandomCopyBytes from Security.framework on macOS
c4d6a49 
I agree to license my contributions to each file under the terms
given at the top of each file I changed.
@dirk dirk force-pushed the dirk-macos-use-secrandomcopybytes branch from 400c09f to c4d6a49 Compare January 17, 2017 20:47
@dirk
Copy link
Contributor Author

dirk commented Jan 17, 2017

@briansmith: Done! 😄

@coveralls
Copy link

coveralls commented Jan 18, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 93.47% when pulling c4d6a49 on dirk:dirk-macos-use-secrandomcopybytes into c431705 on briansmith:master.

@briansmith
Copy link
Owner

Thanks!

This landed in master as dd27044. I also landed a documentation update: caaa3f7.

@briansmith briansmith closed this Jan 22, 2017
@dirk dirk deleted the dirk-macos-use-secrandomcopybytes branch January 23, 2017 07:26
@briansmith
Copy link
Owner

See the follow-up #416 (comment) and cca54a5.

@dirk
Copy link
Contributor Author

dirk commented Jan 24, 2017

@briansmith: Glad to see this land! 😄

@briansmith
Copy link
Owner

Follow-up to fix a Rust Nightly build failure due to stricter unused-item checking in Nightl: 4410207.

Thanks again for contributing this !

@briansmith
Copy link
Owner

Another follow-up, mostly a no-op: b88b633.

@briansmith briansmith mentioned this pull request Jan 26, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@briansmith briansmith briansmith requested changes

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

Successfully merging this pull request may close these issues.
3 participants
@dirk @coveralls @briansmith