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

(MODULES-9578) Create ssh_authorized_key in root path #20

Merged

Conversation

GabrielNagy
Copy link
Contributor

@GabrielNagy GabrielNagy commented Aug 13, 2019

Previously, when the target property was set, the ssh_authorized_key resource could not create directories/files within root-owned paths. This behavior is due to the module switching context to the
user, then attempting to create the directory/file as the specified user, ultimately failing because of insufficient permissions.

This commit removes the context change logic, and creates the authorized_key file as root, then executes FileUtils.chown to make it owned by the target user.

This commit also removes the functionality that created parent directories, since it should not be in the scope of this module.

Cherry-picked 2 commits from @pillarsdotnet's tree, related to tests.

@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch 3 times, most recently from 0028eac to 4eb9141 Compare August 13, 2019 10:49
@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch 3 times, most recently from 54db0bb to baba7be Compare August 13, 2019 12:46
@GabrielNagy
Copy link
Contributor Author

Acceptance tests are passing:
pic-selected-190813-1556-28

@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch from baba7be to f95ddad Compare August 13, 2019 13:03
Copy link
Contributor

@pillarsdotnet pillarsdotnet left a comment

Choose a reason for hiding this comment

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

LGTM; added publicly-visible link from MODULES-9578.

@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch from f95ddad to 51f191a Compare August 13, 2019 14:21
@mihaibuzgau mihaibuzgau requested a review from a team August 16, 2019 17:29
Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

It is not safe for root to write to a users' untrusted home directory. Otherwise, the user could create a symlink and have root overwrite ~root/.ssh/authorized_keys and now the user can login as root.

@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch 7 times, most recently from d8fa1f0 to 4cc041d Compare August 26, 2019 13:00
@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch 3 times, most recently from 978a8f3 to 55d73fc Compare August 27, 2019 11:56
@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch 2 times, most recently from a47a7df to 0755b94 Compare September 11, 2019 11:14
@GabrielNagy
Copy link
Contributor Author

@joshcooper I've restored the functionality that creates the parent directory (in case we're writing as the user/path is not trusted), so this should not be a breaking change anymore.

Copy link
Contributor

@joshcooper joshcooper left a comment

Choose a reason for hiding this comment

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

If the target doesn't exist, then the path isn't considered trusted, and we try, but fail to write the key as the user

@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch 3 times, most recently from 904895b to 7d2002c Compare October 9, 2019 07:12
@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch from 7d2002c to f84ea6f Compare October 10, 2019 08:47
@GabrielNagy
Copy link
Contributor Author

Fixed the rubocop stuff, I'll squash my commits when you're good with this @joshcooper

Previously, when the `target` property was set, the ssh_authorized_key
resource could not create directories/files within root-owned paths.
This behavior is due to the module switching context to the user, then
attempting to create the directory/file as the specified user,
ultimately failing because of insufficient permissions.

This commit adds a new parameter, `drop_privileges` which when set to
false allows the module to write a ssh_authorized_key file in a
privileged path. Due to the possible security implications of this,
the parameter must be manually specified in order to activate this
functionality.

A path is considered to be privileged/trusted if all of its ancestors:
- do not contain any symlinks
- have the same owner as the user who runs Puppet
- are not world/group writable
@GabrielNagy GabrielNagy force-pushed the MODULES-9578/create-file-as-root branch from a46d859 to b2c153b Compare October 23, 2019 09:28
@GabrielNagy
Copy link
Contributor Author

Passing acceptance run (except for ubuntu 14.04 which is no longer in our pipeline and can be ignored): https://jenkins-master-prod-1.delivery.puppetlabs.net/view/modules/view/Core/view/sshkeys_core/view/adhoc/job/forge-module_puppetlabs-sshkeys_core_intn-sys_nightly-adhoc/2/

@mihaibuzgau mihaibuzgau merged commit f78b81b into puppetlabs:master Oct 24, 2019
@GabrielNagy GabrielNagy added the feature New feature or request label Oct 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants