Non-Recoverable Signatures #58
Conversation
|
Any other asn1 tools that you considered? vs https://github.com/eerimoq/asn1tools/network/dependents Also, pyasn1 doesn't have any sub-dependencies, where asn1tools has a few. Normally, I'm relatively liberal with dependencies, but since this is an absolutely security-critical library, it would be nice to minimize the dependency risk. |
|
|
|
Nice, I didn't know of the dependency graph feature. Now that I've seen it, I agree that pyasn1 is the much better choice. Will swap them. |
|
Can I ask whether it is possible to do this without the added dependency? Would it be possible to do it using pure coincurve and then use asn1 in our tests to verify correctness?
Sent from ProtonMail mobile
…-------- Original Message --------
On May 23, 2019, 9:23 AM, jannikluhn wrote:
Nice, I didn't know of the dependency graph feature. Now that I've seen it, I agree that pyasn1 is the much better choice. Will swap them.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, [view it on GitHub](#58?email_source=notifications&email_token=AAGJHAUFUWZGOSX4C6TEDA3PWZA5LA5CNFSM4HL4TPN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWBJ3ZI#issuecomment-495099365), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AAGJHAQC5B3QN7DNLZ7CP2LPWZA5LANCNFSM4HL4TPNQ).
|
|
I didn't dare to try as DER encoding is relatively complicated even if implemented just for the signature object. It's ancient and weird, in some cases even requires checking individual bit values. Here's documentation of the parts that would be relevant to us: Sequence and Integer. So I think using an external library is the better way to go. But if you feel strongly about this, I can also try to reimplement it here. |
|
Looks pretty doable, since we know a lot about the specific spec and the input range. import pytest
import asn1tools
from eth_utils import (
apply_to_return_value,
int_to_big_endian,
)
from hypothesis import (
example,
settings,
strategies as st,
given,
)
ASN1_ECDSA_SPEC_STRING = """\
ECDSASpec DEFINITIONS ::= BEGIN
ECDSASignature ::= SEQUENCE {
r INTEGER,
s INTEGER
}
END
"""
ASN1_SPEC = asn1tools.compile_string(ASN1_ECDSA_SPEC_STRING, "der")
@apply_to_return_value(bytes)
def int_encoder(primitive):
"""
Only handles 32-byte positive ints
"""
# Integer tag
yield 0x02
encoded = int_to_big_endian(primitive)
if encoded[0] >= 128:
# Indicate that integer is positive, if the first bit is 1
# we need to disambiguate it from a 2's complement negative number
yield len(encoded) + 1
yield 0x00
else:
yield len(encoded)
yield from encoded
@apply_to_return_value(bytes)
def two_int_sequence_encoder(int1, int2):
# Sequence tag
yield 0x30
encoded1 = int_encoder(int1)
encoded2 = int_encoder(int2)
# Sequence length
yield len(encoded1) + len(encoded2)
yield from encoded1
yield from encoded2
MAX_32_BYTE_INT = 256 ** 32 - 1
@given(
st.integers(min_value=0, max_value=MAX_32_BYTE_INT),
st.integers(min_value=0, max_value=MAX_32_BYTE_INT),
)
@example(0, 0)
@example(MAX_32_BYTE_INT, MAX_32_BYTE_INT)
@example(MAX_32_BYTE_INT // 2, MAX_32_BYTE_INT // 2)
@example(MAX_32_BYTE_INT // 2 + 1, MAX_32_BYTE_INT // 2 + 1)
@settings(max_examples=1000)
def test_hand_coded_asn1(r, s):
encoded = two_int_sequence_encoder(r, s)
expected = ASN1_SPEC.encode("ECDSASignature", {"r": r, "s": s})
assert encoded == bytes(expected) (though we should re-write the tests to work against |
|
If you like, I can open a separate PR to add the internal asn1 util, since it looks like I'm at least 40% of the way there (need to add the decoder, and to switch to the other library) |
|
Maybe worth having both so we can compare.
Sent from ProtonMail mobile
…-------- Original Message --------
On May 24, 2019, 7:07 PM, Jason Carver wrote:
If you like, I can open a separate PR to add the internal asn1 util, since it looks like I'm at least 40% of the way there (need to add the decoder, and to switch to the other library)
—
You are receiving this because you commented.
Reply to this email directly, [view it on GitHub](#58?email_source=notifications&email_token=AAGJHAXYHJQW7TPSHKPOOWDPXAODTA5CNFSM4HL4TPN2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODWF7SEA#issuecomment-495712528), or [mute the thread](https://github.com/notifications/unsubscribe-auth/AAGJHASFGO32Q2I7EEZJ4KDPXAODTANCNFSM4HL4TPNQ).
|
|
Okay, the decoder is also straightforward: import pytest
import asn1tools
from eth_utils import (
apply_to_return_value,
big_endian_to_int,
int_to_big_endian,
)
from hypothesis import (
example,
settings,
strategies as st,
given,
)
ASN1_ECDSA_SPEC_STRING = """\
ECDSASpec DEFINITIONS ::= BEGIN
ECDSASignature ::= SEQUENCE {
r INTEGER,
s INTEGER
}
END
"""
ASN1_SPEC = asn1tools.compile_string(ASN1_ECDSA_SPEC_STRING, "der")
@apply_to_return_value(bytes)
def int_encoder(primitive):
"""
Only handles 32-byte (yes *byte*) positive ints
"""
# Integer tag
yield 0x02
encoded = int_to_big_endian(primitive)
if encoded[0] >= 128:
# Indicate that integer is positive (it always is, but doesn't always need the flag)
yield len(encoded) + 1
yield 0x00
else:
yield len(encoded)
yield from encoded
@apply_to_return_value(bytes)
def two_int_sequence_encoder(int1, int2):
# Sequence tag
yield 0x30
encoded1 = int_encoder(int1)
encoded2 = int_encoder(int2)
# Sequence length
yield len(encoded1) + len(encoded2)
yield from encoded1
yield from encoded2
def decode_int(encoded):
assert encoded[0] == 0x02
length = encoded[1]
# to_int can handle leading zeros
decoded_int = big_endian_to_int(encoded[2:2 + length])
return decoded_int, encoded[2 + length:]
def two_int_sequence_decoder(encoded):
assert encoded[0] == 0x30
# skip sequence length
int1, rest = decode_int(encoded[2:])
int2, empty = decode_int(rest)
assert len(empty) == 0
return int1, int2
MAX_32_BYTE_INT = 256 ** 32 - 1
@given(
st.integers(min_value=0, max_value=MAX_32_BYTE_INT),
st.integers(min_value=0, max_value=MAX_32_BYTE_INT),
)
@example(0, 0)
@example(MAX_32_BYTE_INT, MAX_32_BYTE_INT)
@example(MAX_32_BYTE_INT // 2, MAX_32_BYTE_INT // 2)
@example(MAX_32_BYTE_INT // 2 + 1, MAX_32_BYTE_INT // 2 + 1)
@settings(max_examples=1000)
def test_hand_coded_asn1(r, s):
encoded = two_int_sequence_encoder(r, s)
expected = ASN1_SPEC.encode("ECDSASignature", {"r": r, "s": s})
assert encoded == bytes(expected)
MAX_32_BYTE_INT = 256 ** 32 - 1
@given(
st.integers(min_value=0, max_value=MAX_32_BYTE_INT),
st.integers(min_value=0, max_value=MAX_32_BYTE_INT),
)
@example(0, 0)
@example(MAX_32_BYTE_INT, MAX_32_BYTE_INT)
@example(MAX_32_BYTE_INT // 2, MAX_32_BYTE_INT // 2)
@example(MAX_32_BYTE_INT // 2 + 1, MAX_32_BYTE_INT // 2 + 1)
@settings(max_examples=1000)
def test_asn1tools_encode_handrolled_decode(r, s):
encoded = ASN1_SPEC.encode("ECDSASignature", {"r": r, "s": s})
end_r, end_s = two_int_sequence_decoder(encoded)
assert (end_r, end_s) == (r, s)If I don't hear back, I'll pick this up on Tuesday into its own PR. |
|
Nice, thank you! That's indeed more straightforward than I expected. I've started to integrate your code, but now that you've written the decoder as well I think it would fit better in its own PR. |
|
Removed the asn1tools dependency, in anticipation of @carver's PR. I just guessed an interface from the snippet above for now, but don't mind updating it if it turns out to be different. |
|
Ok, can rebase on #60 now |
|
I've updated this PR to use new DER codec, cleaned up the commit history a little, and removed some direction duplication in the tests that carver already pointed out but slipped through for some reason. So it's ready from my side, could I get a final review so that we can merge this please? |
| @@ -60,7 +72,7 @@ def test_recover_from_public_key_class(key_api, private_key): | |||
|
|
|||
|
|
|||
| def test_verify_from_public_key_obj(key_api, private_key): | |||
| signature = key_api.ecdsa_sign(MSGHASH, private_key) | |||
| signature = key_api.ecdsa_sign_non_recoverable(MSGHASH, private_key) | |||
There was a problem hiding this comment.
Maybe this one and test_recover_from_public_key_class should both be parametrized with [key_api.ecdsa_sign, key_api.ecdsa_sign_non_recoverable] se we can test against both kinds of signatures?
eth_keys/datatypes.py
Outdated
| def s(self, value: int) -> None: | ||
| validate_integer(value) | ||
| validate_gte(value, 0) | ||
| validate_lt_secpk1n(value) |
There was a problem hiding this comment.
This move out of BaseSignature confused me on the first read-through. I thought the validations were missing from the non-recoverable. Now I see that the validations are duplicated, once in the BaseSignature init and once here. (I suppose this is because we want NonRecoverableSignature to be immutable, which seems like a nice direction).
The problem with the duplication that it would be very easy to update validation in one place but not the other. Maybe we can add a validate_? utility method (not sure what to call the union of r and s). Could maybe be an unbound utility method, or a staticmethod of BaseSignature I suppose. Then the setter here could just call validate_rs(value). BaseSignature init would use it also.
| @@ -381,6 +340,15 @@ def __getitem__(self, index: int) -> int: # type: ignore | |||
| def __repr__(self) -> str: | |||
| return "'{0}'".format(self.to_hex()) | |||
|
|
|||
| def __index__(self) -> int: | |||
| return self.__int__() | |||
There was a problem hiding this comment.
Hah, interesting, when would we ever be using a signature as an index for a slice?
There was a problem hiding this comment.
@carver see: https://docs.python.org/3/library/operator.html#operator.__index__
It's part the python API for interpreting values as integers.
There was a problem hiding this comment.
Yeah, I was basing the comment on this note: https://docs.python.org/2.5/whatsnew/pep-357.html
So AFAICT, obj.__index__() is only called when you do something like mylist[obj:], and I couldn't imagine that being a common thing to do with a signature. :) I don't really have a problem with it, was just surprised that it was included.
There was a problem hiding this comment.
I think it was just a blind inclusion by myself a long time ago. @jannikluhn can you try dropping this method and see if anything in the tests breaks?
There was a problem hiding this comment.
Oooh, I missed that it was there before.
There was a problem hiding this comment.
Removing it breaks this test: https://github.com/ethereum/eth-keys/blob/master/tests/core/test_key_and_signature_datastructures.py#L115
Looks like __index__ is used for hex, instead of __hex__. To me this was surprising. but apparently __hex__ got removed with python 3 (I can't find it in the docs anymore at least).
In my opinion, hex support should probably be removed altogether as it doesn't guarantee a length, so hex(signature) is not always the same as signature.to_hex(). But maybe in a separate PR?
There was a problem hiding this comment.
But maybe in a separate PR?
👍
Can you open an issue for this.
There was a problem hiding this comment.
seconding @carver comments about duplication of validation logic.
eth_keys/backends/native/main.py
Outdated
| msg_hash: bytes, | ||
| private_key: PrivateKey) -> NonRecoverableSignature: | ||
| signature_vrs = ecdsa_raw_sign(msg_hash, private_key.to_bytes()) | ||
| signature_rs = signature_vrs[1:] |
There was a problem hiding this comment.
I'd like this to be precise, us knowing that signature_rs is exactly two values, which this currently allows for it to be anything from empty to many values.
signature_vrs[1], signature_vrs[2]signature_v, signature_r = signature_vrs[1:]
| @@ -381,6 +340,15 @@ def __getitem__(self, index: int) -> int: # type: ignore | |||
| def __repr__(self) -> str: | |||
| return "'{0}'".format(self.to_hex()) | |||
|
|
|||
| def __index__(self) -> int: | |||
| return self.__int__() | |||
There was a problem hiding this comment.
@carver see: https://docs.python.org/3/library/operator.html#operator.__index__
It's part the python API for interpreting values as integers.
|
@jannikluhn let me know when you're read for us to click merge (or you can if you have permission but I don't think you do) |
|
Thanks, you're right, I don't have permission. It's ready from my side, as long as you're happy with the name Also, would be great if you could make a new release so that we can use it in Trinity. |
|
gonna wait for @carver to second this, then I'll merge and cut you a release. |
|
@jannikluhn available as |
Update template
What was wrong?
#57
How was it fixed?
Signatures without
vvalues are implemented in a new class calledNonRecoverableSignature. Some of the shared functionality with the existingSignatureclass has been moved toBaseSignature. Creating non recoverable signatures is handled by new backend methods. Verifying them uses the existing ones, which have been updated to accept allBaseSignatures(i.e. not recover and compare, but direct verification).Coincurve uses a DER format to encode/decode these signatures. As it is quite complicated I added a new dependency: asn1tools which looks relatively well maintained.
This should probably be merged after #56, then I can use the improved validation functions from there.
Cute Animal Picture