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

deps: update to newer rand/hkdf crates #21

Open
warner opened this issue Aug 7, 2019 · 8 comments
Open

deps: update to newer rand/hkdf crates #21

warner opened this issue Aug 7, 2019 · 8 comments

Comments

@warner
Copy link
Member

warner commented Aug 7, 2019

I don't know what the Rust convention is, but when I see cargo outdated telling me that there are newer versions of dependencies that we might use, I'm tempted to upgrade. SPAKE2 is currently out-of-date on HKDF and the rand crate.

I've got a PR for spake2's use of HKDF that I'll submit in a minute, but we can't update to rand-0.7 until curve25519-dalek does the same, because the random-element selection API cites a rand_core::CryptoRng trait that must be the same on both sides of the interface.

I haven't looked closely at SRP, but it's behind on both rand (which should be easy) and generic-array (about which I have no idea).

@newpavlov
Copy link
Member

newpavlov commented Aug 7, 2019

Also note that bumping rand to v0.7 will raise MSRV to 1.32. Personally I prefer a conservative view point stating that MSRV bump should be considered a breaking change. Also it will make sense to migrate to 2018 edition as well.

I have some ideas for SRP API changes, so I wanted to bump dependencies together with them.

@warner
Copy link
Member Author

warner commented Aug 7, 2019

Sounds good to me. I'll keep an eye on curve25519-dalek and will file a PR as soon as they've updated too (I have no idea when that might be).

Incidentally, how does one learn what the MSRV is for any particular crate? Do you just keep trying to compile it with older and older ones until something fails? Or is there some Cargo.toml notation, or tool that tells you what features you're using, or something like that?

@newpavlov
Copy link
Member

newpavlov commented Aug 7, 2019

AFAIK there is no such tool (you can't even declare crate MSRV right now...). You only can check MSRV as part of CI tests and automatically re-run them periodically to detect potential regressions in your dependency tree.

@warner
Copy link
Member Author

warner commented Aug 7, 2019

Got it, thanks.

I ran rustup run 1.31.1 cargo test on the PAKEs repo, and learned that curve25519-dalek v1.2.2 doesn't work, so maybe I accidentally brought us to MSRV=1.32 when I did the 2018-edition update about 8 months ago? (commit bd19c40). Or maybe they incremented their MSRV without a major-version change? I'll experiment to see how e.g. v1.0.x/v1.1.x/v1.2.x differ, maybe we should have used a tighter constraint.

   Compiling curve25519-dalek v1.2.2                                                                               
error[E0658]: `Self` struct constructors are unstable (see issue #51994)                                           
   --> /home/warner/.cargo/registry/src/github.com-1ecc6299db9ec823/curve25519-dalek-1.2.2/src/edwards.rs:707:9    
    |                                                                                                              
707 |         Self(scalar_mul::precomputed_straus::VartimePrecomputedStraus::new(static_points))                   
    |         ^^^^                                                                                                 
                                                                                                                   
error[E0658]: `Self` struct constructors are unstable (see issue #51994)                                           
   --> /home/warner/.cargo/registry/src/github.com-1ecc6299db9ec823/curve25519-dalek-1.2.2/src/ristretto.rs:925:9  
    |                                                                                                              
925 |         Self(                                                                                                
    |         ^^^^                                                                                                 
                                                                                                                   
error: aborting due to 2 previous errors                                                                           
                                                                                                                   
For more information about this error, try `rustc --explain E0658`.                                                
error: Could not compile `curve25519-dalek`.                                                                       

I see that our .travis.yml is pinning 1.31.0 for cargo fmt, but otherwise isn't pinning any particular version (it uses stable and beta and nightly). Should we add a 1.31.1 to that list to test our purported MSRV?

@newpavlov
Copy link
Member

newpavlov commented Aug 7, 2019

rand v0.7 has MSRV equal to 1.32 and I plan to use the same version for future updates of RustCrypto crates, so I think it makes sense to go with it instead of 1.31. And yes, I think it's worth to add MSRV test to CI.

@warner
Copy link
Member Author

warner commented Aug 7, 2019

It looks like curve25519-dalek 1.0.0-1.0.3 worked with rustc-1.31.1, but their 1.1.0 release requires 1.32.0 or newer. So a constraint of 1.0 (instead of just 1) would have avoided the accidental MSRV bump.

I'll file a PR to add CI for 1.32.0, and to tighten the dependency. I'll also increase the RUSTFMT pin to 1.32 since that's what we need now (and it looks like a few cosmetic changes will result).

warner added a commit that referenced this issue Aug 7, 2019
warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
1.31.1 is our intended MSRV (Minimum Supported Rust Version), but we probably
don't actually work there because of an insufficiently-constrained dependency
that requires 1.32.

1.32 is probably our actual MSRV, and will be the one we aim for going
forward.

refs RustCrypto#21
@warner
Copy link
Member Author

warner commented Aug 7, 2019

Ok, the travis PR is filed, and as expected it fails against 1.31.1 (I put it in a allow_failures section).

The cargo fmt from 1.32.0 and 1.33.0 doesn't like the missing semicolons in spake2/src/lib.rs (line 735/743/751/761), but the one from 1.31.1 and 1.34.2 is happy with it.. maybe they had a regression for a few versions. Would you want to run the RUSTFMT build against 1.34.2 (or maybe something newer) even if our MSRV is 1.32.0?

warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
This was just "1", which accidentally caused our MSRV to be raised from 1.31
to 1.32 without a spake2 minor-version bump (dalek-1.0.x compiles with
rustc-1.31, but dalek-1.1.x required 1.32).

Hopefully by making it "1.2", our MSRV will remain at 1.32 until we
explicitly decide to take on a dependency that needs something newer.

refs RustCrypto#21
warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
This will also increase the MSRV to rust-1.32 or later.

refs RustCrypto#21
@newpavlov
Copy link
Member

newpavlov commented Aug 7, 2019

I always prefer to keep rustfmt in allowed failures. I think it's less annoyance for contributors, and IIRC there were cases when rustfmt developers have changed defaults, which in turn resulted in CI failures across projects. So I think it will be enough to test formatting only on the latest stable.

warner added a commit that referenced this issue Aug 7, 2019
warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
This was previously locked down at a specific version (1.31.0, much older
than the current 1.36), so we'd get consistent behavior over time. But since
we allow the rustfmt build to fail (it's in allow_failures), we don't need
this consistency so much. PR authors are expected to format their code
against the current stable rustfmt, but test it against the MSRV (currently
1.32.0).

refs RustCrypto#21
warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
This was just "1", which accidentally caused our MSRV to be raised from 1.31
to 1.32 without a spake2 minor-version bump (dalek-1.0.x compiles with
rustc-1.31, but dalek-1.1.x required 1.32).

Hopefully by making it "1.2", our MSRV will remain at 1.32 until we
explicitly decide to take on a dependency that needs something newer.

refs RustCrypto#21
warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
This was just "1", which allowed our MSRV to be accidentally raised from 1.31
to 1.32 without a deliberate spake2 minor-version bump (dalek-1.0.x compiles
with rustc-1.31, but dalek-1.1.x required 1.32).

Hopefully by making it "1.2", our MSRV will remain at 1.32 until we
explicitly decide to take on a dependency that needs something newer.

refs RustCrypto#21
warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
This will also increase the MSRV to rust-1.32 or later.

refs RustCrypto#21
warner added a commit to warner/PAKEs that referenced this issue Aug 7, 2019
This will also increase the MSRV to rust-1.32 or later.

refs RustCrypto#21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants