Signing context may not be selected at runtime.

[wilfred-centrality]

In an ideal world there would be sign and verify methods able to take a signing context from JS at runtime.

We've been scratching our heads over the current situation. The context is fixed to substrate, and the requirement seems to be that any signing context is defined at build time. Besides forking this repo and publishing a package with the signing contexts cennznet needs, are there any other methods that might work for us?

The underlying cause of the issue is a static lifetime requirement on the signing context that comes from the merlin crate, which is used by w3f's schnorrkel.

[Mischala]

I have run into this issue when creating a wrapper for Schnorrkel in Python.
In that too, it is impossible to change the signing context, which kinda ruins this library's use as a multipurpose solution.

[burdges]

Interesting, we could submit a pull request to merlin, not sure if they'd accept it, but hey.. dalek-cryptography/merlin#44

It's not afaik necessarily undefined behavior for foreign code to pass rust a &'static [u8], just even less safe than the normal FFI boundary. We all know merlin consumes its instantly, and even as a &'a [u8], so passing a reference works and can even live long enough.

Rust's Box<[u8]>: 'static and ditto Vec<u8> but you can only obtain the &'static [u8] safely with leak::<'static>, so you could reallocate the context temporarily and either (a) you initialize only a few signing contexts on startup, which risks memory leaks, (b) you depend upon leak being a wrapper around into_raw(), or (c) you write leak yourself using into_raw. I think (c) is preferable, but maybe just passing the reference is acceptable too.

You could maybe manage some static mut UGLY: HashSet<&'static [u8]> = HashSet::new() that you populate with Box::leak, but ugh..

We could make signing_transcript call Transcript::new("") and then append_label, which avoids the problem without altering merlin.

There is a separate concern that merlin/strobe might not be the fastest hashing library, which sucks if people call SingingContext::bytes. I never quite got around to doing the benchmarks, but we always supported any hash function with an XoF, which currently means Shake128, but Blake2x works too if anyone implements it RustCrypto/hashes#83

Anyways..

There are a bunch of breaking changes in schnorrkel now, so we should resolve this hashing question before the schnorrkel audit finishes and substrate updates.

[burdges]

We cannot expect merlin to be changed, so we should take another route, either abusing leaking, using empty labels, or just dumping or forking merlin. I like both merlin and strobe, but we cannot reject dynamic langues, so..

We should benchmark merlin vs. shake128 for our use case I think. And thanks for bringing this up in a timely fashion!

[wilfred-centrality]

No worries, and thanks. Let me know if there's anything we can do in the meantime. Somewhat time critical for us as well so hoping for speedy resolution.

[burdges]

I've taken the meaningless labels approach while adding our own domain separation in w3f/schnorrkel@fa0a6f7 We should still benchmark merlin vs shake128 though. I also removed merlin from our bls crate where it added nothing.

[wilfred-centrality]

Wonderful news, in that case we're close to a solution. The verify methods still have a static lifetime requirement, for example: https://github.com/w3f/schnorrkel/blob/fee6fdf0b5fd087d9531825c7165abe29f2fe21d/src/sign.rs#L213

[wilfred-centrality]

Please review w3f/schnorrkel#40

[burdges]

Merged. We require 0.6.1 now since I got overzealous and yanked 0.6.0

[burdges]

We've an audit starting this week, so expect a breaking 0.7 before August.

[wilfred-centrality]

Cheers Jeff, will test in the morning and close if resolved. Appreciate the heads up re 0.7.

…

________________________________ From: Jeff Burdges <[email protected]> Sent: Monday, July 8, 2019 8:04:44 PM To: paritytech/schnorrkel-js Cc: Wilfred Godfrey; Author Subject: Re: [paritytech/schnorrkel-js] Signing context may not be selected at runtime. (#12) We've an audit starting this week, so expect a breaking 0.7 before August. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub<#12?email_source=notifications&email_token=AKH72S5QFZ3URW7PZECXL7LP6LYJZA5CNFSM4H4XDLV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZMJSNY#issuecomment-509122871>, or mute the thread<https://github.com/notifications/unsubscribe-auth/AKH72SYFIMC4PG6FW66GPGDP6LYJZANCNFSM4H4XDLVQ>.

[wilfred-centrality]

Please review #13