Add revision enforcement #11263

coolo · 2021-06-21T09:38:01Z

Continuing on #10992

This is a security flaw in our development flow so we want to have this fixed asap - but the original PR didn't move in the last 2 months.

src/api/app/models/bs_request_action.rb

src/api/test/functional/request_controller_test.rb

adrianschroeter · 2021-06-21T10:01:53Z

On Montag, 21. Juni 2021, 11:58:17 CEST Stephan Kulow wrote: @coolo commented on this pull request. > @@ -202,6 +202,40 @@ def test_submit_request_of_new_package_with_devel_package assert_response :success end + def test_submit_with_and_without_revision Yes, we can remove this one IMO

please don't, I like to extend it to check also to verify that wrong submitted revisions get catched. And that won't work with VCR cassettes.

…

-- Adrian Schroeter ***@***.***> Build Infrastructure Project Manager SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany (HRB 247165, AG München), Geschäftsführer: Felix Imendörffer

coolo · 2021-06-21T10:04:01Z

why not? I added exactly that actually

codecov · 2021-06-21T10:22:13Z

Codecov Report

Merging #11263 (a054d7e) into master (3f6dd34) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master   #11263   +/-   ##
=======================================
  Coverage   91.11%   91.11%           
=======================================
  Files         608      608           
  Lines       21890    21893    +3     
=======================================
+ Hits        19944    19947    +3     
  Misses       1946     1946

adrianschroeter · 2021-06-21T11:17:09Z

On Montag, 21. Juni 2021, 12:04:17 CEST Stephan Kulow wrote: why not? I added exactly that actually

I do not see that you constructed a linked package resource where you specified the unexpanded md5sum in a submit action on request creation. Did you?

…

-- Adrian Schroeter ***@***.***> Build Infrastructure Project Manager SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany (HRB 247165, AG München), Geschäftsführer: Felix Imendörffer

coolo · 2021-06-21T12:13:58Z

I submit revision 2, which points to a _link pointing to a 3rd package - and expect an expanded srcmd5 written in the request

adrianschroeter · 2021-06-21T12:21:58Z

On Montag, 21. Juni 2021, 14:14:15 CEST Stephan Kulow wrote: I submit revision 2, which points to a _link pointing to a 3rd package - and expect an expanded srcmd5 written in the request

But rev=2 is not part of the action xml, right? In that case an error must occur, since you reference explicit to a not frozen revision. The user must know that he can not reference _link file changes basically, since he can not submit such changes that way anymore.

…

-- Adrian Schroeter ***@***.***> Build Infrastructure Project Manager SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany (HRB 247165, AG München), Geschäftsführer: Felix Imendörffer

coolo · 2021-06-21T12:24:34Z

rev=2 is part of the original xml, but not of the one "saved". I guess I don't understand the story. Basically the tpackage being a _link?

adrianschroeter · 2021-06-21T12:33:42Z

It is about the source package, we need to verify following situations with the attribute set:

request action xml has no revision specified
request action xml has a revsion specified (no matter if md5sum or number).

2a) Source revision expands to a source without any _link file. We can be sure the source is frozen.
2b) Source revsion expands to a source containing a _link file, means we can not be sure that it is frozen (we do not want to evaluate the _link file content here). So we need to error out, since the user explicit used this revision. (Maybe in good face by suggesting a _link file change, but this is too complex to handle here).

Therefore we need a source server doing the source merging for a proper test case.

coolo · 2021-06-21T12:51:53Z

I mean how can a package expand to a _link? if you can point me to one such case on OBS, I can extend the test.

adrianschroeter · 2021-06-21T13:14:30Z

On Montag, 21. Juni 2021, 14:52:08 CEST Stephan Kulow wrote: I mean how can a package expand to a _link?

just pick any not expanded revision and use it. You need to have a package with a _link as source of course for that. In that case the request creation should fail with the right error. Doing the same with the xsrcmd5 sum needs to work.

…

-- Adrian Schroeter ***@***.***> Build Infrastructure Project Manager SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany (HRB 247165, AG München), Geschäftsführer: Felix Imendörffer

coolo · 2021-06-21T15:12:00Z

If you have a devel project, it will contain a link to Factory. If I submit that with revision=HEAD, it will be expanded and the expanded srcmd5 is written into the database. And these expanded sources are what we'll review

hennevogel · 2021-06-21T15:37:08Z

@adrianschroeter so can we merge this and you write this test case afterward? You won't change the code right?

adrianschroeter · 2021-06-21T15:38:04Z

On Montag, 21. Juni 2021, 17:37:25 CEST Henne Vogelsang wrote: @adrianschroeter so can we merge this and you write this test case afterward? You won't change the code right?

yes, I just asked to keep the mini test.

…

-- Adrian Schroeter ***@***.***> Build Infrastructure Project Manager SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nuernberg, Germany (HRB 247165, AG München), Geschäftsführer: Felix Imendörffer

marcus-h · 2021-06-21T16:12:53Z

On 2021-06-21 02:38:19 -0700, Stephan Kulow wrote: This is a security flaw in our development flow so we want to have this fixed asap - but the original PR didn't move in the last 2 months. You can view, comment on, or merge this pull request online at: diff --git a/src/api/app/models/bs_request_action.rb b/src/api/app/models/bs_request_action.rb index efeddae94d..5d24d5db12 100644 --- a/src/api/app/models/bs_request_action.rb +++ b/src/api/app/models/bs_request_action.rb @@ -786,11 +786,19 @@ class BsRequestAction < ApplicationRecord query[:expand] = 1 unless updatelink query[:rev] = source_rev if source_rev dir = Xmlhash.parse(Backend::Api::Sources::Package.files(source_project, source_package, query)) + unless add_revision + # Enforce revisions? + tprj = Project.get_by_name(target_project) + add_revision = tprj.instance_of?(Project) && tprj.find_attribute('OBS', 'EnforceRevisionsInRequests').present? + if add_revision + # fix the revision to the expanded sources at the time of submission + self.source_rev = dir['srcmd5']

In case of an <options><updatelink>true</updatelink></options> request, self.source_rev could still point to an unexpanded link. Since our tooling only displays the expanded sources (for instance, when using the /request/1234?cmd=diff route), this could be used (untested) to "trick" a reviewer to accept accept a review. Once the request is eventually accepted, the target package contains a _link file. At this point, the _link could expand to "malicious"/unreviewed sources.

coolo · 2021-06-21T16:59:14Z

true, will add an exception for this case

coolo · 2021-06-21T17:25:11Z

readded the minitest (everyone her hobby :) and added code and test protecting against updatelink

marcus-h · 2021-06-21T20:55:44Z

On 2021-06-21 10:25:27 -0700, Stephan Kulow wrote: readded the minitest (everyone her hobby :) and added code and test protecting against updatelink

Maybe we should execute the "enforce revisions?" codepath unconditionally? (That is, do not guard it with "unless add_revision".). Otherwise, one could create a request via POST /request?cmd=create&addrevision=1, which has the updatelink option set to true. Such a request can be created but can never be accepted (if I get the code right). Hence, this is just for sanity and not security reasons:)

coolo · 2021-06-22T13:17:57Z

agreed, changed that now

src/api/app/models/bs_request_action.rb

hennevogel · 2021-06-22T13:24:36Z

src/api/db/data/20210409135426_add_enforce_revision.rb

+  def self.down
+    AttribType.find_by_namespace_and_name('OBS', 'EnforceRevisionsInRequests').delete
+  end
+end


I'm missing changes to db/data_schema.rb, should happen once you run the data migration.

hmm, this is a data migration

Yeah and this is data_schema.rb. So if you setup a fresh database for instance, the data migrations are not running afterward.

src/api/spec/models/bs_request_action_spec.rb

marcus-h · 2021-06-22T21:34:16Z

Maybe we should also forbid source_rev == 'upload' in the enforce revisions codepath (the upload rev can "survive" expand=1).

coolo · 2021-06-23T04:59:55Z

That will never be returned as srcmd5 - and we rewrite the source_rev with the expanded srcmd5. So I don't see an issue here.
It's rather an issue for the non-attributed projects, which brings us to the question: why shouldn't this be the default?

Target projects may decide always to get requests with versioned actions. Note: revisions to unexpanded sources are still possible!

Once the request is created, we take the given revision, expand the sources and take the revision given by the backend in the database. So if any of the components of the submitted sources is changed afterwards, they don't invalidate reviews

Make the comment a little clearer Co-authored-by: Henne Vogelsang <hvogel@opensuse.org>

For OBS:EnforceRevisionsInRequests projects we want to be certain what we review is what we accept - and as such we don't want links to point anywhere, so that updatelink option is incompatible to that use case

As Marcus pointed out if we skip the check if add_revision is set, then the user can skip that by setting add_revision, but having an updatelink option set. As we wouldn't throw the exception in these cases, it would end up in the database and potentially only noticed on accept

marcus-h · 2021-06-23T06:51:17Z

On 2021-06-22 22:00:06 -0700, Stephan Kulow wrote: That will never be returned as srcmd5 - and we rewrite the source_rev with the expanded srcmd5. So I don't see an issue here.

No, the upload rev survives the expand: ***@***.***:~> curl -u Marcus_H 'https://api.opensuse.org/source/home:Marcus_H:lnk2/b3?rev=upload&expand=1' Enter host password for user 'Marcus_H': <directory name="b3" rev="upload" srcmd5="upload"> <entry name="foo" md5="14758f1afd44c09b7992073ccf00b43d" size="7" mtime="1624391770"/> </directory> ***@***.***:~>

It's rather an issue for the non-attributed projects, which brings us to the question: why shouldn't this be the default?

No idea/no opinion:)

coolo · 2021-06-23T07:07:05Z

created request id 901414
coolo@nerissa#build-service>osc api /request/901414
<request id="901414" creator="coolo">
  <action type="submit">
    <source project="home:Marcus_H:lnk2" package="b3" rev="upload"/>
    <target project="home:coolo" package="b3"/>
  </action>
  <state name="review" who="coolo" when="2021-06-23T07:05:22">
    <comment/>
  </state>
  <review state="new" when="2021-06-23T07:05:22" by_package="b3" by_project="home:Marcus_H:lnk2"/>
  <description>trying</description>
</request>

This sounds like an even more severe bug - I guess we treat that one in an extra PR.

hennevogel · 2021-06-23T09:03:29Z

src/api/db/data/20210409135426_add_enforce_revision.rb

+  def self.down
+    AttribType.find_by_namespace_and_name('OBS', 'EnforceRevisionsInRequests').delete
+  end
+end


Yeah and this is data_schema.rb. So if you setup a fresh database for instance, the data migrations are not running afterward.

marcus-h · 2021-06-24T09:13:06Z

On 2021-06-22 22:00:06 -0700, Stephan Kulow wrote: It's rather an issue for the non-attributed projects, which brings us to the question: why shouldn't this be the default?

Hmm thinking about it again: is the OBS:EnforceRevisionsInRequests really the right mean/tool to "fix" the underlying problem? For instance, when using this attribute, we cannot use OBS's automerge feature anymore and, consequently, we could "lose" a commit. Example: - let's assume prjA/pkg and prjB/pkg are "simple" (no fixed rev etc.) branches of prjC/pkg - prjA/pkg: add a new patch to the spec file - create SR prjA/pkg -> prjC/pkg (req1234) - prjB/pkg: mark a file in the spec %files section list as %config - create SR prjB/pkg -> prjC/pkg (req(5678) - review req1234 and accept a review (not the request) - review req5678 and accept a review (not the request) - all bot related reviews are done for req1234 -> accept the request - all bot related reviews are done for req5678 -> accept the request => the change from req1234 is lost (due to the "disabled" automerge) Of course, one could also construct an example where a "hidden" automerge can lead to issues... Maybe we should track against which revision of a/the target package a review was made and reopen the review if the revision changed during the "whole" review process?

coolo · 2021-06-24T09:29:02Z

I'm all for it - but that's a whole different story, the source revision has no real mapping in the API database. For status API we bind them to repo revisions, but request reviews are an old construct

hennevogel self-requested a review June 21, 2021 09:51

coolo force-pushed the adrianschroeter-add_revision_enforcement branch from 7142391 to 5ea8792 Compare June 21, 2021 09:53

hennevogel approved these changes Jun 21, 2021

View reviewed changes

src/api/app/models/bs_request_action.rb Outdated Show resolved Hide resolved

src/api/test/functional/request_controller_test.rb Show resolved Hide resolved

coolo mentioned this pull request Jun 21, 2021

check_source.py: Validate content of _links in submissions openSUSE/openSUSE-release-tools#2570

Closed

4 tasks

coolo force-pushed the adrianschroeter-add_revision_enforcement branch from 69d3d6a to 92c8c47 Compare June 21, 2021 17:24

hennevogel requested changes Jun 22, 2021

View reviewed changes

adrianschroeter and others added 4 commits June 23, 2021 07:02

[api] add attribute to enforce revisions in requests

5c740e3

Target projects may decide always to get requests with versioned actions. Note: revisions to unexpanded sources are still possible!

Fix Xmlhash usage in BsRequestAction

2015254

Add a spec for behavior with OBS:EnforceRevisionInRequests

e32b567

Always set the enforced revision in requests

0ce8b50

Once the request is created, we take the given revision, expand the sources and take the revision given by the backend in the database. So if any of the components of the submitted sources is changed afterwards, they don't invalidate reviews

coolo and others added 3 commits June 23, 2021 07:02

Update src/api/app/models/bs_request_action.rb

313fb27

Make the comment a little clearer Co-authored-by: Henne Vogelsang <hvogel@opensuse.org>

Raise an exception if updatelink is used in enforced projects

088b1b9

For OBS:EnforceRevisionsInRequests projects we want to be certain what we review is what we accept - and as such we don't want links to point anywhere, so that updatelink option is incompatible to that use case

coolo force-pushed the adrianschroeter-add_revision_enforcement branch from 8829c59 to a054d7e Compare June 23, 2021 05:09

coolo mentioned this pull request Jun 23, 2021

Requests with rev=upload should be forbidden #11273

Closed

hennevogel approved these changes Jun 23, 2021

View reviewed changes

hennevogel merged commit 36b8d06 into openSUSE:master Jun 23, 2021

coolo deleted the adrianschroeter-add_revision_enforcement branch June 24, 2021 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add revision enforcement #11263

Add revision enforcement #11263

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

codecov bot commented Jun 21, 2021 •

edited

Loading

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

hennevogel commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

marcus-h commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

coolo commented Jun 21, 2021

marcus-h commented Jun 21, 2021 via email

coolo commented Jun 22, 2021

hennevogel Jun 22, 2021

coolo Jun 22, 2021

hennevogel Jun 23, 2021

marcus-h commented Jun 22, 2021 via email

coolo commented Jun 23, 2021

marcus-h commented Jun 23, 2021 via email

coolo commented Jun 23, 2021

hennevogel Jun 23, 2021

marcus-h commented Jun 24, 2021 via email

coolo commented Jun 24, 2021

Add revision enforcement #11263

Add revision enforcement #11263

Conversation

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

codecov bot commented Jun 21, 2021 • edited Loading

Codecov Report

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021

coolo commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

hennevogel commented Jun 21, 2021

adrianschroeter commented Jun 21, 2021 via email

marcus-h commented Jun 21, 2021 via email

coolo commented Jun 21, 2021

coolo commented Jun 21, 2021

marcus-h commented Jun 21, 2021 via email

coolo commented Jun 22, 2021

hennevogel Jun 22, 2021

Choose a reason for hiding this comment

coolo Jun 22, 2021

Choose a reason for hiding this comment

hennevogel Jun 23, 2021

Choose a reason for hiding this comment

marcus-h commented Jun 22, 2021 via email

coolo commented Jun 23, 2021

marcus-h commented Jun 23, 2021 via email

coolo commented Jun 23, 2021

hennevogel Jun 23, 2021

Choose a reason for hiding this comment

marcus-h commented Jun 24, 2021 via email

coolo commented Jun 24, 2021

codecov bot commented Jun 21, 2021 •

edited

Loading