Skip to content

Conversation

@lifubang
Copy link
Contributor

Signed-off-by: Lifubang lifubang@acmcoder.com

- What I did
As the issue #1476, when use ssh with password, docker run -it will fail.

- How I did it
Because after attachContainer, ssh can't use stdin to type password.
So, in func attachContainer, use a call back func beforeHijack to run ContainerStart before hijackedIOStreamer.

- How to verify it
After fix:

docker -H ssh://localhost  run --name=testssh -it --entrypoint=/bin/bash docker.acmcoder.com/public/ubuntu:ssh
root@localhost's password: 
root@localhost's password: 
root@localhost's password: 
root@25eaf3b846dc:/# ls -lh
total 64K
drwxr-xr-x   2 root root 4.0K Oct  1  2015 bin
drwxr-xr-x   2 root root 4.0K Apr 10  2014 boot

- Description for the changelog
modify attachContainer add a param "beforeHijack func() error"

- A picture of a cute animal (not mandatory but encouraged)
h _zaman0 0 al 3 _luj

@codecov-io
Copy link

codecov-io commented Oct 23, 2018

Codecov Report

Merging #1477 into master will decrease coverage by 0.95%.
The diff coverage is 25%.

@@            Coverage Diff             @@
##           master    #1477      +/-   ##
==========================================
- Coverage   55.14%   54.19%   -0.96%     
==========================================
  Files         289      289              
  Lines       19371    19386      +15     
==========================================
- Hits        10683    10506     -177     
- Misses       7997     8204     +207     
+ Partials      691      676      -15

@lifubang
Copy link
Contributor Author

ping @AkihiroSuda PTAL

@AkihiroSuda
Copy link
Collaborator

Thx, can we have e2e?

@lifubang lifubang force-pushed the runusessh branch 26 times, most recently from f588bbe to 3f8b8a8 Compare October 24, 2018 04:32
@lifubang lifubang force-pushed the runusessh branch 11 times, most recently from b121f24 to ed51c23 Compare October 24, 2018 10:07
@lifubang
Copy link
Contributor Author

Thx, can we have e2e?
@AkihiroSuda It's very hard to add e2e case, because:

  1. We need expect to auto input user's password;
  2. But expect have no stderr;
  3. There is at least one "root@localhost's password:" in stdout, it's very hard to remove it.

So, I think it's very hard to add e2e case.
You can see my commit: lifubang@b121f24
I think we can add e2e cases in a new PR if we need.

AkihiroSuda added a commit to AkihiroSuda/docker-cli that referenced this pull request Oct 25, 2018
The issue with password auth is tracked in docker#1476 and docker#1477 .

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

if beforeHijack != nil {
err := beforeHijack()
if err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: if err := beforeHijack(); err != nil {

AkihiroSuda
AkihiroSuda previously approved these changes Oct 25, 2018
@AkihiroSuda
Copy link
Collaborator

Yes, E2E can be added separately, but I think password auth should not be considered to be supported officially for now then
#1482

thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Oct 25, 2018
The issue with password auth is tracked in docker#1476 and docker#1477 .

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
(cherry picked from commit 16b014e)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
<-statusChan
}
return runStartContainerErr(err)
if !attach {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this if !attach can be moved as an else to the previous condition?

if attach {
...
} else {
// start the container
...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, you are right.

Signed-off-by: Lifubang <lifubang@acmcoder.com>
@AkihiroSuda
Copy link
Collaborator

What's current status?

@lifubang
Copy link
Contributor Author

lifubang commented Mar 2, 2019

This patch can input password correctly. But I can't find a way to add a test case, I have no idea now. If someone have a time, please take a look #1487 . Thanks.

@makinori
Copy link

Will this be fixed soon? I cant use my development docker cli with my home server. I really need this!

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.

7 participants