-
Notifications
You must be signed in to change notification settings - Fork 18k
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
crypto/rand: use getentropy(2) on OpenBSD #13785
Comments
Changing internal/syscall/unix to utilize getentropy() on OpenBSD sounds reasonable to me. It won't be added to package syscall though (which is frozen), and I don't see a need to add it to golang.org/x/sys/unix either, since Linux's getrandom() isn't available there either. There's no need to add syscall.Syscall2. You can use syscall.Syscall and just pass 0 as a dummy third argument. Don't send a Github pull request. Upload a CL to Gerrit, like the Contribution Guidelines explain. (But expect to wait until after 1.6 is released for it to be reviewed.) |
You don't need to add syscall.Syscall2, just use syscall.Syscall
with zero as the 3rd syscall argument.
Adding it to internal/syscall/unix so that crypto/rand automatically
use it sounds good to me.
|
SGTM too, putting it in internal/syscall/unix next to the Linux getrandom stuff. |
CL https://golang.org/cl/18219 mentions this issue. |
Based on the syscall traces I'm seeing, it seems that I misinterpreted the logic in crypto/rand. How should I change it so that |
I don't think we have a CSPRNG in crypto/rand,
all reads are serviced from OS specific CSPRNG.
|
@minux I think there is a CSPRNG - see However, I've drafted a patch that uses OpenBSD's |
@mmcco If you want to propose that crypto/rand feed the kernel entropy source through another userspace CSPRNG, I suggest you do that as a separate issue. I'm not convinced that should be tied to whether crypto/rand uses getentropy on OpenBSD. |
That alternative CSPRNG is only intended for system without
a reliable /dev/urandom (only Plan 9).
I agree with mdempsky that if we decide to seed the alternative
CSPRNG with kernel provided seeds, we should do that in a
separate CL.
|
The creators of getentropy frequently specify that it is intended to be used for seeding, not for direct randomness access. I can try to get more information on why this is the case, although I've mentioned a few probable reasons above. I think there's a chance of a slight misunderstanding, though, based on your wording:
What is meant by "another" here? If you're referring to getentropy as a userland CSPRNG, remember: it's a system call directly to the kernel. |
That said, I'm not opposed to opening a second CL. |
What he meant by "a separate issue" is to first add genentropy syscall
and use it to service all Read calls in crypto/rand.
And then file an issue to discuss whether we should switch to use the
kernel provided entropy to feed a userspace CSPRNG.
How much throughput could OpenBSD's getentropy syscall provide?
I don't think any usual crypto use scenario is limited by randomness
throughput unless you're building a service to generate random keys.
|
@mdempsky @minux In what order to you imagine these CLs being merged? The OpenBSD kernel developers probably won't want getentropy to be used directly even for a little while. But we can't link it into Go's CSPRNG until we support it. Should I remove the assignment of |
@minux From what I've read, calling directly is a misuse of the API. I don't think we should start there. I'll try to ask for more clarification.
This sounds a lot like a network daemon that uses forward secret encryption. :-) |
@mmcco I'm the OpenBSD kernel developer that added getentropy. There's no difference between reading from /dev/urandom and calling getentropy except that the latter limits you to 256 bytes per system call. |
@mdempsky Oh, Jesus, I thought that name looked familiar. :P I assumed I must have been conflating names. That, of course, gives me more confidence. I noticed that there's logic in the kernel's Most importantly, it seems that there can't be an information leak from doing too many getentropy reads because it's functionally no different from a series of |
The extra ChaCha20 logic in randomread for >2048-byte reads is just a performance improvement so the kernel doesn't need to keep re-acquiring rndlock when generating a lot of randomness. It's not otherwise relevant to correctness/security here. So I'd propose you start by just seeing if replacing /dev/urandom reads with getentropy on OpenBSD. Unless there are lots of Read calls that ask for >256 bytes at a time, that should be a pure net win. Separately, you can propose using a userspace CSPRNG that we seed from the kernel, but I'm not convinced at this time that that's necessary. (I also agree with DJB that "We want the OS to Cheers |
As per https://github.com/golang/go/wiki/OpenBSD, please open a new issue if you want to drop support for OpenBSD 5.5 in Go 1.7 and above. No need to change codes, just for release notes and WiKi. |
@mikioh Thanks for that, I wasn't aware. I'll edit the wiki if/when this gets merged. It's planned for Go 1.7, which will ship first with OpenBSD 6.0, so I don't think this should be an issue for anyone being remotely responsible with system upgrades. |
Please open a new issue that describes Go 1.7 drops support for OpenBSD 5.5. and below for people who have to write doc/go1.7.html, WiKi, etc. |
It's the progenitor of Linux's
getrandom(2)
, which Go already supports. I have a patch that works on AMD64. I won't submit a pull request until it's tested on i386 and ARM (help welcome!).This generally just involves mirroring the approach used for
getrandom(2)
. However, we don't need to test for availability, as all supported OpenBSD releases havegetentropy(2)
. The only other caveat is that I had to addsyscall.Syscall2()
for OpenBSD, asgetentropy(2)
takes two arguments.The interface is very simple - here's the man page.
It's a pretty straightforward change. I just wanted to mention it as early as possible, as per the Contribution Guidelines.
Thoughts?
The text was updated successfully, but these errors were encountered: