feat(Signature): refactor and expand public-key signature API #170
Conversation
jhenahan
force-pushed
the
feat/new-signature-api
branch
4 times, most recently
from
4161b82 to
38a88a3
Compare
jhenahan
force-pushed
the
feat/new-signature-api
branch
from
38a88a3 to
55c20db
Compare
| -- | Signal your intent to encode secret key material for transmission | ||
| -- by wrapping a t'SecretKey' in t'UnsafeSecretKey'. | ||
| -- | ||
| -- @since 0.0.3.0 | ||
| newtype UnsafeSecretKey = UnsafeSecretKey SecretKey | ||
| deriving newtype | ||
| ( Eq | ||
| -- ^ @since 0.0.3.0 | ||
| -- Follows the t'SecretKey' instance. | ||
| ) | ||
| deriving | ||
| ( Display | ||
| -- ^ @since 0.0.3.0 | ||
| -- Hexadecimal-encoded bytes. | ||
| ) | ||
| via (ShowInstance UnsafeSecretKey) | ||
|
|
||
| -- | Hexadecimal-encoded bytes. | ||
| -- | ||
| -- @since 0.0.3.0 | ||
| instance Show UnsafeSecretKey where | ||
| show = ByteString.unpackChars . encodeSecretKeyHexByteString | ||
|
|
||
| -- | By lexicographical comparison of pointer contents. | ||
| -- | ||
| -- ⚠️ Vulnerable to timing attacks! | ||
| -- | ||
| -- @since 0.0.3.0 | ||
| instance Ord UnsafeSecretKey where | ||
| a `compare` b = | ||
| foreignPtrOrd (coerce a) (coerce b) cryptoSignSecretKeyBytes |
There was a problem hiding this comment.
This feels a bit over-engineered. encodeSecretKeyHexByteString ought to be a much simpler interface, no? I don't understand if there are invariants that are preserved by this type and its instances.
There was a problem hiding this comment.
It's intended as a guardrail to prevent you from doing things like serializing secret keys and using Ord in a way that's vulnerable to timing attacks without some kind of signal to readers that you're doing something that demands some extra attention.
There was a problem hiding this comment.
We do already have a (albeit low-tech) mechanism for signalling unsafe operations:
There was a problem hiding this comment.
I tend to prefer sticking such things behind a gate that's very slightly higher than that since unsafe is a very overloaded word. To my mind, it implies there's some safeSecretKeyToHexByteString waiting to be discovered, and there just isn't. Disjoint concepts <=> disjoint types. I'm not married to it, though, and with constant-time Ord coming quite soon I could be convinced the power-to-weight isn't there.
There was a problem hiding this comment.
That option is available in the present API as unsafeSecretKeyHexByteString, which just wraps whatever SecretKey in the newtype first.
There was a problem hiding this comment.
Pushed commits to strip that all out. Will update the API golden test once #171 lands and I rebase this PR on top of it.
jhenahan
force-pushed
the
feat/new-signature-api
branch
3 times, most recently
from
6fb7ca6 to
b19f0de
Compare
jhenahan
force-pushed
the
feat/new-signature-api
branch
4 times, most recently
from
f46f827 to
765c0de
Compare
Highlights: - Models input validation errors (pointer length and hex encoding) - Serde and rendering functions for `PublicKey`, `SecretKey`, and `SignedMessage` - Internals under `Scoped` - Uses constant-time `Eq` for `SecretKey`
For consistency, use `Foreign.copyBytes` everywhere that `memcpy` was once used.
jhenahan
force-pushed
the
feat/new-signature-api
branch
from
765c0de to
0501798
Compare
jhenahan
force-pushed
the
feat/new-signature-api
branch
from
0501798 to
53c2a55
Compare
2 participants
Highlights:
Models input validation errors (pointer length and hex encoding)
Serde and rendering functions for
PublicKey,SecretKey, andSignedMessageInternals under
ScopedUses constant-time
EqandOrdforSecretKeyFor consistency, use
Foreign.copyByteseverywhere thatmemcpywas once used