From ed016ea5b3d65af0967e48c24bc2563b82f9529b Mon Sep 17 00:00:00 2001 From: Theo Technicguy <19630890+TheoTechnicguy@users.noreply.github.com> Date: Fri, 15 Dec 2023 23:19:42 +0100 Subject: [PATCH 1/7] fix(client): prefer CLI args The client prefered using values from the config file. The expected behaviour is to prefer vales from the CLI and fill the missing ones with defaults in the configuration files. This commit fixes 35 by prefering CLI over config. fix 35 --- cli/client/main.go | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/cli/client/main.go b/cli/client/main.go index c8e2877..30c3d18 100644 --- a/cli/client/main.go +++ b/cli/client/main.go @@ -410,20 +410,18 @@ func mainWithStatusCode() int { return -1 } - hostname := configHostname + hostname := urlHostname if hostname == "" { - hostname = urlHostname + hostname = configHostname } hostnameIsAnIP := net.ParseIP(hostname) != nil - - port := configPort - if port == -1 && urlPort != "" { - port, err = strconv.Atoi(urlPort) - if err != nil { - log.Error().Msgf("invalid port number: %s: %s", urlPort, err) + port, err := strconv.Atoi(urlPort) + if err != nil && configPort == -1 { + log.Fatal().Msg("No valid port is configured!") return -1 - } + } else if err != nil { + port = configPort } username := parsedUrl.User.Username() From 630071455d1efb6344607ead5025e2eb0093c3f0 Mon Sep 17 00:00:00 2001 From: Theo Technicguy <19630890+TheoTechnicguy@users.noreply.github.com> Date: Sat, 16 Dec 2023 22:34:20 +0100 Subject: [PATCH 2/7] fix(client): prefer config port over default ed016ea fixed the config port being preferred over the CLI port, but introduced a bug where the default was preferred over the config port. This commit addresses this by setting the default port only after checking config port availability. --- cli/client/main.go | 32 +++++++++++++++++++++++++------- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/cli/client/main.go b/cli/client/main.go index 30c3d18..71f16b3 100644 --- a/cli/client/main.go +++ b/cli/client/main.go @@ -400,9 +400,6 @@ func mainWithStatusCode() int { } urlHostname, urlPort := parsedUrl.Hostname(), parsedUrl.Port() - if urlPort == "" { - urlPort = "443" - } configHostname, configPort, configUser, configAuthMethods, err := ssh3.GetConfigForHost(urlHostname, sshConfig) if err != nil { @@ -416,12 +413,33 @@ func mainWithStatusCode() int { } hostnameIsAnIP := net.ParseIP(hostname) != nil - port, err := strconv.Atoi(urlPort) - if err != nil && configPort == -1 { - log.Fatal().Msg("No valid port is configured!") + // *** Port selection *** + // There are six scenarios: + // + // | CLI port | parse | config port | program behaviour | + // | -------- | ----- | ----------- | -------------------- | + // | yes | nok | yes | throw error and exit | + // | yes | nok | no | throw error and exit | + // | yes | ok | yes | use CLI port | + // | yes | ok | no | use CLI port | + // | no | --- | yes | use config port | + // | no | --- | no | use default port | + + var port int + if urlPort != "" { + if parsedPort, err := strconv.Atoi(urlPort); err != nil || parsedPort > 0xffff { + // use WithLevel(zerolog.FatalLevel) to log a fatal level, but let us handle + // programm termination. log.Fatal() exits with os.Exit(1). + log.WithLevel(zerolog.FatalLevel).Str("Port", urlPort).Err(err).Msg("cli contains an invalid port") + fmt.Fprintf(os.Stderr, "Bad port '%s'\n", urlPort) return -1 - } else if err != nil { + } else { + port = parsedPort + } + } else if configPort != -1 { port = configPort + } else { + port = 443 } username := parsedUrl.User.Username() From 0012b92d68eec059e6fd08839bc44b6246142aa7 Mon Sep 17 00:00:00 2001 From: Theo Technicguy <19630890+TheoTechnicguy@users.noreply.github.com> Date: Sat, 16 Dec 2023 23:03:27 +0100 Subject: [PATCH 3/7] fix(client): prefer config hostname over CLI Follow OpenSSH behaviour and prefer using the in configuration set hostname over the one in the CLI. fix: 35 --- cli/client/main.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/client/main.go b/cli/client/main.go index 71f16b3..fa701ed 100644 --- a/cli/client/main.go +++ b/cli/client/main.go @@ -407,9 +407,9 @@ func mainWithStatusCode() int { return -1 } - hostname := urlHostname + hostname := configHostname if hostname == "" { - hostname = configHostname + hostname = urlHostname } hostnameIsAnIP := net.ParseIP(hostname) != nil From 1c216c71f0e53b34ba59d652f13fff3c723466fe Mon Sep 17 00:00:00 2001 From: Theo Technicguy <19630890+TheoTechnicguy@users.noreply.github.com> Date: Mon, 18 Dec 2023 16:10:56 +0100 Subject: [PATCH 4/7] refactor(client): port comment typo --- cli/client/main.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cli/client/main.go b/cli/client/main.go index fa701ed..a84d6e8 100644 --- a/cli/client/main.go +++ b/cli/client/main.go @@ -429,7 +429,7 @@ func mainWithStatusCode() int { if urlPort != "" { if parsedPort, err := strconv.Atoi(urlPort); err != nil || parsedPort > 0xffff { // use WithLevel(zerolog.FatalLevel) to log a fatal level, but let us handle - // programm termination. log.Fatal() exits with os.Exit(1). + // program termination. log.Fatal() exits with os.Exit(1). log.WithLevel(zerolog.FatalLevel).Str("Port", urlPort).Err(err).Msg("cli contains an invalid port") fmt.Fprintf(os.Stderr, "Bad port '%s'\n", urlPort) return -1 From 274c10bf12f5d90088004983ec3cd804cfa8bc87 Mon Sep 17 00:00:00 2001 From: Theo Technicguy <19630890+TheoTechnicguy@users.noreply.github.com> Date: Mon, 18 Dec 2023 22:18:58 +0100 Subject: [PATCH 5/7] refactor(client): review changes See . --- cli/client/main.go | 63 ++++++++++++++++++---------------------------- 1 file changed, 25 insertions(+), 38 deletions(-) diff --git a/cli/client/main.go b/cli/client/main.go index a84d6e8..d4d5370 100644 --- a/cli/client/main.go +++ b/cli/client/main.go @@ -51,7 +51,6 @@ func homedir() string { } } - func forwardAgent(parent context.Context, channel ssh3.Channel) error { sockPath := os.Getenv("SSH_AUTH_SOCK") if sockPath == "" { @@ -393,7 +392,6 @@ func mainWithStatusCode() int { keyLog = f } - parsedUrl, err := url.Parse(urlFromParam) if err != nil { log.Fatal().Msgf("%s", err) @@ -413,32 +411,25 @@ func mainWithStatusCode() int { } hostnameIsAnIP := net.ParseIP(hostname) != nil - // *** Port selection *** - // There are six scenarios: - // - // | CLI port | parse | config port | program behaviour | - // | -------- | ----- | ----------- | -------------------- | - // | yes | nok | yes | throw error and exit | - // | yes | nok | no | throw error and exit | - // | yes | ok | yes | use CLI port | - // | yes | ok | no | use CLI port | - // | no | --- | yes | use config port | - // | no | --- | no | use default port | var port int if urlPort != "" { - if parsedPort, err := strconv.Atoi(urlPort); err != nil || parsedPort > 0xffff { + if parsedPort, err := strconv.Atoi(urlPort); err == nil && parsedPort < 0xffff { + // There is a port in the CLI and the port is valid. Use the CLI port. + port = parsedPort + } else { + // There is a port in the CLI but it is not valid. // use WithLevel(zerolog.FatalLevel) to log a fatal level, but let us handle // program termination. log.Fatal() exits with os.Exit(1). log.WithLevel(zerolog.FatalLevel).Str("Port", urlPort).Err(err).Msg("cli contains an invalid port") fmt.Fprintf(os.Stderr, "Bad port '%s'\n", urlPort) return -1 - } else { - port = parsedPort } } else if configPort != -1 { + // There is no port in the CLI, but one in a config file. Use the config port. port = configPort } else { + // There is no port specified, neither in the CLI, nor in the configuration. port = 443 } @@ -467,13 +458,11 @@ func mainWithStatusCode() int { parsedUrl.RawQuery = urlQuery.Encode() requestUrl := parsedUrl.String() - pool, err := x509.SystemCertPool() if err != nil { log.Fatal().Msgf("%s", err) } - tlsConf := &tls.Config{ RootCAs: pool, InsecureSkipVerify: *insecure, @@ -517,7 +506,6 @@ func mainWithStatusCode() int { defer roundTripper.Close() - // connect to SSH agent if it exists var agentClient agent.ExtendedAgent var agentKeys []ssh.PublicKey @@ -540,7 +528,6 @@ func mainWithStatusCode() int { } } - log.Debug().Msgf("dialing QUIC host at %s", fmt.Sprintf("%s:%d", hostname, port)) if hostnameIsAnIP { @@ -556,16 +543,16 @@ func mainWithStatusCode() int { tlsConf, &qconf) if err != nil { - if transportErr, ok := err.(*quic.TransportError); ok { + if transportErr, ok := err.(*quic.TransportError); ok { if transportErr.ErrorCode.IsCryptoError() { if tty == nil { log.Error().Msgf("insecure server cert in non-terminal session, aborting") return -1 } if _, ok = knownHosts[hostname]; ok { - log.Error().Msgf("The server certificate cannot be verified using the one installed in %s. " + - "If you did not change the server certificate, it could be a machine-in-the-middle attack. "+ - "TLS error: %s", knownHostsPath, err) + log.Error().Msgf("The server certificate cannot be verified using the one installed in %s. "+ + "If you did not change the server certificate, it could be a machine-in-the-middle attack. "+ + "TLS error: %s", knownHostsPath, err) log.Error().Msgf("Aborting.") return -1 } @@ -579,9 +566,9 @@ func mainWithStatusCode() int { } _, err := quic.DialAddrEarly(ctx, - fmt.Sprintf("%s:%d", hostname, port), - tlsConf, - &qconf) + fmt.Sprintf("%s:%d", hostname, port), + tlsConf, + &qconf) if !errors.Is(err, certError) { log.Error().Msgf("could not create client QUIC connection: %s", err) return -1 @@ -594,22 +581,22 @@ func mainWithStatusCode() int { // first, carriage return _, _ = tty.WriteString("\r") _, err = tty.WriteString("Received an unknown self-signed certificate from the server.\n\r" + - "We recommend not using self-signed certificates.\n\r" + - "This session is vulnerable a machine-in-the-middle attack.\n\r" + - "Certificate fingerprint: " + - "SHA256 " + util.Sha256Fingerprint(peerCertificate.Raw) + "\n\r" + - "Do you want to add this certificate to ~/.ssh3/known_hosts (yes/no)? ") + "We recommend not using self-signed certificates.\n\r" + + "This session is vulnerable a machine-in-the-middle attack.\n\r" + + "Certificate fingerprint: " + + "SHA256 " + util.Sha256Fingerprint(peerCertificate.Raw) + "\n\r" + + "Do you want to add this certificate to ~/.ssh3/known_hosts (yes/no)? ") if err != nil { log.Error().Msgf("cound not write on /dev/tty: %s", err) return -1 } - + answer := "" reader := bufio.NewReader(tty) for { answer, _ = reader.ReadString('\n') answer = strings.TrimSpace(answer) - _, _ = tty.WriteString("\r") // always ensure a carriage return + _, _ = tty.WriteString("\r") // always ensure a carriage return if answer == "yes" || answer == "no" { break } @@ -665,7 +652,7 @@ func mainWithStatusCode() int { if *privKeyFile != "" { authMethods = append(authMethods, ssh3.NewPrivkeyFileAuthMethod(*privKeyFile)) } - + if *pubkeyForAgent != "" { if agentClient == nil { log.Warn().Msgf("specified a public key (%s) but no agent is running", *pubkeyForAgent) @@ -683,7 +670,7 @@ func mainWithStatusCode() int { return -1 } } - + for _, candidateKey := range agentKeys { if pubkey == nil || bytes.Equal(candidateKey.Marshal(), pubkey.Marshal()) { log.Debug().Msgf("found key in agent: %s", candidateKey) @@ -692,11 +679,11 @@ func mainWithStatusCode() int { } } } - + if *passwordAuthentication { authMethods = append(authMethods, ssh3.NewPasswordAuthMethod()) } - + } else { // for now, only perform OIDC if it was explicitly asked by the user if *issuerUrl != "" { From d11b7fbe36b09b3704ea0e43909e4ec9de9b5632 Mon Sep 17 00:00:00 2001 From: Theo Technicguy <19630890+TheoTechnicguy@users.noreply.github.com> Date: Mon, 18 Dec 2023 22:40:30 +0100 Subject: [PATCH 6/7] Revert "refactor(client): review changes" This reverts commit 274c10bf12f5d90088004983ec3cd804cfa8bc87. --- cli/client/main.go | 63 ++++++++++++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 25 deletions(-) diff --git a/cli/client/main.go b/cli/client/main.go index d4d5370..a84d6e8 100644 --- a/cli/client/main.go +++ b/cli/client/main.go @@ -51,6 +51,7 @@ func homedir() string { } } + func forwardAgent(parent context.Context, channel ssh3.Channel) error { sockPath := os.Getenv("SSH_AUTH_SOCK") if sockPath == "" { @@ -392,6 +393,7 @@ func mainWithStatusCode() int { keyLog = f } + parsedUrl, err := url.Parse(urlFromParam) if err != nil { log.Fatal().Msgf("%s", err) @@ -411,25 +413,32 @@ func mainWithStatusCode() int { } hostnameIsAnIP := net.ParseIP(hostname) != nil + // *** Port selection *** + // There are six scenarios: + // + // | CLI port | parse | config port | program behaviour | + // | -------- | ----- | ----------- | -------------------- | + // | yes | nok | yes | throw error and exit | + // | yes | nok | no | throw error and exit | + // | yes | ok | yes | use CLI port | + // | yes | ok | no | use CLI port | + // | no | --- | yes | use config port | + // | no | --- | no | use default port | var port int if urlPort != "" { - if parsedPort, err := strconv.Atoi(urlPort); err == nil && parsedPort < 0xffff { - // There is a port in the CLI and the port is valid. Use the CLI port. - port = parsedPort - } else { - // There is a port in the CLI but it is not valid. + if parsedPort, err := strconv.Atoi(urlPort); err != nil || parsedPort > 0xffff { // use WithLevel(zerolog.FatalLevel) to log a fatal level, but let us handle // program termination. log.Fatal() exits with os.Exit(1). log.WithLevel(zerolog.FatalLevel).Str("Port", urlPort).Err(err).Msg("cli contains an invalid port") fmt.Fprintf(os.Stderr, "Bad port '%s'\n", urlPort) return -1 + } else { + port = parsedPort } } else if configPort != -1 { - // There is no port in the CLI, but one in a config file. Use the config port. port = configPort } else { - // There is no port specified, neither in the CLI, nor in the configuration. port = 443 } @@ -458,11 +467,13 @@ func mainWithStatusCode() int { parsedUrl.RawQuery = urlQuery.Encode() requestUrl := parsedUrl.String() + pool, err := x509.SystemCertPool() if err != nil { log.Fatal().Msgf("%s", err) } + tlsConf := &tls.Config{ RootCAs: pool, InsecureSkipVerify: *insecure, @@ -506,6 +517,7 @@ func mainWithStatusCode() int { defer roundTripper.Close() + // connect to SSH agent if it exists var agentClient agent.ExtendedAgent var agentKeys []ssh.PublicKey @@ -528,6 +540,7 @@ func mainWithStatusCode() int { } } + log.Debug().Msgf("dialing QUIC host at %s", fmt.Sprintf("%s:%d", hostname, port)) if hostnameIsAnIP { @@ -543,16 +556,16 @@ func mainWithStatusCode() int { tlsConf, &qconf) if err != nil { - if transportErr, ok := err.(*quic.TransportError); ok { + if transportErr, ok := err.(*quic.TransportError); ok { if transportErr.ErrorCode.IsCryptoError() { if tty == nil { log.Error().Msgf("insecure server cert in non-terminal session, aborting") return -1 } if _, ok = knownHosts[hostname]; ok { - log.Error().Msgf("The server certificate cannot be verified using the one installed in %s. "+ - "If you did not change the server certificate, it could be a machine-in-the-middle attack. "+ - "TLS error: %s", knownHostsPath, err) + log.Error().Msgf("The server certificate cannot be verified using the one installed in %s. " + + "If you did not change the server certificate, it could be a machine-in-the-middle attack. "+ + "TLS error: %s", knownHostsPath, err) log.Error().Msgf("Aborting.") return -1 } @@ -566,9 +579,9 @@ func mainWithStatusCode() int { } _, err := quic.DialAddrEarly(ctx, - fmt.Sprintf("%s:%d", hostname, port), - tlsConf, - &qconf) + fmt.Sprintf("%s:%d", hostname, port), + tlsConf, + &qconf) if !errors.Is(err, certError) { log.Error().Msgf("could not create client QUIC connection: %s", err) return -1 @@ -581,22 +594,22 @@ func mainWithStatusCode() int { // first, carriage return _, _ = tty.WriteString("\r") _, err = tty.WriteString("Received an unknown self-signed certificate from the server.\n\r" + - "We recommend not using self-signed certificates.\n\r" + - "This session is vulnerable a machine-in-the-middle attack.\n\r" + - "Certificate fingerprint: " + - "SHA256 " + util.Sha256Fingerprint(peerCertificate.Raw) + "\n\r" + - "Do you want to add this certificate to ~/.ssh3/known_hosts (yes/no)? ") + "We recommend not using self-signed certificates.\n\r" + + "This session is vulnerable a machine-in-the-middle attack.\n\r" + + "Certificate fingerprint: " + + "SHA256 " + util.Sha256Fingerprint(peerCertificate.Raw) + "\n\r" + + "Do you want to add this certificate to ~/.ssh3/known_hosts (yes/no)? ") if err != nil { log.Error().Msgf("cound not write on /dev/tty: %s", err) return -1 } - + answer := "" reader := bufio.NewReader(tty) for { answer, _ = reader.ReadString('\n') answer = strings.TrimSpace(answer) - _, _ = tty.WriteString("\r") // always ensure a carriage return + _, _ = tty.WriteString("\r") // always ensure a carriage return if answer == "yes" || answer == "no" { break } @@ -652,7 +665,7 @@ func mainWithStatusCode() int { if *privKeyFile != "" { authMethods = append(authMethods, ssh3.NewPrivkeyFileAuthMethod(*privKeyFile)) } - + if *pubkeyForAgent != "" { if agentClient == nil { log.Warn().Msgf("specified a public key (%s) but no agent is running", *pubkeyForAgent) @@ -670,7 +683,7 @@ func mainWithStatusCode() int { return -1 } } - + for _, candidateKey := range agentKeys { if pubkey == nil || bytes.Equal(candidateKey.Marshal(), pubkey.Marshal()) { log.Debug().Msgf("found key in agent: %s", candidateKey) @@ -679,11 +692,11 @@ func mainWithStatusCode() int { } } } - + if *passwordAuthentication { authMethods = append(authMethods, ssh3.NewPasswordAuthMethod()) } - + } else { // for now, only perform OIDC if it was explicitly asked by the user if *issuerUrl != "" { From 5798137cbb098e3d3f7b2e6caffcecd90162fcad Mon Sep 17 00:00:00 2001 From: Theo Technicguy <19630890+TheoTechnicguy@users.noreply.github.com> Date: Mon, 18 Dec 2023 22:42:32 +0100 Subject: [PATCH 7/7] refactor(client): corrected review changes See . --- cli/client/main.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) diff --git a/cli/client/main.go b/cli/client/main.go index a84d6e8..ec362ed 100644 --- a/cli/client/main.go +++ b/cli/client/main.go @@ -413,32 +413,25 @@ func mainWithStatusCode() int { } hostnameIsAnIP := net.ParseIP(hostname) != nil - // *** Port selection *** - // There are six scenarios: - // - // | CLI port | parse | config port | program behaviour | - // | -------- | ----- | ----------- | -------------------- | - // | yes | nok | yes | throw error and exit | - // | yes | nok | no | throw error and exit | - // | yes | ok | yes | use CLI port | - // | yes | ok | no | use CLI port | - // | no | --- | yes | use config port | - // | no | --- | no | use default port | var port int if urlPort != "" { - if parsedPort, err := strconv.Atoi(urlPort); err != nil || parsedPort > 0xffff { + if parsedPort, err := strconv.Atoi(urlPort); err == nil && parsedPort < 0xffff { + // There is a port in the CLI and the port is valid. Use the CLI port. + port = parsedPort + } else { + // There is a port in the CLI but it is not valid. // use WithLevel(zerolog.FatalLevel) to log a fatal level, but let us handle // program termination. log.Fatal() exits with os.Exit(1). log.WithLevel(zerolog.FatalLevel).Str("Port", urlPort).Err(err).Msg("cli contains an invalid port") fmt.Fprintf(os.Stderr, "Bad port '%s'\n", urlPort) return -1 - } else { - port = parsedPort } } else if configPort != -1 { + // There is no port in the CLI, but one in a config file. Use the config port. port = configPort } else { + // There is no port specified, neither in the CLI, nor in the configuration. port = 443 }