deis / builder

Git server and application builder for Deis Workflow
https://deis.com
MIT License
40 stars 41 forks source link

feat(pkg/sshd): Store private keys in kubernetes secret #302

Closed bacongobbler closed 8 years ago

bacongobbler commented 8 years ago

Summary of Changes

This PR moves the SSH private keys into a kubernetes secret such that when the builder is restarted (or destroyed), it retains the same private keys for authenticating clients.

Since the host keys are supplied by kubernetes, we no longer need to generate the host keys, so that code has also been removed.

Issue(s) that this PR Closes

Closes #157

Associated End To End Test PR(s)

No changes required. The existing end-to-end tests ensure that the user can deploy applications to Workflow given this change. However, end-to-end won't work until deis/charts#237 is merged.

Associated Documentation PR(s)

No changes required.

Associated Design Document(s)

No design document required.

Testing Instructions

  1. Create a Deis Cluster using the chart changes in deis/charts#237
  2. Deploy a buildpack app (example-go)
  3. Destroy the builder with kubectl --namespace=deis destroy pod deis-builder-xxxx
  4. Deploy a second buildpack app

Originally, you would see a warning that "WARNING: REMOTE HOST IDENTIFICATION HAS CHANGED!" and to check ~/.ssh/known_hosts, but now it should pass without a warning.

Pull Request Hygiene TODOs

Please make sure the below checklist is complete.

smothiki commented 8 years ago

Again I'm little against hardcoding keys . As every have the same public key for each deis install. rather we should generate them randomly via helm generate

bacongobbler commented 8 years ago

All righty. I'll keep this out for beta3 until I can get helm generating the ssh keys for us. If not then we can keep this one out.

Marking as a WIP

bacongobbler commented 8 years ago

@smothiki see Masterminds/sprig#10 for an implementation :)

smothiki commented 8 years ago

see Masterminds/sprig#10 for an implementation :) :+1:

bacongobbler commented 8 years ago

How can I reproduce that?

smothiki commented 8 years ago

@bacongobbler wrong comments meant for this pR https://github.com/deis/controller/pull/651 deleteing and commenting in appropriate pr

smothiki commented 8 years ago

Masterminds PR got merged. So helm should work with the new ssh keys generating features?

mboersma commented 8 years ago

helm should work with the new ssh keys generating features

Once helm/helm#440 is merged and the 0.6.0 release is tagged, but not quite yet.

Update: helm 0.6.0 is released and will be a requirement for workflow-beta3.

Update: Did I say 0.6.0? Of course I meant 0.6.1.

smothiki commented 8 years ago

@mboersma no need to update helm :P

bacongobbler commented 8 years ago

@smothiki looks like ParseHostKeys is important:

><> git push deis master
ssh_exchange_identification: Connection closed by remote host
fatal: Could not read from remote repository.

Please make sure you have the correct access rights
and the repository exists.
><> kd logs deis-builder-8h6r3
2016/04/22 17:30:25 Running in debug mode
2016/04/22 17:30:25 Starting health check server on port 8092
2016/04/22 17:30:25 Starting deleted app cleaner
2016/04/22 17:30:25 Starting SSH server on 0.0.0.0:2223
[debug] Name: EXTERNAL_PORT, Val: 2223
[info] Listening on 0.0.0.0:2223
[info] Accepting new connections.
[info] Checking closer.
[info] Builder is running.
[info] Checking closer.
[info] Accepted connection.
[debug] Failed handshake: ssh: server has no host keys
bacongobbler commented 8 years ago

Manually tested with deis/charts#237 and helm 0.6.0:

><> deis create go
Creating Application... done, created go
Git remote deis added
><> git push deis master
The authenticity of host '[deis.10.245.1.3.xip.io]:2222 ([10.245.1.3]:2222)' can't be established.
ECDSA key fingerprint is 61:a2:5a:da:b8:28:71:f7:fb:0f:d0:2c:b0:cf:7c:c2.
Are you sure you want to continue connecting (yes/no)? yes
Warning: Permanently added '[deis.10.245.1.3.xip.io]:2222,[10.245.1.3]:2222' (ECDSA) to the list of known hosts.
Counting objects: 80, done.
...
><> kd get po
NAME                        READY     STATUS    RESTARTS   AGE
deis-builder-9hk88          1/1       Running   0          13m
deis-controller-6p9px       1/1       Running   1          37m
deis-database-n44zy         1/1       Running   0          37m
deis-logger-fluentd-3q84h   1/1       Running   0          37m
deis-logger-sc0xm           1/1       Running   0          37m
deis-minio-kbvvv            1/1       Running   0          37m
deis-registry-ukqdb         1/1       Running   1          37m
deis-router-kca6k           1/1       Running   0          37m
><> kd delete po deis-builder-9hk88
pod "deis-builder-9hk88" deleted
><> git remote rm deis
><> deis create goo
Creating Application... done, created goo
Git remote deis added
remote available at ssh://git@deis.10.245.1.3.xip.io:2222/goo.git
><> git push deis master
Counting objects: 80, done.
Delta compression using up to 8 threads.
Compressing objects: 100% (45/45), done.
Writing objects: 100% (80/80), 19.02 KiB | 0 bytes/s, done.
Total 80 (delta 30), reused 75 (delta 30)
...
smothiki commented 8 years ago

Oh I didn't realize the importance . Please add parse host keys I'll folowup on this issue again

bacongobbler commented 8 years ago

I forgot that this PR will keep going red until deis/charts#237 is merged. Since that PR does not affect anything else other than reliability on helm 0.6.0, I've merged it so we can test this.

arschles commented 8 years ago

:+1: for removing code