fix(client): prefer CLI arguments, following OpenSSH behaviour #45

TheoTechnicguy · 2023-12-16T22:14:24Z

This PR fixes issue #35 where the client preferred the values from the configuration file ~/.ssh/config instead of the CLI arguments. The new behaviour reflects the one used by OpenSSH, preferring the port from the CLI and the hostname from the configuration.

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

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.

Follow OpenSSH behaviour and prefer using the in configuration set hostname over the one in the CLI. fix: 35

francoismichel · 2023-12-17T08:31:25Z

The integration test suite might be a bit agressive sometimes, I'll run it locally later and let you know

TheoTechnicguy · 2023-12-17T09:41:07Z

How do I run the tests? go test ./... says everything is OK.

TheoTechnicguy · 2023-12-17T10:15:18Z

In my opinion, the failures are not a result of code changes, as I get the same errors when running integration tests on the main branch.

I'm not familiar with the Ginko test framework, so I don't know where the faults are coming from. I am willing to help resolve the failing tests with some guidance.
Furthermore, I find it interesting that macOS builds pass, but Linux do not. Do you happen to know the reason for this?

francoismichel · 2023-12-17T10:44:22Z

Sorry for the delay, I am bit busy on week ends :-)

Sooo, it is a bit surprising, especially that I am not able to reproduce the fails on my own setup on several distros. It seems either the server in the integration tests crashes at some point. What is also weird is that I cannot reproduce the fails on the main branch when I rerun the Github action on it, but the error seems unrelated to your PR. We probably need a bit more logging on the server-side, and in the test suite, check that the server Consistently does not crash with an error, so that we can get the actual error if it crashes. I can try to add that additional checks on the test suite to debug that.

Concerning the MacOS build, it does not run the integration test suite because MacOS cannot run the server yet.

Thanks for the PR ad the help ! Let's continue to investigate.

francoismichel · 2023-12-17T12:19:49Z

Allright, both the integration test suite and your PR seem fine, the problem is probably the github secrets that do not seem accessible in PRs from non-maintainers. These secrets are not real secrets so I'll remove these and generate all the stuff (mock certs and keys) in the CI script.
This has been fixed in #51, so I think if you rebase it should work.
Let's get back to the real topic of this PR, I'll do a review soon. :-)

TheoTechnicguy · 2023-12-17T16:31:46Z

Thank you for investing the time looking into this. I have just merged with main. We'll see how GitHub likes the changes 😋

francoismichel · 2023-12-17T16:37:54Z

Github seems to love it.
I'll do the PR review tomorrow.
Thanks again!

TheoTechnicguy · 2023-12-17T16:38:22Z

Nice! Your changes seem to make GitHub happy, which makes me happy (^u^). Thanks again for investigating the problem. Please don't hesitate to ask for changes if needed.

glensc · 2023-12-18T13:39:55Z

cli/client/main.go

+	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).


Suggested change

// programm termination. log.Fatal() exits with os.Exit(1).

// program termination. log.Fatal() exits with os.Exit(1).

Nice catch!

francoismichel

Thank you for the PR !

Just a few comments but it's great. :-)

If you disagree with the comments we can iterate.

francoismichel · 2023-12-18T16:28:14Z

cli/client/main.go

-		port, err = strconv.Atoi(urlPort)
-		if err != nil {
-			log.Error().Msgf("invalid port number: %s: %s", urlPort, err)
+	// *** Port selection ***


Overall I am not a huge fan of the table. I think we can make it more concise. The error case when the port cannot be parsed seems obvious.
I think it is fine if we just say that the CLI port takes precedence over the config port when specified.

francoismichel · 2023-12-18T16:33:13Z

cli/client/main.go

+	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


Would you mind inverting the two parts if this branch ? Putting the non-error case in the if part and the error case in the else part ? I think it makes it more readable because the default flow goes towards what we want to achieve. But maybe it is just me ? If you have good reasons to let it like that, let me know.

Sure, will do. I think this is more a go habit, as most of my if statements are used to check for the absence of errors 😄

francoismichel · 2023-12-18T16:34:12Z

cli/client/main.go

 		}
+	} else if configPort != -1 {


Add the comment // no port specified in the CLI args ?

francoismichel · 2023-12-18T16:35:28Z

cli/client/main.go

+	} else if configPort != -1 {
+		port = configPort
+	} else {
+		port = 443


Add a comment, like // no explicit port from the CLI and no port specified in config ?

See <francoismichel#45>.

francoismichel

Great ! If you could just remove the part when the code is reformatted it it would make it easier to review and make the final commit cleaner. :-)

Other than that, I am happy with your changes !

francoismichel · 2023-12-18T21:32:04Z

cli/client/main.go

 			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. " + 


I am okay to reformat the code in a standard manner but could we do that in another PR ? :-)

Ouch! That was not intended — you're absolutely right about keeping one topic per PR.

Done! I don't want to monopolize your time, so I won't re-re-request a review at this time.

You can request it anytime, don't worry.

This reverts commit 274c10b.

See <francoismichel#45>.

francoismichel

Looks good, thanks!

TheoTechnicguy added 3 commits December 15, 2023 23:19

fix(client): prefer CLI args

ed016ea

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

fix(client): prefer config hostname over CLI

0012b92

Follow OpenSSH behaviour and prefer using the in configuration set hostname over the one in the CLI. fix: 35

TheoTechnicguy mentioned this pull request Dec 17, 2023

draft: fix(util): match non-pointer public key types #48

Merged

francoismichel mentioned this pull request Dec 17, 2023

Integration tests server checks #49

Merged

Merge branch 'main' into iss35

732da5a

glensc reviewed Dec 18, 2023

View reviewed changes

refactor(client): port comment typo

1c216c7

francoismichel requested changes Dec 18, 2023

View reviewed changes

refactor(client): review changes

274c10b

See <francoismichel#45>.

TheoTechnicguy requested a review from francoismichel December 18, 2023 21:27

francoismichel requested changes Dec 18, 2023

View reviewed changes

TheoTechnicguy added 2 commits December 18, 2023 22:40

Revert "refactor(client): review changes"

d11b7fb

This reverts commit 274c10b.

refactor(client): corrected review changes

5798137

See <francoismichel#45>.

francoismichel approved these changes Dec 18, 2023

View reviewed changes

francoismichel merged commit d868c6c into francoismichel:main Dec 18, 2023
7 checks passed

francoismichel mentioned this pull request Dec 18, 2023

bug: SSH configuration file takes precedence over CLI #35

Closed

TheoTechnicguy deleted the iss35 branch December 18, 2023 21:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(client): prefer CLI arguments, following OpenSSH behaviour #45

fix(client): prefer CLI arguments, following OpenSSH behaviour #45

TheoTechnicguy commented Dec 16, 2023

francoismichel commented Dec 17, 2023

TheoTechnicguy commented Dec 17, 2023

TheoTechnicguy commented Dec 17, 2023 •

edited

Loading

francoismichel commented Dec 17, 2023 •

edited

Loading

francoismichel commented Dec 17, 2023 •

edited

Loading

TheoTechnicguy commented Dec 17, 2023

francoismichel commented Dec 17, 2023 •

edited

Loading

TheoTechnicguy commented Dec 17, 2023

glensc Dec 18, 2023

TheoTechnicguy Dec 18, 2023

francoismichel left a comment

francoismichel Dec 18, 2023

francoismichel Dec 18, 2023

TheoTechnicguy Dec 18, 2023

francoismichel Dec 18, 2023

francoismichel Dec 18, 2023

francoismichel left a comment

francoismichel Dec 18, 2023

TheoTechnicguy Dec 18, 2023

TheoTechnicguy Dec 18, 2023

francoismichel Dec 18, 2023

francoismichel left a comment

	// programm termination. log.Fatal() exits with os.Exit(1).
	// program termination. log.Fatal() exits with os.Exit(1).

fix(client): prefer CLI arguments, following OpenSSH behaviour #45

fix(client): prefer CLI arguments, following OpenSSH behaviour #45

Conversation

TheoTechnicguy commented Dec 16, 2023

francoismichel commented Dec 17, 2023

TheoTechnicguy commented Dec 17, 2023

TheoTechnicguy commented Dec 17, 2023 • edited Loading

francoismichel commented Dec 17, 2023 • edited Loading

francoismichel commented Dec 17, 2023 • edited Loading

TheoTechnicguy commented Dec 17, 2023

francoismichel commented Dec 17, 2023 • edited Loading

TheoTechnicguy commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoismichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoismichel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francoismichel left a comment

Choose a reason for hiding this comment

TheoTechnicguy commented Dec 17, 2023 •

edited

Loading

francoismichel commented Dec 17, 2023 •

edited

Loading

francoismichel commented Dec 17, 2023 •

edited

Loading

francoismichel commented Dec 17, 2023 •

edited

Loading