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

Adding deterministic SecureRandom for testing #661

Open
realcr opened this issue Jun 9, 2018 · 13 comments
Open

Adding deterministic SecureRandom for testing #661

realcr opened this issue Jun 9, 2018 · 13 comments

Comments

@realcr
Copy link

@realcr realcr commented Jun 9, 2018

One of the latest update to ring (v0.13.0-alpha5) broke some code we use for a mock deterministic random generator. We use it for testing. This is what it looks like:

use std::cell::RefCell;

use rand::{self, Rng, StdRng};
use ring::{error::Unspecified, rand::SecureRandom};

pub struct DummyRandom<R> {
    rng: RefCell<R>,
}

impl DummyRandom<StdRng> {
    pub fn new(seed: &[usize]) -> Self {
        let rng_seed: &[_] = seed;
        let rng: StdRng = rand::SeedableRng::from_seed(rng_seed);

        // Note: We use UnsafeCell here, because of the signature of the function fill.
        // It takes &self, it means that we can not change internal state with safe rust.
        DummyRandom {
            rng: RefCell::new(rng),
        }
    }
}

impl<R: Rng> SecureRandom for DummyRandom<R> {
    fn fill(&self, dest: &mut [u8]) -> Result<(), Unspecified> {
        self.rng.borrow_mut().fill_bytes(dest);
        Ok(())
    }
}

The compilation error I get is:

error[E0277]: the trait bound `crypto::test_utils::DummyRandom<R>: ring::private::Sealed` is not satisfied
  --> src/crypto/test_utils.rs:23:14
   |
