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

[ADR] 0004 - Separate Transport and Application Keys #35

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

bryanchriswhite
Copy link
Contributor

@bryanchriswhite bryanchriswhite commented Apr 17, 2023

Summary

This pull request adds an Architecture Decision Record (ADR) proposing the use of separate private keys for the transport and consensus layers in the system. The ADR discusses the context, decision drivers, considered options, and the pros and cons of each option, ultimately recommending the use of separate private keys.

The chosen option is to keep the P2P and consensus private keys separate. By using separate private keys, the system can achieve better security, flexibility, and isolation in the event of key compromise. The ADR will also serve as a reference for future discussions or changes related to key management in the system.

Please review the ADR and provide feedback on:

  • The clarity of the problem statement and context
  • The thoroughness of the decision drivers and considered options
  • The appropriateness of the chosen option given the decision drivers
  • Any additional considerations that may have been overlooked
  • Any changes in formatting, grammar, or language for better readability

Once the ADR has been reviewed and revised as necessary, it will be merged into the repository to serve as a reference for the project.

Related Issue

@bryanchriswhite bryanchriswhite added p2p architecture decision Related to architecture decision records (ADRs) consensus Consensus specific changes labels Apr 17, 2023
@bryanchriswhite bryanchriswhite self-assigned this Apr 17, 2023
@bryanchriswhite bryanchriswhite marked this pull request as ready for review April 17, 2023 11:49
@bryanchriswhite bryanchriswhite changed the title [ADR] 0004 - Separate Consensus and Transport Private Keys [ADR] 0004 - Separate Consensus and P2P Private Keys Apr 17, 2023
@bryanchriswhite bryanchriswhite marked this pull request as draft April 18, 2023 17:34
@Olshansk Olshansk removed their request for review April 19, 2023 19:41
@bryanchriswhite bryanchriswhite marked this pull request as ready for review April 21, 2023 07:23
ADRs/0004-separate-consensus-and-p2p-keys.md Outdated Show resolved Hide resolved
ADRs/0004-separate-consensus-and-p2p-keys.md Outdated Show resolved Hide resolved
ADRs/0004-separate-consensus-and-p2p-keys.md Outdated Show resolved Hide resolved
@Olshansk
Copy link
Member

Olshansk commented May 16, 2023

@bryanchriswhite Bump. Wanted to point attention to this comment specifically: #35 (comment)

* pokt/main:
  styling
  updates contribution process in ADR README
  update to unsuitable terminology
  Removes conclusion from problem statement section
  adds details to README about withdrawing ADRs
  [ADRs] Update ADR docs & template (#36)
  ADR Template Updates (#40)
@bryanchriswhite bryanchriswhite marked this pull request as draft May 19, 2023 13:35
@bryanchriswhite bryanchriswhite changed the title [ADR] 0004 - Separate Consensus and P2P Private Keys [ADR] 0004 - Separate Transport and Application Private Keys May 19, 2023
@bryanchriswhite bryanchriswhite changed the title [ADR] 0004 - Separate Transport and Application Private Keys [ADR] 0004 - Separate Transport and Application Keys May 19, 2023
@bryanchriswhite bryanchriswhite marked this pull request as ready for review May 19, 2023 15:02
@bryanchriswhite bryanchriswhite requested a review from Olshansk May 19, 2023 15:02
@Olshansk Olshansk removed the request for review from gokutheengineer May 20, 2023 01:42
Copy link
Member

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

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

LGTM up until Decision Outcome, but from thereon, I think there are more details we should fill if this is really the path we want to go down.

Comment on lines 36 to 40
- Security: Minimizing the risks associated with key compromise
- Simplification: Reducing complexity in identity management
- Flexibility: Allowing different key management strategies for different modules
- Isolation: Minimizing the impact of compromise on other system components
- Optionality: Enabling future changes and extensions to the protocol
Copy link
Member

Choose a reason for hiding this comment

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

NIT: Bold the words used for driving the decision.

Ditto elsewhere where applicable

ADRs/0004-separate-transport-and-application-keys.md Outdated Show resolved Hide resolved
ADRs/0004-separate-transport-and-application-keys.md Outdated Show resolved Hide resolved
ADRs/0004-separate-transport-and-application-keys.md Outdated Show resolved Hide resolved
1. Use a single private key for both P2P and other functionalities
2. Use separate private keys for P2P and other functionalities

## Decision Outcome
Copy link
Member

Choose a reason for hiding this comment

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

There is no mention of things like:

  1. TLS
  2. Networking key rotation
  3. The fact that "utility" keys are tied to the actor's stake/earnings/utility (e.g. think Applications, Servicres) but P2P is primarily used for networking and security
  4. What's automatic or manual for a user (e.g. P2P key rotation can be automated)
  5. Some pubKeys corresponding to privKeys are stored on-chain while others are not

Co-authored-by: Daniel Olshansky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture decision Related to architecture decision records (ADRs) consensus Consensus specific changes p2p
Projects
Status: In Review
Development

Successfully merging this pull request may close these issues.

2 participants