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

Secret key generation and cleaning #749

Open
real-or-random opened this issue May 1, 2020 · 6 comments
Open

Secret key generation and cleaning #749

real-or-random opened this issue May 1, 2020 · 6 comments

Comments

@real-or-random
Copy link
Contributor

My feeling is this entire issue of secret key handling deserves a broader discussion.

We don't have a key generation function and Core won't need one. If we're now targeting different users more, maybe it's time to have one. But it's hard, see all the discussion points above. And it really depends on the user's platform. So actually I'd be happy with good examples and some references to correct methods to get randomness on different OSes.

On the other hand, nothing should stop us from exporting a cleaning function in #636, even if we don't have one for key gen.

What are other libraries doing for key generation?

Originally posted by @real-or-random in #748 (comment)

@real-or-random
Copy link
Contributor Author

See libsodium for example.

https://github.com/jedisct1/libsodium/blob/2f915846ff41191c1a17357f0efaae9d500e9858/src/libsodium/randombytes/internal/randombytes_internal_random.c

This is heavy stuff. If we really want a function, we could simply steal this or another one. No need to reinvent the wheel.

I don't like to have that complexity in our code but the question is whether we prefer to have it in our code or let our users write correct system PRG calls...

@elichai
Copy link
Contributor

elichai commented May 1, 2020

There's also what bitcoin does https://github.com/bitcoin/bitcoin/blob/master/src/random.cpp

But I'd assume this is the main reason why there's no key generation function in this library, because all of the needed complexity.

(I also think that in the examples regardless of if we add a key generation function we can still provide something simple that'll work on common modern systems)

@real-or-random
Copy link
Contributor Author

Well, the thing that people would usually do is to move that code to the library and just call it. But for SHA256 we're working on doing it the other way around actually. I've never quite heard and/or understood the full reasoning behind this. @sipa Can you elaborate a little bit?

@jonasnick
Copy link
Contributor

I'm fine with keeping key generation out of scope. But adding a good reference to the usage section would be helpful. @real-or-random do you have a particular reference in mind?

@sipa
Copy link
Contributor

sipa commented May 4, 2020

@real-or-random My thinking is that libsecp256k1 should be focussing on doing secp256k1 EC crypto, and not become a general purpose crypto library.

Part of this is scope creep... there is a lot that can be done around efficient hash functions for various architectures, random number generators for various platforms... and I think it would take us too far. If we want to do all those things near-perfectly it will mean duplicating a lot of work that's done elsewhere, plus maintenance to keep up with new developments in hardware and operating systems. If we do less than a near optimal job, callers who care will still be forced to reimplement or use other libraries.

Of course, this is partially in conflict with the goal of exposing a safe high-level API. We've already been forced to include a (naive) SHA256 implementation, but even there I feel like it would be a waste of our efforts to try to extend it to e.g. SSE4/AVX2/SHA-NI support. Permitting the caller to plug in their own version (at runtime or compile time) feels like a better option to getting better performance out of that. Most callers won't care about the mediocre performance of our SHA256, and those that do probably already have access to better versions.

I don't think it's that different for random generation. I wouldn't be opposed to adding a simple minimal built-in one, but I fear that any decent one is already going to be a pain in terms of build system integration etc alone.

@real-or-random
Copy link
Contributor Author

real-or-random commented May 5, 2020

That seems wise.

What does that mean for us? I think then giving a bit of useful advice in the examples and/or the usage section is what we should do right now. This helps the user but we don't promise to maintain any code in our library. We should cover at least (modern versions) Linux, MacOS, and Windows. I can try to find some references.

I'm against adding a minimal key gen function then, because then the user may just use that one without looking at it. If we offer nothing, the user may indeed read our docs.

edit:
I think we can safely export a cleaning function though. Ours is pretty portable and depends much less on the environment than randomness generation. It still depends on the compiler, but if our cleaning function is not effective, then it anyway won't help that the user uses a different working one just for his code.

Sorry, didn't want to close here actually

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

4 participants