Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

SecretStore: bunch of fixes and improvements #6168

Merged
merged 29 commits into from
Aug 16, 2017
Merged

SecretStore: bunch of fixes and improvements #6168

merged 29 commits into from
Aug 16, 2017

Conversation

svyatonik
Copy link
Collaborator

on top of #6146

  1. improved logging.
  2. fixed generation session failure due to network lags.
  3. cli option to disable SecretStore HTTP API, so that you can have less than N (in M-of-N scheme) external 'public' nodes, which process client requests. It also decreases startup time (which has been very important in testing) + frees network port in cases where it is not needed to have exactly N public nodes.
  4. cli option to disable ACL check. It is extremely useful in testing (previously I had to recompile parity with slightly changed source file to achieve the same effect). However, it could be a security hole, if misused (i.e. used in production). So please argue if you feel this is a bad option.

svyatonik added 24 commits July 14, 2017 13:41
@svyatonik
do not cache ACL storage contract
18582d7
@svyatonik
when error comes before initialization
5cc40d4
@svyatonik
initial KeyServerSet commit
81de7e1
@svyatonik
update_nodes_set in maintain
5080cc3
@svyatonik
do not connect to self
7664ff5
@svyatonik
fixed connection establishing
80b9e93
@svyatonik
removed println
9a9c4f6
@svyatonik
improved KeyServerSet tracing
a35db9f
@svyatonik
moved parsing to KeyServerSet
b9ad093
@svyatonik
re-read only when blockchain is changed
c94c799
@svyatonik
do not try to connect if not a part of cluster
023e5b4
@svyatonik
improved logging
5fb9652
@svyatonik
fixed tests
b31b067
@svyatonik
NodeKeyPAir trait
45f2b82
@svyatonik
fixed parity to use new trait
fb68b09
@svyatonik
continue integrating with parity
9e30d85
@svyatonik
updated parity for NodeKeyPair
2e9df2c
@svyatonik
completed KeyStoreNodeKeyPair
eb895fb
@svyatonik
removed comment
4938dfd
@svyatonik
improved logging
417a037
@svyatonik
fixed generation session lags
dcfb8c1
@svyatonik
improved logging
c466def
@svyatonik
cli option to disable SS HTTP API
7c05a90
@svyatonik
cli option to disable SS ACL check
c345bc3
@svyatonik svyatonik added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Jul 28, 2017
@svyatonik
Copy link
Collaborator Author

inprogress as 'parent' #6146 is also inprogress

@svyatonik svyatonik added the A0-pleasereview 🤓 Pull request needs code review. label Aug 9, 2017
@svyatonik svyatonik added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. A0-pleasereview 🤓 Pull request needs code review. labels Aug 9, 2017
debris
debris reviewed Aug 9, 2017
View reviewed changes
secret_store/src/key_server_cluster/signing_session.rs Outdated
@@ -572,7 +572,7 @@ mod tests {
use std::collections::{BTreeMap, VecDeque};
use ethkey::{self, Random, Generator, Public};
use util::H256;
use super::super::super::acl_storage::tests::DummyAclStorage;
use super::super::super::acl_storage::DummyAclStorage;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use acl_storage::DummyAclStorage; should work just fine

secret_store/src/key_server_cluster/decryption_session.rs Outdated
@@ -467,7 +467,7 @@ impl Ord for DecryptionSessionId {
mod tests {
use std::sync::Arc;
use std::collections::BTreeMap;
use super::super::super::acl_storage::tests::DummyAclStorage;
use super::super::super::acl_storage::DummyAclStorage;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use acl_storage::DummyAclStorage; should work just fine

secret_store/src/http_listener.rs Outdated
handler: shared_handler.clone(),
};
};*/
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should be removed

@debris
Copy link
Collaborator

debris commented Aug 9, 2017

cli option to disable ACL check. It is extremely useful in testing (previously I had to recompile parity with slightly changed source file to achieve the same effect). However, it could be a security hole, if misused (i.e. used in production). So please argue if you feel this is a bad option.

When starting parity with this option enabled, I think we should at least display warning (with bold yellow/red letters).

@svyatonik
Copy link
Collaborator Author

svyatonik commented Aug 9, 2017 •
edited
Loading

Thanks for suggestion, will update PR a little bit later

@svyatonik 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 Aug 9, 2017
@svyatonik
Copy link
Collaborator Author

Fixed style + added warning message

@debris debris added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 14, 2017
rphmeier
rphmeier reviewed Aug 16, 2017
View reviewed changes
secret_store/src/acl_storage.rs Outdated
@@ -47,6 +48,12 @@ struct CachedContract {
contract: Option<SecretStoreAclStorage>,
}

#[derive(Default, Debug)]
/// Dummy ACL storage implementation (check always passed).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: attributes under docs

@rphmeier rphmeier merged commit fefc756 into master Aug 16, 2017
@rphmeier rphmeier deleted the secretstore_stresstest branch August 16, 2017 12:20
@svyatonik svyatonik mentioned this pull request Aug 30, 2017
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rphmeier rphmeier rphmeier left review comments

@debris debris debris approved these changes

Assignees
No one assigned
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@svyatonik @debris @rphmeier