mock-build to support --git-url option for scratch build #278

rhyw · 2024-05-23T01:14:34Z

Resolves: https://issues.redhat.com/browse/OSH-592

osh/client/commands/cmd_mock_build.py

hanchuntao · 2024-06-03T10:45:08Z

osh/client/commands/cmd_mock_build.py

@@ -15,6 +16,16 @@ def options(self):

        # this option cannot be used for diff-build tasks


Should we need to change the comment?

Not sure I understand. The changes in this MR doesn't change the behavior of diff-build, I think it's still true that the option cannot be used for diff-build.

I think diff-build would not work with cspodman anyway. It should probably work with version-diff-build though.

Latested updates applied both client/hub side validation.

osh/client/commands/cmd_mock_build.py

hanchuntao · 2024-06-05T06:27:39Z

@rhyw After installing the test rpm, run the command: osh-cli mock-build --config cspodman --hub=https://covscan.lab.eng.brq2.redhat.com/osh/xmlrpc --git-url 'git+https://pkgs.devel.redhat.com/git/containers/amq-broker#d90c3cfb9c69e5fa023be1c08bd8ec98a876dfa3', I got the following error: https://privatebin.corp.redhat.com/?4f46af6856f23901#9sZE8pR8t8bwfBAuxUvLQLguCXCuMcUjRiMyxpixnwd1

kdudka · 2024-06-05T07:05:24Z

@hanchuntao Did you also update the hub and worker?

Note that links to the RH internal pastebin instance are not much useful for external community members.

kdudka · 2024-06-05T13:22:40Z

osh/worker/csmock_runner.py

+        if profile != "cspodman":
+            print("dist-git URL scan is incompatible with profile:", profile, file=sys.stderr)
+            return None, 2
+        return self.analyze(analyzers, dist_git_url, profile, su_user, additional_arguments, **kwargs)


Where do we protect dist_git_url against shell command injection? My quick check suggests that nowhere:

osh-cli mock-build --hub https://covscan.lab.eng.brq2.redhat.com/osh/xmlrpc --config=cspodman --git-url "https://example/ --help; ls -l ~/.config/configstore; #hash"

yes, I tested it

[root@chhan-rhel9 ~]# osh-cli mock-build --hub https://covscan.lab.eng.brq2.redhat.com/osh/xmlrpc --config=cspodman --git-url "https://example/ --help; ls -l ~/.config/configstore; #hash" Task info: https://covscan.lab.eng.brq2.redhat.com/osh/task/467528/ Watching tasks (this may be safely interrupted)... 467528 MockBuild: FREE --> FREE: 1 [total: 1] 467528 MockBuild: FREE -> OPEN (osh-test-worker-001.osh-001.preprod.iad2.dc.redhat.com) --> OPEN: 1 [total: 1] 467528 MockBuild: OPEN (osh-test-worker-001.osh-001.preprod.iad2.dc.redhat.com) -> FAILED (osh-test-worker-001.osh-001.preprod.iad2.dc.redhat.com) --> FAILED: 1 [total: 1] [root@chhan-rhel9 ~]#

result in https://covscan.lab.eng.brq2.redhat.com/osh/task/467528/log/stdout.log

total 8 -rw-------. 1 csmock csmock 58 Nov 24 2023 snyk.json -rw-------. 1 csmock csmock 58 Oct 10 2023 snyk.json.bak Task did not produce any results

There is a dist git URL check here: https://github.com/openscanhub/openscanhub/pull/278/files#diff-26bebd5671ea9c96d9eba3bdf8d8fcdcd3a4f8c594e359d29464c3623ff3aee3R36

But it appears the error was thrown before the validation is even triggerred. I'll see what can be done to throw this error earlier, probably to add some validation in cspodman cli.

Validation in osh-cli is not a solution to the security issue because anyone can comment out the validation in their copy of osh-cli or just use the XML-RPC interface directly.

... and validation in cspodman would not help either because the shell injection happens in the shell that runs cspodman, not in the implementation of cspodman.

I think it's still helpful from client side because we should always raise/return earlier. I had this commit addressed: 69a902a

I'll see how similar sanitation/validation can be done from hub too. working in progress.

hanchuntao · 2024-06-06T01:51:40Z

@hanchuntao Did you also update the hub and worker?

Note that links to the RH internal pastebin instance are not much useful for external community members.

yes, I update osh-client, osh-worker, but forget update osh-hub, after update it, the command run correctly,and get correct result:

[root@chhan-rhel9 ~]# osh-cli mock-build --config cspodman --hub=https://covscan.lab.eng.brq2.redhat.com/osh/xmlrpc --git-url 'git+https://pkgs.devel.redhat.com/git/containers/amq-broker#d90c3cfb9c69e5fa023be1c08bd8ec98a876dfa3'
Task info: https://covscan.lab.eng.brq2.redhat.com/osh/task/467527/
Watching tasks (this may be safely interrupted)...
467527 MockBuild: FREE
--> FREE: 1 [total: 1]
467527 MockBuild: FREE -> OPEN (osh-test-worker-001.osh-001.preprod.iad2.dc.redhat.com)
--> OPEN: 1 [total: 1]
467527 MockBuild: OPEN (osh-test-worker-001.osh-001.preprod.iad2.dc.redhat.com) -> CLOSED (osh-test-worker-001.osh-001.preprod.iad2.dc.redhat.com)
--> CLOSED: 1 [total: 1]
[root@chhan-rhel9 ~]#

rhyw · 2024-06-14T08:54:44Z

/packit rebuild-failed

kdudka

@rhyw Let's not over-complicate this. The git URL is now checked at multiple places using multiple regular expressions. Manually maintaining a set of disallowed characters is not a way to protect against shell command injection. To protect against shell injection, please use shlex.quote() as we do for other arguments passed to csmock or cspodman. To reject invalid URLs before starting a scan, I would just rewrite the regex that splits the URL parts so that it explicitly lists the set of allowed characters in each part of the URL.

rhyw · 2024-06-19T03:32:50Z

@kdudka thanks for the suggestion, I have revised the way how validation was done a bit. The existing REs we use in csmock/cspodman are not enough to check the validity of the git URLs, some additional checks are added in latest code.

kdudka · 2024-06-19T15:04:26Z

@rhyw Is this ready for review?

The commits seem to be in wrong order with some unnecessary changes back and forth:

a41fb53 introduces a call to the parse_git_url() function, which is not yet defined.
The parse_git_url() function is defined later in commit ae3c67a.
The parse_git_url() function is again removed in commit 0a44958.

rhyw · 2024-06-20T01:55:49Z

@rhyw Is this ready for review?

The pipeline is failed though I was checking the cause of the failure.

The commits seem to be in wrong order with some unnecessary changes back and forth:

I had some problem rebasing earlier due to merge conflicts.

I'll mark this as Draft first, and mark it as Ready once these issues are fixed. Thanks

Related: https://issues.redhat.com/browse/OSH-592

so that it's easier to reuse the code snippets.

cmd construction to a separate function, output_path to be determined in a separate function. The change split the function into smaller ones, to make it ealier to partially re-use.

rhyw · 2024-06-21T07:27:14Z

I think the pipeline failure is not relevent. I had some tests on stage, and I think it's working as expected.

Validation is done on both cli/hub side. e.g., on client side with an invalid URL provided:

$ osh-cli mock-build --nowait --hub <hub-xmlrpc-endpoint> --git-url "https://example.com/--help;ls-l ~/.config/configstore;#hash"
Usage: osh-cli mock-build [options] <args>

osh-cli: error: Invalid path in dist-git URL: /--help;ls-l ~/.config/configstore;

rhyw requested review from kdudka and lzaoral as code owners May 23, 2024 01:14

rhyw force-pushed the git-url-option branch from 18c6fdc to f39455e Compare May 23, 2024 01:16

hanchuntao reviewed Jun 3, 2024

View reviewed changes

osh/client/commands/cmd_mock_build.py Show resolved Hide resolved

hanchuntao reviewed Jun 3, 2024

View reviewed changes

kdudka requested changes Jun 5, 2024

View reviewed changes

lzaoral mentioned this pull request Jun 6, 2024

Add osh-cli unit test cases #273

Open

rhyw force-pushed the git-url-option branch from f39455e to 69a902a Compare June 12, 2024 10:34

rhyw marked this pull request as draft June 12, 2024 10:53

rhyw force-pushed the git-url-option branch from 9cef45a to 749f9c2 Compare June 13, 2024 10:26

rhyw marked this pull request as ready for review June 14, 2024 08:51

rhyw self-assigned this Jun 14, 2024

kdudka requested changes Jun 14, 2024

View reviewed changes

rhyw force-pushed the git-url-option branch from 749f9c2 to 0a44958 Compare June 19, 2024 01:48

rhyw marked this pull request as draft June 20, 2024 01:56

rhyw force-pushed the git-url-option branch from 0a44958 to 9a4c9f6 Compare June 21, 2024 01:27

rhyw added 6 commits June 21, 2024 10:07

client: add git-url option in mock-build

f331323

Related: https://issues.redhat.com/browse/OSH-592

client: update task options validation in mock-build

d384979

so that it's easier to reuse the code snippets.

hub: refine check_srpm to support git_url

f9769c8

hub: refine determine_result_filename

d0b91ca

worker: refactor CsmockRunner.analyze

5f209d0

cmd construction to a separate function, output_path to be determined in a separate function. The change split the function into smaller ones, to make it ealier to partially re-use.

worker: handle dist-git URL scan in runner

4ef7d05

rhyw force-pushed the git-url-option branch from 9a4c9f6 to 42dc5e6 Compare June 21, 2024 02:08

rhyw added 2 commits June 21, 2024 14:47

hub/client: add dist-git URL validation for hub/client

232c14b

client: check_build to accept git_url

3b406b4

rhyw force-pushed the git-url-option branch from 42dc5e6 to 3b406b4 Compare June 21, 2024 06:47

rhyw requested review from kdudka and hanchuntao June 21, 2024 07:25

rhyw marked this pull request as ready for review June 21, 2024 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mock-build to support --git-url option for scratch build #278

mock-build to support --git-url option for scratch build #278

rhyw commented May 23, 2024

hanchuntao Jun 3, 2024

rhyw Jun 5, 2024

kdudka Jun 5, 2024

rhyw Jun 14, 2024

hanchuntao commented Jun 5, 2024

kdudka commented Jun 5, 2024

kdudka Jun 5, 2024

hanchuntao Jun 6, 2024

rhyw Jun 12, 2024

kdudka Jun 12, 2024

kdudka Jun 12, 2024

rhyw Jun 12, 2024

hanchuntao commented Jun 6, 2024

rhyw commented Jun 14, 2024

kdudka left a comment

rhyw commented Jun 19, 2024

kdudka commented Jun 19, 2024 •

edited

rhyw commented Jun 20, 2024

rhyw commented Jun 21, 2024 •

edited

		@@ -15,6 +16,16 @@ def options(self):

		# this option cannot be used for diff-build tasks

mock-build to support --git-url option for scratch build #278

Are you sure you want to change the base?

mock-build to support --git-url option for scratch build #278

Conversation

rhyw commented May 23, 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

hanchuntao commented Jun 5, 2024

kdudka commented Jun 5, 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

hanchuntao commented Jun 6, 2024

rhyw commented Jun 14, 2024

kdudka left a comment

Choose a reason for hiding this comment

rhyw commented Jun 19, 2024

kdudka commented Jun 19, 2024 • edited

rhyw commented Jun 20, 2024

rhyw commented Jun 21, 2024 • edited

kdudka commented Jun 19, 2024 •

edited

rhyw commented Jun 21, 2024 •

edited