Repository: betrusted-io/xous-core (language: python)

Issue:
tls.probe() hangs on Precursor
`Tls.probe()` establishes a tls connection to the target host, extracts the certificates offered and immediately closes the connection. The certificates are presented by modal to the user, and saved to the pddb if trusted.

`net tls probe matrix.org` hangs on Precursor hardware built with `cargo xtask --app-image --feature tls` but runs correctly in hosted mode with `cargo xtask run --feature tls`

`Tls.probe()` hangs at [TcpStream::connect(url)](https://github.com/betrusted-io/xous-core/blob/26a1277097e4c6843319ac9b40b12dfd6ca7ed99/libs/tls/src/lib.rs#L315) (e.g. url = "matrix.org:433" ) **before** the tls handshake can begin.

`Tls.probe()` was:

-  merged at [Merge pull request #409 from nworbnhoj/tls.probe](https://github.com/betrusted-io/xous-core/commit/1ebee74bb1c7b6f0ae4bf0121f73fc2ba02a3b61)
-  working correctly at [update pins in lockfile](https://github.com/betrusted-io/xous-core/commit/2e1b3afc8b7b959b5c4aa39e5c93e696f378c518) (note requires minor corrections to `use` statements)
-  temporarily removed at [Revert "Tls.probe"](https://github.com/betrusted-io/xous-core/commit/7efcca39d578ccdab451417b87798558de46e536)
- reintroduced at [Revert "Revert "Tls.probe""](https://github.com/betrusted-io/xous-core/commit/b29d29eb0e81533527e25565dfa1ae2901412728)
-  and is failing from [fix missing use statements in tls lib](https://github.com/betrusted-io/xous-core/commit/a00672cd2718d2bb7a2fd5ec371522dc86013014)

During this span of commits, there have been:
- no changes to `/libs/tls/src/lib.rs` (contains `Tls.probe()`)
-  version upgrades to `rustls` 20.7 -> 21.7 : this is likely irrelevant because while the `rustls::ClientConnection` struct is constructed prior to the hang point, the first method is called after the hang point (`ClientConnection.complete_io`)

The hang point was isolated on hw by placing modals immediately before and after the hang point.
```
modals.show_notification("pre-connect", None).expect("fail");  // LAST POINT REACHED BEFORE HANG
match TcpStream::connect(url) {
    Ok(mut sock) => {
        modals.show_notification("OK connect", None).expect("fail");   // NEVER REACHED
        match conn.complete_io(&mut sock) {
            Ok(_) => log::info!("handshake complete"),
            Err(e) => log::warn!("{e}"),
        }
        conn.send_close_notify();
        match conn.peer_certificates() {
            Some(certificates) => Ok(self.check_trust(certificates)),
            None => Ok(0),
        }
    }
    Err(e) => {
        modals.show_notification("Err connect", None).expect("fail");  // NEVER REACHED
        log::warn!("{e}");
        Err(e)
    }
}
```


Discussion / hints:
Here's one thing I've discovered. The routine works if you do the following substitution:

```rust
                let url = format!("{}:443", target);
                match TcpStream::connect(url) {
```
with
```rust
                // let url = format!("{}:443", target);   // not needed
                match TcpStream::connect((target, 443)) {  // use a tuple instead of a string
```

I'm not quite sure how `std` implements the magical decode-anything-to-address function, but presumably something broke there and we're now just silently failing to parse addresses of the form `https://foo.bar:443`

I think @xobs might understand how that integration detail happens inside `std`, for me it's always just been a magical thing that sometimes worked, and when it didn't I'd just walk around it until I found a way that worked.


When you write `TcpStream::connect(&format!("{}:443", target))` you're passing a `&str`. When you write `TcpStream::connect((target, 443))` you're passing a `(&str, u16)`.

The former is handled at https://github.com/betrusted-io/rust/blob/e687c6f1659a78c2ef1575fba9f7a87200d12429/library/std/src/sys/xous/net/dns.rs#L96-L115

The latte ris handled at https://github.com/betrusted-io/rust/blob/e687c6f1659a78c2ef1575fba9f7a87200d12429/library/std/src/sys/xous/net/dns.rs#L117-L124

With `target:443` it looks something like this:

```rust
let (host, port_str) = "target:443".split_once(':')?;
let port: u16 = port_str.parse().ok()?;
TcpStream::connect((host, port))
```

For what it's worth, my desktop also hangs if I try to connect to matrix.org:433 -- it works if I go to matrix.org:443 however.
`match TcpStream::connect((target, 443))` I will take that! It is more elegant, it reduces unnecessary string parsing, and it works!
Just for my own curiosity -- you wrote that `Tls.probe() hangs at [TcpStream::connect(url)](https://github.com/betrusted-io/xous-core/blob/26a1277097e4c6843319ac9b40b12dfd6ca7ed99/libs/tls/src/lib.rs#L315) (e.g. url = "matrix.org:433" )` -- was that a copy-paste issue, or did it have `matrix.org:433`? I'm curious to know if that approach works if you give it port 443 instead.

Splitting the port and the host is definitely cleaner, and I'm just curious here!
 simply typed 
> Tls.probe() hangs at TcpStream::connect(url) (e.g. url = "matrix.org:433" )

to try and capture the essence of the issue here - rather than force the reader to drill into the code and figure it out 

If I knew I could pass (&str, u16) I would most certainly have done so - but there were plenty of other hurdles - and it worked at the time - but not for long

Fix patch:
diff --git a/libs/tls/src/cmd.rs b/libs/tls/src/cmd.rs
index ebc533cbe..9df851149 100644
--- a/libs/tls/src/cmd.rs
+++ b/libs/tls/src/cmd.rs
@@ -111,8 +111,7 @@ pub fn shellchat<'a>(
                     .unwrap();
 
             log::info!("connect TCPstream to {}", target);
-            let url = format!("{}:443", target);
-            match TcpStream::connect(url) {
+            match TcpStream::connect((target, 443)) {
                 Ok(mut sock) => {
                     log::info!("tcp connected");
                     write!(ret, "{}", t!("tls.test_success_tcp", locales::LANG)).ok();
diff --git a/libs/tls/src/lib.rs b/libs/tls/src/lib.rs
index 6fd678673..ea6b549ca 100644
--- a/libs/tls/src/lib.rs
+++ b/libs/tls/src/lib.rs
@@ -311,8 +311,7 @@ impl Tls {
                 let mut conn =
                     rustls::ClientConnection::new(Arc::new(config), server_name).unwrap();
                 log::info!("connect TCPstream to {}", target);
-                let url = format!("{}:443", target);
-                match TcpStream::connect(url) {
+                match TcpStream::connect((target, 443)) {
                     Ok(mut sock) => {
                         match conn.complete_io(&mut sock) {
                             Ok(_) => log::info!("handshake complete"),


Test patch:
diff --git a/services/net/src/tests.rs b/services/net/src/tests.rs
index 743ce0157..c1bd41492 100644
--- a/services/net/src/tests.rs
+++ b/services/net/src/tests.rs
@@ -47,7 +47,7 @@ fn bind_error() {
 
 #[test]
 fn connect_error() {
-    match TcpStream::connect("0.0.0.0:1") {
+    match TcpStream::connect(("0.0.0.0",1)) {
         Ok(..) => panic!(),
         Err(e) => assert!(
             e.kind() == ErrorKind::ConnectionRefused
diff --git a/services/shellchat/src/nettests.rs b/services/shellchat/src/nettests.rs
index d634080c0..fccfd4a78 100644
--- a/services/shellchat/src/nettests.rs
+++ b/services/shellchat/src/nettests.rs
@@ -180,7 +180,7 @@ fn bind_error() {
 }
 
 fn connect_error() {
-    match TcpStream::connect("0.0.0.0:1") {
+    match TcpStream::connect(("0.0.0.0",1)) {
         Ok(..) => panic!(),
         Err(e) => {
             log::info!("connect_error is {:?}", e);