Rebuild and allow user SSH #2977

FrostyX · 2023-11-02T12:33:37Z

Implementation of fedora-copr/debate#6

Fixes: #2364

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py

backend/copr_backend/background_worker_build.py

rpmbuild/bin/copr-builder

praiskup · 2023-11-06T11:48:41Z

Per @lachmanfrantisek claim, @nforro wants to be the early adopter! :-)

rpmbuild/bin/copr-builder

github-advanced-security

vcs-diff-lint found more than 10 potential problems in the proposed changes. Check the Files changed tab for more details.

FrostyX · 2023-12-03T21:03:23Z

There is still work to be done but can you please do a preliminary review @praiskup? I don't want to sink time in fine-tuning the details without knowing, that the general idea looks good.

What remains to be done:

There is a lot of valid linter errors that need to be fixed
Obtaining SSH keys from FAS (or fallback to a general textarea) is not there yet
I only tested in the docker-compose so far. Still requires some STG testing
Small bugs (mainly copr-builder show)
Some conceptual questions code TODOs, I am very much interested in your opinion

backend/copr_backend/background_worker_build.py

frontend/coprs_frontend/coprs/auth.py

praiskup · 2023-12-04T09:14:32Z

frontend/coprs_frontend/coprs/logic/builds_logic.py

@@ -862,6 +865,8 @@ def add(cls, user, pkgs, copr, source_type=None, source_json=None,
            copr_dir=copr_dir,
            bootstrap=bootstrap,
            isolation=isolation,
+            allow_user_ssh=allow_user_ssh,
+            ssh_public_key=ssh_public_key,


Would it be possible to have this in YAML/JSON format or something...? We
could have things like:
:

--- type: FAS fas_user: xyzuser

--- type: plaintext pub_key: ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDj5HauAOur/6+...

--- type: online url: https://github.com/praiskup.keys

We don't have to implement all the options in the web-UI right now, nor in cli.
But we can be "prepared".

This sounds reasonable, not done yet.

I thought about this and it is probably not needed now. We IMHO still want one big textarea for user input and then parse the different (types of) keys somewhere in our code. Personally, I would do the parsing on backend (we have workers there) or even on builder. Because we can fetch the keys from FAS or URL right after that.

Right now only plaintext and one key is supported, so the value could only be

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDj5HauAOur/6+.

Seeing some other comments in the PR, we will probably implement support for multiple keys in this PR, so newline would be separator, e.g.

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDj5HauAOur/6+. ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDj5HauAOur/6+.

Once we start to support other methods, the input could be

ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAACAQDj5HauAOur/6+. FAS:xyzuser https://github.com/praiskup.keys GitHub:frostyx

We can always save the blob to the database as is and parse it when needed. That would be consistent with how e.g. copr:// external repositories work.

Right now only plaintext and one key is supported

Huh, I've successfully tested before with multiple keys :-)

We IMHO still want one big textarea for user input and then parse the different (types of) keys somewhere in our code.

I'd prefer to have a separate text area in the future, rather than changing the field and start "guessing" the type of input.

My thought was to start doing it right away in the background -- aka giving the user a simple textarea for multi-line key-only input, converting it to the yaml thing in the background, and store into DB. This way we would have open doors for later enhancements without touching the old entries. But I agree that this is rather a nice to have thing, there are many such things.

frontend/coprs_frontend/coprs/models.py

backend/copr_backend/background_worker_build.py

rpmbuild/bin/copr-builder

backend/copr_backend/background_worker_build.py

praiskup · 2023-12-04T10:11:08Z

This is a very exciting change. Thank you for the progress!

praiskup · 2023-12-04T10:28:28Z

Can we have the per/user (maybe per arch) limit? I dont remember whether we decided to not implement it..

FrostyX

Thank you very much for the review. I updated almost everything but the PR is still WIP.

backend/copr_backend/background_worker_build.py

FrostyX · 2023-12-15T13:01:08Z

frontend/coprs_frontend/coprs/logic/builds_logic.py

@@ -862,6 +865,8 @@ def add(cls, user, pkgs, copr, source_type=None, source_json=None,
            copr_dir=copr_dir,
            bootstrap=bootstrap,
            isolation=isolation,
+            allow_user_ssh=allow_user_ssh,
+            ssh_public_key=ssh_public_key,


This sounds reasonable, not done yet.

frontend/coprs_frontend/coprs/models.py

rpmbuild/bin/copr-builder

backend/copr_backend/background_worker_build.py

rpmbuild/bin/copr-builder

backend/copr_backend/background_worker_build.py

rpmbuild/bin/copr-builder

backend/copr_backend/rpm_builds.py

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py

backend/copr_backend/rpm_builds.py

FrostyX · 2024-01-13T18:44:44Z

Thank you very much for the review @praiskup,
I addressed all your comments. Everything is in a separate commit for the reviewing convenience, I will squash them before merging.

no motd ...

I spent a couple of hours on this before Christmas and wasn't able to set up MOTD on the builders. And then @xsuchy suggested it might be a good idea not having MOTD because of cases like if $(ssh ... cat foo|head -n1) == "foo". So I put in the instructions to run copr-builder help instead.

praiskup · 2024-01-15T09:52:42Z

if $(ssh ... cat foo|head -n1) == "foo"

But MOTD shouldn't affect stdout:

$ ssh batcave01.iad2.fedoraproject.org echo ahoj
ahoj

praiskup · 2024-01-15T11:50:33Z

jakub to ping pavel and take a look at motd problems..

praiskup · 2024-01-17T08:27:25Z

backend/copr_backend/background_worker_build.py

@@ -307,6 +318,9 @@ def _parse_results(self):
        """
        Parse `results.json` and update the `self.job` object.
        """
+        if self.job.ssh_public_keys:


The implication here deserves an inline comment :-)

praiskup · 2024-01-17T08:32:18Z

backend/copr_backend/background_worker_build.py

@@ -683,6 +704,9 @@ def _collect_built_packages(self, job):
        """
        self.log.info("Listing built binary packages in %s", job.results_dir)

+        if self.job.ssh_public_keys:
+            return "Public SSH access was enabled, no results were stored"


Isn't it safer to return empty dict here?

praiskup · 2024-01-17T08:40:05Z

backend/copr_backend/sshcmd.py

+            "{}/".format(dest),
+            "&>", log_filepath,
+        ])
+        command = " ".join(command)


praiskup · 2024-01-17T08:45:19Z

backend/copr_backend/rpm_builds.py

+            if not x._task.get("ssh_public_keys"):
+                return None
+            return x.owner
+        super().__init__(predicate, limit, name="userssh")


s/predicate/hasher/?

praiskup · 2024-01-17T08:46:54Z

backend/copr_backend/rpm_builds.py

+    """
+    Limit the number of builders that allow user SSH
+    """
+    def __init__(self, _hasher, limit):


You can drop the unused hasher argument I think.

praiskup · 2024-01-17T08:50:09Z

frontend/coprs_frontend/coprs/templates/coprs/detail/_builds_forms.html

+    <input type="hidden" name="allow_user_ssh" value="{{ allow_user_ssh }}">
+    {{ render_field(form.ssh_public_keys, label='Public SSH keys',
+                    placeholder='Newline separated public SSH keys, e.g. ssh-rsa AAAAB3NzaC1yc2EAAAADAQABAAABAQDR+QU9...',
+                    rows=8, cols=50) }}


This seems damn awesome! :-) Can't wait for testing it.

backend/copr_backend/daemons/build_dispatcher.py

backend/copr_backend/rpm_builds.py

praiskup · 2024-01-18T15:46:58Z

Small style issue:

praiskup · 2024-01-20T08:23:05Z

I'm a bit uncertain about the windows-like EOLs, in vim I see:

... praiskup.usersys.redhat.com^M

praiskup · 2024-01-20T08:24:20Z

I tried to drop .ssh/authorized_keys file - checking what happens :-) - and it just finished the otherwise succeeding build. Is this OK?

praiskup · 2024-01-20T08:30:56Z

backend/copr_backend/background_worker_build.py

@@ -51,6 +58,9 @@

 COMMANDS = {
    "rpm_q_builder": "rpm -q copr-rpmbuild --qf \"%{VERSION}\n\"",
+    "echo_authorized_keys": "echo {0} >> /root/.ssh/authorized_keys",
+    "set_expiration": "echo -n {0} > " + USER_SSH_EXPIRATION_PATH,
+    "cat_expiration": "cat {0}".format(USER_SSH_EXPIRATION_PATH),


I think we should document here that we must be especially careful about error handling; users may replace the echo or cat commands, and (in theory) cause damage to the backend machine. This deserves documenting IMO (here ore in a later PR).

praiskup · 2024-01-20T08:32:18Z

backend/copr_backend/background_worker_build.py

+        if self.job.ssh_public_keys:
+            self.log.info("Builder allowed user SSH, not downloading the "
+                          "results for safety reasons.")
+            filter_ = ["+ success", "+ *.spec", "- *"]


I'm thinking ... what if user replaces the remote side (server) rsync binary. Can that trick the backend to download some unwanted stuff?

Ideas?

This looks quite dangerous. Should we maybe verify some checksum of the binary, or something like that? Same for the cat commands mentioned above? But maybe that leads to the same issue (i.e. we can't be sure that the checksum tool wasn't tampered with)?

praiskup · 2024-01-20T08:33:18Z

Just a few comments above, otherwise it seems OK to squash and merge. The motd and other things can be done later.

FrostyX · 2024-01-21T15:24:15Z

Small style issue:

Do you want the instructions in the "SSH access to the builder" section to be indented to the right?

I'm a bit uncertain about the windows-like EOLs, in vim I see:
... praiskup.usersys.redhat.com^M

Hmm, I haven't encountered this. Where do you see the ^M characters?

FrostyX · 2024-01-22T11:51:09Z

backend/copr_backend/background_worker_build.py

+        """
+        Find the user preference for the builder expiration.
+        """
+        rc, out, _err = self.root_ssh.run_expensive(COMMANDS["cat_expiration"])


Don't use run_expensive and check output length and/or timeout.

I couldn't use run because it doesn't return the output. So I kept run_expensive and added a timeout for it.

Fix fedora-copr#2364 See fedora-copr/debate#6

FrostyX · 2024-01-26T11:57:30Z

Small style issue:

I changed the style a bit.

praiskup

Thank you!

FrostyX marked this pull request as draft November 2, 2023 12:33

FrostyX added the WIP label Nov 2, 2023

github-advanced-security bot found potential problems Nov 2, 2023

View reviewed changes

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py Fixed Show fixed Hide fixed

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Nov 4, 2023

View reviewed changes

github-advanced-security bot found potential problems Nov 12, 2023

View reviewed changes

rpmbuild/bin/copr-builder Fixed Show fixed Hide fixed

rpmbuild/bin/copr-builder Fixed Show fixed Hide fixed

rpmbuild/bin/copr-builder Fixed Show fixed Hide fixed

rpmbuild/bin/copr-builder Fixed Show fixed Hide fixed

rpmbuild/bin/copr-builder Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Nov 19, 2023

View reviewed changes

FrostyX force-pushed the rebuild-and-keep-ssh branch from d8bf463 to f964c9b Compare December 3, 2023 20:42

praiskup reviewed Dec 4, 2023

View reviewed changes

backend/copr_backend/background_worker_build.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

frontend/coprs_frontend/coprs/auth.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

frontend/coprs_frontend/coprs/auth.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

frontend/coprs_frontend/coprs/models.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

frontend/coprs_frontend/coprs/models.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

backend/copr_backend/background_worker_build.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

rpmbuild/bin/copr-builder Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

rpmbuild/bin/copr-builder Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

rpmbuild/bin/copr-builder Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

backend/copr_backend/background_worker_build.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

backend/copr_backend/background_worker_build.py Outdated Show resolved Hide resolved

praiskup reviewed Dec 4, 2023

View reviewed changes

backend/copr_backend/background_worker_build.py Show resolved Hide resolved

FrostyX commented Dec 15, 2023

View reviewed changes

github-advanced-security bot found potential problems Dec 15, 2023

View reviewed changes

FrostyX force-pushed the rebuild-and-keep-ssh branch 2 times, most recently from 7a4cd5d to 7f0b348 Compare December 20, 2023 21:06

github-advanced-security bot found potential problems Dec 20, 2023

View reviewed changes

backend/copr_backend/rpm_builds.py Fixed Show fixed Hide fixed

backend/copr_backend/rpm_builds.py Fixed Show fixed Hide fixed

FrostyX force-pushed the rebuild-and-keep-ssh branch from 87e8c7e to 9fa616e Compare December 21, 2023 20:31

praiskup reviewed Jan 8, 2024

View reviewed changes

frontend/coprs_frontend/coprs/views/coprs_ns/coprs_builds.py Outdated Show resolved Hide resolved

github-advanced-security bot found potential problems Jan 13, 2024

View reviewed changes

backend/copr_backend/rpm_builds.py Fixed Show fixed Hide fixed

praiskup reviewed Jan 17, 2024

View reviewed changes

backend/copr_backend/sshcmd.py

"{}/".format(dest),

"&>", log_filepath,

])

command = " ".join(command)

Copy link

Member

praiskup Jan 17, 2024

Choose a reason for hiding this comment

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

Nice!

praiskup reviewed Jan 17, 2024

View reviewed changes

praiskup mentioned this pull request Jan 17, 2024

Messaging: Set the agent_name, usernames and groups properties #3099

Merged

github-advanced-security bot found potential problems Jan 17, 2024

View reviewed changes

backend/copr_backend/daemons/build_dispatcher.py Fixed Show fixed Hide fixed

FrostyX force-pushed the rebuild-and-keep-ssh branch from 1bf44bb to 153d521 Compare January 18, 2024 14:09

github-advanced-security bot found potential problems Jan 18, 2024

View reviewed changes

backend/copr_backend/rpm_builds.py Fixed Show fixed Hide fixed

praiskup reviewed Jan 20, 2024

View reviewed changes

FrostyX commented Jan 22, 2024

View reviewed changes

frontend, backend, common, rpmbuild: allow user SSH to builders

c5d0ca5

Fix fedora-copr#2364 See fedora-copr/debate#6

FrostyX force-pushed the rebuild-and-keep-ssh branch from 7d1741a to c5d0ca5 Compare January 26, 2024 11:54

praiskup approved these changes Jan 26, 2024

View reviewed changes

praiskup mentioned this pull request Jan 26, 2024

The ssh run_expensive() method should have a _safe() variant #3118

Closed

FrostyX mentioned this pull request Jan 26, 2024

MOTD for builders with user SSH access #3119

Closed

FrostyX merged commit c11e59f into fedora-copr:main Jan 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebuild and allow user SSH #2977

Rebuild and allow user SSH #2977

FrostyX commented Nov 2, 2023 •

edited by praiskup

Loading

praiskup commented Nov 6, 2023

github-advanced-security bot left a comment

FrostyX commented Dec 3, 2023

praiskup Dec 4, 2023

FrostyX Dec 15, 2023

FrostyX Jan 12, 2024

praiskup Jan 17, 2024

praiskup Jan 17, 2024

praiskup commented Dec 4, 2023

praiskup commented Dec 4, 2023

FrostyX left a comment

FrostyX Dec 15, 2023

FrostyX commented Jan 13, 2024

praiskup commented Jan 15, 2024

praiskup commented Jan 15, 2024

praiskup Jan 17, 2024

praiskup Jan 17, 2024

praiskup Jan 17, 2024

praiskup Jan 17, 2024

praiskup Jan 17, 2024

praiskup Jan 17, 2024

praiskup commented Jan 18, 2024

praiskup commented Jan 20, 2024

praiskup commented Jan 20, 2024

praiskup Jan 20, 2024

praiskup Jan 20, 2024

FrostyX Jan 21, 2024

praiskup commented Jan 20, 2024

FrostyX commented Jan 21, 2024

FrostyX Jan 22, 2024

FrostyX Jan 26, 2024

FrostyX commented Jan 26, 2024

praiskup left a comment

Rebuild and allow user SSH #2977

Rebuild and allow user SSH #2977

Conversation

FrostyX commented Nov 2, 2023 • edited by praiskup Loading

praiskup commented Nov 6, 2023

github-advanced-security bot left a comment

Choose a reason for hiding this comment

FrostyX commented Dec 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Dec 4, 2023

praiskup commented Dec 4, 2023

FrostyX left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrostyX commented Jan 13, 2024

praiskup commented Jan 15, 2024

praiskup commented Jan 15, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Jan 18, 2024

praiskup commented Jan 20, 2024

praiskup commented Jan 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

praiskup commented Jan 20, 2024

FrostyX commented Jan 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FrostyX commented Jan 26, 2024

praiskup left a comment

Choose a reason for hiding this comment

FrostyX commented Nov 2, 2023 •

edited by praiskup

Loading