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

Add JENKINS_LOW_SECURITY_DOMAIN feature. #4

Open
wants to merge 5 commits into
base: 2.x
Choose a base branch
from

Conversation

grayside
Copy link
Contributor

This adds the ability to set an env var with a domain to add to github and bitbucket as "keyscan hosts".

It also has a fix for phase2/docker-build#7, which we need to solve in both repos. Maybe that script should be moved to a utils repo and curl'd on docker build?

@febbraro
Copy link
Member

Just a thought, another option is to disable host verification.

@febbraro
Copy link
Member

I see. you are doing that already. :)

Do we want this to be a comma separated list of domain?

@@ -3,10 +3,9 @@
KEY_BASE=/root/.ssh
KEY_FILE=$KEY_BASE/outrigger.key

if [ -e $KEY_FILE ]; then

if [ -e $KEY_FILE ] && [ ! -d $KEY_FILE ]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would that be a directory?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you mount a non existent file on your host with a volume mount an empty directory shows up on both sides.

@grayside
Copy link
Contributor Author

Role of the Key Scan

I found we were already disabling host verification, but in my reading to understand what I was about to do, I found the ssh-keyscan helps mitigate some of the more braindead MitM attacks that could be run. That only works if you /do not/ send keyscan output to /dev/null. In fact, keyscan to dev/null seems to negate the purpose of it?

Comma-delimited

I could not figure out how to support a comma-delimited list without doing string parsing.. unless the ssh-keyscan and ssh config can both take comma-delimited lists, which did not seem like the case.

Why would it be a directory?

Per phase2/docker-build#7, with a default volume mount in place, if you do not have the default key it will do what any source-missing docker volume mount will do, and create an empty directory. This closes that edge case so the script fails gracefully instead of erroring through all the steps.

@febbraro
Copy link
Member

febbraro commented Apr 5, 2017

I see you left in the 2>/dev/null, but you said that is a security issue? Should we remove it?

@grayside
Copy link
Contributor Author

grayside commented Apr 5, 2017

I wasn't 100% that removing it was the correct thing to do, so I waited to see if you knew better.

From the manpage:

SECURITY
I an ssh_known_hosts file is constructed using ssh-keyscan without verifying the keys, users will be vulnerable to man in the middle attacks. On the other hand, if the security model allows such a risk, ssh-keyscan can help in the detection of tampered keyfiles or man in the middle attacks which have begun after the ssh_known_hosts file was created.

Furthermore, this indicates we should only need the keyscan:
http://askubuntu.com/questions/123072/ssh-automatically-accept-keys

I'm increasingly unsure what our best bet is. Maybe we should allow strict key checking and rely on the keyscan to preload known hosts properly? If I understand, that means we can be MitM'd when the keyscan is run, but after that the key checking will make sure the same host is being reached so new/different attacks will fail.

@febbraro
Copy link
Member

febbraro commented Apr 5, 2017

Basically what you mean is that by not seeing the big honking warning we could miss the fact that we are being MitM'd. Since we --rm the container generally with our runs docker-compose -f build.yml run --rm <command container> are we not downloading it each and every time anyway? If that is the case we wont know if it is changed if each and every command run has to request that file each time. No?

@grayside
Copy link
Contributor Author

grayside commented Apr 5, 2017

That assumes processes that don't hit the same SSH server/git server multiple times. We do have that use case: think of a build that hits github, bitbucket, or drupal.org multiple times each for git checkouts. The combination of /dev/null with no strict checking means we are skipping any guarantee of knowing who we are talking to. The keyscan with no strict checking seems like it might mean we know we are talking to the same server that we keyscanned, though we are not checking that server is who we think it is.

@febbraro
Copy link
Member

febbraro commented Apr 7, 2017

So, the main reason we hid that output was in case you wanted to parse output from the container. Do we want to rethink that? or have some env var we can sent to quiet that?

@grayside
Copy link
Contributor Author

Okay, circled back here to follow-up on our decision to skip the keyscan and just go with disabling strict key checking for designated domains. The README documentation has been updated to describe how this works.

Unfortunately in my local testing, something is going wrong with the confd templating process for ~/.ssh/config, and I can't see what's wrong. Expand the details element below to review.

docker run output
╚╩═▶ docker run -l com.dnsdock.name=jenkins -l com.dnsdock.image=outrigger outrigger/jenkins-docker
[s6-init] making user provided files available at /var/run/s6/etc...exited 0.
[s6-init] ensuring user provided files have correct perms...exited 0.
[fix-attrs.d] applying ownership & permissions fixes...
[fix-attrs.d] 00-logs-nobody: applying...
[fix-attrs.d] 00-logs-nobody: exited 0.
[fix-attrs.d] done.
[cont-init.d] executing container initialization scripts...
[cont-init.d] 10-run-confd: executing...
=> Running confd to write configuration templates
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Backend set to env
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Starting confd
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Backend nodes set to
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Target config /var/jenkins_home/init.groovy.d/basic-security.groovy out of sync
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Target config /var/jenkins_home/init.groovy.d/basic-security.groovy has been updated
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Target config /var/jenkins_home/jenkins.install.InstallUtil.lastExecVersion out of sync
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Target config /var/jenkins_home/jenkins.install.InstallUtil.lastExecVersion has been updated
2017-04-26T06:11:24Z 87111d91b024 confd[197]: ERROR open /root/.ssh/.config432196586: no such file or directory
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Target config /var/log/services/README.txt out of sync
2017-04-26T06:11:24Z 87111d91b024 confd[197]: INFO Target config /var/log/services/README.txt has been updated
[cont-init.d] 10-run-confd: exited 1.

!!!!!
 init-stage2 failed.
!!!!!

@tekante
Copy link
Member

tekante commented Jul 9, 2017

The error was because there is no /root/.ssh directory by default and confd doesn't create directories (and I couldn't find a configuration that would make it do so). I pushed up a commit that creates it in the Dockerfile. We could also create the directory structure in root with a .gitkeep file instead though I don't see any particular advantage to that if we don't want to drop other files in.

Seems to boot up and honor the low security domains just fine though there are a significant number of plugin errors that get reported.

@grayside
Copy link
Contributor Author

grayside commented Jul 10, 2017

Thanks for digging in here @tekante. Given we are planning to deprecate Jenkins do you think continuing to spend energy to finish fixing this is worthwhile?

@tekante
Copy link
Member

tekante commented Jul 10, 2017

I'm not sure there is great value for us in fixing it (especially against Jenkins 2 which we aren't even widely using yet) though having this as a configurable option is a nice feature.

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

Successfully merging this pull request may close these issues.

3 participants