Improve detection & handling of duplicate Node ID: #4195
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nbougalis
requested review from
thejohnfreeman,
manojsdoshi,
HowardHinnant and
brettmollin
| @@ -1627,10 +1644,17 @@ ApplicationImp::run() | |||
| } | |||
|
|
|||
| void | |||
| ApplicationImp::signalStop() | |||
| ApplicationImp::signalStop(std::string msg) | |||
There was a problem hiding this comment.
Consider passing msg by const& since signalStop doesn't modify msg. Theoretically this could save an allocation, though after reviewing all uses of signalStop, it won't make any difference today.
HowardHinnant
suggested changes
Jun 10, 2022
There was a problem hiding this comment.
I'm getting 3 test failures:
$ rippled --unittest=ProtocolVersion
ripple.overlay.ProtocolVersion Convert protocol version to string
ripple.overlay.ProtocolVersion Convert strings to protocol versions
ripple.overlay.ProtocolVersion Protocol version negotiation
#2 failed: ProtocolVersion_test.cpp(85)
#3 failed: ProtocolVersion_test.cpp(87)
#4 failed: ProtocolVersion_test.cpp(90)
105ms, 1 suite, 3 cases, 15 tests total, 3 failures
Weird... I'll look into it. |
|
Fixed: the unit test was expecting support for obsolete protocol version 2.0 which this commit removed support for. |
|
The test failures I'm now seeing have slightly changed: |
Each node on the network is supposed to have a unique cryptographic identity. Typically, this identity is generated randomly at startup and stored for later reuse in the (poorly named) file `wallet.db`. If the file is copied, it is possible for two nodes to share the same node identity. This is generally not desirable and existing servers will detect and reject connections to other servers that have the same key. This commit achives three things: 1. It improves the detection code to pinpoint instances where two distinct servers with the same key connect with each other. In that case, servers will log an appropriate error and shut down pending intervention by the server's operator. 2. It makes it possible for server administrators to securely and easily generate new cryptographic identities for servers using the new `--newnodeid` command line arguments. When a server is started using this command, it will generate and save a random secure identity. 3. It makes it possible to configure the identity using a command line option, which makes it possible to derive it from data or parameters associated with the container or hardware where the instance is running by passing the `--nodeid` option, followed by a single argument identifying the infomation from which the node's identity is derived. For example, the following command will result in nodes with different hostnames having different node identities: `rippled --nodeid $HOSTNAME` The last option is particularly useful for automated cloud-based deployments that minimize the need for storing state and provide unique deployment identifiers. **Important note for server operators:** Depending on variables outside of the the control of this code, such as operating system version or configuration, permissions, and more, it may be possible for other users or programs to be able to access the command line arguments of other processes on the system. If you are operating in a shared environment, you should avoid using this option, preferring instead to use the `[node_seed]` option in the configuration file, and use permissions to limit exposure of the node seed. A user who gains access to the value used to derive the node's unique identity could impersonate that node. The commit also updates the minimum supported server protocol version to `XRPL/2.1`, which has been supported since version 1.5.0 and eliminates support for `XPRL/2.0`.
HowardHinnant
approved these changes
Jul 1, 2022
thejohnfreeman
approved these changes
Jul 11, 2022
intelliot
pushed a commit
to intelliot/rippled
that referenced
this pull request
Feb 25, 2023
Fix an issue introduced by XRPLF#4195 / 5a15229 (part of 1.10.0-b1)
1 task
intelliot
added a commit
to intelliot/rippled
that referenced
this pull request
Feb 25, 2023
Partially revert the functionality introduced with XRPLF#4195 / 5a15229 (part of 1.10.0-b1). Co-authored-by: Nik Bougalis <nikb@bougalis.net>
intelliot
added a commit
that referenced
this pull request
Feb 28, 2023
Partially revert the functionality introduced with #4195 / 5a15229 (part of 1.10.0-b1). Acknowledgements: Aaron Hook for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the rippled code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: bugs@xrpl.org --------- Co-authored-by: Nik Bougalis <nikb@bougalis.net>
dangell7
pushed a commit
to Transia-RnD/rippled
that referenced
this pull request
Mar 5, 2023
Partially revert the functionality introduced with XRPLF#4195 / 5a15229 (part of 1.10.0-b1). Acknowledgements: Aaron Hook for responsibly disclosing this issue. Bug Bounties and Responsible Disclosures: We welcome reviews of the rippled code and urge researchers to responsibly disclose any issues they may find. To report a bug, please send a detailed report to: bugs@xrpl.org --------- Co-authored-by: Nik Bougalis <nikb@bougalis.net>
intelliot
referenced
this pull request
Mar 7, 2023
Each node on the network is supposed to have a unique cryptographic identity. Typically, this identity is generated randomly at startup and stored for later reuse in the (poorly named) file `wallet.db`. If the file is copied, it is possible for two nodes to share the same node identity. This is generally not desirable and existing servers will detect and reject connections to other servers that have the same key. This commit achives three things: 1. It improves the detection code to pinpoint instances where two distinct servers with the same key connect with each other. In that case, servers will log an appropriate error and shut down pending intervention by the server's operator. 2. It makes it possible for server administrators to securely and easily generate new cryptographic identities for servers using the new `--newnodeid` command line arguments. When a server is started using this command, it will generate and save a random secure identity. 3. It makes it possible to configure the identity using a command line option, which makes it possible to derive it from data or parameters associated with the container or hardware where the instance is running by passing the `--nodeid` option, followed by a single argument identifying the infomation from which the node's identity is derived. For example, the following command will result in nodes with different hostnames having different node identities: `rippled --nodeid $HOSTNAME` The last option is particularly useful for automated cloud-based deployments that minimize the need for storing state and provide unique deployment identifiers. **Important note for server operators:** Depending on variables outside of the the control of this code, such as operating system version or configuration, permissions, and more, it may be possible for other users or programs to be able to access the command line arguments of other processes on the system. If you are operating in a shared environment, you should avoid using this option, preferring instead to use the `[node_seed]` option in the configuration file, and use permissions to limit exposure of the node seed. A user who gains access to the value used to derive the node's unique identity could impersonate that node. The commit also updates the minimum supported server protocol version to `XRPL/2.1`, which has been supported since version 1.5.0 and eliminates support for `XPRL/2.0`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Each node on the network is supposed to have a unique cryptographic identity. Typically, this identity is generated randomly at startup and stored for later reuse in the (poorly named) file
wallet.db.If the file is copied, it is possible for two nodes to share the same node identity. This is generally not desirable and existing servers will detect and reject connections to other servers that have the same key.
This commit achives three things:
--newnodeidcommand line arguments. When a server is started using this command, it will generate and save a random secure identity.--nodeidoption, followed by a single argument identifying the infomation from which the node's identity is derived. For example, the following command will result in nodes with different hostnames having different node identities:rippled --nodeid $HOSTNAMEThe last option is particularly useful for automated cloud-based deployments that minimize the need for storing state.
Important note for server operators:
Depending on variables outside of the the control of this code, such as operating system version or configuration, permissions, and more, it may be possible for other users or programs to be able to access the command line arguments of other processes on the system.
If you are operating in a shared environment, you should avoid using this option, preferring instead to use the [node_seed] option in the configuration file.
The commit also updates the minimum supported server protocol version to
XRPL/2.1, which has been supported since version 1.5.0 and eliminates support forXPRL/2.0.High Level Overview of Change
Context of Change
Type of Change