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

[AUDIT][MEDIUM][O-1] Orchestrator slots may be trivially exhausted #3621

Merged
merged 12 commits into from
Aug 29, 2024
22 changes: 11 additions & 11 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

13 changes: 5 additions & 8 deletions crates/orchestrator/run-config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,22 +39,19 @@ seed = [
]
start_delay_seconds = 0
cdn_marshal_address = "127.0.0.1:9000"
public_keys = [
"BLS_VER_KEY~p-JKk1VvO1RoMrDrqyjz0P1VGwtOaEjF5jLjpOZbJi5O747fvYEOg0OvCl_CLe4shh7vsqeG9uMF9RssM12sLSuaiVJkCClxEI5mRLV4qff1UjZAZJIBgeL1_hRhRUkpqC0Trm1qtvXtZ8FwOCIzYXv8c300Au824k7FxjjcWLBL",
]

[config]
num_nodes_with_stake = 10
num_nodes_without_stake = 0
start_threshold = [
8,
10,
]
start_threshold = [8, 10]
staked_da_nodes = 10
non_staked_da_nodes = 0
fixed_leader_for_gpuvid = 1
next_view_timeout = 30000
timeout_ratio = [
11,
10,
]
timeout_ratio = [11, 10]
round_start_delay = 1
start_delay = 1
num_bootstrap = 5
Expand Down
21 changes: 21 additions & 0 deletions crates/orchestrator/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -208,6 +208,10 @@ pub struct NetworkConfig<KEY: SignatureKey> {
pub builder: BuilderType,
/// random builder config
pub random_builder: Option<RandomBuilderConfig>,
/// The list of public keys that are allowed to connect to the orchestrator
pub public_keys: Vec<KEY>,
/// Whether or not to disable registration verification.
pub disable_registration_verification: bool,
}

/// the source of the network config
Expand Down Expand Up @@ -439,6 +443,8 @@ impl<K: SignatureKey> Default for NetworkConfig<K> {
commit_sha: String::new(),
builder: BuilderType::default(),
random_builder: None,
public_keys: vec![],
disable_registration_verification: false,
}
}
}
Expand Down Expand Up @@ -491,10 +497,23 @@ pub struct NetworkConfigFile<KEY: SignatureKey> {
/// random builder configuration
#[serde(default)]
pub random_builder: Option<RandomBuilderConfig>,
/// The list of public keys that are allowed to connect to the orchestrator
#[serde(default)]
pub public_keys: Vec<KEY>,
lukeiannucci marked this conversation as resolved.
Show resolved Hide resolved
/// Whether or not to disable registration verification.
#[serde(default)]
pub disable_registration_verification: bool,
}

impl<K: SignatureKey> From<NetworkConfigFile<K>> for NetworkConfig<K> {
fn from(val: NetworkConfigFile<K>) -> Self {
#[cfg(not(debug_assertions))]
{
if val.disable_registration_verification {
panic!("Registration verification cannot be turned off in production builds");
}
}

NetworkConfig {
rounds: val.rounds,
indexed_da: val.indexed_da,
Expand Down Expand Up @@ -536,6 +555,8 @@ impl<K: SignatureKey> From<NetworkConfigFile<K>> for NetworkConfig<K> {
commit_sha: String::new(),
builder: val.builder,
random_builder: val.random_builder,
public_keys: val.public_keys,
disable_registration_verification: val.disable_registration_verification,
}
}
}
Expand Down
73 changes: 67 additions & 6 deletions crates/orchestrator/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ pub mod client;
pub mod config;

use std::{
collections::HashMap,
collections::{HashMap, HashSet},
fs::OpenOptions,
io::{self, ErrorKind},
time::Duration,
Expand All @@ -23,7 +23,10 @@ use client::{BenchResults, BenchResultsDownloadConfig};
use config::BuilderType;
use csv::Writer;
use futures::{stream::FuturesUnordered, FutureExt, StreamExt};
use hotshot_types::{traits::signature_key::SignatureKey, PeerConfig};
use hotshot_types::{
traits::signature_key::{SignatureKey, StakeTableEntryType},
PeerConfig,
};
use libp2p::{
identity::{
ed25519::{Keypair as EdKeypair, SecretKey},
Expand Down Expand Up @@ -83,6 +86,8 @@ struct OrchestratorState<KEY: SignatureKey> {
peer_pub_ready: bool,
/// A map from public keys to `(node_index, is_da)`.
pub_posted: HashMap<Vec<u8>, (u64, bool)>,
/// A collection for storing `ready` public keys.
ready_posted: HashSet<KEY>,
/// Whether nodes should start their HotShot instances
/// Will be set to true once all nodes post they are ready to start
start: bool,
Expand Down Expand Up @@ -114,6 +119,7 @@ impl<KEY: SignatureKey + 'static> OrchestratorState<KEY> {
config: network_config,
peer_pub_ready: false,
pub_posted: HashMap::new(),
ready_posted: HashSet::new(),
nodes_connected: 0,
start: false,
bench_results: BenchResults::default(),
Expand Down Expand Up @@ -228,7 +234,7 @@ pub trait OrchestratorApi<KEY: SignatureKey> {
/// post endpoint for whether or not all nodes are ready
/// # Errors
/// if unable to serve
fn post_ready(&mut self) -> Result<(), ServerError>;
fn post_ready(&mut self, pubkey: &mut Vec<u8>) -> Result<(), ServerError>;
/// post endpoint for manually starting the orchestrator
/// # Errors
/// if unable to serve
Expand Down Expand Up @@ -328,7 +334,21 @@ where

let node_index = self.pub_posted.len() as u64;

// Deserialize the public key
let staked_pubkey = PeerConfig::<KEY>::from_bytes(pubkey).unwrap();

// Check if the node is allowed to connect
if !self
.config
.public_keys
.contains(&staked_pubkey.stake_table_entry.public_key())
Copy link
Contributor

Choose a reason for hiding this comment

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

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)

{
return Err(ServerError {
status: tide_disco::StatusCode::FORBIDDEN,
message: "You are unauthorized to register with the orchestrator".to_string(),
});
}

self.config
.config
.known_nodes_with_stake
Expand Down Expand Up @@ -416,8 +436,33 @@ where
}

// Assumes nodes do not post 'ready' twice
// TODO ED Add a map to verify which nodes have posted they're ready
fn post_ready(&mut self) -> Result<(), ServerError> {
fn post_ready(&mut self, pubkey: &mut Vec<u8>) -> Result<(), ServerError> {
// Deserialize the public key
let staked_pubkey = PeerConfig::<KEY>::from_bytes(pubkey).unwrap();

// Is this node allowed to connect?
if !self
.config
.public_keys
.contains(&staked_pubkey.stake_table_entry.public_key())
{
return Err(ServerError {
status: tide_disco::StatusCode::FORBIDDEN,
message: "You are unauthorized to register with the orchestrator".to_string(),
});
}

// Have they already connected?
if !self
.ready_posted
.insert(staked_pubkey.stake_table_entry.public_key().clone())
{
return Err(ServerError {
status: tide_disco::StatusCode::BAD_REQUEST,
message: "You have already reported your ready status".to_string(),
});
}

self.nodes_connected += 1;

println!("Nodes connected: {}", self.nodes_connected);
Expand Down Expand Up @@ -636,7 +681,23 @@ where
})?
.post(
"post_ready",
|_req, state: &mut <State as ReadState>::State| async move { state.post_ready() }.boxed(),
|req, state: &mut <State as ReadState>::State| {
async move {
let mut body_bytes = req.body_bytes();
jparr721 marked this conversation as resolved.
Show resolved Hide resolved
body_bytes.drain(..12);
// Decode the payload-supplied pubkey
let Ok(mut pubkey) =
vbs::Serializer::<OrchestratorVersion>::deserialize(&body_bytes)
else {
return Err(ServerError {
status: tide_disco::StatusCode::BAD_REQUEST,
message: "Malformed body".to_string(),
});
};
state.post_ready(&mut pubkey)
}
.boxed()
},
)?
.post(
"post_manual_start",
Expand Down
7 changes: 6 additions & 1 deletion crates/types/src/stake_table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,16 @@ pub struct StakeTableEntry<K: SignatureKey> {
pub stake_amount: U256,
}

impl<K: SignatureKey> StakeTableEntryType for StakeTableEntry<K> {
impl<K: SignatureKey> StakeTableEntryType<K> for StakeTableEntry<K> {
/// Get the stake amount
fn stake(&self) -> U256 {
self.stake_amount
}

/// Get the public key
fn public_key(&self) -> K {
self.stake_key.clone()
}
}

impl<K: SignatureKey> StakeTableEntry<K> {
Expand Down
6 changes: 4 additions & 2 deletions crates/types/src/traits/signature_key.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,9 +25,11 @@ use super::EncodeBytes;
use crate::{utils::BuilderCommitment, vid::VidSchemeType};

/// Type representing stake table entries in a `StakeTable`
pub trait StakeTableEntryType {
pub trait StakeTableEntryType<K> {
/// Get the stake value
fn stake(&self) -> U256;
/// Get the public key
fn public_key(&self) -> K;
}

/// Trait for abstracting public key signatures
Expand Down Expand Up @@ -60,7 +62,7 @@ pub trait SignatureKey:
+ for<'a> Deserialize<'a>
+ Hash;
/// The type of the entry that contain both public key and stake value
type StakeTableEntry: StakeTableEntryType
type StakeTableEntry: StakeTableEntryType<Self>
+ Send
+ Sync
+ Sized
Expand Down
Loading