23 | impl<R: Rng> SecureRandom for DummyRandom<R> {
   |              ^^^^^^^^^^^^ the trait `ring::private::Sealed` is not implemented for `crypto::test_utils::DummyRandom<R>`

I assume that a new ring::private::Sealed trait was introduced for safety reasons, to make sure exactly that I don't do something like the code above. Having the ability to generate deterministic random is very important for my tests, because I would like the tests to be reproducible if something goes wrong. I don't want to have a test that sometimes succeeds and sometimes fails.

Here is some part from a test case where I used DummyRandom:

    use super::super::test_utils::DummyRandom;

    #[test]
    fn test_rand_values_store() {
        let rng = DummyRandom::new(&[1, 2, 3, 4, 5]);

        // Generate some unrelated rand value:
        let rand_value0 = RandValue::new(&rng);

        let mut rand_values_store = RandValuesStore::new(&rng, 50, 5);
        let rand_value = rand_values_store.last_rand_value();
        // ...

Now the options I am left with are to use one of:

  • FixedByteRandom: A mock random generator that always returns the same byte. It is not very random looking.
  • FixedSliceRandom: I have to prepare a slice of random
  • FixedSliceSequenceRandom: I have to prepare a sequence of slices of random ahead of time.

In some test cases I am not sure exactly how much random will be requested from the random generator. I also don't think that I should deal with this. I would like to have a random generator that is deterministic and I can just plug into my tests, without serious preparation.

I understand the safety considerations and reasons for adding ring::private::Sealed. Could we have something like DummyRandom above as a workaround? I can make a pull request for this with my current implementation.

If this is not possible, what would be a good workaround?

@briansmith

This comment has been minimized.

Copy link
Owner

@briansmith briansmith commented Jun 9, 2018

We can put something new in ring::test::rand, maybe something like this:

pub struct NotRandom(FnMut(&mut [u8]) -> Result<(), error::Unspecified>);

I haven't thought it all the way through but I think you could then implement your rand-based dummy implementation in terms of that.

I would like to avoid RefCell if possible and definitely I want to avoid rand.

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Jun 9, 2018

@briansmith : Thanks for the quick answer.
I think that NotRandom could be a reasonable idea. Would you like me to write a pull request with an attempt to implement this solution?

I can fully understand why you want to avoid rand in ring.
Taking the risk of asking a noob question, why would you want to avoid RefCell?
I had to use it in DummyRandom because the fill function signature has &self and not &mut self as argument:

fn fill(&self, dest: &mut [u8]) -> Result<(), Unspecified> {

I wonder if it will be possible to use NotRandom(FnMut(&mut [u8]) -> Result<(), error::Unspecified>); together with SecureRandom trait method: fill(&self,..., because FnMut closures need to be mutable when called.

@briansmith

This comment has been minimized.

Copy link
Owner

@briansmith briansmith commented Jun 9, 2018

@realcr If you need to use RefCell for this then it isn't the end of the world because it is test-only code.

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Dec 13, 2018

@briansmith : Is there a plan to merge the PR proposed by @juchiast to allow mock random for testing? I assume you must be busy with other parts of ring. If I can somehow help to make progress for this issue please let me know.

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Jan 23, 2019

Hi, I added two possible solutions for this issue:

The offst project relies on Ring for cryptographic primitives. I pinned Ring to version ring "=0.13.0-alpha", which was probably the last ring version allowing for mock random generation structs. This is critical for the ability to write deterministic tests.

I recently attempted to bump our dependencies versions using cargo update and got this error:

$ cargo update
    Updating crates.io index
error: failed to select a version for the requirement `ring = "= 0.13.0-alpha"`
  candidate versions found which didn't match: 0.14.3, 0.14.2, 0.14.1, ...
  location searched: crates.io index
required by package `offst-crypto v0.1.0 (/home/real/projects/d/offst/components/crypto)`

Checking crates.io, I noticed that many old versions of Ring were yanked from crates.io, see here: https://crates.io/crates/ring/versions

This means that on one hand I can not stay with the pinned version 0.13.0-alpha, because it was yanked. On the other hand I can not move to the next versions because the introduction of the private sealed::Sealed.

I can not rely on non-deterministic tests for cryptographic code, and using the alternatives (FixedSliceSequenceRandom for example) is too fragile and difficult to construct, as I have to reverse engineer ahead of time all the sizes of all random slices requested.

Are there another alternatives to write deterministic tests that involve random generation using Ring?
If not, what is required to solve this issue? I assume the dev team might be busy, I am more than willing to help!

@briansmith

This comment has been minimized.

Copy link
Owner

@briansmith briansmith commented Jan 29, 2019

@realcr Which ring features are you trying to make deterministic for testing purposes that you currently can't? ring::agreement? Something else?

Obviously, I'm not that comfortable allowing people to plug in new implementations of SecureRandom. It would be better if ring could expose test-only APIs, i.e. APIs that will only be exposed during cargo test but not any other time. But, AFAICT, there's been no progress towards making that happen on the Rust language side; I don't even know if there is (still) an issue tracking it.

Anyway, I would like to help you out, but first I'd like to learn more about the specifics of the issue you're running into to see if there are alternatives we can try first.

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Jan 29, 2019

Hi @briansmith , thanks for the reply, and thanks for your work on Ring!

The things I'm currently doing that relate to random are the following:

  • Generation of random nonces to be used inside protocols (Mostly used to avoid replay attacks).
  • Generation of EphemeralPrivateKey for agreements.
  • Generation of Ed25519KeyPair (For this case I could probably generate a few pairs once and save them inside a file, loading using from_pkcs8 every time, but generating from a deterministic seed is so much simpler to code).

I am in strong agreement with you about making Ring strict to make sure people don't make fatal cryptographic mistakes. My opinion though is that there is another side to this decision. if we don't allow any escape hatches, you will end up having to debate with every single person that tries to do something that you haven't thought about when you designed Ring. It's like having a Rust compiler that doesn't have any unsafe keyword, and you don't get to have all the Rcs, RefCells.

I thought about it for a while, and proposed a possible solution in the form of unsafe for the Sealed trait at this PR: #772 . What is your opinion about this idea?

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Oct 26, 2019

@briansmith : Hi, more than a year has passed since I opened this issue. is there any planned solution?

Ring is my favourite crate for Rust cryptography and I really appreciate your work on it.
My situation is that I am currently stuck on a very old version of Ring due to the breaking change introduced in v0.13.0-alpha5. If I upgrade to the latest version, I will lose my ability to write deterministic tests for my code.

One important example of something I can not test deterministically is EphemeralPrivateKey (for key exchange).

I provided detailed information for the issue, and also provided 2 different pull requests attempting to solve this issue. Is there anything else I can do to help advance solving this issue?

How are other Ring users testing their cryptographic code?

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Oct 26, 2019

I added PR #912 as a proposal to solve this issue.

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Oct 31, 2019

I tried to gather some ideas about what would a testing interface will look like for a user of the ring crate. Link: https://users.rust-lang.org/t/exporting-function-from-crate-to-only-be-used-in-tests/34052

I have two proposals:

  1. Adding a feature, for example: #[cfg(feature = "unsafe-internals")]. The user of the Ring crate will then be able to import deterministic random for testing only if using this feature. This idea is due to Yandros: https://users.rust-lang.org/t/exporting-function-from-crate-to-only-be-used-in-tests/34052/18?u=realcr

  2. Making SecureRandom public and unsafe. This means that the implementer of SecureRandom will have to use the unsafe keyword himself. I think that this design goes in line with choices made for other struct traits, like Unpin. In a sense, when someone implements SecureRandom, he asserts that he really knows what he is doing, and that the random he creates is actually safe.

What do you think?

@juchiast

This comment has been minimized.

Copy link

@juchiast juchiast commented Oct 31, 2019

Hi @realcr, long time no see! Brian seems to be inactive recently, I bet you can fork the crate and add the changes yourself. This crate isn't very active anyway so you won't have to spend a lot of efforts to keep up with master branch.

@realcr

This comment has been minimized.

Copy link
Author

@realcr realcr commented Oct 31, 2019

@juchiast : Hi, good to meet you online again!
I actually thought that the development of this crate is very active. Last commit is on the 30.8.2019, for version 0.16.9. Maybe the maintainers don't have time to get to all the issues?

@juchiast

This comment has been minimized.

Copy link

@juchiast juchiast commented Oct 31, 2019

@realcr Well, rust is moving very fast in some areas (e.g. async/await) that I'm interested in, so my opinion is slightly biased. Anyway, that's a solution you could consider.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.