[AUDIT][MEDIUM][O-1] Orchestrator slots may be trivially exhausted #3621
Conversation
crates/orchestrator/src/config.rs
Outdated
| if val.disable_registration_verification { | ||
| tracing::error!("REGISTRATION VERIFICATION IS TURNED OFF"); | ||
| } | ||
|
|
There was a problem hiding this comment.
I think this should probably go in run_orchestrator in orchestrator/src/lib.rs, but that's pretty minor
crates/orchestrator/src/lib.rs
Outdated
| @@ -317,7 +323,7 @@ where | |||
| return Ok((*node_index, *is_da)); | |||
| } | |||
|
|
|||
| if !self.accepting_new_keys { | |||
| if !self.config.disable_registration_verification && !self.accepting_new_keys { | |||
There was a problem hiding this comment.
Was the disable_registration_verification check meant to go in the other conditional? (I don't think it should be here)
crates/orchestrator/src/lib.rs
Outdated
| if !self | ||
| .config | ||
| .public_keys | ||
| .contains(&staked_pubkey.stake_table_entry.public_key()) |
There was a problem hiding this comment.
I think this should be if !self.config.disable_verification && !self.config.public_keys.contains(..)
(though the double negatives are confusing, maybe it should be enable_verification? that's very minor though)
crates/orchestrator/src/lib.rs
Outdated
| @@ -371,7 +391,7 @@ where | |||
| } | |||
| } | |||
|
|
|||
| println!("Posted public key for node_index {node_index}"); | |||
| tracing::info!("Posted public key for node_index {node_index}"); | |||
There was a problem hiding this comment.
can this be error? it's a start-up only log and it's very useful
crates/orchestrator/src/lib.rs
Outdated
| if !self.config.disable_registration_verification { | ||
| // Is this node allowed to connect? | ||
| if !self.config.public_keys.contains(pubkey) { |
There was a problem hiding this comment.
I think we can replace both of these and just check known_nodes_with_stake.contains(pubkey) (a node has to register before posting ready)
There was a problem hiding this comment.
This one was painful 😅 but we got it.
Closes #<ISSUE_NUMBER>
This PR:
Ochestrator is susceptible to resource exhaustion on startup for a malicious attacker. Within the orchestrator’s post_ready method, there is a limited number of available slots (also, it logs using println, which is not good) and a motivated attacker with even a modest internet connection can easily absorb all of the slots as the increment for the connected nodes is unauthenticated and has no validation. This can only occur when the server is in manual mode.
This PR fixes the problem by doing three critical steps
This PR does not:
Key places to review: