atproto/crypto: in-repo SDK for atproto cryptography #270
Conversation
…blicKey This simplifies the API a bit by making "uncompressed" and "legacy multibase" special cases.
There was a problem hiding this comment.
This interface and the GoDoc look pretty good to me.
I'm not a cryptographer but I have strong opinions about never writing a panic in Go code, especially in a library, so I've requested you remove those and make those functions return errors even if it's less ergonomic.
There was a spot where we might wanna bounds check too.
Overall, looks great and I'm happy to have nice ergonomic functions to work with the strange collection of key representations we've gotta deal with.
|
@ericvolp12 thanks for the review! An alternative API, which wouldn't be too hard to update to, would be to define an interface and have separate P256 and K256 implementations of that interface. That would remove a bunch of the I would probably name them |
|
@ericvolp12 I iterated on the API to have it use interfaces instead of a hacked enum struct. This got rid of most of the panics. There are two remaining panics in the P-256 implementation. These are relatively safe because I call the same code block in all of construction code paths (which return A couple methods are probably going to be deprecated. I guess we could could just not include those here so the new interface is clean and stable? But they might also linger longer or be permanent. I feel comfortable yanking them in a couple months as long as they were marked as such from the begining. |
|
After sleeping on it, i'll probably ease the stuff for working with "legacy" DID document (weird multibase encoding, "uncompressed" byte formats, suite names) out of any interface definitions. Will keep the uncompressed helpers on the current public key types, but move the rest to the (forthcoming) identity package. TODO to update this PR reflecting those changes. |
We can re-implement these internally in identity SDK, as needed.
|
@ericvolp12 updated and ready for (final?) review godoc preview: https://static.bnewbold.net/tmp/atproto_crypto_godoc2.html |
New interface looks sensible to me, great work! Only question I have is still around the use of Afaict it's a no-op: https://goplay.tools/snippet/Hvt9pv9jAAX If that's the case, can we pull the |
|
@ericvolp12 I had a reply to a review comment above about it, the SHA-256 return is typed. If I try removing the |
|
Also just because it felt sneaky, to ensure you noticed, I replaced the "should be impossible" |
I don't think it should be possible for this library to make your program exit |
Ah, okay yeah I missed that, interesting... |
To elaborate on this, afaik the general convention in Go is that "panics should never leave your code" so if there is a panic somewhere, it should be caught by the library and handled appropriately, not bubbled up to the caller. I don't see any reason why a cryptography library should make someone's CLI or long-running service suddenly exit without warning. Even in a "this should never happen" case imo, that's what error handling is for. Calling |
|
@ericvolp12 glad I took another pass, because I found a way to remove any This is a small memory overhead cost (tens of bytes per public or private key) for this one specific key type (P-256) to buy the simpler API. If we think we might keep millions of keys in memory I can update the API to return errors in more places instead. Sorry for all the go around on this... maybe i'm tilting at windmills trying to keep the API simple, but, eg, the underlying K-256 implementation has no error returns for any of these routines, and I strongly suspect the K-256 would never possibly actually error on these conditions either. |
|
How does the new size of a key compare to the previous size of a key in terms of %? Is it a doubling? I use a LRU cache often to stash keys after doing a DID doc lookup for JWT validation and I'm guessing others are doing something similar so keeping hundreds of thousands and/or millions of keys in memory isn't a wild idea necessarily, but things are gonna get too big anyhow to keep all in memory as user count grows so it might be fine. I'm fine with either approach you mentioned, the current one or returning more errors in places. Maybe Filo will have an opinion though. |
Based on double-checking the crypto/ecdsa stdlib interface, Public() often returns a crypto.Public, and PublicKey() is used to return specifically the package-local PublicKey type.
|
Didn't actually need the duplicate key representations for PublicKey, so got rid of that duplication and overhead. There is still the overhead (2x, I assume) for private keys. It seems less likely that folks would have millions of private keys hanging around in memory (compared to public keys). I am not strongly against just adding error to basically all the return types here to avoid any future issues and leave wiggle room, even though it doesn't seem necessary for now. |
|
I think having 2x overhead for private keys is fine. Caching a ton of private keys is a lot less common of a scenario than caching a ton of public keys imo. |
This borrows some code from https://github.com/whyrusleeping/go-did, but has a refactored API, exposes fewer internals, and has a couple differences:
crypto/x509with a wrapping formatLooking for API-level feedback, and thoughts on the safety/risk of the "low-S" hack code. We could consider asking external folks for API feedback as well, or just iterate on the API later.
If this looks good, I would imagine merging as-is, and then refactoring the code base to use this code instead of
go-didas a separate PR. One operational concern i'd want to address is the private key serialization format used in our infra: do we use P-256 private keys, or K-256 private keys, or actually no private keys at all yet?Can preview godoc HTML output here: https://static.bnewbold.net/tmp/atproto_crypto_godoc.html
cc: @whyrusleeping @ericvolp12