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

feat: nostd, core+alloc support #22

Merged
merged 8 commits into from Aug 7, 2020
Merged

Conversation

roblabla
Copy link
Contributor

@roblabla roblabla commented Aug 18, 2019

Blocked on dignifiedquire/num-bigint#12

Missing: docs, tests and a way to avoid having base64 break our builds to rust-lang/cargo#4866.

Cargo.toml Outdated
@@ -50,7 +51,8 @@ name = "key"
# debug = true

[features]
default = []
default = ["std"]
std = []
Copy link
Member

Choose a reason for hiding this comment

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

shouldn’t this enable the std features on the crates that got it disabled above?

@newpavlov
Copy link
Member

newpavlov commented Aug 18, 2019

I wonder if we can write a custom library for stack allocated bigints bounded by a prime constant. it would be also useful for the SRP implementation. Do you know any prior art libraries which have done it?

@dignifiedquire
Copy link
Member

dignifiedquire commented Aug 18, 2019 via email

@roblabla
Copy link
Contributor Author

roblabla commented Aug 18, 2019

That's exactly what smallvec does. A SmallVec<[u8; 4096]> will store up to 4096 bytes on the stack, and then switch to heap-allocated vector. BigUint uses a SmallVec<[u64; 4]> (or u32; 8 if u64_digit feature is not used), so digits of up to 256 bit will be stored on the stack.

@newpavlov
Copy link
Member

newpavlov commented Aug 18, 2019

I meant bigints without the heap fallback. Since most operations are done modulo some value with a known length, it should be doable? Plus this hypothetical crate can be significantly smaller, since we don't need general purpose bigints.

@roblabla
Copy link
Contributor Author

roblabla commented Aug 18, 2019

Well for statically allocated vecs, arrayvec exists and is pretty good. But it would require changing the bigint library to be generic over the storage backend somehow, since otherwise we'd have to copy paste a fairly significant portion of the bigint algorithms over.

@dignifiedquire
Copy link
Member

@roblabla I have updated num-bigint-dig, want to update this PR so we can ship it?

@roblabla
Copy link
Contributor Author

I'll try to squeeze some time during the week to get this up and running.

@roblabla
Copy link
Contributor Author

I did some fixes here and there, but I'm stuck against a wall again. base64 is enabling the std feature of byteorder, which breaks no_std build. Now, base64 is a dev dependency, but because cargo is, frustratingly, still susceptible to unifying dev-dependencies and dependencies features.

A PR was merged in August allowing nostd usage of base64, but there hasn't been a new release of base64 since. So we're waiting for upstream to release a new version.

@tarcieri
Copy link
Member

@roblabla you can use subtle_encoding::base64 instead of base64, which is fully no_std friendly, has no dependencies on crates with activated std features, and provides constant-time encoding and decoding if you do end up using it for private keys: https://github.com/iqlusioninc/crates/tree/develop/subtle-encoding

@roblabla
Copy link
Contributor Author

roblabla commented Oct 25, 2019

The new base64 dependency breaks Rust 1.31 builds 😭. Subtle-encoding also has an MSRV of 1.36. May I bump the MSRV @dignifiedquire ?

@dignifiedquire
Copy link
Member

there is always sth :D feel free to bump the version

@@ -2,6 +2,7 @@ use num_bigint::traits::ModInverse;
use num_bigint::{BigUint, RandPrime};
use num_traits::{FromPrimitive, One, Zero};
use rand::Rng;
use libm::F64Ext;
Copy link
Member

Choose a reason for hiding this comment

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

Could you update to libm@0.2, I did that in num-bigint-dig, unfortunately that doesn't have the extension anymore, so you need to do some manual wrapping of things.

.travis.yml Outdated
@@ -2,14 +2,16 @@ language: rust

matrix:
include:
- rust: 1.31.0
- rust: 1.34.0
Copy link
Member

Choose a reason for hiding this comment

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

I bumped the msr to 1.36, because of smallvec@1.0 in #33

@dignifiedquire
Copy link
Member

@roblabla merged everything else on master, any chance you want to pick this up again and bring it over the finishline?

@roblabla
Copy link
Contributor Author

I'll take a look!

@roblabla
Copy link
Contributor Author

Some quick thoughts:

  • RSA now relies on thiserror. This crate is not no-std compatible. I'll probably have to rip it out and implement the traits by hand.
  • Regarding the possibility of a future no-std no-alloc version, the RSA crate now makes some fairly heavy use of the DynDigest trait, which relies on liballoc. In theory it could be swapped with a trait that takes a buffer instead of returning a boxed slice, and panics if the buffer is the wrong size.
  • The parse functionality will have to be gated on libstd for now, since simple_asn1 is not no_std-compatible. I don't think people working in no_std environments are likely to need PEM parsing though, so that shouldn't be a big issue.

@tarcieri
Copy link
Member

You can also use displaydoc if you'd like a proc macro for impl'ing Display for the error type which supports no_std.

@roblabla
Copy link
Contributor Author

roblabla commented Jun 11, 2020

I ended up using cargo expand and copy pasting the output of thiserror lol. I'll look into displaydoc. I pushed a first draft of the no-std support. It's WIP and untested, but at least it builds.

Things to do:

  • I added an "alloc" feature, the idea being that down the line, we might want a no-std no-alloc build to be possible. In order to support this use-case without introducing a breaking change, I figured the best idea would be to add an alloc feature now, and force the build to fail if neither std nor alloc features are given. Does that sound good?
  • I'll try to get qemu setup to run the tests on ARM on CI.
  • Fix the tests. I had to play with the dev-dependencies, which broke them :^).

Things that will happen in a follow-up PR:

  • ASN1 parsing in core+alloc environment.
  • Find a way to ditch lazy_static. That dependency is a pain in the neck, it uses a feature to enable libcore support (instead of the convention of adding a feature to add libstd support), which means we force lazy_static to spinlock even in std environments.

@tarcieri
Copy link
Member

tarcieri commented Jun 11, 2020

+1 to no_std + alloc. Many of the other RustCrypto crates support features which use this combination.

Also +1 to ditching lazy_static. I have similar pains regarding its (mis)use of subtractive cargo features.

You might check out conquer-once as a no_std-friendly replacement (I haven't used it personally, but I've heard good things):

https://github.com/oliver-giersch/conquer-once

Edit: hrmm, it looks like conquer-once doesn't provide lazy init on no_std. Alas. Never mind!

@roblabla
Copy link
Contributor Author

roblabla commented Jun 11, 2020

Ooh, I hadn't heard of conquer-once. And it does seem to provide lazy init on no_std: https://docs.rs/conquer-once/0.2.0/conquer_once/spin/type.Lazy.html is available in no-std, and is exactly what we want. We should be able to swap between this and the top-level Lazy depending on whether we enabled the std feature!

I'll try it out in a follow-up PR.

@roblabla roblabla changed the title feat: nostd support feat: nostd, core+alloc support Jun 11, 2020
@tarcieri
Copy link
Member

Is this ready to merge? Looks good to me at first glance

.travis.yml Outdated Show resolved Hide resolved
@roblabla roblabla force-pushed the nostd branch 3 times, most recently from 6b45d82 to 9b935e5 Compare July 25, 2020 14:25
@dignifiedquire
Copy link
Member

@roblabla almost there one more rebase please and can you check out those clippy warnings please? Sorry for the long turnaround times

@roblabla
Copy link
Contributor Author

roblabla commented Aug 7, 2020

Should be good to go I hope!

Copy link
Member

@dignifiedquire dignifiedquire left a comment

Choose a reason for hiding this comment

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

awesome, thanks for getting this through!

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 this pull request may close these issues.

None yet

4 participants