Make `SignContent` struct or `Signable` trait public

[mgeisler]

Description of feature:

We would like to use the SignWithLabel and VerifyWithLabel functionality from Section 5.1.2 with our data.

Implementation discussion (Optional)

I did this internally with this simple code:

#[derive(Clone, MlsSize, MlsEncode)]
struct SignContent<'a> {
    #[mls_codec(with = "mls_rs_codec::byte_vec")]
    label: Vec<u8>,
    #[mls_codec(with = "mls_rs_codec::byte_vec")]
    content: &'a [u8],
}

impl fmt::Debug for SignContent<'_> {
    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
        f.debug_struct("SignContent")
            .field("label", &mls_rs_core::debug::pretty_bytes(&self.label))
            .field("content", &mls_rs_core::debug::pretty_bytes(self.content))
            .finish()
    }
}

impl SignContent<'_> {
    pub fn new<'a>(label: &'a str, content: &'a [u8]) -> SignContent<'a> {
        SignContent { label: [b"MLS 1.0 ", label.as_bytes()].concat(), content }
    }
}

This is a trimmed down version of the functionality used by the Signable trait.

Do you think we could make either the full trait public, or make a SignContent struct available?

Thanks for considering!

[mgeisler]

I made SignContent borrow the label since I found that I only need the struct to stay around for a split second in my use case: I create it and then immediately use it with a sign or a verify method.

[mulmarta]

Maybe expose let s = sign_with_label(label, bytes) and verify_with_label(label, bytes, s) would make a better API? Signable is a bit cumbersome and exposing SignContent means the application has to deal with signatures.

(The reason content is owned in SignContent is that Signable::signable_content typically runs mls_encode which creates a vec, an you can't return a reference to locally created variable which would be dropped.)

[mgeisler]

Maybe expose let s = sign_with_label(label, bytes) and verify_with_label(label, bytes, s) would make a better API?

Yes, that would be a nice and very usable API for me!

Signable is a bit cumbersome and exposing SignContent means the application has to deal with signatures.

(The reason content is owned in SignContent is that Signable::signable_content typically runs mls_encode which creates a vec, an you can't return a reference to locally created variable which would be dropped.)

I saw the Signable trait and I must confess that I'm not sure why this functionality needs so much machinery 😄 But if we can provide two simple functions for others, then I'm more than happy.

I'll put up a PR for this next.

[mgeisler]

@mulmarta, would you be up for having a new method on CipherSuiteProvider, with a default implementation:

    /// Sign `data` using `secret_key` and `label`.
    ///
    /// This implements [SignWithLabel][1]. The `label` can be used to
    /// distinguish between signatures on the same `data` made in
    /// different contexts.
    ///
    /// [1]: https://www.rfc-editor.org/rfc/rfc9420.html#name-signing
    async fn sign_with_label(
        &self,
        secret_key: &SignatureSecretKey,
        label: &str,
        data: &[u8],
    ) -> Result<Vec<u8>, Self::Error> {
        let sign_content = SignContent::new(label, data).mls_encode_to_vec()?;
        self.sign(&secret_key, &sign_content)
    }

The whole signer module could then be moved to mls-rs-core if we go this way.

Alternatively, I can expose a new top-level function in signer:

    /// Sign `data` using `secret_key` and `label`.
    ///
    /// This implements [SignWithLabel][1]. The `label` can be used to
    /// distinguish between signatures on the same `data` made in
    /// different contexts.
    ///
    /// [1]: https://www.rfc-editor.org/rfc/rfc9420.html#name-signing
    async fn sign_with_label<P: CipherSuiteProvider>(
        signature_provider: &P,
        secret_key: &SignatureSecretKey,
        label: &str,
        data: &[u8],
    ) -> Result<Vec<u8>, MlsError> {

Which approach do you like better? I think the second means moving less code around, so it might fit better with the current layering of mls-rs vs mls-rs-core.

[mulmarta]

Sorry @mgeisler we're a bit under water at the moment. I like approach 1 better, somehow sign-with-label conceptually fits more in the crypto provider part. You could use it without using MLS FWIW.

I don't know why Signable ended up a bit complicated, that's a very very old piece of code. If you have ideas how to simplify, help is welcome :-)

[mgeisler]

Sorry @mgeisler we're a bit under water at the moment.

No worries at all and happy new year! 🎉

I like approach 1 better, somehow sign-with-label conceptually fits more in the crypto provider part. You could use it without using MLS FWIW.

Alright, so we're talking about adding sign_with_label and verify_with_label methods to mls_rs_core::crypto::CipherSuiteProvider, right?

Since the Signable trait is in mls_rs, it seems like the easiest would be to move mls_rs/src/signer.rs to mls_rs_core.

However, there are two things to fix then:

• Signable uses the MlsError type from mls_rs. That could probably be easily changed by letting the trait return AnyError values instead.
• The unit tests depend on things like the load_test_case_json! macro, which in term depends on data files in mls_rs/test_data/. Moving or refactoring this seems like a bigger thing to do, especially because this test infrastructure is only enabled when #[cfg(test)] is set, so it's hard to reuse

I started looking into moving the things from mls_rs to mls_rs_core, but it seems complicated as explained above. Could I perhaps add top-level sign_with_label and verify_with_label functions to mls_rs as a start and then add a TODO to move them to mls_rs_core along with Signable at a later date?

I don't know why Signable ended up a bit complicated, that's a very very old piece of code. If you have ideas how to simplify, help is welcome :-)

Yeah, no clear ideas yet, sorry :-) Now that I've looked a bit more at the code, I see that different places need different contexts, so perhaps the current design is necessary, unfortunately.

[mgeisler]

Hi @mulmarta, do you have any more inputs here? It would be nice to get this merged before the 1.0 release is made.

[mulmarta]

Conceptually, I think Signable belongs to mls_rs_crypto_traits and these interop tests belong to mls_rs. We can have simple unit tests for Signable with a mocked-out CipherSuiteProvider and move signable tests to interop_test_vectors. What do you think?