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

docs(overlay): add URL of blog post and clarify wording #4635

Merged
merged 6 commits into from
Sep 19, 2023

Conversation

ckeshava
Copy link
Collaborator

There appears to be a mistake in the wording of the Overlay README file. I believe the private key is used to sign the fingerprint and the public key is used to verify the authenticity of the signature. This is a change to that effect.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactor (non-breaking change that only restructures code)
  • Tests (You added tests for code that already exists, or your new feature included in this PR)
  • Documentation Updates
  • Release

…ingerprint to prevent Man-in-the-middle attacks in a Peer connection
@ckeshava
Copy link
Collaborator Author

I'd like to ask a question, since we are looking into the Overlay corner of the codebase.

Why did we choose the xor operation to compare the local and remote data (data received from the peer)?

auto const result = (*cookie1 ^ *cookie2);

In order to prevent Man-in-the-middle attacks, we are including a "Session-Signature" field in the response. (

That fingerprint, which is never shared over the wire (since each endpoint will
) I don't think we are doing this in a robust way.

Isn't it likely that the local and remote data is identical? (For ex: sharing identical sets of validations, proposals, etc) Isn't it better to concatenate the two sets of data and then sign them with SecretKey::signDigest() ? @nbougalis @gregtatcam

@ckeshava
Copy link
Collaborator Author

ok 👍

Copy link
Collaborator

@gregtatcam gregtatcam left a comment

Choose a reason for hiding this comment

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

👍 LGTM

@scottschurr scottschurr added the Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required. label Aug 9, 2023
@ximinez ximinez removed their request for review August 10, 2023 23:16
@intelliot intelliot changed the title rectify the wording of the Overlay README docs(overlay): add URL of blog post and clarify wording Aug 27, 2023
@intelliot
Copy link
Collaborator

@manojsdoshi I think this could be ok to consider for 1.12.0-rc2 because it is only docs/comments, and does not touch any code

@intelliot intelliot modified the milestones: 1.12, 1.13 Aug 27, 2023
@intelliot intelliot merged commit a948203 into XRPLF:develop Sep 19, 2023
ckeshava added a commit to ckeshava/rippled that referenced this pull request Sep 22, 2023
sophiax851 pushed a commit to sophiax851/rippled that referenced this pull request Jun 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Passed Passed code review & PR owner thinks it's ready to merge. Perf sign-off may still be required.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants