Make Servers Stop Rejecting Requests #599

dnwiebe · 2025-03-22T12:04:53Z

No description provided.

dnwiebe · 2025-03-22T12:05:45Z

multinode_integration_tests/Cargo.toml

 masq_lib = { path = "../masq_lib" }
 native-tls = "0.2.8"
-node = { path = "../node", features = [ "expose_test_privates" ] }
+node = { path = "../node" }


Removed; Bert said it's obsolete

node/Cargo.toml

node/src/proxy_server/mod.rs

dnwiebe · 2025-03-22T12:09:42Z

node/src/proxy_server/mod.rs

-                let stream_key = self.stream_key_factory.make();
+                let stream_key = self
+                    .stream_key_factory
+                    .make(self.main_cryptde.public_key(), server_addr);


Hmm. Maybe the StreamKeyFactory should carry around the Node's public key? It's not like it's going to change...

Just to understand: Is the stream key used in the originating node to find the proper stream for a particular host? That means inboud_client_data.peer_addr is always the host IP we are asking for the web page or data?

We already talked about this, but for posterity: no. InboundClientData.peer_addr (now called InboundClientData.client_addr) is the address associated with the stream from the browser (or other client) we're dealing with. Hashed with our public key, it ought to be universally unique.

dnwiebe · 2025-03-22T12:10:02Z

node/src/proxy_server/mod.rs

-                    "make_stream_key() inserted new key {} for {}", &stream_key, ibcd.peer_addr
+                    "find_or_generate_stream_key() inserted new key {} for {}",
+                    &stream_key,
+                    ibcd.peer_addr


Another neglected name change

node/src/sub_lib/stream_key.rs

dnwiebe · 2025-03-22T12:12:19Z

node/src/sub_lib/stream_key.rs

-        hash.update(uuid_bytes);
+        hash.update(public_key.as_ref());
+        hash = add_socket_addr_to_hash(hash, server_addr);
+        hash.update(STREAM_KEY_SALT.as_slice());


This is the main change, right here; everything else supports it

dnwiebe · 2025-03-22T12:13:22Z

node/src/sub_lib/stream_key.rs

+            hash.update(v4.ip().octets().as_slice());
+        }
+        SocketAddr::V6(v6) => {
+            hash.update(v6.ip().octets().as_slice());


Technically, I don't think the Node officially supports IPv6 yet; but when it does, this will be one less thing to change.

dnwiebe · 2025-03-22T12:14:10Z

node/src/sub_lib/stream_key.rs

+        }
+    }
+    let port = socket_addr.port();
+    hash.update(&[(port & 0xFF) as u8, (port >> 8) as u8]);


Lower byte of the port, then upper byte of the port

dnwiebe · 2025-03-22T12:22:59Z

node/src/sub_lib/stream_key.rs

+    fn stream_keys_from_different_public_keys_are_different() {
+        let server_addr = SocketAddr::new(IpAddr::from_str("1.2.3.4").unwrap(), 1024);
+
+        let stream_keys = vec![PublicKey::new(&[1, 2, 3]), PublicKey::new(&[1, 2, 2])]


The public keys are only one bit different

dnwiebe · 2025-03-22T12:24:33Z

node/src/sub_lib/stream_key.rs

+        let attack = StreamKey {
+            hash: hash.digest().bytes(),
+        };
+        assert_ne!(attack, result)


Make a StreamKey without hashing in the salt, and it'll be wrong

dnwiebe · 2025-03-22T12:25:17Z

node/tests/financials_test.rs


 #[test]
-fn financials_command_retrieves_payable_and_receivable_records() {
+fn financials_command_retrieves_payable_and_receivable_records_integration() {


Because this test wasn't named xxx_integration, it wasn't being run. Since it wasn't being run, it didn't work. We fixed it.

dnwiebe · 2025-03-22T12:28:08Z

node/tests/financials_test.rs

-    assert_eq!(receivable[0].wallet, wallet_receivable_1.to_string());
-    assert_eq!(receivable[0].balance_gwei, 9000_i64);
-    assert_eq!(receivable[1].wallet, wallet_receivable_2.to_string());
-    assert_eq!(receivable[1].balance_gwei, 345678_i64);


We can't assert this way, because we can't depend on the order of the receivables. So we first have to figure out which order they're in, and then assert on them.

dnwiebe · 2025-03-22T12:31:34Z

node/tests/utils.rs

-        let data_dir = if sterile_database {
-            ensure_node_home_directory_exists("integration", test_name)
-        } else {
-            node_home_directory("integration", test_name)


If the config_opt contains a --data-directory specification, it shouldn't be ignored like this.

czarte

we decided to add host_name and port to streamkeys

czarte · 2025-03-24T11:16:06Z

masq_lib/Cargo.toml

 tiny-hderive = "0.3.0"
 toml = "0.5.8"
 websocket = {version = "0.26.2", default-features = false, features = ["sync"]}
+rand = { version = "0.9.0", features = ["thread_rng"] }


put this to alphabetical order please

czarte · 2025-03-24T14:55:32Z

node/src/proxy_server/mod.rs

-                let stream_key = self.stream_key_factory.make();
+                let stream_key = self
+                    .stream_key_factory
+                    .make(self.main_cryptde.public_key(), server_addr);


Just to understand: Is the stream key used in the originating node to find the proper stream for a particular host? That means inboud_client_data.peer_addr is always the host IP we are asking for the web page or data?

czarte · 2025-03-24T15:06:50Z

node/src/proxy_server/mod.rs

    }

-    fn find_or_generate_stream_key(&mut self, ibcd: &InboundClientData) -> StreamKey {
+    fn find_or_generate_stream_key(


I can't see how we want to select the stream for particular host, as I understand, maybe wrongly, the peer_addr is something different of server_addr

We talked about this, but for posterity: peer_addr (now called client_addr) is the address of the stream from the browser, not the stream to the server.

czarte · 2025-03-24T15:46:19Z

node/src/proxy_server/mod.rs

            return Err("Browser request rejected due to missing consuming wallet".to_string());
        }
-        let stream_key = proxy.find_or_generate_stream_key(&msg);
+        let stream_key = proxy.find_or_generate_stream_key(&msg, source_addr);


shouldn't source_addr be host_addr?

czarte · 2025-03-25T09:54:07Z

node/src/sub_lib/stream_key.rs

-        let uuid_bytes: &[u8] = uuid.as_bytes();
-        hash.update(uuid_bytes);
+        hash.update(public_key.as_ref());
+        hash = add_socket_addr_to_hash(hash, server_addr);


server_addr is host_addr?

…rts non-optional

This reverts commit 10735ad.

dnwiebe · 2025-03-28T10:53:37Z

node/src/dispatcher.rs

        let ibcd_in = InboundClientData {
            timestamp: SystemTime::now(),
-            peer_addr,
+            client_addr,


Renamed for clarity

dnwiebe · 2025-03-28T10:55:15Z

node/src/proxy_server/client_request_payload_factory.rs

-                None => (Some(host.name), protocol_pack.standard_port()),
-            },
+        let (target_hostname_opt, target_port) = match target_host {
+            Some(host) => (Some(host.name), host.port),


host.port is no longer optional

dnwiebe · 2025-03-28T10:56:34Z

node/src/proxy_server/client_request_payload_factory.rs

                last_data: ibcd.last_data,
            },
-            target_hostname,
+            target_hostname: target_hostname_opt,


Can't correct the field name of ClientRequestPayload_0v1 without migrating to ClientRequestPayload_0v2, but I can correct the variable name...

dnwiebe · 2025-03-28T10:57:17Z

node/src/proxy_server/http_protocol_pack.rs

+        match HttpProtocolPack::find_header_host(data.as_slice()) {
            Some(host) => Some(host),
-            None => HttpProtocolPack::find_header_host(data.as_slice()),
+            None => HttpProtocolPack::find_url_host(data.as_slice()),


Corrected the host-name priority

dnwiebe · 2025-03-28T11:01:44Z

node/src/proxy_server/http_protocol_pack.rs

-            2 => Some(Host {
-                name: parts.remove(0).to_string(),
-                port: Self::port_from_string(parts.remove(0).to_string()),
+                port: HTTP_PORT,


Now, instead of making a missing port number None here in HttpProtocolPack, where we know missing port numbers mean a default of 80, and then figuring out the number later when we run into the None outside the HTTP context, we're ditching the Option and defaulting it to 80 right here.

dnwiebe · 2025-03-28T11:02:46Z

node/src/proxy_server/http_protocol_pack.rs

-            Err(_) => None,
-            Ok(port) => Some(port),
+            Err(_) => Err(format!("Port '{}' is not a number", port_str)),
+            Ok(port) => Ok(port),


If we can't parse a port number, that's not just "no port was specified;" that's an error that needs to be reported and logged somewhere.

dnwiebe · 2025-03-28T11:04:50Z

node/src/proxy_server/http_protocol_pack.rs


-        assert_eq!(String::from("header.host.com"), host.name);
+        assert_eq!(host, Host::new("header.host.com", HTTP_PORT));
+        assert_eq!(HTTP_PORT, host.port);


Now that Host has a constructor, we can easily assert on the whole thing, rather than just one field.

dnwiebe · 2025-03-28T11:05:35Z

node/src/proxy_server/http_protocol_pack.rs


    #[test]
-    fn returns_host_name_and_port_from_url_if_both_exist() {
+    fn returns_host_name_and_port_from_header_if_both_exist() {


Priorities have been corrected.

dnwiebe · 2025-03-28T11:07:18Z

node/src/proxy_server/http_protocol_pack.rs

        let host = HttpProtocolPack {}.find_host(&data).unwrap();

-        assert_eq!(String::from("top.host.com"), host.name);
+        assert_eq!(host.name, String::from("top.host.com"));


This is old, old code, back from the days when Rust followed the JUnit convention of assert_eq!(expected, actual). Where I happened across these, I changed them to conform to the more modern assert_eq!(actual, expected) standard.

dnwiebe · 2025-03-28T11:12:02Z

node/src/proxy_server/http_protocol_pack.rs

    fn returns_host_name_from_url_when_no_scheme() {
        let data = PlainData::new(
-            b"CONNECT good.url.dude/path.html HTTP/1.1\r\nHost: wrong.url.dude\r\n\r\n",
+            b"GET wrong.url.dude/path.html HTTP/1.1\r\nHost: good.url.dude\r\n\r\n",


CONNECTs are unusual in the way their URL parameters are formed. The code under test doesn't care about that, but I still don't like a test with a manifestly incorrect request syntax, if I can avoid it. And I can. So I did.

dnwiebe · 2025-03-28T11:43:44Z

node/src/proxy_server/http_protocol_pack.rs

-    }
-
-    #[test]
-    fn from_integration_test() {


Redundant test; removed.

dnwiebe · 2025-03-28T11:47:36Z

node/src/proxy_server/mod.rs

        let msg_from_dispatcher = InboundClientData {
            timestamp: SystemTime::now(),
-            peer_addr: socket_addr.clone(),
+            client_addr: socket_addr,


SocketAddr is Copy: no need for .clone().

dnwiebe · 2025-03-28T11:50:32Z

node/src/sub_lib/route.rs

-Encrypted with 0x03040506: LiveHop { public_key: 0x, payer: Some(Payer { wallet: Wallet { kind: Address(0x71d0fc7d1c570b1ed786382b551a09391c91e33d) }, proof: Signature { v: 1, r: "9ca23557adf96d7aed407a06ce96851a4184e947a7b29b6c3872eef902fcba1e", s: "9ca23557adf96d7aed407a06ce96851a4184e947a7b29b6c3872eef902fcba1e" } }), component: Neighborhood }
+Encrypted with 0x01020304: LiveHop { public_key: 0x02030405, payer: Some(Payer { wallet: Wallet { kind: Address(0x71d0fc7d1c570b1ed786382b551a09391c91e33d) }, proof: Signature { v: 0, r: "3e3a92d7284c2c2ff7119e9f7a7e183b062a335a598e965a47c36a2f288b6f8d", s: "3e3a92d7284c2c2ff7119e9f7a7e183b062a335a598e965a47c36a2f288b6f8d" } }), component: Hopper }
+Encrypted with 0x02030405: LiveHop { public_key: 0x03040506, payer: Some(Payer { wallet: Wallet { kind: Address(0x71d0fc7d1c570b1ed786382b551a09391c91e33d) }, proof: Signature { v: 0, r: "4324a40295bb36ef2b927fb24250fe42397a57b861ea152bbbe4f84150d4ff5a", s: "4324a40295bb36ef2b927fb24250fe42397a57b861ea152bbbe4f84150d4ff5a" } }), component: Hopper }
+Encrypted with 0x03040506: LiveHop { public_key: 0x, payer: Some(Payer { wallet: Wallet { kind: Address(0x71d0fc7d1c570b1ed786382b551a09391c91e33d) }, proof: Signature { v: 1, r: "8649b8f6db6232cb1e4f1f04786ad4ef33488c968e64bec74ecd893d6d05c1b9", s: "8649b8f6db6232cb1e4f1f04786ad4ef33488c968e64bec74ecd893d6d05c1b9" } }), component: Neighborhood }


Switching blockchains from EthRopsten to BaseSepolia resulted in different signatures.

dnwiebe · 2025-03-28T11:51:41Z

node/src/sub_lib/stream_key.rs

-    fn default() -> Self {
-        Self::new()
-    }
-}


Creating a StreamKey now requires two parameters: no more Default.

dnwiebe · 2025-03-28T11:53:06Z

node/src/sub_lib/stream_key.rs


 type HashType = [u8; sha1::DIGEST_LENGTH];

+fn add_socket_addr_to_hash(mut hash: sha1::Sha1, client_addr: SocketAddr) -> sha1::Sha1 {


Copilot wrote this whole function, just working from the name of it (and probably the tests for the caller), and I didn't modify it. Pretty cool, hey?

Tests passing

c596a3b

dnwiebe commented Mar 22, 2025

View reviewed changes

node/Cargo.toml Show resolved Hide resolved

dnwiebe commented Mar 22, 2025

View reviewed changes

node/src/proxy_server/mod.rs Outdated Show resolved Hide resolved

dnwiebe commented Mar 22, 2025

View reviewed changes

node/src/proxy_server/mod.rs Show resolved Hide resolved

dnwiebe commented Mar 22, 2025

View reviewed changes

node/src/sub_lib/stream_key.rs Show resolved Hide resolved

dnwiebe added 2 commits March 22, 2025 08:26

Removed dead code

1a9f51a

Moved test back where it belongs

e24bd9a

dnwiebe commented Mar 22, 2025

View reviewed changes

dnwiebe added 3 commits March 22, 2025 08:39

Formatting

bcedf60

Finished TEST_DEFAULT_CHAIN update

8975ef5

More BaseSepolia stuff

10735ad

czarte reviewed Mar 25, 2025

View reviewed changes

dnwiebe added 7 commits March 26, 2025 08:14

Corrected host and port detection in protocol packs; about to make po…

70e3ead

…rts non-optional

Abandoning this approach; will revert, but wanted to preserve

0c50d1e

Revert "More BaseSepolia stuff"

45ea8b4

This reverts commit 10735ad.

Everything's compiling now

f4f6403

Interim commit

a4f6ec8

Fixed the protocol pack

5240c18

Merged in -reverted and completed migration to base-sepolia

4ff7096

dnwiebe commented Mar 28, 2025

View reviewed changes

node/src/proxy_server/http_protocol_pack.rs

}

#[test]

fn from_integration_test() {

Copy link

Collaborator Author

dnwiebe Mar 28, 2025

Choose a reason for hiding this comment

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

Redundant test; removed.

dnwiebe commented Mar 28, 2025

View reviewed changes

dnwiebe added 2 commits March 28, 2025 07:56

Review issues

3b41718

Formatting

4168a58

czarte approved these changes Mar 31, 2025

View reviewed changes

dnwiebe merged commit f2a2470 into master Apr 1, 2025
6 checks passed

dnwiebe deleted the GH-554-dan branch April 1, 2025 11:35

utkarshg6 added a commit that referenced this pull request Jul 10, 2025

Merge branch 'GH-599' into GH-605

89bc3a0


		type HashType = [u8; sha1::DIGEST_LENGTH];

		fn add_socket_addr_to_hash(mut hash: sha1::Sha1, client_addr: SocketAddr) -> sha1::Sha1 {

Make Servers Stop Rejecting Requests #599

Make Servers Stop Rejecting Requests #599

Uh oh!

Conversation

dnwiebe commented Mar 22, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

czarte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment