Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix] #2422: Hide private keys in configuration endpoint response #2435

Merged
merged 15 commits into from
Jul 25, 2022

Conversation

Erigara
Copy link
Contributor

@Erigara Erigara commented Jul 5, 2022

Description of the Change

  • Aggregate all configurations in iroha_config;
  • Add proc macro view! to generate configuration view (without private fields) and conversation Config->View;
  • Fix GetConfiguration endpoint to return configuration view without private keys.

Issue

Closses #2422.

Benefits

  • No sensitive information are leaked.

Possible Drawbacks

  • iroha_config depends on iroha_data_model.

Alternate Designs

Simpler solution would be to mask or hide private keys during serialization but this would affect generated reference configuration.

Proc-macro view! was decided to make function-like because it should be expanded first, so with function-like invocation it's made explicit.

Further work

  • As now all configuration located in single place we can move part of kagami functionality to generate reference configuration to build.rs;
  • Removeiroha_config dependency on iroha_data_model to be able to use Configuration in iroha_data_model and share config through query;
  • Remove view! assumption that configuration has Default implemented, it will be possible after Rethink Default configuration #2118.

@github-actions github-actions bot added the iroha2-dev The re-implementation of a BFT hyperledger in RUST label Jul 5, 2022
@Erigara Erigara added Bug Something isn't working macros labels Jul 5, 2022
@codecov
Copy link

codecov bot commented Jul 5, 2022

Codecov Report

Merging #2435 (5a28b8c) into iroha2-dev (a16d9c3) will decrease coverage by 0.46%.
The diff coverage is 67.45%.

@@              Coverage Diff               @@
##           iroha2-dev    #2435      +/-   ##
==============================================
- Coverage       67.61%   67.14%   -0.47%     
==============================================
  Files             140      155      +15     
  Lines           26173    27552    +1379     
==============================================
+ Hits            17696    18499     +803     
- Misses           8477     9053     +576     
Impacted Files Coverage Δ
actor/src/broker.rs 88.05% <ø> (ø)
cli/src/torii/mod.rs 28.88% <ø> (ø)
client/src/client.rs 45.33% <0.00%> (-0.99%) ⬇️
client_cli/src/main.rs 0.26% <ø> (ø)
config/base/src/runtime_upgrades.rs 50.57% <ø> (ø)
core/src/block.rs 70.00% <ø> (ø)
core/src/queue.rs 95.67% <ø> (-0.08%) ⬇️
core/src/smartcontracts/isi/asset.rs 54.82% <ø> (ø)
core/src/smartcontracts/isi/mod.rs 56.77% <ø> (+3.90%) ⬆️
core/src/smartcontracts/isi/triggers.rs 0.00% <0.00%> (ø)
... and 100 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@Erigara Erigara force-pushed the public-config branch 3 times, most recently from a770f2e to 3fcac59 Compare July 6, 2022 08:22
@SamHSmith SamHSmith self-assigned this Jul 7, 2022
cli/src/config.rs Outdated Show resolved Hide resolved
Comment on lines 10 to 36
pub struct Configuration {
/// Public key of this peer.
pub public_key: PublicKey,
/// Disable coloring of the backtrace and error report on panic.
pub disable_panic_terminal_colors: bool,
/// `Kura` related configuration.
pub kura: kura::Configuration,
/// `Sumeragi` related configuration.
pub sumeragi: sumeragi::Configuration,
/// `Torii` related configuration.
pub torii: torii::Configuration,
/// `BlockSynchronizer` configuration.
pub block_sync: block_sync::Configuration,
/// `Queue` configuration.
pub queue: queue::Configuration,
/// `Logger` configuration.
pub logger: logger::Configuration,
/// Configuration for `GenesisBlock`.
pub genesis: genesis::Configuration,
/// Configuration for `WorldStateView`.
pub wsv: wsv::Configuration,
/// Network configuration
pub network: network::Configuration,
/// Configuration for telemetry
#[cfg(feature = "telemetry")]
pub telemetry: telemetry::Configuration,
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought that such config structure duplication will be simplified with code generated by View macro. Why we can't use it for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Duplicating by hand is quite dangerous for future changes

Copy link
Contributor Author

@Erigara Erigara Jul 7, 2022

Choose a reason for hiding this comment

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

I agree that having almost the same structure in two places could be not the best option.

However i can't came up with idea how we can deal with nested structures where structure's view fields are themself views (data_mode::config::Configuration is example of such structure).

Any idea?

Copy link
Contributor

Choose a reason for hiding this comment

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

What about to annotate such fields view #[view(...)] attribute?

Copy link
Contributor

Choose a reason for hiding this comment

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

The view attribute sounds good, but it might be difficult to implement.

logger/src/config.rs Show resolved Hide resolved
config/src/lib.rs Outdated Show resolved Hide resolved
config/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines 96 to 97
/// let view = View { a: 32 };
/// let structure: Structure = view.into();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why we may need to cast view to the actual struct if it's missing some fields? I'd like to have only one way conversion: Config -> View

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This situation could arise when we send public configuration to other peer and peer what to create full configuration from public (bootstrap peers).

Copy link
Contributor

Choose a reason for hiding this comment

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

Does creating private keys with default() will help much here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then we can load keys from environment variable or other places.

Copy link
Contributor

@appetrosyan appetrosyan Jul 8, 2022

Choose a reason for hiding this comment

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

Careful! We did a lot of hard work to get rid of impl Default for *Key*, and we should avoid reintroducing that. This is a security risk.

Copy link
Contributor Author

@Erigara Erigara Jul 12, 2022

Choose a reason for hiding this comment

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

@appetrosyan, ok, i will make one way conversion Config -> View as suggested by @Arjentix to avoid relying on Default, backward conversation could be achieved be using proxy configuration objects.

config/derive/src/lib.rs Outdated Show resolved Hide resolved
@Arjentix
Copy link
Contributor

Arjentix commented Jul 7, 2022

Also why do we need View-like configs in the data_model? My suggestion is to remove it and generate View in place using derive macro.
If we really need them in the data_model then let's move basic Configurations in the data_model and generate View here

@Arjentix Arjentix self-assigned this Jul 7, 2022
@Erigara
Copy link
Contributor Author

Erigara commented Jul 7, 2022

Also why do we need View-like configs in the data_model?

If we want something to be queue result it must be in data_model.

My suggestion is to remove it and generate View in place using derive macro. If we really need them in the data_model then let's move basic Configurations in the data_model and generate View here

I'm not sure if it will be good thing to aggregate all configuration from different domains in one place.

@Arjentix
Copy link
Contributor

Arjentix commented Jul 7, 2022

I'm not sure if it will be good thing to aggregate all configuration from different domains in one place.

If we will split this then we can't generate view from config. Then with updating of config we have to remember to update view. And event compiler won't say anything, because if we forgot then it will use default() for this field

@Erigara
Copy link
Contributor Author

Erigara commented Jul 7, 2022

If we will split this then we can't generate view from config. Then with updating of config we have to remember to update view. And event compiler won't say anything, because if we forgot then it will use default() for this field.

It won't compile because without #[view(ignore)] it will try to assign this field to view structure and view doesn't have this field.

@Arjentix
Copy link
Contributor

Arjentix commented Jul 7, 2022

It won't compile because without #[view(ignore)] it will try to assign this field to view structure and view doesn't have this field.

Oh, okay, I've missed that

@Erigara
Copy link
Contributor Author

Erigara commented Jul 7, 2022

If we really need them in the data_model then let's move basic Configurations in the data_model and generate View here

Also i think defining all configuration in one place go against upcoming refactoring #2392.

IMO we should gather opinions from other team members.

SamHSmith
SamHSmith previously approved these changes Jul 7, 2022
Copy link
Contributor

@SamHSmith SamHSmith left a comment

Choose a reason for hiding this comment

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

Looks good to me.

config/derive/src/lib.rs Outdated Show resolved Hide resolved
config/src/lib.rs Outdated Show resolved Hide resolved
config/src/lib.rs Outdated Show resolved Hide resolved
config/src/lib.rs Outdated Show resolved Hide resolved
data_model/src/config.rs Outdated Show resolved Hide resolved
data_model/src/config.rs Outdated Show resolved Hide resolved
data_model/src/config.rs Outdated Show resolved Hide resolved
data_model/src/config.rs Outdated Show resolved Hide resolved
data_model/src/config.rs Outdated Show resolved Hide resolved
data_model/src/config.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@SamHSmith SamHSmith left a comment

Choose a reason for hiding this comment

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

You must rebase onto iroha2-dev. And you must squash all your commits into one fix commit. That commit should be called [fix] #2422: Hide private keys in configuration endpoint response.

@Erigara
Copy link
Contributor Author

Erigara commented Jul 25, 2022

You must rebase onto iroha2-dev. And you must squash all your commits into one fix commit. That commit should be called [fix] #2422: Hide private keys in configuration endpoint response.

i will squash commits on merge

Erigara and others added 15 commits July 25, 2022 15:38
Co-authored-by: Ekaterina Mekhnetsova <[email protected]>
Signed-off-by: Shanin Roman <[email protected]>
Signed-off-by: Shanin Roman <[email protected]>
Signed-off-by: Shanin Roman <[email protected]>
Signed-off-by: Shanin Roman <[email protected]>
@appetrosyan appetrosyan merged commit 0420529 into hyperledger-iroha:iroha2-dev Jul 25, 2022
@Erigara Erigara deleted the public-config branch July 25, 2022 13:37
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 6, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 6, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 6, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 7, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 8, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
mversic pushed a commit to mversic/iroha that referenced this pull request Sep 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working iroha2-dev The re-implementation of a BFT hyperledger in RUST macros
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants