[JENKINS-69149] Set 'Known hosts file' strategy as default #882

Pldi23 · 2022-07-29T20:38:20Z

JENKINS-69149 Set 'Known hosts file' strategy as default

Alternatively to #881.
Change default ssh host key checking strategy to 'Known hosts file' and add hint log for users that uses cli git and 'Accept first connection' to avoid using it on OpenSSH < 7.6.

MarkEWaite · 2022-07-29T21:39:07Z

I prefer this over #881 but want to do some interactive testing with CentOS 7 systems and want to check the upgrade behavior for users that have already installed the git client plugin with the security fix.

dwnusbaum · 2022-07-29T22:21:05Z

src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java

@@ -2697,6 +2698,9 @@ private String launchCommandIn(ArgumentListBuilder args, File workDir, EnvVars e

            return stdout;
        } catch (GitException | InterruptedException e) {
+            if (getHostKeyFactory() instanceof AcceptFirstConnectionVerifier) {
+                listener.getLogger().println("If you are using OpenSSH < 7.6 please choose another strategy to verify ssh host key in ‘Manage Jenkins’ -> ‘Configure Global Security’ -> ‘Git Host Key Verification Configuration'");


Can we tighten the check for printing this message? For example, if (e.getMessage().contains("unsupported option "accept-new")) { ... }.

Also, it could be nice to make the message into a hyperlink to https://plugins.jenkins.io/git-client/#plugin-content-ssh-host-key-verification using HyperlinkNote.

Also, I think you need to update README.md to update the reference to the default setting. Maybe a help.html file also needs to be updated?

One other thing - for Jenkins instances on fresh servers, KnownHostsFileVerificationStrategy will fail because known_hosts will be empty, right? That was one nice thing about AcceptFirstConnectionVerifier. Maybe we should also check what that error message is like and print a special message here in that case as well.

Can we tighten the check for printing this message? For example, if (e.getMessage().contains("unsupported option "accept-new")) { ... }.

Also, it could be nice to make the message into a hyperlink to https://plugins.jenkins.io/git-client/#plugin-content-ssh-host-key-verification using HyperlinkNote.

Also, I think you need to update README.md to update the reference to the default setting. Maybe a help.html file also needs to be updated?

done in 9dea3f2

One other thing - for Jenkins instances on fresh servers, KnownHostsFileVerificationStrategy will fail because known_hosts will be empty, right? That was one nice thing about AcceptFirstConnectionVerifier. Maybe we should also check what that error message is like and print a special message here in that case as well.

In that case the error message would be a 'usual' message, signalling that no host keys was provided for Host, see
console screenshots:

Cli git:

JGit:

I guess on fresh servers known_hosts file most likely not exists, what if we manually check it and print hint message in that case? see f8d6af0

…nown_hosts_file # Conflicts: # images/ssh-host-key-verification.png

MarkEWaite · 2022-07-31T18:32:21Z

I'd like to see additional interactive testing to check some cases that I think might hide surprises. I hope to assist with that Monday or Tuesday so that we can merge and release this change this in the first week of August.

Interactive test results I'd like to see:

New install of controller on CentOS 7 (or AWS Linux 2 or ...), confirm that known hosts strategy is used as default and that ssh authenticated repository clone fails as expected because the known hosts file is not yet available. Confirm that once the known hosts file is created on the controller, then ssh authenticated repository clone passes
New install of agent on CentOS 7, confirm that known hosts is used as default and that ssh authenticated repository clone fails as expected because the known hosts file is not yet available. Confirm that once the known hosts file is created on the agent, then ssh authenticated repository clone passes
Upgrade from git client plugin 3.11.0 on CentOS 7 controller, confirm that known hosts is used as default and that it fails until the known hosts file is created (Confirmed upgrade from 3.11.0 to 3.11.2-snapshot used known_hosts when it existed and failed when it did not. This check passed. Note that the known_hosts format accepted by OpenSSH 7.4 is slightly different from the format accepted by newer versions. The simple format I use elsewhere with github.com key-type key-value was not accepted)
Upgrade from git client plugin 3.11.0 on CentOS 7 agent, confirm that known hosts is used as default and that it fails until the known hosts file is create
Upgrade from git client plugin 3.11.0 on CentOS 7 controller, confirm that known hosts is used as default and that it fails until the known hosts file is created (confirmed that clone fails on new controller until a known_hosts file is created. This check passed)
Upgrade from git client plugin 3.11.1 on CentOS 7 controller, confirm that previously configured strategy is not reset by the installation of the new plugin version and that it fails until the known hosts file is create (upgrade from 3.11.1 to the PR build did not retain the previous configuration. I had not saved the previous configuration, so possibly it was not retained because it had never been written to the configuration file. When I saved the previous configuration with git client plugin 3.11.1 then upgraded to this plugin, the previous configuration was retained as expected. This check passed)

Pldi23 · 2022-08-01T06:56:31Z

Thanks @MarkEWaite , I’ve tested all 5 cases (p3 and p5 looks the same) locally on MacOS + docker linux agent. Please re-verify fix on CentOS 7.

Could you also please test that hint message for AcceptFirstConnection displayed correctly (I don’t have environment to reproduce unsupported option "accept-new”. exception)?

Steps to quickly test it:

use System with OpenSSH < 7.6.
use cli git as Git installation.
use 'Accept first connection' as strategy.
start a build and check console displays message "If you are using OpenSSH < 7.6 please choose another strategy to verify ssh host key in ‘Manage Jenkins’ -> ‘Configure Global Security’ -> ‘Git Host Key Verification Configuration'".

jtnord · 2022-08-01T09:37:50Z

src/main/java/org/jenkinsci/plugins/gitclient/verifier/KnownHostsFileVerifier.java

@@ -54,4 +64,11 @@ public boolean verifyServerHostKey(String hostname, int port, String serverHostK
        }
    }

+    private void logHint(TaskListener listener) {
+        listener.getLogger().println(HyperlinkNote.encodeTo("https://plugins.jenkins.io/git-client/#plugin-content-ssh-host-key-verification/","You're using 'Known host file' strategy to verify ssh host keys," +
+                " but your known_hosts file not exists, please go to " +


Suggested change

" but your known_hosts file not exists, please go to " +

" but your known_hosts file does not exist, please go to " +

jimklimov · 2022-08-01T14:11:39Z

src/main/java/org/jenkinsci/plugins/gitclient/verifier/KnownHostsFileVerifier.java

-                " but your known_hosts file not exists, please go to " +
-                "‘Manage Jenkins’ -> ‘Configure Global Security’ -> ‘Git Host Key Verification Configuration' " +
+                " but your known_hosts file does not exist, please go to " +
+                "‘Manage Jenkins’ -> ‘Configure Global Security’ -> ‘Git Host Key Verification Configuration‘ " +


Should there be non-ASCII "funny quotes" emitted here? I think standard single-quotes can suffice to keep things simple?

Or is there a precedent for using them anyway?

let's use simple quotes to be safe

MarkEWaite

Thanks for the changes.

src/main/java/org/jenkinsci/plugins/gitclient/verifier/KnownHostsFileVerifier.java

src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java

src/main/java/org/jenkinsci/plugins/gitclient/verifier/KnownHostsFileVerifier.java

README.adoc

MarkEWaite

I think we're ready to merge and release. Any objections from other reviewers?

Do we need to backport this change to other branches that were used for the security fix?

…nown_hosts_file

MarkEWaite · 2022-08-03T20:03:18Z

@Pldi23 I think that we should merge the 3.11.x branch into the master branch rather than merge this pull request. That has the side effect of squashing all the changes made in this pull request into the few that arrived on the 3.11.x branch. Is that OK with you?

That will then allow me to use the master branch for the 3.12.0 release that includes JGit 5.13.1 and can update the minimum required Jenkins version to something much newer than 2.289.1

Pldi23 · 2022-08-03T20:06:09Z

@Pldi23 I think that we should merge the 3.11.x branch into the master branch rather than merge this pull request. That has the side effect of squashing all the changes made in this pull request into the few that arrived on the 3.11.x branch. Is that OK with you?

@MarkEWaite yes, sure

MarkEWaite · 2022-08-03T20:14:31Z

Thankkos @Pldi23 . I'll release 3.10.0.2 now and will try to release 3.11.2 within the next hour.

MarkEWaite · 2022-08-04T14:42:00Z

Thanks to @Pldi23 for all the work on this one. Included in the master branch from #888. Closing this pull request

change default strategy and log hint

f5506e7

github-actions bot added the test Automated test addition or improvement label Jul 29, 2022

Pldi23 mentioned this pull request Jul 29, 2022

[JENKINS-69149] Use Java for accept-new in StrictHostKeyChecking #881

Closed

9 tasks

dwnusbaum reviewed Jul 29, 2022

View reviewed changes

Pldi23 added 3 commits July 30, 2022 23:12

print hint as Hyperlink, update ReadMe and help

9dea3f2

Merge branch 'master' into JENKINS_69149_switch_default_strategy_to_k…

df03157

…nown_hosts_file # Conflicts: # images/ssh-host-key-verification.png

print hint for 'Known host file' if known_hosts not exists

f8d6af0

jtnord reviewed Aug 1, 2022

View reviewed changes

grammatical mistake in log

6c9828f

jimklimov reviewed Aug 1, 2022

View reviewed changes

change quotes

274abd8

MarkEWaite requested changes Aug 1, 2022

View reviewed changes

src/main/java/org/jenkinsci/plugins/gitclient/verifier/KnownHostsFileVerifier.java Outdated Show resolved Hide resolved

Remove trailing slash from hyperlink

369bbf5

MarkEWaite self-requested a review August 2, 2022 00:00

MarkEWaite reviewed Aug 2, 2022

View reviewed changes

src/main/java/org/jenkinsci/plugins/gitclient/CliGitAPIImpl.java Outdated Show resolved Hide resolved

Remove another trailing slash on an anchor

dcadfda

MarkEWaite reviewed Aug 2, 2022

View reviewed changes

src/main/java/org/jenkinsci/plugins/gitclient/verifier/KnownHostsFileVerifier.java Outdated Show resolved Hide resolved

MarkEWaite reviewed Aug 2, 2022

View reviewed changes

README.adoc Outdated Show resolved Hide resolved

MarkEWaite added 2 commits August 1, 2022 18:32

Minor change of word

83eac00

Minor phasing change

077f683

MarkEWaite self-requested a review August 2, 2022 00:35

MarkEWaite approved these changes Aug 2, 2022

View reviewed changes

Merge branch 'master' into JENKINS_69149_switch_default_strategy_to_k…

a6483e0

…nown_hosts_file

basil approved these changes Aug 2, 2022

View reviewed changes

quotes change for accept-new message

978264c

NotMyFault mentioned this pull request Aug 2, 2022

SECURITY-1468's solution impacts ATH builds jenkinsci/acceptance-test-harness#877

Closed

Pldi23 mentioned this pull request Aug 3, 2022

[Backport 3.10.0.x][JENKINS-69149] Set 'Known hosts file' strategy as default #887

Merged

Pldi23 mentioned this pull request Aug 3, 2022

[Backport 3.11.x][JENKINS-69149] Set 'Known hosts file' strategy as default #888

Merged

Use narrower image that includes menu breadcrumbs

59d9ed8

MarkEWaite added bugfix and removed test Automated test addition or improvement labels Aug 3, 2022

MarkEWaite closed this Aug 4, 2022

MarkEWaite added bug Incorrect or flawed behavior and removed bugfix labels Nov 12, 2023

	" but your known_hosts file not exists, please go to " +
	" but your known_hosts file does not exist, please go to " +

[JENKINS-69149] Set 'Known hosts file' strategy as default #882

[JENKINS-69149] Set 'Known hosts file' strategy as default #882

Uh oh!

Conversation

Pldi23 commented Jul 29, 2022