Add secp256r1 support

[jayz22]

What

Add support for secp256r1 signature verification (picking up env changes stellar/rs-soroban-env#1376).

And adapts the existing Crypto module by split the cryptographic functions into two sets:

• Crypto: standard, recommended set of cryptographic functions. This includes secp256k1_recover and secp256r1_verify taking the full message: Bytes as input and performs hashing underneath.
• CryptoHazmat: hazardous material. Contains low-level, unsecure if used incorrectly functions including secp256k1_recover_prehash and secp256r1_verify_prehash, taking a message_hash: BytesN<32> as input.

Design rationales were discussed in the env PR (started on stellar/rs-soroban-env#1376 (comment) and onward).

XDR changes associated with the new contract spec: stellar/stellar-xdr#182, stellar/stellar-xdr#183
rs-xdr: stellar/rs-stellar-xdr#361

End-to-end with secp256r1 account contract: stellar/rs-soroban-env#1402

Why

[TODO: Why this change is being made. Include any context required to understand the why.]

Known limitations

[TODO or N/A]

[leighmcculloch]

Continuing the conversation from stellar/rs-soroban-env#1376 (comment):

Could we change up the API a little so that the flow is something like the below? It is inspired by the ecdsa crate, and reduces the number of functions to maintain to two:

Change the sha256 function to return a digest type, I've named that type Hash here for consistency with our existing vocab and XDR, but we could name it Digest.

The Hash type is only creatable via a env.crypto() hash function.

Provide a non-hazmat verify prehash function that accepts a Hash.

env.crypto().sha256(bytes: &Bytes) -> Hash<32>

env.crypto().secp256r1_verify(public_key, message_hash: &Hash<32>, signature)

env.crypto_hazmat().secp256r1_verify(public_key, message_hash: &BytesN<32>, signature)

pub struct Hash<const N: usize>(BytesN<N>);

impl<const N: usize> Into<BytesN<N>> for Hash<N> { ... }
impl<const N: usize> Into<[u8; N]> for Hash<N> { ... }

The above is pretty similar with how the ecdsa crate presents it's functionality on the VerifyingKey type. The type exposes three interfaces, where:

• the first is a verify function with built in hash (not provided in the API above because of its duplicity with the below)
• the second allows for prehash verifying of a trusted object (same safety as prior, and just a different way to present hash type choice in the API without has having to figure out how to extend that later)
• and the last allows prehash verifying of anything and is hazmat

The reason I suggest leaving out the first interface with built-in hashing is:

• A common use case for us for verifying signatures in custom auth accounts will already be hashed, and I don't think we benefit from folks accidentally rehashing the signature payload.
• The second interface is just as safe, and presents the same capability in a slightly different way.
• The second interface is easier to extend over time without introducing more functions or hash type enums.

Instead of making a specialised custom auth object (mentioned in stellar/rs-soroban-env#1376 (comment)) to encompass the hash, the custom auth interface in the SDK can use the Hash type:

pub trait CustomAccountInterface {
    type Signature;
    type Error: Into<Error>;
    fn __check_auth(
        env: Env,
        signature_payload_hash: Hash<32>,
        signatures: Self::Signature,
        auth_contexts: Vec<Context>
    ) -> Result<(), Self::Error>;
}

[jayz22]

@leighmcculloch Thanks for your input! I like your suggestion of a wrapper Hash type that can only be created from a hash method (to be easily extended to other hash methods), and making the CustomAccountInterface take it. This make the code maintenance area smaller.
Only question I have is, is it fully compatible with env? I.e. when auth calls it with a Val containing a BytesObject, will it be compatible to Hash<32> type? I think that'll require some env type change to make it work, as you suggested in the other thread?
Either way, I'm gonna give it a shot.

[leighmcculloch]

That's a good point. The way to make this the most seamless would be to make a change to the env. By either making a HashObject its own value type, or by adding annotations (flags) to BytesObject to signal that it is a hash.

I understand that we don't want to take on a change that further modifies the env right now.

A less perfect approach in the SDK would be to create the Hash type there, and hand craft the necessary conversion types for converting to and from Val, via Bytes internally. But as much as we try it would be possible for someone to misuse the type, because the type would have to be convertible from Val, so someone could convert from Bytes to Val to Hash. We might have somethings to do with contract specs to make it work, but I'm not sure on the exact changes required there without looking at the macros again.

So it wouldn't be perfect. Be good if we have time to experiment with it a little. We could start by adding the hazmat API only, merge that, then experiment with the ideas for the safe API.

[leighmcculloch]

@graydon Is there a way to privately implement a publicly accessible trait on a publicly accessible type? So that while the type does implement the trait, the only person who can see that or call it is an internal? I suspect no, but that there might be some type trickery we can do.

[jayz22]

Is there a way to privately implement a publicly accessible trait on a publicly accessible type?

Pretty sure it can work. Implementing a foreign trait for an owned type should work.
I will experiment it a bit and get back.

[jayz22]

Is there a way to privately implement a publicly accessible trait on a publicly accessible type?

Pretty sure it can work. Implementing a foreign trait for an owned type should work. I will experiment it a bit and get back.

Yep, everything works!
With one caveat, the newly declared type, which goes into the CustomAccountInterface, cannot be generic because our type-mapping macro doesn't support it.
Given the host aren't supporting any hash length other than 32 bytes, I think it is totally fine to make it non-generic. I've named the type Digest(BytesN<32>) (instead of Hash, to avoid confusion with the existing xdr::Hash type).
I've committed the changes in the draft, still got a few touch-ups to do before marking it ready for review. In the meanwhile please feel free to take a look, and let me know if you have any feedback.

[leighmcculloch]

I've named the type Digest(BytesN<32>) (instead of Hash, to avoid confusion with the existing xdr::Hash type).

I think it's more confusing to introduce a second term for our APIs that have a name in use already. I understand the possibility for conflict, but we also use this name reuse pattern in other places. For example, there's a Timepoint type and a xdr::Timepoint type.

[leighmcculloch]

Couple comments inline.

In regards to the generic issue, it should be possible to use generics in SDK types at the boundaries, so once you add it as an SDK type, it should just work. For example: https://github.com/stellar/stellar-xdr/blob/59062438237d5f77fd6feb060b76288e88b7e222/Stellar-contract-spec.x#L50-L53

[jayz22]

I've named the type Digest(BytesN<32>) (instead of Hash, to avoid confusion with the existing xdr::Hash type).

I think it's more confusing to introduce a second term for our APIs that have a name in use already.

Got it, I have renamed it back to Hash. (I also thought about HashN like BytesN but thought it is redundant here, since there is another Bytes type).

In regards to the generic issue, it should be possible to use generics in SDK types at the boundaries, so once you add it as an SDK type, it should just work.

Yes it works exactly as you described. I've added the Hash type as a first-class contract spec type. The contract is able to define it as its argument type no problem. I've also attached the env end-to-end test PR above.

This PR should be feature ready. I will mark it ready-for-review so after the dependencies have been merged.

[leighmcculloch]

I think this looks good. The last thing I'm thinking is how do we prevent someone from using this type as a function argument on a general function.

[jayz22]

I think this looks good. The last thing I'm thinking is how do we prevent someone from using this type as a function argument on a general function.

I think it is sufficient to make the Hash type unconstructable from bytes, i.e. the only way to use it is either output of a secure hash function provided by the sdk, or returned from the host (via TryFromVal conversion).
There is always the escape hatch of CryptoHazmat if the user wants to directly construct the input from bytes.

I've made the fix, rebased and it should be ready for review.

[jayz22]

I think this looks good. The last thing I'm thinking is how do we prevent someone from using this type as a function argument on a general function.

I think it is sufficient to make the Hash type unconstructable from bytes, i.e. the only way to use it is either output of a secure hash function provided by the sdk, or returned from the host (via TryFromVal conversion). There is always the escape hatch of CryptoHazmat if the user wants to directly construct the input from bytes.

I've made the fix, rebased and it should be ready for review.

Actually, I realize it is still possible for a contract to pass another contract a Hash, via BytesN->Val->Hash path, which is what you have been suggesting.

I've add further restrictions (in the last two commits) to not allow it being used in public user methods and UDTs. This should hopefully be enough to present it from being misused.

[leighmcculloch]

@jayz22 I pushed a few commits that address the comments I left at #1246 (comment) and #1246 (comment) that were largely a result of my poor suggestion previously about adding it to the spec. The changes are:

• disallow hash in contract types
• disallow hash in container types (Vec, Map, etc)
• map hash in allowed cases to BytesN in the spec instead of the Hash type
• allow hash only as first argument of __check_auth

If you disagree with the changes let's chat tomorrow, or you're welcome to unwind any.

[jayz22]

@leighmcculloch Thank you for the fixes. I think they make sense. It is good to make the type restricted by making the Hash type purely "annotation" for byte in special scenarios (hash functions, ecdsa, and first input of the __check_auth). I've added a comment to make it more clear that the type itself does not provide any security guarantee, those guarantees only come from the places it is used in, and it cannot be used as user types. I hope that makes it more clear.

[leighmcculloch]

There's still a place that Hash can be used that isn't safe, where you can store a BytesN<32> and then get it back out of storage as a Hash<32>.

[leighmcculloch]

I pushed some changes that make it so that Hash cannot be created via TryFromVal. To do this I made a new trait specifically for creating values from contract invocations. Hash is still convertible to Val.

[leighmcculloch]

Tagging another couple folks for review now that I wrote code in this PR.