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

BLUE-276 Restrict first node selection based on the specified node ip and port #85

Merged
merged 8 commits into from
Sep 19, 2024
8 changes: 4 additions & 4 deletions debug_mode.patch
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
diff --git a/archiver-config.json b/archiver-config.json
index a892baf..ffac25a 100644
index 7fafd0a..da32776 100644
--- a/archiver-config.json
+++ b/archiver-config.json
@@ -56,7 +56,7 @@
@@ -56,6 +56,6 @@
"publicKey": "aec5d2b663869d9c22ba99d8de76f3bff0f54fa5e39d2899ec1f3f4543422ec7"
}
],
Expand All @@ -12,10 +12,10 @@ index a892baf..ffac25a 100644
}
\ No newline at end of file
diff --git a/src/Config.ts b/src/Config.ts
index 6b41ee4..a812003 100644
index 49bb21a..69cda2a 100644
--- a/src/Config.ts
+++ b/src/Config.ts
@@ -86,7 +86,7 @@ let config: Config = {
@@ -127,7 +127,7 @@ let config: Config = {
save: true,
interval: 1,
},
Expand Down
44 changes: 40 additions & 4 deletions src/API.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,36 @@

if (State.isFirst && NodeList.isEmpty() && !NodeList.foundFirstNode) {
try {
let err = Utils.validateTypes(signedFirstNodeInfo, {
nodeInfo: 'o',
sign: 'o',
})
if (err) {
reply.send({ success: false, error: err })
return
}
err = Utils.validateTypes(signedFirstNodeInfo.nodeInfo, {
externalIp: 's',
externalPort: 'n',
publicKey: 's',
})
if (err) {
reply.send({ success: false, error: err })
return
}
err = Utils.validateTypes(signedFirstNodeInfo.sign, {
owner: 's',
sig: 's',
})
if (err) {
reply.send({ success: false, error: err })
return
}
if (signedFirstNodeInfo.nodeInfo.publicKey !== signedFirstNodeInfo.sign.owner) {
Logger.mainLogger.error('nodeInfo.publicKey does not match signature owner', signedFirstNodeInfo)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
reply.send({ success: false, error: 'nodeInfo.publicKey does not match signature owner' })
return
}
const isSignatureValid = Crypto.verify(signedFirstNodeInfo)
if (!isSignatureValid) {
Logger.mainLogger.error('Invalid signature', signedFirstNodeInfo)
Expand All @@ -93,16 +123,22 @@
reply.send({ success: false, error: 'Signature verification failed' })
return
}
const ip = signedFirstNodeInfo.nodeInfo.externalIp

Choose a reason for hiding this comment

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

what if ip in payload will be different from actual ip of the sender ?

Copy link
Contributor

Choose a reason for hiding this comment

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

The IP and Port are not even being checked in here are they? The title of this PR says restricted based off IP and Port, but I only see the public key being checked.
@mhanson-github @jairajdev

const port = signedFirstNodeInfo.nodeInfo.externalPort
const publicKey = signedFirstNodeInfo.nodeInfo.publicKey
if (config.restrictFirstNodeSelectionByPublicKey) {
if (publicKey !== config.firstNodePublicKey) {
Logger.mainLogger.error('Invalid publicKey of first node info', signedFirstNodeInfo)

Check warning

Code scanning / CodeQL

Log injection Medium

Log entry depends on a
user-provided value
.
reply.send({ success: false, error: 'Invalid publicKey of first node info' })
return
}
}
if (NodeList.foundFirstNode) {
const res = NodeList.getCachedNodeList()
reply.send(res)
return
}
NodeList.toggleFirstNode()
const ip = signedFirstNodeInfo.nodeInfo.externalIp
const port = signedFirstNodeInfo.nodeInfo.externalPort
const publicKey = signedFirstNodeInfo.nodeInfo.publicKey

const firstNode: NodeList.ConsensusNodeInfo = {
ip,
port,
Expand Down Expand Up @@ -961,7 +997,7 @@
'ARCHIVER_PUBLIC_KEY',
]
try {
const { sign, ...newConfig } = _request.body

Check warning on line 1000 in src/API.ts

View workflow job for this annotation

GitHub Actions / ci / QA merge checks

'sign' is assigned a value but never used
const validKeys = new Set(Object.keys(config))
const payloadKeys = Object.keys(newConfig)
const invalidKeys = payloadKeys.filter(
Expand Down
4 changes: 4 additions & 0 deletions src/Config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,8 @@ export interface Config {
txCronSchedule: string
}
workerProcessesDebugLog: boolean // To enable debug logs for worker processes managed by the main process
restrictFirstNodeSelectionByPublicKey: boolean // The flag to pick the first node that matches the PUBLIC_KEY specified in the firstNodeInfo
firstNodePublicKey: string // The public key of the first node to be selected
}

let config: Config = {
Expand Down Expand Up @@ -184,6 +186,8 @@ let config: Config = {
txCronSchedule: '*/5 * * * *',
},
workerProcessesDebugLog: false,
restrictFirstNodeSelectionByPublicKey: false,
firstNodePublicKey: '',
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be an environment variable? When and how does this get set?

}
// Override default config params from config file, env vars, and cli args
export async function overrideDefaultConfig(file: string): Promise<void> {
Expand Down
Loading