Implementing SSRS – Substrate Simple Remote Signer protocol by gnunicorn · Pull Request #7365 · paritytech/substrate

gnunicorn · 2020-10-20T17:15:02Z

Implementation of the jsonrpc remote keystore over the http/ws protocol as the substrate keystore, including a separate example server implementation.

To Do:

docs
Metadata, License
hide behind non-default feature-flag
implement remaining (currently unused) functions on server

Note: test had ecdsa support, but was removed because of #7490

NikVolf · 2020-10-21T10:15:23Z

over the http/ws

Shouldn't it be https/wss? How do you secure the transport anyway?

gnunicorn · 2020-10-21T10:37:01Z

Shouldn't it be https/wss? How do you secure the transport anyway?

This is using regular jsonrpc_client_transports, which only support http and ws afaics.

NikVolf · 2020-10-21T12:41:17Z

Shouldn't it be https/wss? How do you secure the transport anyway?

This is using regular jsonrpc_client_transports, which only support http and ws afaics.

it is not a secure transport, especially for this task

gnunicorn · 2020-10-21T14:33:17Z

it is not a secure transport, especially for this task

That is correct, and no one said it was. The expected environment for this until now, was to have a secured network, like run in a VPN and firewalled sub-network and/or expect the connection to be tunneled through external means (like SSH).

I see though that the regular user might not realize they have to do that, so I could see that we might want hide that this behind a feature flag you'd have to explicitly activate until we have a secure-default-way of doing it instead. WDYT?

NikVolf · 2020-10-21T15:03:56Z

it is not a secure transport, especially for this task

That is correct, and no one said it was. The expected environment for this until now, was to have a secured network, like run in a VPN and firewalled sub-network and/or expect the connection to be tunneled through external means (like SSH).

I see though that the regular user might not realize they have to do that, so I could see that we might want hide that this behind a feature flag you'd have to explicitly activate until we have a secure-default-way of doing it instead. WDYT?

Signer will be custom software implemented anyway. So I don't think there is much sense in using non-secure transport at all, when there are secure alternatives (for example, noise). If it is properly used, there will be no need in tunnelling and stuff. Noise has implementation in many languages if that is what required - even in pure C.

JSON-RPC overhead need is also not clear to me, but I am not sure if this is critical. How much signing happens on validator node per second/minute?

gnunicorn · 2020-10-21T16:32:20Z

@NikVolf the goal was to get something out of the door quickly with the least developer overhead from our side to allow others to start experimenting with the workflow and use case – JSONRPC was all there and quick to get up and running, that's the only reason it was used. The way it stands, there are known security problems (like, the server still needs to have some knowledge of chain-info otherwise they can't securely sign VRFs, but that also means they have some connection to a chain or chain state, which requires internet to stay up to date) and performance issues (https and JSON being two things that could be replaced with more efficient things instead, but also that signing is mostly sync for in substrate still and the example and known others approaches also follow linear service patterns – like TEE can only do one signing at a time).

burdges · 2020-10-21T19:22:26Z

I've a stupid question: I'd think half the tools for doing JSON RPC should do TLS, no?

kirushik · 2020-11-11T11:05:04Z

After conversation with @gnunicorn I've removed the needsaudit flag; our current SSRS implementation is for experimentation only, hidden behind a feature flag and should under no circumstances be used in anything production-related.

@gnunicorn is extending the readme to explicitly state the above.

(This shouldn't stop us from raising and documenting security-related issues with it, though — it's just by far not in shape to sustain any professional attention yet, and probably never will in its current form.)

bkchr · 2020-11-11T11:14:41Z

After conversation with @gnunicorn I've removed the needsaudit flag; our current SSRS implementation is for experimentation only, hidden behind a feature flag and should under no circumstances be used in anything production-related.

@gnunicorn is extending the readme to explicitly state the above.

(This shouldn't stop us from raising and documenting security-related issues with it, though — it's just by far not in shape to sustain any professional attention yet, and probably never will in its current form.)

Maybe we should than put this into its own repo?

And what is the path we continue here? I thought this issue is soo pressing that we can deprecate sentries?

kirushik · 2020-11-11T11:19:08Z

And what is the path we continue here?

I must admit, I'm a bit confused here too.
I'm talking to @gnunicorn in private to better understand what should and what shouldn't go into our codebase/pass the review here.

gnunicorn · 2020-11-13T10:00:34Z

