Secret store - initial version #4567

svyatonik · 2017-02-16T05:30:19Z

As proposed by @gavofyork :

first milestone is a RESTful API for getting keys encrypted with the requesting identity's public key
i.e. something like localhost:8082/<content_hash>/<request_signature> -> <encrypted_key>

Unfortunately, you'll be unable to test it, as there's no currently way to fill database with some contents. But if you fill it, it will work like that:

curl http://localhost:8082/0000000000000000000000000000000000000000000000000000000000000001/a199fb39e11eefb61c78a4074a53c0d4424600a3e74aad4fb9d93a26c30d067e1d4d29936de0c73f19827394a1dd049480a0d581aee7ae7546968da7d3d1c2fd01
8add166961bba5e20be6580109c1b59e1179b13ebc310ad024fffcc70d1fb379e230b33e7e842c3e2d12e4ad3ff9c866c471b054f8a0c69604c04de482a76f66e3254a13

PR outline:

added separate directory/package at the root of the project. I could move it somewhere else, but it is kinda similar to stratum (it also listens for some incoming requests) && stratum is also there. So maybe it is a good choice.
it was suggested that key server should be an ipc module. @NikVolf has told me that IPCs are turned off in current master (after I spend some time trying to beat compilation errors :) ). So now it will work in the same process. But there is some IPC stuff left, that I have already made. Idk - if it is better to remove or left it now.
I've added new compilation feature to parity (sstore), as I suppose most of parity users do not want to run key server. I do not know if it is a good idea ( @arkpar has told some time ago that you guys had some troubles with features). So I can remove it if you think it must be removed. For now, to build/test with sstore support:

cargo build --features sstore
cargo test --features store -p ethcore-secstore

finally - the 'core' key server implementation is just a hyper-based http server, listening for GET requests. Then recovering public key from passed signature, checking ACLs (now it has some dummy implementation, as it was not a part of first milestone), then searching for the key in sstore-database (I use util::Database to store mapping: content_hash -> encryption key), encrypting this key with recovered public && returning this encrypted key as text/plain.

gavofyork · 2017-02-16T08:22:51Z

as a minor point on naming, best to stick to one of "sstore", "secstore" and "secretstore" throughout the codebase. i like "secretstore".

different IPC module, even if it's in the same process for now, is nice because it forces clear interfaces.

svyatonik · 2017-02-16T08:41:56Z

@gavofyork thanks, will update PR

tomusdrw

Looks good to me, although I'm a bit concerned with the number of independent servers we're building into Parity:

IPFS
Secret Store
Dapps
UI
RPC

Do we really need separate ports for all these?
If possible I would be in favour of integrating some stuff or at least abstracting server implementations to make future migrations easier.
Currently we use different version of hyper (sync/async) and we have a lot of duplicated code (and we're gonna have more) - spinning up server, parsing URLs, parsing headers, adding security headers etc.

tomusdrw · 2017-02-16T09:19:55Z

Cargo.lock

+ "ethcore-util 1.6.0",
+ "ethcrypto 0.1.0",
+ "ethkey 0.2.0",
+ "hyper 0.9.14 (registry+https://github.com/rust-lang/crates.io-index)",


hyper is a bit heavy and slow, have you considered our async version or for instance tokio-minihttp?

Not actually :( By 'our async version' do you mean https://github.com/ethcore/hyper ? I'll take a look into it. tokio-minihttp sounds very good, as it has 'mini' in its name :) I'll check it out - thanks

agreed to use tokio-minihttp with @tomusdrw but left it for another PR, as now it doesn't have a way to be closed programatically. so first fix tokio-minihttp, then switch to it

tomusdrw · 2017-02-16T09:24:18Z

Cargo.toml

@@ -81,6 +82,7 @@ evm-debug = ["ethcore/evm-debug"]
 evm-debug-tests = ["ethcore/evm-debug-tests"]
 slow-blocks = ["ethcore/slow-blocks"]
 final = ["ethcore-util/final"]
+sstore = ["ethcore-secstore"]


I think it would be best to enable it by default (or build it on the CI), otherwise it's hard to detect any possible breaking changes causing compilation errors in that crate, since people don't alter the features and builds are not including this as well.

Maybe it will be better to enable it later? When this functionality will be finished && polished. Now it has only basic features && it is not usable at all. I don't mind fixing any compilation issues by myself until it is ready for production, if you're worried about it.

tomusdrw · 2017-02-16T09:29:23Z

secret_store/src/http_listener.rs

+
+impl<T> HttpHandler for KeyServerHttpHandler<T> where T: KeyServer + 'static {
+	fn handle(&self, req: HttpRequest, mut res: HttpResponse) {
+		if req.method != HttpMethod::Get {


Might be good to limit the requests depending on the planned usage:

If we're not intending to use this server from browser context we can deny requests with Origin header.

If we do intend to use it from the browser we need to add CORS support, cause currently the website will be able to perform a request, but won't be able to read the response.

I also do not yet fully understand final shape of this functionality - who will be responsible for IPFS/Swarm integration. Is it key server, or some other component. If that's a key server, then most probably it will have its own UI. Or maybe it is user himself && it is his duty to save document to IPFS and then upload key here (that's a strange decision, but it can be so). I think @gavofyork could help us here. Or I can just add TODO about your comment here && update it later.

@svyatonik Probably we could have a trait "DocumentStore" with a single function "fn fetch(H256) -> BoxFuture<Option>`, and this could be trivially implemented for any swarm/ipfs/disk/memory store. This server should be concerned solely with the logic above that.

as an aside, couldn't this be a separate executable?

@rphmeier Yes - we can go that way, but I'm not sure that this would be a part of this particular server/service :) Maybe user/external app will access this keyserver just for storing/retrieving keys && will upload/download files to this DocumentStore itself. Another option that this keyserver would be extended to something bigger && will have this functionality itself - then yes, I'll add something like DocumentStore here, thanks :)

It will need access to the blockchain data, so it can't be totally separate application. But it is intended to be separate executable in future (after IPC will be enabled) - yes.

You could actually use rust-web3 and existing IPC transport :)

Thanks, I like this idea :) So @gavofyork maybe create a completely separate executable for key-server and talk to Parity using RPC calls (it is enough for calling contract methods)? I think that's a great idea

@svyatonik Yes, the eth_call RPC is suitable for calling contracts. rust-web3 also has ethabi integration for easy usage of contracts, abstracting over any direct eth_call nastiness.

Left as is, until @gavofyork approves

IPC transport layer was designed to be a very efficient RPC interface, supporting efficient underlying transport paradigms.

this would allow adding new, potentially non-ethereum, functionality without compromising the integrity of the existing codebase. whether the functionality is added as a separately spawned instance of parity (as with sync and stratum) or a completely separate executable, isn't so important.

unless there's a particularly good reason to go with the eminently inefficient, inconcise, prehistoric and not-all-that-well designed JSONRPC, i would prefer to stick to using (and improving) our trait-based IPC module architecture.

i would lightly favour the same executable for now since it will make system deployment easier; that said, if you're desperate to have it in a different executable (rather than just a different crate/module) i'm not going to stand in the way.

ok, I don't feel myself confident enough to make such decisions. So let it be an IPC module (in future).

rphmeier · 2017-02-16T12:16:21Z

parity/sstore.rs

+
+	/// Noop key server implementation
+	pub struct KeyServer {
+	}


braced empty struct?

rphmeier · 2017-02-16T12:16:32Z

parity/sstore.rs

+	impl KeyServer {
+		/// Create new noop key server
+		pub fn new(_conf: Configuration, _deps: Dependencies) -> Result<Self, String> {
+			Ok(KeyServer {})


Ok(KeyServer)

svyatonik · 2017-02-16T14:39:39Z

tried to make KeyServer trait IPC-ready + renamed sstore + secstore to secretstore everywhere

arkpar · 2017-02-17T16:08:40Z

For this one I'd also suggest disabling it by default

gavofyork · 2017-02-20T15:13:18Z

@svyatonik will leave the disabled-by-default for a separate PR...

svyatonik · 2017-02-20T18:01:40Z

@gavofyork The feature is disabled by default from the beginning.

svyatonik added 4 commits February 15, 2017 14:35

initial secret store commit

b75467a

Merge branch 'master' into secret_store

2e191de

various fixes

b625310

license

13d65cf

svyatonik added the A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. label Feb 16, 2017

tomusdrw reviewed Feb 16, 2017

View reviewed changes

rphmeier added the M4-core ⛓ Core client code / Rust. label Feb 16, 2017

(sstore, secstore) -> secretstore

725354c

rphmeier reviewed Feb 16, 2017

View reviewed changes

svyatonik added 5 commits February 16, 2017 17:11

marked KeyServer trait as IPC-ready

515e482

fixed style

a9eed77

ignore requests with Origin header

75fe95d

fixed tests

3c96472

fixed Origin header check

61a597e

svyatonik added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Feb 16, 2017

Merge branch 'master' into secret_store

0eb95e5

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Feb 20, 2017

gavofyork merged commit b9665c7 into master Feb 20, 2017

gavofyork deleted the secret_store branch February 20, 2017 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Secret store - initial version #4567

Secret store - initial version #4567

svyatonik commented Feb 16, 2017

gavofyork commented Feb 16, 2017

svyatonik commented Feb 16, 2017

tomusdrw left a comment •

edited

Loading

tomusdrw Feb 16, 2017

svyatonik Feb 16, 2017

svyatonik Feb 16, 2017

tomusdrw Feb 16, 2017

svyatonik Feb 16, 2017

tomusdrw Feb 16, 2017

svyatonik Feb 16, 2017

rphmeier Feb 16, 2017

svyatonik Feb 16, 2017

tomusdrw Feb 16, 2017

svyatonik Feb 16, 2017

rphmeier Feb 16, 2017

svyatonik Feb 16, 2017

gavofyork Feb 17, 2017 •

edited

Loading

svyatonik Feb 17, 2017

rphmeier Feb 16, 2017

svyatonik Feb 16, 2017

rphmeier Feb 16, 2017

svyatonik Feb 16, 2017

svyatonik commented Feb 16, 2017

arkpar commented Feb 17, 2017

gavofyork commented Feb 20, 2017

svyatonik commented Feb 20, 2017

Secret store - initial version #4567

Secret store - initial version #4567

Conversation

svyatonik commented Feb 16, 2017

gavofyork commented Feb 16, 2017

svyatonik commented Feb 16, 2017

tomusdrw left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork Feb 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

svyatonik commented Feb 16, 2017

arkpar commented Feb 17, 2017

gavofyork commented Feb 20, 2017

svyatonik commented Feb 20, 2017

tomusdrw left a comment •

edited

Loading

gavofyork Feb 17, 2017 •

edited

Loading