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: check ip of archiver against archiver map node maintains #296

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

ahmxdiqbal
Copy link
Member

No description provided.

Copy link

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Key issues to review

Debug Code
Lines 495-496 introduce debug logging that should not be present in production code. Consider removing or guarding it with a debug-level check.

Commented Code
Lines 498-503 contain commented out code that checks the validity of the archiver's IP. This code should either be reinstated or removed after confirming its necessity.

Comment on lines 495 to 496
console.log('FOR DEBUG PURPOSES: our IP', Self.ip)
console.log('FOR DEBUG PURPOSES: archiverIP', archiverIP)
Copy link
Contributor

Choose a reason for hiding this comment

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

These don't need to make it into dev

Copy link
Member Author

Choose a reason for hiding this comment

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

yeah for sure

Comment on lines 505 to 510
const isValidSig = this.crypto.verify(archiverCreds, archiverCreds.publicKey)
if (!isValidSig) {
this.mainLogger.error(`❌ Invalid Signature from Archiver @ ${archiverIP}`)
this.mainLogger.error(`❌ Invalid Signature from Archiver`)
socket.disconnect()
return
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Crypto.verify is expensive so move this to be the last check. Also use the public key from the archiver map, not the public key included in the socket data. Either use the public key from Archivers map directly, or make sure the public key included in the request matches the one for that specific archiver in the Archivers map

Copy link
Contributor

Choose a reason for hiding this comment

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

just to confirm is this what you are asking for @mssabr01 ?

  1. get public key from data
  2. get matching archiver from Archivers.recipients with key from data (fail if no matching archiver)
  3. get key from recipient
  4. validate signature using recipient pubkey instead of pubkey from data

Copy link
Contributor

Choose a reason for hiding this comment

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

it is mapped by public key

const recipient = Archivers.recipients.get(archiverCreds.publicKey)

https://github.com/shardeum/shardus-core/blob/dev/src/p2p/Archivers.ts#L47-L50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants