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

Add users keys as authorized keys for yumrepostage #1948

Merged
merged 1 commit into from Oct 27, 2023

Conversation

ehelms
Copy link
Member

@ehelms ehelms commented Oct 2, 2023

No description provided.

@ehelms
Copy link
Member Author

ehelms commented Oct 2, 2023

Based upon the design for pushing locally generated stage repository from Copr to the staging repository -- theforeman/theforeman-rel-eng#280

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Argh, I forgot to submit the review yesterday.

puppet/modules/secure_ssh/manifests/receiver_setup.pp Outdated Show resolved Hide resolved
puppet/modules/web/manifests/vhost/stagingyum.pp Outdated Show resolved Hide resolved
@ehelms
Copy link
Member Author

ehelms commented Oct 3, 2023

Switched to using keys as the naming instead of users

@ehelms ehelms force-pushed the add-user-keys-yumrepostage branch 2 times, most recently from 0441175 to 2046338 Compare October 3, 2023 12:42
Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

I still question about the bigger picture. This allows a user to call a script, but they're still not allowed to upload their own content. How do you plan to address this?

puppet/modules/web/manifests/vhost/stagingyum.pp Outdated Show resolved Hide resolved
@ehelms
Copy link
Member Author

ehelms commented Oct 3, 2023

I still question about the bigger picture. This allows a user to call a script, but they're still not allowed to upload their own content. How do you plan to address this?

Explain? The goal is to allow users to rsync RPMs as the yumrepostage user to the staging repository location.

@ekohl
Copy link
Member

ekohl commented Oct 4, 2023

You can see it forces a command to run, which is $HOME/bin/secure_$USER. That has

YUM_PATH=`echo "${SSH_ORIGINAL_COMMAND}" | awk '{ print $NF }'`
as the source.

My reading is that it only allows the command to start with rsync --server, which means you can't upload files from your local disk.

Am I missing anything?

@ehelms
Copy link
Member Author

ehelms commented Oct 4, 2023

I do not follow where you are getting that from, but this is what Jenkins does which has no --server, what I believe it relies on is the presence of rsync_cache:

if [[ `echo "${SSH_ORIGINAL_COMMAND}" | awk '{ print $NF }'` =~ ^rsync_cache/.* ]] ; then

You can see what Jenkins does here, which is what a user would be doing:

https://github.com/theforeman/jenkins-jobs/blob/master/theforeman.org/pipelines/lib/packaging.groovy#L474-L485

Which is my goal here: https://github.com/theforeman/theforeman-rel-eng/pull/280/files#diff-75ffb578a1aa224516bbb4130ba24c902b62748edddd6c77fc637648471bb3e6

@ekohl
Copy link
Member

ekohl commented Oct 4, 2023

Perhaps internally rsync rewrites the SSH command to include --server. From man rsync:

INTERNAL OPTIONS
The options --server and --sender are used internally by rsync, and should never be typed by a user under normal circumstances. Some awareness of these options may be needed in certain scenarios, such as when setting up a login that can only run an rsync command. For instance, the support directory of the rsync distribution has an example script named rrsync (for restricted rsync) that can be used with a restricted ssh login.

Which makes me think it would be good to use rrsync instead of plain rsync. It has --no-del --no-overwrite as options. Those sound like they can be used to easily enforce we don't overwrite already signed RPMs. I wonder about metadata or other things, but if we'll regenerate the metadata on the server (which AFAIK we should do anyway) then it sounds like something we can deal with. We can also use temporary files which the script always removes after regenerating the metadata if needed.

@ehelms
Copy link
Member Author

ehelms commented Oct 4, 2023

Which makes me think it would be good to use rrsync instead of plain rsync. It has --no-del --no-overwrite as options. Those sound like they can be used to easily enforce we don't overwrite already signed RPMs. I wonder about metadata or other things, but if we'll regenerate the metadata on the server (which AFAIK we should do anyway) then it sounds like something we can deal with. We can also use temporary files which the script always removes after regenerating the metadata if needed.

For the stage repository, we are not generating the repository and metadata on the webserver. Production and staging are handled differently and this is only targeting staging.

The workflow for staging:

  • generate repository locally (or on Jenkins)
  • rsync packages and metadata over to the staging vhost location

And a reminder stage to production is handled:

  • rsync from staging on web01 to production
  • runs createrepo to regenerate the metadata since only the diff is copied

@ehelms
Copy link
Member Author

ehelms commented Oct 13, 2023

@evgeni @ekohl I'm blocked on any further Copr work until this and theforeman/theforeman-rel-eng#280 are merged

Copy link
Member

@evgeni evgeni left a comment

Choose a reason for hiding this comment

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

For the sake of my own sanity, here is how I read this:

Today, there is a set of systems (identified by the foreman_search in stagingyum.pp who are allowed to push to this repo. These pushes happen to a "special" rsync_cache folder from where then the receiver script is moving them to the "correct place" after the push happened. There is no post-processing of the repo happening (contrary to, e.g. when we push to yum.tfm.o).

This change adds a set of additional keys that are allowed to perform the same action as above (push to cache, get the cached content copied over to the real destination), without the IP address restriction. Those additional keys are taken from the users module that is already handling our normal SSH logins.

While I think the way we configure the set of users whose keys are added could be better configurable, I wouldn't block on this to get the copr integration further.

@@ -6,6 +6,7 @@
String $user = 'yumrepostage',
Stdlib::Absolutepath $home = "/home/${user}",
Integer[0] $rsync_max_connections = 5,
Array[String[1]] $usernames = ['ehelms', 'evgeni', 'ekohl'],
Copy link
Member

Choose a reason for hiding this comment

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

Something tells me this list should be configured somewhere else, but I guess it's good enough here for starting and testing?

@ehelms ehelms merged commit cc80720 into theforeman:master Oct 27, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants