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

GH-716: Our StreamKeys are insecure #336

Merged
merged 32 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
f0a6040
GH-716: add UUID
utkarshg6 Sep 6, 2023
0b224d6
Merge branch 'master' into GH-716
utkarshg6 Sep 8, 2023
2ed9872
Revert "GH-716: add UUID"
utkarshg6 Sep 11, 2023
ba3189a
GH-716: use the version of uuid that's been used for actix
utkarshg6 Sep 11, 2023
3a803a9
GH-716: add dbnavigator to gitignore
utkarshg6 Sep 11, 2023
009ef1e
GH-716: use UUIDs to make stream keys
utkarshg6 Sep 11, 2023
df7701d
GH-716: remove parameters from the constructor of StreamKey
utkarshg6 Sep 12, 2023
ee9f2eb
GH-716: remove warnings
utkarshg6 Sep 12, 2023
8eea2eb
GH-716: tests in stream_key.rs are passing
utkarshg6 Sep 12, 2023
e6898e3
GH-716: fix test handles_http_with_a_port
utkarshg6 Sep 12, 2023
fb3955e
GH-716: fix some more tests
utkarshg6 Sep 12, 2023
6989e6b
GH-716: make hash public for StreamKey for make_meaningless_stream_key()
utkarshg6 Sep 18, 2023
4b62eff
GH-716: small test fixes
utkarshg6 Sep 18, 2023
19d0980
GH-716: fix test inbound_server_data_is_translated_to_cores_packages
utkarshg6 Sep 18, 2023
2252676
GH-716: make the test inbound_server_data_is_translated_to_cores_pack…
utkarshg6 Sep 18, 2023
d626d17
GH-716: introduce fn make_meaningful_stream_key
utkarshg6 Sep 18, 2023
ebc7040
GH-716: improve test inbound_server_data_is_translated_to_cores_packages
utkarshg6 Sep 18, 2023
b6e5ce1
GH-716: remove todo
utkarshg6 Sep 18, 2023
1a5da8f
GH-716: improve tests inside stream_key.rs
utkarshg6 Sep 18, 2023
2dd3d98
GH-716: remove clippy warnings
utkarshg6 Sep 19, 2023
c78e6c6
GH-716: use default() instead of new() in tests
utkarshg6 Sep 19, 2023
241d140
GH-716: fix test proxy_server_receives_tls_packet_other_than_handshak…
utkarshg6 Sep 19, 2023
5baa912
Merge branch 'master' into GH-716
utkarshg6 Sep 19, 2023
15ca1bb
GH-716: trigger actions
utkarshg6 Sep 19, 2023
953878d
GH-716: an attempt to fix multinode tests
utkarshg6 Sep 20, 2023
6281eb3
GH-716: review changes
utkarshg6 Sep 21, 2023
71708e6
GH-716: make the field name of StreamKey named 'hash' private
utkarshg6 Sep 21, 2023
86ce618
GH-716: improve tests in file client_request_payload_factory.rs
utkarshg6 Sep 21, 2023
f8a6979
GH-716: share the helper functions in other crates
utkarshg6 Sep 21, 2023
a559726
Merge branch 'master' into GH-716
utkarshg6 Oct 4, 2023
9398c46
Merge branch 'master' into GH-716
utkarshg6 Oct 10, 2023
96ca1fe
Merge branch 'master' into GH-716
utkarshg6 Oct 30, 2023
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
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
Loading