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

Add rdrand feature to getrandom dependencies #146

Closed
gelven4sec opened this issue Mar 10, 2022 · 19 comments
Closed

Add rdrand feature to getrandom dependencies #146

gelven4sec opened this issue Mar 10, 2022 · 19 comments

Comments

@gelven4sec
Copy link

gelven4sec commented Mar 10, 2022

Hi,

At first, thanks for this awesome library !

I'm trying to build rsa for the x86_64-unknown-uefi target, so in no_std environement.

After fixing the bug #138 by upgrading num-bigint-dig to 0.8.1 (btw you guys might want to fix that too).
I'm getting an error from building getrandom (which is called from rand) as it doesn't support my target.

The fix is to enable the rdrand feature from getrandom.

I tried messing with rsa's Cargo.toml but I couldn't achieve to make it work.

I hoped you guys can help me.

EDIT: I just the saw the num-bigint-dig upgrade in a current PR

@newpavlov
Copy link
Member

The rdrand feature should not be enabled by library crates. You should enable it in your application using the following line in your Cargo.toml:

getrandom = { version = "0.2", features = ["rdrand"] }

@gelven4sec
Copy link
Author

gelven4sec commented Mar 10, 2022

Well, I tried that but it seems that it's not affecting the getrandom called from rsa, i still get error that it doesn't support my target.
But compiling getrandom alone works. (without rsa)

@newpavlov
Copy link
Member

Are you sure you don't have getrandom v0.1 somewhere in your dependency tree?

@gelven4sec
Copy link
Author

gelven4sec commented Mar 10, 2022

Are you sure you don't have getrandom v0.1 somewhere in your dependency tree?

No sign of getrandom v0.1 in my cargo tree, 0.2.5 everywhere, I really dont get it.

EDIT: Okay I used [patch] in my cargo.tom to override getrandom with a local version and it works.

I'm now hitting a problem build rand_chacha :

LVM ERROR: Do not know how to split the result of this operator!
error: could not compile 'rand_chacha'

@tarcieri
Copy link
Member

@sven-eliasen that's very strange. Feature unification should function identically between a transitive and direct dependency

Can you paste the relevant lines of your Cargo.toml where you include both rsa and getrandom in your [dependences] section for us to double check?

@gelven4sec
Copy link
Author

gelven4sec commented Mar 10, 2022

[dependencies]
uefi = {path = "../uefi-rs", features = ["exts", "logger"]}
uefi-services = {path = "../uefi-rs/uefi-services"}
log = {version = "0.4.0", default-features = false }
rsa = { version = "0.5.0", default-features = false }

[patch.crates-io]
getrandom = { path = "../getrandom", default-features = false, features = ["rdrand"] }
rsa = { path = "../RSA", default-features = false }

I use a local version of rsa because I needed the num-bigint-dig upgrade to compile.
And getrandom to force the rdrand feature.

EDIT: Removed rand as it's already called from rsa

@newpavlov
Copy link
Member

newpavlov commented Mar 10, 2022

You should not patch getrandom, simply add it to your [dependencies] section, as I wrote earlier.

@tarcieri tarcieri changed the title Add rdrand feature to getrandom dependencie Add rdrand feature to getrandom dependencies Mar 10, 2022
@gelven4sec
Copy link
Author

gelven4sec commented Mar 10, 2022

Same result, but it also passed the getrandom build error.

Pretty strange because that didn't work first time, but anyway I'm now fighting rand_chacha

@gelven4sec
Copy link
Author

Screenshot if needed :
image info

@newpavlov
Copy link
Member

newpavlov commented Mar 10, 2022

I suggest reporting the rand_chacha error in the rust-lang repository, since it's clearly a compiler issue.

@tarcieri tarcieri mentioned this issue Mar 10, 2022
@tarcieri
Copy link
Member

@sven-eliasen I just cut a v0.6.0-pre release of rsa: https://crates.io/crates/rsa/0.6.0-pre

Can you try that instead of the [patch.crates-io] directive? I'm going to guess you have multiple copies of getrandom in your Cargo.lock so features aren't being unified across them

@gelven4sec
Copy link
Author

screenshot

Pretty much the same result. I deleted Cargo.lock, cargo clean, cargo update.
Don't know what I can try more.

I read somewhere that RSA could work with rand_core instead, wouldn't that be great ?

@tarcieri
Copy link
Member

Were you previously having trouble building getrandom, and is that working now? Or was getrandom building but rand failed as above?

That's a very strange error you're getting in rand.

@gelven4sec
Copy link
Author

Yeah the issue has moved on sorry 😅

getrandom seems to build now, by adding getrandom = {version = "0.2", features = ["rdrand"]}.

Now i'm having problem with rand_chacha

@tarcieri
Copy link
Member

@newpavlov is there a replacement for ChaCha20Rng in the chacha20 crate? It looks like it's gone in the v0.9 release

@newpavlov
Copy link
Member

newpavlov commented Mar 10, 2022

is there a replacement for ChaCha20Rng in the chacha20 crate? It looks like it's gone in the v0.9 release

No, I have removed it for code simplification. I plan to implement a rand compatibility layer as part of the cipher crate.

Either way, the error comes from rand_chacha, which is a required dependency for ThreadRng users. I would say we should remove the hard dependency on rand and instead make code generic over RngCore + CryptoRng like in our other crates. It would allow @sven-eliasen to use OsRng, which relies only on getrandom.

@gelven4sec
Copy link
Author

That would be great ! :D

@newpavlov
Copy link
Member

newpavlov commented Mar 11, 2022

#148 removes hard dependency on rand, so it should be a viable temporary workaround for the rand_chacha compilation error. Thus I think we can close this issue.

I still suggest that you report the rand_core issue to the Rust repository. rand is a quite common dependency, so you probably will hit it again sooner or later.

@gelven4sec
Copy link
Author

I agree you can closed this issue, thanks a lot for taking care of this.

I tested your branch and I still get a rand_chacha error but not from rsa itself so I'm gonna take this somewhere else.

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

No branches or pull requests

3 participants