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

ssh_auth.present creates ~/~${USER}/.ssh #32799

Closed
belt opened this issue Apr 23, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@belt
Copy link

commented Apr 23, 2016

When populating a NEW public key i.e. no authorized_keys file, the file ${USER}/${USER}/.ssh/authorized_keys gets created

SshAuth.present(user='deploy', source='salt://keys/deploy.id_rsa.pub', config='/home/deploy/.ssh/authorized_keys')

example output: /home/deploy/~deploy/.ssh/authorized_keys ... where /home/deploy/~deploy is owned by root:root

salt version: 2015.5.8

@martin-paulus

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2016

It would be useful to confirm whether this bug still exists in develop. I will have a go at it.

@martin-paulus

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2016

@belt According to the documentation at https://docs.saltstack.com/en/latest/ref/states/all/salt.states.ssh_auth.html, the config parameter takes in a relative path not an absolute path like the one you mention in your report.

@martin-paulus

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2016

Funny story: In my test environment based on the current develop branch the config parameter to ssh_auth.present accepts both relative and absolute paths.

Disclaimer: I set up my test environment according to the instructions at https://docs.saltstack.com/en/latest/topics/development/hacking.html#running-a-self-contained-development-version, so I am running salt as an unprivileged user and operating on that user's home.

@martin-paulus

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2016

states/ssh_auth.present's config maps to modules/ssh._expand_authorized_keys_path's path

after resolving the %h and %u macros, this becomes modules/ssh._expand_authorized_keys_path's converted_path

modules/ssh._get_config_file re-assigns its config with _expand_authorized_keys_path's converted_path and then checks whether the path is relative and composes an absolute path in case it is, finally the path is returned

In other words: the documentation is incomplete. I will submit a fix.

Unfortunately, I have been unable to reproduce OP's results.

@martin-paulus

This comment has been minimized.

Copy link
Contributor

commented Apr 24, 2016

@belt We should thank @exowaucka for fixing this problem: 👍

in 2015.5 the _expand_authorized_keys_path function is absent
in 2015.8 the _expand_authorized_keys_path function is present, but its call from _get_config_file function is placed awkwardly. the call ordering has been fixed in the develop branch:

commit 57bb428
Author: Alex Wauck alexwauck@exosite.com
Date: Thu Feb 11 13:38:43 2016 -0600

Evaluate %h and %u before deciding if the ssh config path is absolute

Since %h is the user's home directory, it's not very useful unless it
appears at the beginning of the path.  However, putting it at the
beginning of the path does not have the expected effect: %h/.ssh
will become /home/someuser/home/someuser/.ssh, since "%h/.ssh" is
identified by Python as a non-absolute path, causing the user's
home directory to be tacked on the front.

@jfindlay jfindlay added this to the Blocked milestone Apr 25, 2016

@jfindlay

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2016

@belt, thanks for reporting, and @martin-paulus, thanks for your excellent research. Does anything more need to be done here? Is this problem unique to the 2015.5 branch?

@martin-paulus

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

@jfindlay Both the 2015.5 and 2015.8 branches are affected. What we could do is cherry-pick the relevant commits to the 2015.5 and 2015.8 branch. Or urge all 2015.5 and 2015.8 users to upgrade to 2016.3 as soon as possible.

If you like, I could prepare PR for 2015.5 and 2015.8 regarding this issue.

@exowaucka

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

Given the simplicity of the change, I think cherry-picking it would be the way to go. I probably should have advocated that when I made the change in the first place.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

@exowaucka and @martin-paulus In the future, feel free to make bug-fix changes against the earliest supported release branch that the bug is present on. We merge older release branches forward into newer branches so the fix will ripple through each newer branch. That way we don't have to do so much cherry-picking and the branches are overall more stable.

You can read more about this process in the Which Salt Branch? section of our Contributing docs.

I can back-port PR #31139 to the 2015.8 branch, but the 2015.5 branch isn't as applicable or clean as the _expand_authorized_key_paths function doesn't exist on the older branch.

@rallytime

This comment has been minimized.

Copy link
Contributor

commented Apr 26, 2016

@belt Does the fix from @exowaucka which I just back-ported to the 2015.8 branch with PR #32868 resolve this issue for you?

[Edit] Ahhh - I realize I just read your version report wrong. I swtiched the 5 and 8. So you're on an older branch.

@belt

This comment has been minimized.

Copy link
Author

commented Oct 6, 2016

Thank you all. Sorry, severely lagged close.

@belt belt closed this Oct 6, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.