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.

Sign up for GitHub

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

Allowed adversary to control message delivery #141

Closed
wants to merge 2 commits into from
Closed

Allowed adversary to control message delivery #141

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cf9b611
allowed adversary to control message delivery
vkomenda Jul 17, 2018
9354303
removed cloning of a message
vkomenda Jul 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions tests/broadcast.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use std::sync::Arc;

use rand::Rng;

use hbbft::broadcast::{Broadcast, BroadcastMessage};
use hbbft::broadcast::Broadcast;
use hbbft::crypto::SecretKeySet;
use hbbft::messaging::{DistAlgorithm, NetworkInfo, Target, TargetedMessage};
use network::{
Expand Down Expand Up @@ -56,8 +56,12 @@ impl Adversary<Broadcast<NodeUid>> for ProposeAdversary {
self.scheduler.pick_node(nodes)
}

fn push_message(&mut self, _: NodeUid, _: TargetedMessage<BroadcastMessage, NodeUid>) {
// All messages are ignored.
fn push_message(
&mut self,
msg: MessageWithSender<Broadcast<NodeUid>>,
) -> Vec<MessageWithSender<Broadcast<NodeUid>>> {
// Output the message without changes.
vec![msg]
}

fn step(&mut self) -> Vec<MessageWithSender<Broadcast<NodeUid>>> {
Expand Down
15 changes: 8 additions & 7 deletions tests/honey_badger.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use std::sync::Arc;
use itertools::Itertools;
use rand::Rng;

use hbbft::honey_badger::{self, Batch, HoneyBadger, MessageContent};
use hbbft::honey_badger::{Batch, HoneyBadger, MessageContent};
use hbbft::messaging::{NetworkInfo, Target, TargetedMessage};
use hbbft::transaction_queue::TransactionQueue;

Expand Down Expand Up @@ -66,21 +66,22 @@ impl Adversary<UsizeHoneyBadger> for FaultyShareAdversary {

fn push_message(
&mut self,
sender_id: NodeUid,
msg: TargetedMessage<honey_badger::Message<NodeUid>, NodeUid>,
) {
let NodeUid(sender_id) = sender_id;
msg: MessageWithSender<UsizeHoneyBadger>,
) -> Vec<MessageWithSender<UsizeHoneyBadger>> {
let NodeUid(sender_id) = msg.sender;
if sender_id < self.num_good {
if let TargetedMessage {
target: Target::All,
message,
} = msg
} = msg.tm.clone()
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should be able to avoid the cloning with:

if let TargetedMessage {
    target: Target::All,
    ref message,
} = msg.tm

{
let epoch = message.epoch();
// Set the trigger to simulate decryption share messages.
self.share_triggers.entry(epoch).or_insert(true);
}
}
// Output the message without changes.
vec![MessageWithSender::new(NodeUid(sender_id), msg.tm)]
}

fn step(&mut self) -> Vec<MessageWithSender<UsizeHoneyBadger>> {
Expand Down Expand Up @@ -137,7 +138,7 @@ where
}
if node.outputs().last().unwrap().is_empty() {
let last = node.outputs().last().unwrap().epoch;
node.queue.retain(|(_, ref msg)| msg.epoch() < last);
node.queue.retain(|(_, msg)| msg.epoch() < last);
}
false
};
Expand Down
110 changes: 73 additions & 37 deletions tests/network/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,15 +143,20 @@ impl<D: DistAlgorithm> MessageWithSender<D> {
/// An adversary that can control a set of nodes and pick the next good node to receive a message.
///
/// See `TestNetwork::step()` for a more detailed description of its capabilities.
pub trait Adversary<D: DistAlgorithm> {
pub trait Adversary<D: DistAlgorithm>
where
<D as DistAlgorithm>::Message: Clone,
{
/// Chooses a node to be the next one to handle a message.
///
/// Starvation is illegal, i.e. in every iteration a node that has pending incoming messages
/// must be chosen.
fn pick_node(&self, nodes: &BTreeMap<D::NodeUid, TestNode<D>>) -> D::NodeUid;

/// Called when a node controlled by the adversary receives a message.
fn push_message(&mut self, sender_id: D::NodeUid, msg: TargetedMessage<D::Message, D::NodeUid>);
/// Called when a correct node emits a message. The adversary is allowed to perform an arbitrary
/// transformation of the message. It returns the result of the transformation in a map from
/// node IDs to targeted messages.
fn push_message(&mut self, msg: MessageWithSender<D>) -> Vec<MessageWithSender<D>>;

/// Produces a list of messages to be sent from the adversary's nodes.
fn step(&mut self) -> Vec<MessageWithSender<D>>;
Expand Down Expand Up @@ -179,13 +184,17 @@ impl SilentAdversary {
}
}

impl<D: DistAlgorithm> Adversary<D> for SilentAdversary {
impl<D: DistAlgorithm> Adversary<D> for SilentAdversary
where
<D as DistAlgorithm>::Message: Clone,
{
fn pick_node(&self, nodes: &BTreeMap<D::NodeUid, TestNode<D>>) -> D::NodeUid {
self.scheduler.pick_node(nodes)
}

fn push_message(&mut self, _: D::NodeUid, _: TargetedMessage<D::Message, D::NodeUid>) {
// All messages are ignored.
fn push_message(&mut self, msg: MessageWithSender<D>) -> Vec<MessageWithSender<D>> {
// Output the message without changes.
vec![msg]
}

fn step(&mut self) -> Vec<MessageWithSender<D>> {
Expand Down Expand Up @@ -257,6 +266,8 @@ impl<D: DistAlgorithm, F> RandomAdversary<D, F> {

impl<D: DistAlgorithm, F: Fn() -> TargetedMessage<D::Message, D::NodeUid>> Adversary<D>
for RandomAdversary<D, F>
where
<D as DistAlgorithm>::Message: Clone,
{
fn init(
&mut self,
Expand All @@ -272,18 +283,18 @@ impl<D: DistAlgorithm, F: Fn() -> TargetedMessage<D::Message, D::NodeUid>> Adver
self.scheduler.pick_node(nodes)
}

fn push_message(&mut self, _: D::NodeUid, msg: TargetedMessage<D::Message, D::NodeUid>) {
fn push_message(&mut self, msg: MessageWithSender<D>) -> Vec<MessageWithSender<D>> {
// If we have not discovered the network topology yet, abort.
if self.known_node_ids.is_empty() {
return;
return vec![msg];
}

// only replay a message in some cases
if !randomly(self.p_replay) {
return;
return vec![msg];
}

let TargetedMessage { message, target } = msg;
let TargetedMessage { message, target } = msg.tm;

match target {
Target::All => {
Expand All @@ -292,23 +303,34 @@ impl<D: DistAlgorithm, F: Fn() -> TargetedMessage<D::Message, D::NodeUid>> Adver
// network topology. To re-send a broadcast message from one of the attacker
// controlled nodes, we would have to get a list of attacker controlled nodes
// here and use a random one as the origin/sender, this is not done here.
return;
vec![MessageWithSender {
sender: msg.sender,
tm: Target::All.message(message),
}]
}
Target::Node(our_node_id) => {
Target::Node(target_id) => {
// Choose a new target to send the message to. The unwrap never fails, because we
// ensured that `known_node_ids` is non-empty earlier.
let mut rng = rand::thread_rng();
let new_target_node = rng.choose(&self.known_node_ids).unwrap().clone();
let new_target_id = rng.choose(&self.known_node_ids).unwrap().clone();

// TODO: We could randomly broadcast it instead, if we had access to topology
// information.
self.outgoing.push(MessageWithSender::new(
our_node_id,
target_id.clone(),
TargetedMessage {
target: Target::Node(new_target_node),
message,
target: Target::Node(new_target_id),
message: message.clone(),
},
));
// Return the original message unchanged.
vec![MessageWithSender::new(
msg.sender,
TargetedMessage {
target: Target::Node(target_id),
message,
},
)]
}
}
}
Expand Down Expand Up @@ -351,7 +373,10 @@ impl<D: DistAlgorithm, F: Fn() -> TargetedMessage<D::Message, D::NodeUid>> Adver
/// 2. Send arbitrary messages to any node originating from one of the nodes they control.
///
/// See the `step` function for details on actual operation of the network.
pub struct TestNetwork<A: Adversary<D>, D: DistAlgorithm> {
pub struct TestNetwork<A: Adversary<D>, D: DistAlgorithm>
where
<D as DistAlgorithm>::Message: Clone,
{
pub nodes: BTreeMap<D::NodeUid, TestNode<D>>,
pub observer: TestNode<D>,
pub adv_nodes: BTreeMap<D::NodeUid, Arc<NetworkInfo<D::NodeUid>>>,
Expand Down Expand Up @@ -439,28 +464,39 @@ where
Q: IntoIterator<Item = TargetedMessage<D::Message, NodeUid>> + Debug,
{
for msg in msgs {
match msg.target {
Target::All => {
for node in self.nodes.values_mut() {
if node.id != sender_id {
node.queue.push_back((sender_id, msg.message.clone()))
// Transform the queued message by means of the adversary.
let adv_msgs = self.adversary.push_message(MessageWithSender {
sender: sender_id,
tm: msg.clone(),
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should at least make sure that the original message, if it was from a correct to a correct node, is unchanged. E.g. we could just give a reference to push_message, and dispatch its outputs in addition to the original one.

Also, we should probably assert that all the adv_msgs have as sender one of the adversary's nodes: The adversary is not able to forge, modify or indefinitely block messages from a different sender.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For #37, push_message can withhold the message within the adversary and return an empty vector. In that case nothing should be dispatched. In the current tests this is still not implemented. That's why all push_messages return the same message. But it is not the case in general.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What you propose for adv_msgs would not allow implementing #37. adv_msgs is the result of the adversarial message transformation such as reordering of messages from correct nodes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But maybe reordering should be handled differently: by allowing adversaries to flip entries in the nodes' incoming queues? That, in combination with picking the next node to handle a message, should allow the adversary to do everything they are allowed to do.
(Alternatively, I think @mbr suggested to have just one queue for all the nodes, where entries have both sender and recipient IDs. Reordering that queue would replace both of the above: It allows the adversary to choose message order and the next node.)

At least I think that functionality should be separate from the method that informs the adversary about a message arriving in one of their own nodes. (Which is all that push_message currently does, I think. Not a great name, admittedly. 😬 )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also think one queue for all nodes would be great. This PR is in that direction. The adversary owns that queue. Do we really need removing incoming queues of nodes? If so, can we remove the incoming queues in a separate PR? This would require modifying the current scheduling mechanism which is at the moment based on picking nodes and delivering all queued incoming messages to it. With the single queue, we could remove those messages from it which have the scheduled node as the recipient.

The alternative of the adversary directly modifying nodes (that is, their incoming queues) sounds more complicated. It would need to keep the order of all messages across all individual queues somehow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This would require modifying the current scheduling mechanism which is at the moment based on picking nodes and delivering all queued incoming messages to it.

Exactly; it would replace pick_node with something like shuffle_queue, and we could keep push_message in its current form. (Or later replace it with something else, too — I think one idea was to actually instantiate correct nodes for the adversary, and just allow the adversary to modify their incoming and outgoing messages?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we keep push_message in its current form, where do you think the messages should be pushed onto the single queue? shuffle_queue is a great idea. I don't quite see where the queue itself comes from in your proposal though.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I was thinking that the TestNetwork itself would own the queue, and only allow the adversary to move messages inside it. (Not sure how the Adversary interface would have to look for that… maybe a method that gets a non-mutable reference to the queue and outputs a list of positions, i.e. a permutation?) That would make it impossible to write an adversary that cheats by removing or modifying entries.

But I guess it's also okay to do something simpler, and just make sure all our adversaries behave.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm maybe OK with the permutation interface. However, if we need another kind of attack in the future, we would probably rewrite the queue to have mutable access rather than add another interface to it.

@mbr, what do you think, does the single queue have to be owned by Adversary, in which case we have to ensure adversaries don't perform unexpected transformations on the queue, or should the queue be owned by TestNetwork with specialized interfaces for particular kinds of adversarial transformations?

// Dispatch the result of the transformation.
for adv_msg in adv_msgs {
match adv_msg.tm.target {
Target::All => {
for node in self.nodes.values_mut() {
if node.id != adv_msg.sender {
node.queue
.push_back((adv_msg.sender, adv_msg.tm.message.clone()));
}
}
self.observer
.queue
.push_back((adv_msg.sender, adv_msg.tm.message));
}
self.observer
.queue
.push_back((sender_id, msg.message.clone()));
self.adversary.push_message(sender_id, msg);
}
Target::Node(to_id) => {
if self.adv_nodes.contains_key(&to_id) {
self.adversary.push_message(sender_id, msg);
} else if let Some(node) = self.nodes.get_mut(&to_id) {
node.queue.push_back((sender_id, msg.message));
} else {
warn!(
"Unknown recipient {:?} for message: {:?}",
to_id, msg.message
);
Target::Node(to_id) => {
if self.adv_nodes.contains_key(&to_id) {
self.adversary.push_message(MessageWithSender {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Or should that even be a different method? This one is for messages sent to the adversary, while the other invocation is kind of eavesdropping on any communication.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we could rename push_message. It is more of a transform_message now.

sender: sender_id,
tm: msg.clone(),
});
} else if let Some(node) = self.nodes.get_mut(&to_id) {
node.queue.push_back((sender_id, msg.message.clone()));
} else {
warn!(
"Unknown recipient {:?} for message: {:?}",
to_id, msg.message
);
}
}
}
}
Expand Down