Converted this back to draft to more clearly signal:

we don't the intend to merge or ship this as-is
this is an example implementation showing how in the new API other remote-protocols can easily be integrated
this is a highly insecure-by-default-example and before any of it could be used, must be given appropriate documentation

Unfortunately, no other, more secure protocol has been proposed until now, which leaves us with a couple of path to go ahead, but not decided on any yet:

leave this as is and wait for other protocols to come around
replace the given transport with an at least TLS-wrapped one and probably some further hardening, like certificate pinning or off-protocol key-exchange-requirement – none of which is currently supported by the libraries used
provide a generic protocol-plugging-mechanism and move the specific implementation into a example/experimental-named repository to make clear this is more about showing the API then the specific protocol

burdges · 2020-11-13T10:53:13Z

I'd assumed remote signers were vaguely HSM-like with special hardware connections, like a second ethernet card or USB connection. Yes, I think an internet remote signer wants a secure handshake like TLS but with fixed transport keys and IP addresses for both sides, so no CAs or DNS, but even then..

Interestingly, if you look at Noise IK and KK then you'll observe the difference is sending the initiators long term secret inside the first flow, encrypted only by an ephemeral key exchange.

IK:
  <- s
  ...
  -> e, es, s, ss
  <- e, ee, se,

KK:
  -> s
  <- s
  ...
  -> e, es, ss
  <- e, ee, se,

As the initiator knows the respondents long-term secret, one could've a hybrid handshake between IK and KK in which foreign initiators reveal their identity, which permits whitelisting on the first flow, but special distinguished initiators hide their identity against an adversary who compromises the respondents' IP address but not the respondent machine. In other words, your remote signer can open connections that look exactly like normal node connections, but avoids immediately exposing your remote singer's IP address against BGP attacks, etc. This is probably useless since a signer's messages look different anyways, but technically the traffic observing attacker looks different form the BGP attacker.

redzsina · 2020-11-13T15:10:29Z

For now, we consider our review done for this PR - please refer to issue 21 in the internal tracker.

rakanalh · 2020-11-27T11:29:42Z

 	)
 >, ServiceError> {
+	if config.keystore_remotes.len() > 1 {
+		return Err(ServiceError::Other(


In this case, why is it remotes and not remote?

rakanalh · 2020-11-27T11:32:39Z

+	// FIXME: here would the concrete keystore been build,
+	//        must return a concrete type (NOT `LocalKeystore`) that
+	//        implements `CryptoStore` and `SyncCryptoStore`
+	Err("Remote Keystore not supported.")


Question: How would this be extended? do i have to fork substrate in this case? or is this crate node-template used to generate a new node implementation for the user?

rakanalh · 2020-11-27T11:37:59Z

+		})
+	}
+
+	/// Create a local keystore in memory.


This comment doesn't reflect what's happening inside the method

rakanalh · 2020-11-27T11:39:54Z

+
+/// A remote based keystore that is either memory-based or filesystem-based.
+pub struct RemoteKeystore {
+	client: RwLock<Option<Client>>,


Why would we want to read/write lock on a remote client? IMO, locking should only happen on the server side.

rakanalh · 2020-11-27T11:48:11Z

+	}
+}
+
+impl<Store: CryptoStore + 'static> Stream for KeystoreReceiver<Store> {


Wouldn't it be less verbose to have a method such as:

async fn run(mut self) { loop { request = self.receiver.next().await; self.process_request(request) } }

rakanalh · 2020-11-27T11:57:31Z

+
+	fn sr25519_public_keys(&self, id: KeyTypeId) -> BoxFuture<Vec<sr25519::Public>> {
+		let receiver = self.send_request(RequestMethod::Sr25519PublicKeys(id));
+		Box::new(receiver.map(|e| match e {


Nitpick: e here implies error but i think response here or result might better imply what the value is.

rakanalh · 2020-11-27T11:59:46Z

+impl crate::RemoteSignerApi for GenericRemoteSignerServer {
+
+	fn sr25519_public_keys(&self, id: KeyTypeId) -> BoxFuture<Vec<sr25519::Public>> {
+		let receiver = self.send_request(RequestMethod::Sr25519PublicKeys(id));


Follow up to my above suggestion to use async fn, here send_request could be replaced with a direct call to the client's sr25519_public_keys which we can return as a boxed future without .await.

rakanalh · 2020-11-27T12:03:09Z


-enum KeystoreContainerInner {
-	Local(Arc<LocalKeystore>)
+trait AsCryptoRef {


Nitpick: can this be renamed to AsCryptoStoreRef?

rakanalh · 2020-11-27T12:06:40Z

+
+struct DoubleWrap<T>(Arc<T>);
+
+impl<T> AsCryptoRef for DoubleWrap<T> where T: CryptoStore + SyncCryptoStore + 'static {


Question: What happens if we get rid of DoubleWrap and implement AsCryptoRef on Arc<T> directly? the below methods could return self.clone() in that case and the code would be a bit simpler.

I have modified this snippet you showed me earlier to do this:
https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=5d6a9deb6c38c00cab9d0198b95d690c

rakanalh · 2020-11-27T12:07:44Z

 /// Construct and hold different layers of Keystore wrappers
-pub struct KeystoreContainer(KeystoreContainerInner);
+pub struct KeystoreContainer {
+	remotes: Vec<Box<dyn AsCryptoRef>>,


If we plan to support multiple remotes, it would be nice to document the use case.

gnunicorn · 2020-11-30T10:38:53Z

superseeded by #7628 .

update to latest master

66fb709

github-actions Bot added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 20, 2020

updates on docs, license, meta

ea6abb0

gnunicorn changed the title ~~Remote keystore interface, Client & Server~~ Implementing SSRS – Substrate Simple Remote Signer protocol Oct 21, 2020

NikVolf added the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Oct 21, 2020

gnunicorn added 3 commits October 27, 2020 08:38

hide ssrs behind feature flag

790f00e

Merge remote-tracking branch 'origin/master' into ben-remote-signer

29a68ee

implement remaining functions on the server

1f292b5

gnunicorn marked this pull request as ready for review November 3, 2020 09:25

github-actions Bot added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Nov 3, 2020

gnunicorn added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Nov 3, 2020

gnunicorn added 6 commits November 3, 2020 10:27

sign server line length fix

7ad8521

fix tests

d310105

fixup in-memory-keystore

2ec7541

adding failsafe

fc9adfe

skipping ecdsa test for now

ac8fb13

Merge remote-tracking branch 'origin/master' into ben-remote-signer

95fb534

kirushik removed the D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited label Nov 11, 2020

gnunicorn marked this pull request as draft November 13, 2020 09:52

gnunicorn added 5 commits November 25, 2020 11:12

Merge remote-tracking branch 'origin/master' into ben-remote-signer

a9377a3

remote keystore param

610dce8

remote sign urls made available

0d58447

integrating keystore remotes features

a8ca9ba

don't forget the dependency

81f9356

rakanalh reviewed Nov 27, 2020

View reviewed changes

gnunicorn added 2 commits November 27, 2020 14:57

remove old cruft

418d893

Merge remote-tracking branch 'origin/master' into ben-remote-signer

24eede6

gnunicorn mentioned this pull request Nov 30, 2020

minor fix and improvements on localkeystore #7626

Merged

gnunicorn added 2 commits November 30, 2020 11:19

reset local keystore

df50b0d

applying suggestions

4651095

gnunicorn closed this Nov 30, 2020

bkchr deleted the ben-remote-signer branch November 30, 2020 12:37


		struct DoubleWrap<T>(Arc<T>);

		impl<T> AsCryptoRef for DoubleWrap<T> where T: CryptoStore + SyncCryptoStore + 'static {

Conversation

gnunicorn commented Oct 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NikVolf commented Oct 21, 2020

Uh oh!

gnunicorn commented Oct 21, 2020

Uh oh!

NikVolf commented Oct 21, 2020

Uh oh!

gnunicorn commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NikVolf commented Oct 21, 2020

Uh oh!

gnunicorn commented Oct 21, 2020

Uh oh!

burdges commented Oct 21, 2020

Uh oh!

kirushik commented Nov 11, 2020

Uh oh!

bkchr commented Nov 11, 2020

Uh oh!

kirushik commented Nov 11, 2020

Uh oh!

gnunicorn commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

burdges commented Nov 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

redzsina commented Nov 13, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnunicorn commented Nov 30, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

gnunicorn commented Oct 20, 2020 •

edited

Loading

gnunicorn commented Oct 21, 2020 •

edited

Loading

gnunicorn commented Nov 13, 2020 •

edited

Loading

burdges commented Nov 13, 2020 •

edited

Loading