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

Mount ssh directory as separate volume #315

Merged
merged 3 commits into from
Aug 28, 2021

Conversation

ihor-sviziev
Copy link
Contributor

@ihor-sviziev ihor-sviziev commented Mar 5, 2021

This PR fixes a similar issue to #289, but with ssh known_hosts file.
Basically it does the same as #304 and #308

Steps to reproduce:

  1. start the environment and connect to GitHub:
warden env up
warden shell -T -c 'ssh git@github.com'
first time it will ask you to confirm fingerprint, like this:
  The authenticity of host 'github.com (140.82.121.3)' can't be established.
RSA key fingerprint is SHA256:nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8.
RSA key fingerprint is MD5:16:27:ac:a5:76:28:2d:36:63:1b:56:4d:eb:df:a6:48.
Are you sure you want to continue connecting (yes/no)? yes
Warning: Permanently added 'github.com,140.82.121.3' (RSA) to the list of known hosts.
PTY allocation request failed on channel 0
Hi ihor-sviziev! You've successfully authenticated, but GitHub does not provide shell access.
Connection to github.com closed.
  1. try again to run warden shell -T -c 'ssh git@github.com' - there should not be any additional confirmations
PTY allocation request failed on channel 0
Hi ihor-sviziev! You've successfully authenticated, but GitHub does not provide shell access.
Connection to github.com closed.
  1. Stop the environment, run it again and try the same command again:
warden env down
warden env up
warden shell -T -c 'ssh git@github.com'

Actual result: (Before)
❌ known_hosts file is missing, it's asking again for confirming GitHub host:

  The authenticity of host 'github.com (140.82.121.4)' can't be established.
RSA key fingerprint is SHA256:nThbg6kXUpJWGl7E1IGOCspRomTxdCARLviKw6E5SY8.
RSA key fingerprint is MD5:16:27:ac:a5:76:28:2d:36:63:1b:56:4d:eb:df:a6:48.
Are you sure you want to continue connecting (yes/no)? yes
Warning: Permanently added 'github.com,140.82.121.4' (RSA) to the list of known hosts.
PTY allocation request failed on channel 0
Hi ihor-sviziev! You've successfully authenticated, but GitHub does not provide shell access.
Connection to github.com closed.

Expected result: (After)
✔ known_hosts file is present, it already trusts to GitHub host:

PTY allocation request failed on channel 0
Hi ihor-sviziev! You've successfully authenticated, but GitHub does not provide shell access.
Connection to github.com closed.

This needed to save the known_hosts file during container re-creation
This needed to save the known_hosts file during container re-creation
@stale
Copy link

stale bot commented Jun 3, 2021

Is this still relevant? If so, what is blocking it? Is there anything you can do to help move it forward?

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

@stale stale bot added the stale label Jun 3, 2021
@ihor-sviziev
Copy link
Contributor Author

@davidalger could you review this PR?

@stale stale bot removed the stale label Jun 4, 2021
@navarr
Copy link
Member

navarr commented Jun 28, 2021

@davidalger This PR looks clean. Anything holding it back?

@davidalger
Copy link
Collaborator

I perhaps could/should have commented this long ago, but what's held this back is basically that the way CHOWN_DIR_LIST works, it walks up the directory tree chown'ing each parent directory. It was implemented this way as the original intent of the code in the image entrypoint was specifically applicable to contents of the /var/www/html, as it typically works with relative paths.

The ability to work with absolute paths is a side-benefit perhaps, but the downside is, any directory above the targeted dir will also be chowned. I.e. with this change, /home/www-data/.ssh and /home/www-data and /home would all be chowned, and I'm not sure of the consequences of changing ownership on /home from root:root to www-data:www-data. This is also why the mount for /bash_history was placed at / instead of within /home/www-data/bash_history, to avoid potential downsides without requiring a change to the image entry-point script.

You can see the logic here: https://github.com/davidalger/warden/blob/develop/images/php-fpm/context/docker-entrypoint#L42-L55

The side-effect may be fine since this is strictly a local dev image, but it's certainly not ideal, and I would like to avoid it if at all possible. Thoughts?

@ihor-sviziev
Copy link
Contributor Author

Looks like we should introduce one more environment variable and process in in the php-fpm image

Prevent changing owner on the /home directory
@ihor-sviziev
Copy link
Contributor Author

Hi @davidalger,
I just added /home directory to the list where we need to stop doing the chmod. I think it should solve the described issue.
What do you think about it?

@ihor-sviziev
Copy link
Contributor Author

@davidalger ? :)

@ihor-sviziev
Copy link
Contributor Author

@davidalger could you check this PR as well? :)

@davidalger
Copy link
Collaborator

@ihor-sviziev Apologies for the delay on this. I think that works, since there shouldn't be any real adverse affects from the setting change with the old image (and on the off chance that there were, an image pull would solve it)

@davidalger davidalger merged commit 2e1135e into wardenenv:develop Aug 28, 2021
@ihor-sviziev
Copy link
Contributor Author

@davidalger could you release a new stable version? Seems like we already have enough merged changes 🙂

davidalger added a commit that referenced this pull request Aug 28, 2021
@davidalger
Copy link
Collaborator

@ihor-sviziev Updating the changelog now and will do that.

@ihor-sviziev ihor-sviziev deleted the patch-3 branch August 28, 2021 14:46
@davidalger
Copy link
Collaborator

New version 0.12.0 is tagged and released, new images built. I tested before and after pulling new images and it seemed to work. Thanks for the useful contribution (and all the other efforts you put into this project), they're much appreciated. And sorry again for the delay.

@davidalger davidalger added this to the Warden 0.12.0 milestone Aug 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants