Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SECURITY-1468's solution impacts ATH builds #877

Closed
NotMyFault opened this issue Aug 2, 2022 · 10 comments · Fixed by #886
Closed

SECURITY-1468's solution impacts ATH builds #877

NotMyFault opened this issue Aug 2, 2022 · 10 comments · Fixed by #886
Labels

Comments

@NotMyFault
Copy link
Member

NotMyFault commented Aug 2, 2022

After investigating into the failure of #876, ATH is impacted by the security fix in the git-client-plugin too, jenkinsci/git-client-plugin#875, because ATH runs on CentOS 7 and doesn't know how to deal with "accept new" as strategy.

Later reading over JENKINS-69149, am I correct assuming that jenkinsci/git-client-plugin#882 restores the functionality of ATH, once merged and released?

@NotMyFault NotMyFault added the bug label Aug 2, 2022
@dwnusbaum
Copy link
Member

dwnusbaum commented Aug 2, 2022

am I correct assuming that jenkinsci/git-client-plugin#882 restores the functionality of ATH, once merged and released?

Unless Main.isUnitTest is set when running the ATH (I don't think it is), then if the ATH is built on ephemeral agents with a blank known_hosts file, or if it connects to dynamically created Git servers where the host keys change every time the test runs, then no, it will still need to be adapted.

From a quick look, I think that GitPluginTest and similar tests will need to either populate known_hosts on the agent with the host key information from the container running the Git server, or they will need to configure the controller to use the "Manually Provided Keys" host key verifications strategy and provide the keys for the Git server there, or maybe configure the "No verification" host key verification strategy if we know the ATH only connects to local Git servers and we do not care about MitM attacks.

CC @Pldi23 in case my comment is not quite right

@timja
Copy link
Member

timja commented Aug 2, 2022

Or we can move to recent Ubuntu with accept-new

@NotMyFault
Copy link
Member Author

Or we can move to recent Ubuntu with accept-new

I started with that in #878 earlier, needs some fixup in terms of Java directories to get it to work 👀

@dduportal
Copy link
Collaborator

Dropping a note: we discussed this issue in today's infra meeting because the weekly release 2.363 was also bitten by this behavior on ci.jenkins.io.

Expect a fix in ci.jenkins.io wednesday 10th of August to apply the config as code change.

@timja
Copy link
Member

timja commented Aug 9, 2022

It’s an ATH framework change though, we’re trying to upgrade to a more maintained OS but tests are failing on it :(

@dduportal
Copy link
Collaborator

It’s an ATH framework change though, we’re trying to upgrade to a more maintained OS but tests are failing on it :(

Depends if the git-client is configured to use JGit or native git on ci.jenkins.io if I'm not mistaken.
At least the infra team can set up the expected host keys from github.com for cases with JGit.

If the ATH uses the native git client, then adding an instruction ssh-keyscan -H github.com >> /home/jenkins/.ssh/known_hosts should also help

@dduportal
Copy link
Collaborator

Manual configuration to validate:

@timja
Copy link
Member

timja commented Aug 10, 2022

it's not checking out via ci.jenkins.io, it's checking out in the tests.

Those changes won't impact it

@Pldi23
Copy link
Contributor

Pldi23 commented Aug 11, 2022

I created a #886 with proposal to use 'No verification' strategy for ATH, because git-client 3.11.2 uses 'Known hosts file' as default strategy instead of 'Accept first connection' used in git-client 3.11.1, so tests will continue to fail even when we switch recent Ubuntu. I only worries if we are sure that ATH only connects to local git servers and we could not worry about MitM attacks?

@timja
Copy link
Member

timja commented Aug 11, 2022

We ignored it the last 10 years or whatever I don't see it being a concern in this repository

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants