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

x/crypto/nacl: Support for libsodium "sealed box" #35346

Closed
mastahyeti opened this issue Nov 4, 2019 · 13 comments
Closed

x/crypto/nacl: Support for libsodium "sealed box" #35346

mastahyeti opened this issue Nov 4, 2019 · 13 comments

Comments

@mastahyeti
Copy link

@mastahyeti mastahyeti commented Nov 4, 2019

The x/crypto/nacl package doesn't currently implement the libsodium "sealed box" primitive (docs). This functionality is a very lightweight extension of the functionality provided by x/crypto/nacl/box, providing anonymous encryption using the receiver's public key (encrypt a message to the receiver without the sender having their own keypair).

My inclination would be to create another package — x/crypto/nacl/sealedbox — exposing this functionality. My only concern with this approach is that key generation between box and sealedbox is identical. Would it make sense to expose a separate sealedbox.GenerateKey() function or to simply instruct the user to call box.GenerateKey()?

The other option would be to build this functionality into the box package, providing SealAnonymous() and OpenAnonymous() functions.

/cc @FiloSottile

@gopherbot gopherbot added this to the Proposal milestone Nov 4, 2019
@gopherbot gopherbot added the Proposal label Nov 4, 2019
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 4, 2019

I think I like adding APIs to x/crypto/nacl/box better, if nothing else because the real box API is awkward, and adding the much more useful anonymous options next to it would help users who are confused about how to use box.

@mastahyeti

This comment has been minimized.

Copy link
Author

@mastahyeti mastahyeti commented Nov 4, 2019

Sounds good to me. In that case, I propose adding two new functions to the box package

func SealAnonymous(out, message []byte, peersPublicKey *[32]byte) []byte {}
func OpenAnonymous(out, box []byte, privateKey *[32]byte) ([]byte, bool) {}

The nonce generation will be calling into blake2b functions that may return errors. In practice, errors are only returned from those functions if a bad hash size is specified. Since we know the hash size will be valid, I propose panicking if an error is encountered, rather than surfacing them to the caller.

@mastahyeti

This comment has been minimized.

Copy link
Author

@mastahyeti mastahyeti commented Nov 4, 2019

This actually might be a bit more complicated. The ephemeral keypair needs to be generated randomly. So, SealAnonymous should also probably take a rand io.Reader argument. Since reading this can fail, I suppose SealAnonymous should also return an error? It's rather unfortunate to add this complexity to what should be a simple API.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 4, 2019

Alas, there's a lot of unfortunate things in that API: bool instead of error, array pointers, append instead of inlined allocation... but I guess we should be consistent, so yeah.

func SealAnonymous(out, message []byte, recipient *[32]byte, rand io.Reader) ([]byte, error)
func OpenAnonymous(out, box []byte, key *[32]byte) (message []byte, ok bool)

I am tempted by the more idiomatic SealWithoutSender name, but I guess we should be consistent with libsodium's naming.

A nil rand should default to crypto/rand.Reader.

@mastahyeti

This comment has been minimized.

Copy link
Author

@mastahyeti mastahyeti commented Nov 4, 2019

Sounds good. Do I need to write a design doc for this? Do I need to wait for an official approval here, or can I go ahead with a PR?

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Nov 4, 2019

Feel free to open a CL, we should leave this issue open for a week or so before marking it accepted to give time to the community to comment.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

For what it's worth, please let the proposal review meetings add Proposal-FinalCommentPeriod so that anything in final comment period is listed in the minutes (golang.org/s/proposal-minutes). We'll list it this week. Thanks.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Nov 27, 2019

Based on the discussion above this seems like a likely accept.
Will add to the proposal minutes and leave open for another week for comments.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Dec 2, 2019

Something I didn't realize is that the OpenAnonymous API requires the local public key to generate the nonce. It can be generated from the private key, but that's quite the performance overhead. It's also too late to make the private key carry both.

https://go-review.googlesource.com/c/crypto/+/205241/1/nacl/box/box.go#156

libsodium takes the public key from the caller.

int crypto_box_seal_open(unsigned char *m, const unsigned char *c,
                         unsigned long long clen,
                         const unsigned char *pk, const unsigned char *sk);

I kind of hate that they are two *[32]byte that are easy to swap, but the API cleanliness probably doesn't justify an extra scalar multiplication at every decryption, basically doubling the fixed overhead.

func OpenAnonymous(out, box []byte, publicKey, privateKey *[32]byte) (message []byte, ok bool)

The only alternative I see is to fork the API entirely, with a new GenerateKey that returns a bundled private+public key, at that point maybe in a new package, like golang.org/x/crypto/box/anonymous? It'd be a good opportunity to use slices rather than byte arrays, return slices instead of appending, and return error values, too.

Opinions?

(@rsc, yep, will do that going forward. I think I marked this one before we discussed it.)

@mastahyeti

This comment has been minimized.

Copy link
Author

@mastahyeti mastahyeti commented Dec 3, 2019

My vote would be to take the public key as an argument. We could even make it optional and calculate the public key if it isn't provided. It seems less than ideal to have a separate package with a new key type that isn't interoperable with the existing keys.

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Dec 3, 2019

Change https://golang.org/cl/205241 mentions this issue: nacl/box: support anonymous seal/open

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Dec 3, 2019

Alright, let's go for the libsodium-like API.

func SealAnonymous(out, message []byte, recipient *[32]byte, rand io.Reader) ([]byte, error)
func OpenAnonymous(out, box []byte, publicKey, privateKey *[32]byte) (message []byte, ok bool)
@rsc rsc added this to Incoming in Proposals Dec 4, 2019
@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Dec 4, 2019

No change in consensus, so accepted.

@rsc rsc changed the title Proposal: x/crypto/nacl: Support for libsodium "sealed box" x/crypto/nacl: Support for libsodium "sealed box" Dec 4, 2019
@rsc rsc moved this from Incoming to Likely Accept in Proposals Dec 4, 2019
@rsc rsc removed the Proposal label Dec 11, 2019
@rsc rsc modified the milestones: Proposal, Unreleased Dec 11, 2019
@rsc rsc moved this from Likely Accept to Accepted in Proposals Dec 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Proposals
Accepted
5 participants
You can’t perform that action at this time.