Mount the sshdir into the helm-operator too

[stephenmoloney]

What is this MR for ?

• Mounting the sshdir into the helm operator container

• The known_hosts is only mounted in the flux container thus far.

I'm not 100% sure that the known_hosts are meant to be available in the helm-operator but
my guess is that they are. Perhaps someone can confirm?

[stephenmoloney]

note to self: I should add this change to the helm-deploy folder too. Lets see if in principle this change will be accepted first, then I'll add it to the helm-deploy too.

[squaremo]

Yes, this looks good and right.

I think we'll also need to change the helm-operator in line with #1154, so that mounting into /root/.ssh doesn't wipe out the config from the image.

[stephenmoloney]

I had an alternative thought on how to handle the known hosts.
Rather than bake the github.com, gitlab.com bitbucket.com hosts known_keys into the docker image, maybe a different approach might be to specify the hosts as a list in the yaml values in helm .
Then an init container will run and generate the known hosts at startup and save them into the /root/.ssh/known_keys file.
I kinda like this idea as from a user's perspective, entering multiline keys in the yaml under known_hosts is actually quite cumbersome.

[squaremo]

Rather than bake the github.com, gitlab.com bitbucket.com hosts known_keys into the docker image, maybe a different approach might be to specify the hosts as a list in the yaml values in helm .
Then an init container will run and generate the known hosts at startup and save them into the /root/.ssh/known_keys file.

This would work. But the reason we switch on StrictHostKeyChecking though is that it prevents man-in-the-middle attacks, by making someone supply the key(s) ahead of time[1]. Scanning for the keys on startup undermines this, because you're introducing an opportunity for someone to give you a duff host key.

If you provided the fingerprint alongside the host name in the initial config, that would be enough to avoid that problem, while still being more convenient than assembling the configmap.

[1] We don't do this properly, mind you -- the docs ought to advise that you check the fingerprints; and, we really ought to check in a known_hosts file for github etc., rather than scanning for keys in the build.

[stephenmoloney]

Ok, the Dockerfile gave me the impression that the practice of scanning for the hostkey was ok.

So if it is not ok, the Dockerfile is missing a step to check the host keys - we should open an issue for that so that it is checked in the Dockerfile.

Also, in this case, the idea of an init_container doesn't work.

So using known_hosts is fine.

Also you are right about the known hosts are added to the volume, they should (maybe?) be appended to the existing file rather than outright overriding the existing file. I suppose that operation could be put into an init_container.

[squaremo]

So if it is not ok, the Dockerfile is missing a step to check the host keys - we should open an issue for that so that it is checked in the Dockerfile.

#1169

[stephenmoloney]

I think we'll also need to change the helm-operator in line with #1154, so that mounting into /root/.ssh doesn't wipe out the config from the image.

I've changed it to use a subPath now and a specific file so the /root/.ssh/config crystallized inside the docker image is unaffected.

[stephenmoloney]

Also, would you prefer I squash this PR or leave as is?

[squaremo]

You could squash the second commit (use a subPath) into the first, but it's OK as-is.

(The required change is to not use template syntax in the example YAML)

[squaremo]

"uncomment the ssh config settings in helm-operator-deployment.yaml"

^ this ought to read "Comments the ssh config ..." (in the commit message as well)

[stephenmoloney]

@squaremo I think the git history is a bit cleaner now if you want to have another look.

[squaremo]

Brill, thanks for your perseverance @stephenmoloney.