Skip to content

Commit

Permalink
GH-716: Our StreamKeys are insecure (#336)
Browse files Browse the repository at this point in the history
* GH-716: add UUID

* Revert "GH-716: add UUID"

This reverts commit f0a6040.

* GH-716: use the version of uuid that's been used for actix

* GH-716: add dbnavigator to gitignore

* GH-716: use UUIDs to make stream keys

* GH-716: remove parameters from the constructor of StreamKey

* GH-716: remove warnings

* GH-716: tests in stream_key.rs are passing

* GH-716: fix test handles_http_with_a_port

* GH-716: fix some more tests

* GH-716: make hash public for StreamKey for make_meaningless_stream_key()

* GH-716: small test fixes

* GH-716: fix test inbound_server_data_is_translated_to_cores_packages

* GH-716: make the test inbound_server_data_is_translated_to_cores_packages not pass easily

* GH-716: introduce fn make_meaningful_stream_key

* GH-716: improve test inbound_server_data_is_translated_to_cores_packages

* GH-716: remove todo

* GH-716: improve tests inside stream_key.rs

* GH-716: remove clippy warnings

* GH-716: use default() instead of new() in tests

* GH-716: fix test proxy_server_receives_tls_packet_other_than_handshake_from_dispatcher_then_sends_cores_package_to_hopper

* GH-716: trigger actions

* GH-716: an attempt to fix multinode tests

* GH-716: review changes

* GH-716: make the field name of StreamKey named 'hash' private

* GH-716: improve tests in file client_request_payload_factory.rs

* GH-716: share the helper functions in other crates
  • Loading branch information
utkarshg6 authored Nov 2, 2023
1 parent b2d0b07 commit d48ff8d
Show file tree
Hide file tree
Showing 17 changed files with 244 additions and 301 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ use node_lib::sub_lib::route::{Route, RouteSegment};
use node_lib::sub_lib::sequence_buffer::SequencedPacket;
use node_lib::sub_lib::stream_key::StreamKey;
use node_lib::sub_lib::versioned_data::VersionedData;
use node_lib::test_utils::make_meaningless_stream_key;
use node_lib::test_utils::neighborhood_test_utils::{db_from_node, make_node_record};
use std::io;
use std::net::SocketAddr;
Expand Down Expand Up @@ -298,7 +297,10 @@ fn context_from_request_lcp(
}

fn arbitrary_context() -> (StreamKey, u32) {
(make_meaningless_stream_key(), 12345678)
(
StreamKey::make_meaningful_stream_key("arbitrary_context"),
12345678,
)
}

fn create_request_icp(
Expand Down Expand Up @@ -353,7 +355,8 @@ fn create_meaningless_icp(
exit_node: &MASQRealNode,
) -> IncipientCoresPackage {
let socket_addr = SocketAddr::from_str("3.2.1.0:7654").unwrap();
let stream_key = StreamKey::new(PublicKey::new(&[9, 8, 7, 6]), socket_addr);
let stream_key =
StreamKey::make_meaningful_stream_key("Chancellor on brink of second bailout for banks");
IncipientCoresPackage::new(
originating_node.main_cryptde_null().unwrap(),
Route::round_trip(
Expand Down
1 change: 1 addition & 0 deletions node/Cargo.lock

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

1 change: 1 addition & 0 deletions node/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,7 @@ variant_count = "1.1.0"
web3 = {version = "0.11.0", default-features = false, features = ["http", "tls"]}
websocket = {version = "0.26.2", default-features = false, features = ["async", "sync"]}
secp256k1secrets = {package = "secp256k1", version = "0.17.2"}
uuid = "0.7.4"

[target.'cfg(target_os = "macos")'.dependencies]
system-configuration = "0.4.0"
Expand Down
25 changes: 8 additions & 17 deletions node/src/hopper/routing_service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -521,9 +521,9 @@ mod tests {
use crate::test_utils::recorder::{make_recorder, peer_actors_builder};
use crate::test_utils::{
alias_cryptde, main_cryptde, make_cryptde_pair, make_meaningless_message_type,
make_meaningless_stream_key, make_paying_wallet, make_request_payload,
make_response_payload, rate_pack_routing, rate_pack_routing_byte, route_from_proxy_client,
route_to_proxy_client, route_to_proxy_server,
make_paying_wallet, make_request_payload, make_response_payload, rate_pack_routing,
rate_pack_routing_byte, route_from_proxy_client, route_to_proxy_client,
route_to_proxy_server,
};
use actix::System;
use masq_lib::test_utils::environment_guard::EnvironmentGuard;
Expand All @@ -537,7 +537,7 @@ mod tests {
fn dns_resolution_failures_are_reported_to_the_proxy_server() {
let cryptdes = make_cryptde_pair();
let route = route_to_proxy_server(&cryptdes.main.public_key(), cryptdes.main);
let stream_key = make_meaningless_stream_key();
let stream_key = StreamKey::make_meaningless_stream_key();
let dns_resolve_failure = DnsResolveFailure_0v1::new(stream_key);
let lcp = LiveCoresPackage::new(
route,
Expand Down Expand Up @@ -873,7 +873,7 @@ mod tests {
let alias_cryptde = alias_cryptde();
let (proxy_server, _, proxy_server_recording_arc) = make_recorder();
let route = route_to_proxy_server(&main_cryptde.public_key(), main_cryptde);
let payload = make_response_payload(0, alias_cryptde);
let payload = make_response_payload(0);
let lcp = LiveCoresPackage::new(
route,
encodex::<MessageType>(
Expand Down Expand Up @@ -1903,10 +1903,7 @@ mod tests {
&MessageType::ClientRequest(VersionedData::new(
&crate::sub_lib::migrations::client_request_payload::MIGRATIONS,
&ClientRequestPayload_0v1 {
stream_key: StreamKey::new(
PublicKey::new(b"1234"),
SocketAddr::from_str("1.2.3.4:1234").unwrap(),
),
stream_key: StreamKey::make_meaningless_stream_key(),
sequenced_packet: SequencedPacket::new(vec![1, 2, 3, 4], 1234, false),
target_hostname: Some("hostname".to_string()),
target_port: 1234,
Expand All @@ -1932,10 +1929,7 @@ mod tests {
&MessageType::DnsResolveFailed(VersionedData::new(
&crate::sub_lib::migrations::dns_resolve_failure::MIGRATIONS,
&DnsResolveFailure_0v1 {
stream_key: StreamKey::new(
PublicKey::new(b"1234"),
SocketAddr::from_str("1.2.3.4:1234").unwrap(),
),
stream_key: StreamKey::make_meaningless_stream_key(),
},
)),
)
Expand Down Expand Up @@ -1975,10 +1969,7 @@ mod tests {
&MessageType::ClientResponse(VersionedData::new(
&crate::sub_lib::migrations::client_request_payload::MIGRATIONS,
&ClientResponsePayload_0v1 {
stream_key: StreamKey::new(
PublicKey::new(b"1234"),
SocketAddr::from_str("1.2.3.4:1234").unwrap(),
),
stream_key: StreamKey::make_meaningless_stream_key(),
sequenced_packet: SequencedPacket::new(vec![1, 2, 3, 4], 1234, false),
},
)),
Expand Down
33 changes: 18 additions & 15 deletions node/src/proxy_client/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ mod tests {
#[should_panic(expected = "StreamHandlerPool unbound")]
fn panics_if_unbound() {
let request = ClientRequestPayload_0v1 {
stream_key: make_meaningless_stream_key(),
stream_key: StreamKey::make_meaningless_stream_key(),
sequenced_packet: SequencedPacket {
data: b"HEAD http://www.nyan.cat/ HTTP/1.1\r\n\r\n".to_vec(),
sequence_number: 0,
Expand Down Expand Up @@ -641,7 +641,7 @@ mod tests {
fn logs_nonexistent_stream_key_during_dns_resolution_failure() {
init_test_logging();
let cryptde = main_cryptde();
let stream_key = make_meaningless_stream_key();
let stream_key = StreamKey::make_meaningless_stream_key();
let stream_key_inner = stream_key.clone();
thread::spawn(move || {
let system = System::new("logs_nonexistent_stream_key_during_dns_resolution_failure");
Expand Down Expand Up @@ -677,7 +677,7 @@ mod tests {
init_test_logging();
let cryptde = main_cryptde();
let (hopper, hopper_awaiter, hopper_recording_arc) = make_recorder();
let stream_key = make_meaningless_stream_key();
let stream_key = StreamKey::make_meaningless_stream_key();
let return_route = make_meaningless_route();
let originator_key = make_meaningless_public_key();
let stream_key_inner = stream_key.clone();
Expand Down Expand Up @@ -744,7 +744,7 @@ mod tests {
fn data_from_hopper_is_relayed_to_stream_handler_pool() {
let cryptde = main_cryptde();
let request = ClientRequestPayload_0v1 {
stream_key: make_meaningless_stream_key(),
stream_key: StreamKey::make_meaningless_stream_key(),
sequenced_packet: SequencedPacket {
data: b"inbound data".to_vec(),
sequence_number: 0,
Expand Down Expand Up @@ -804,7 +804,7 @@ mod tests {
init_test_logging();
let cryptde = main_cryptde();
let request = ClientRequestPayload_0v1 {
stream_key: make_meaningless_stream_key(),
stream_key: StreamKey::make_meaningless_stream_key(),
sequenced_packet: SequencedPacket {
data: b"inbound data".to_vec(),
sequence_number: 0,
Expand Down Expand Up @@ -861,7 +861,7 @@ mod tests {
let main_cryptde = main_cryptde();
let alias_cryptde = alias_cryptde();
let request = ClientRequestPayload_0v1 {
stream_key: make_meaningless_stream_key(),
stream_key: StreamKey::make_meaningless_stream_key(),
sequenced_packet: SequencedPacket {
data: b"inbound data".to_vec(),
sequence_number: 0,
Expand Down Expand Up @@ -926,11 +926,12 @@ mod tests {
#[test]
fn inbound_server_data_is_translated_to_cores_packages() {
init_test_logging();
let test_name = "inbound_server_data_is_translated_to_cores_packages";
let (hopper, _, hopper_recording_arc) = make_recorder();
let (accountant, _, accountant_recording_arc) = make_recorder();
let stream_key = make_meaningless_stream_key();
let stream_key = StreamKey::make_meaningful_stream_key(test_name);
let data: &[u8] = b"An honest politician is one who, when he is bought, will stay bought.";
let system = System::new("inbound_server_data_is_translated_to_cores_packages");
let system = System::new(test_name);
let route = make_meaningless_route();
let mut subject = ProxyClient::new(ProxyClientConfig {
cryptde: main_cryptde(),
Expand All @@ -948,6 +949,7 @@ mod tests {
paying_wallet: Some(make_wallet("paying")),
},
);
subject.logger = Logger::new(test_name);
let subject_addr: Addr<ProxyClient> = subject.start();
let peer_actors = peer_actors_builder()
.hopper(hopper)
Expand Down Expand Up @@ -1068,17 +1070,18 @@ mod tests {
);
assert_eq!(accountant_recording.len(), 2);
let tlh = TestLogHandler::new();
tlh.exists_log_containing(format!("ERROR: ProxyClient: Received InboundServerData from 1.2.3.4:5678: stream +dKB2Lsh3ET2TS/J/cexaanFQz4, sequence 1236, length {}; but no such known stream - ignoring", data.len()).as_str());
tlh.exists_log_containing(format!("ERROR: ProxyClient: Received InboundServerData (last_data) from 1.2.3.4:5678: stream +dKB2Lsh3ET2TS/J/cexaanFQz4, sequence 1237, length {}; but no such known stream - ignoring", data.len()).as_str());
tlh.exists_log_containing(format!("ERROR: {test_name}: Received InboundServerData from 1.2.3.4:5678: stream MBqy2yoLFeyqzyArXNTwzbNG16c, sequence 1236, length {}; but no such known stream - ignoring", data.len()).as_str());
tlh.exists_log_containing(format!("ERROR: {test_name}: Received InboundServerData (last_data) from 1.2.3.4:5678: stream MBqy2yoLFeyqzyArXNTwzbNG16c, sequence 1237, length {}; but no such known stream - ignoring", data.len()).as_str());
}

#[test]
fn inbound_server_data_without_paying_wallet_does_not_report_exit_service() {
init_test_logging();
let (accountant, _, accountant_recording_arc) = make_recorder();
let stream_key = make_meaningless_stream_key();
let stream_key = StreamKey::make_meaningless_stream_key();
let data: &[u8] = b"An honest politician is one who, when he is bought, will stay bought.";
let system = System::new("inbound_server_data_is_translated_to_cores_packages");
let system =
System::new("inbound_server_data_without_paying_wallet_does_not_report_exit_service");
let mut subject = ProxyClient::new(ProxyClientConfig {
cryptde: main_cryptde(),
dns_servers: vec![SocketAddr::from_str("8.7.6.5:4321").unwrap()],
Expand Down Expand Up @@ -1128,9 +1131,9 @@ mod tests {
init_test_logging();
let (hopper, _, hopper_recording_arc) = make_recorder();
let (accountant, _, accountant_recording_arc) = make_recorder();
let stream_key = make_meaningless_stream_key();
let stream_key = StreamKey::make_meaningless_stream_key();
let data: &[u8] = b"An honest politician is one who, when he is bought, will stay bought.";
let system = System::new("inbound_server_data_is_translated_to_cores_packages");
let system = System::new("error_creating_incipient_cores_package_is_logged_and_dropped");
let mut subject = ProxyClient::new(ProxyClientConfig {
cryptde: main_cryptde(),
dns_servers: vec![SocketAddr::from_str("8.7.6.5:4321").unwrap()],
Expand Down Expand Up @@ -1178,7 +1181,7 @@ mod tests {
let cryptde = main_cryptde();
let (hopper, _, hopper_recording_arc) = make_recorder();
let (accountant, _, accountant_recording_arc) = make_recorder();
let stream_key = make_meaningless_stream_key();
let stream_key = StreamKey::make_meaningless_stream_key();
let data: &[u8] = b"An honest politician is one who, when he is bought, will stay bought.";
let system = System::new("new_return_route_overwrites_existing_return_route");
let mut subject = ProxyClient::new(ProxyClientConfig {
Expand Down
5 changes: 2 additions & 3 deletions node/src/proxy_client/stream_establisher.rs
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ mod tests {
use super::*;
use crate::sub_lib::proxy_server::ProxyProtocol;
use crate::test_utils::main_cryptde;
use crate::test_utils::make_meaningless_stream_key;
use crate::test_utils::recorder::make_recorder;
use crate::test_utils::recorder::peer_actors_builder;
use crate::test_utils::stream_connector_mock::StreamConnectorMock;
Expand Down Expand Up @@ -179,7 +178,7 @@ mod tests {
};
subject.spawn_stream_reader(
&ClientRequestPayload_0v1 {
stream_key: make_meaningless_stream_key(),
stream_key: StreamKey::make_meaningless_stream_key(),
sequenced_packet: SequencedPacket {
data: vec![],
sequence_number: 0,
Expand Down Expand Up @@ -212,7 +211,7 @@ mod tests {
assert_eq!(
ibsd,
InboundServerData {
stream_key: make_meaningless_stream_key(),
stream_key: StreamKey::make_meaningless_stream_key(),
last_data: false,
sequence_number: 0,
source: SocketAddr::from_str("1.2.3.4:5678").unwrap(),
Expand Down
Loading

0 comments on commit d48ff8d

Please sign in to comment.