-
Notifications
You must be signed in to change notification settings - Fork 292
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
Support for SSH connection #1282
Conversation
@dmikusa-pivotal please try this out. You might need to turn off |
d1bc4f0
to
aa529c0
Compare
Work is still in progress. There are no prompts for password/passphrase. |
bf687e0
to
a999d08
Compare
Why is CI using go 1.14? |
Codecov Report
@@ Coverage Diff @@
## main #1282 +/- ##
==========================================
+ Coverage 80.11% 80.46% +0.35%
==========================================
Files 143 144 +1
Lines 8760 9039 +279
==========================================
+ Hits 7017 7272 +255
- Misses 1285 1301 +16
- Partials 458 466 +8
Flags with carried forward coverage won't be shown. Click here to find out more. |
f8f7fff
to
38b1b11
Compare
cmd/cmd.go
Outdated
Identity: os.Getenv("DOCKER_HOST_SSH_IDENTITY"), | ||
PassPhrase: os.Getenv("DOCKER_HOST_SSH_IDENTITY_PASSPHRASE"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How should we name these envvars?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the same ones that Docker uses? If so, we should keep them
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No they are not used by docker. It's my addition. With docker you need to put identity at default location with default name or add it to ssh-agent, you cannot specify identity file AFAIK.
@dmikusa-pivotal please try this out. |
Also it would be great if somebody tried this on Windows. |
@joshuawhite929 please try this out. |
Sorry for the delay. It is working for me. I checked out your code, Looks good! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(fearfully) attempting to validate on Windows here. But not really sure how these VMs are set up to be able to communicate over SSH.
@dmikusa-pivotal, do you have some sort of ssh proxy forwarding traffic from 22 to a Docker daemon listening on a port? Maybe we can look at your setup together to speed this along.
cmd/cmd.go
Outdated
Identity: os.Getenv("DOCKER_HOST_SSH_IDENTITY"), | ||
PassPhrase: os.Getenv("DOCKER_HOST_SSH_IDENTITY_PASSPHRASE"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these the same ones that Docker uses? If so, we should keep them
You need a VM with two things to make this work:
There's nothing specific you have to do to Docker to make it support this. It's basically functionality in the docker client & now pack to run docker commands remotely. Hope that helps! |
@dmikusa-pivotal Thanks for the help. I followed your instructions but am still having trouble using an ssh connection on a windows machine. I am able to use the ssh client directly. Please let me know if I'm doing something wrong. Happy to look at this with someone. PS> $env:DOCKER_HOST="ssh://aemengo@127.0.0.1"
PS> $env:DOCKER_HOST_SSH_IDENTITY="C:\Users\aemengo\Desktop\rdp\id_ed25519"
PS> pack build aemengo/hello -p ..\hello -B cnbs/sample-builder:dotnet-framework-1809
ERROR: failed to build: failed to fetch builder image 'index.docker.io/cnbs/sample-builder:dotnet-framework-1809': error during connect: Post "http://example.com%2F/v1.38/images/create?fromImage=cnbs%2Fsample-builder&tag=dotnet-framework-1809": ssh: rejected: connect failed (open failed) |
@aemengo - FYI, I only tested with password-less login. I'm not sure if it works if you have to login over ssh. Are you able to Also, can you |
There should already be implemented prompt for password and for temporary trusting the server. |
So |
@aemengo Does something like: |
also what Daniel said: try whether |
@aemengo could you please try Windows again, I did some changes. |
@dmikusa-pivotal I did some changes could you please re-test it? |
How can I rerun |
@matejvasek Don't know your magic, but things seems to be working well on my windows machine 👍🏾 |
I allowed use of standard |
I could also do it other way around: use tunneling if possible (i.e. with everything but |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I'm a little afraid of introducing something that a select few maintainers are likely to debug. But it did work stellarly during my testing.
Once again, we can't thank you enough for a PR of such depth.
Nice work on buildpack ssh support @matejvasek |
cmd/cmd.go
Outdated
if err != nil { | ||
return pack.Client{}, err | ||
} | ||
|
||
return *client, nil | ||
} | ||
|
||
func tryInitSSHDockerClient() (dockerClient.CommonAPIClient, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we move these functions to a separate file in this directiory, maybe docker_init.go
? I like keeping cmd/cmd.go
simple and self-contained
This is wildly impressive work, @matejvasek . I am a bit nervous about the future upkeep for this, so I would definitely rather the ssh_dialers functionality comes through an imported function from podman; if they'd consider exposing the required api and we could use that, I think it would definitely make it a lot safer for us to import it. |
cmd/cmd.go
Outdated
func newHostKeyCbk() sshdialer.HostKeyCallback { | ||
var trust []byte | ||
return func(hostPort string, pubKey ssh.PublicKey) error { | ||
if bytes.Equal(trust, pubKey.Marshal()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see trust
being persisted. Should it be? What's the experience connecting to the same connection a second time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be written into ~/.ssh/known_hosts
. But I am not too eager to write into system files. I could however print hint, something like: "add this line {key record here} into your ~/.ssh/known_hosts
, or "to trust the key connect to the host via ssh
first". BTW standard docker
CLI is not solving this at all, you must add trust by yourself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So right now you are asked each time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW standard docker CLI is not solving this at all, you must add trust by yourself.
to be more specific you will get:
~> docker image ls
error during connect: Get "http://docker.example.com/v1.24/images/json": command [ssh -l root -p 45383 -- localhost docker system dial-stdio] has exited with exit status 255, please make sure the URL is valid, and Docker 18.09 or later is installed on the remote host: stderr=ssh_askpass: exec(/usr/libexec/openssh/ssh-askpass): No such file or directory
Host key verification failed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also standard docker
CLI does not support password authentication.
And you cannot specify identity file (and its passphrase) -- it either has to be at standard location with standard name,
or it has to be added to ssh-agent
. Passphrase reading also has to be handled by ssh-agent
.
@jromero wrt host key callbck
We are not persisting it anywhere yet. I could print instruction what line should be appended to
You can forward output of the |
@dfreilich I totally agree this is not best place to keep it. Initial I created separate repo: https://github.com/matejvasek/sshdialer. So latter I moved it here, one of the reasons was that I really liked your GithubActions. You even have custom runner for Windows with Linux containerization which I found really useful. I could talk to people from https://github.com/containers about it. But I am afraid I won't be able to run WIndows tests in CI there. |
ab7c7ec
to
60c9002
Compare
cmd/cmd.go
Outdated
|
||
if answer == "yes" || answer == "y" { | ||
trust = pubKey.Marshal() | ||
fmt.Fprintf(os.Stderr, "To avoid this in future add following line into your ~/.ssh/known_hosts:\n%s %s %s\n", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jromero here I added message for user with instruction how to make server trusted. Is it OK?
118639b
to
712ff06
Compare
@jromero @dfreilich PTAL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I appreciate the rigorous tests, which definitely makes it a lot easier to support. At the same time, if the podman team is open to supporting this, that would be great, and I'd appreciate adding a link to an issue on their repo so we can track this integration.
@jromero , what do you think? Holding it off until your approval.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nits.
Overall good. Excited to get this in.
Could you resolve the merge conflict? I can then give it a quick test and merge this in.
EDIT: Looks like conflict is resolved.
I am going to squash review requested fixups. |
Signed-off-by: Matej Vasek <mvasek@redhat.com>
81503c3
to
224e0a4
Compare
Summary
This allows for connection via SSH by setting
DOCKER_HOST
.Documentation
Related
Resolves #1259