BEWARE: OBS may allow source change after review
Hi, tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for. Turns out submit request in OBS do not necessarily refer to a specific revision in the originating package. Means the sources can change after a review passed. Unfortunately changing the sources does not reset reviews and reviews do not record the revision they were done for. That obviously defeats the purpose of source reviews. Accepting a request without revision may therefore lead to accepting something different from what was reviewed (resp staged). AFAIK this feature has been in OBS since the very beginning, it's just well hidden as the official interfaces osc and the webui do add a revision when creating requests. There is no way to turn that off either. Also tools such as bots that use osc as python module automatically generate submit request with revision. This is not because the server enforces it but because the client code does it. So anyone who wants to fool reviewers just has to use a custom client. According to the OBS team this hidden behavior is actually considered a feature. The security team doesn't treat it as security issue as it's kind of documented¹, even though osc doesn't behave in the documented way. There are plans to add a special attribute to disallow unversioned requests per project². IMHO a global setting to require revisions by default with the option for exceptions would have been more sensible. Anyway, apparently the fix needs some time to ripe still. Meanwhile the factory-auto bot was enhanced to decline unversioned requests to Factory (thanks Fabian). If you are using reviews in eg your devel project or run your own OBS you need to create your own bot or watch out manually. The webui does not seem to display any revision at all, so not helpful. In osc it looks like this: Request: #12345 submit: foo:bar/baz@42 -> blah Absence of the @number means no revision. cu Ludwig [1] https://openbuildservice.org/help/manuals/obs-user-guide/cha.obs.request_and... [2] https://github.com/openSUSE/open-build-service/pull/10992 -- (o_ Ludwig Nussel //\ V_/_ http://www.suse.com/ SUSE Software Solutions Germany GmbH, GF: Felix Imendörffer HRB 36809 (AG Nürnberg)
Hi Ludwig, On Fri, May 28, 2021 at 4:10 PM Ludwig Nussel <ludwig.nussel@suse.de> wrote:
Hi,
tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for.
Turns out submit request in OBS do not necessarily refer to a specific revision in the originating package. Means the sources can change after a review passed. Unfortunately changing the sources does not reset reviews and reviews do not record the revision they were done for. That obviously defeats the purpose of source reviews. Accepting a request without revision may therefore lead to accepting something different from what was reviewed (resp staged).
Thanks for publicizing this issue, would it be possible to get a patch to disable this functionality? Doing this per-project basis sounds fine but for some OBS instances the alternative is never desired. Regards, ismail
On Dienstag, 1. Juni 2021, 20:05:43 CEST İsmail Dönmez wrote:
Hi Ludwig,
On Fri, May 28, 2021 at 4:10 PM Ludwig Nussel <ludwig.nussel@suse.de> wrote:
Hi,
tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for.
Turns out submit request in OBS do not necessarily refer to a specific revision in the originating package. Means the sources can change after a review passed. Unfortunately changing the sources does not reset reviews and reviews do not record the revision they were done for. That obviously defeats the purpose of source reviews. Accepting a request without revision may therefore lead to accepting something different from what was reviewed (resp staged).
Thanks for publicizing this issue, would it be possible to get a patch to disable this functionality? Doing this per-project basis sounds fine but for some OBS instances the alternative is never desired.
it also breaks functionality like submitting modified link information. One could enforce it always via an config/options.yml setting, but it has more consequences therefore. It was always communicated that it is up to the reviewer to decide if he wants to accept this or not (people should be used to this situation as it is also the default on platforms like github). It is nothing new since more then 10 years after all... -- Adrian Schroeter <adrian@suse.de> 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
On Wed, Jun 2, 2021 at 7:39 AM Adrian Schröter <adrian@suse.de> wrote:
On Dienstag, 1. Juni 2021, 20:05:43 CEST İsmail Dönmez wrote:
Hi Ludwig,
On Fri, May 28, 2021 at 4:10 PM Ludwig Nussel <ludwig.nussel@suse.de> wrote:
Hi,
tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for.
Turns out submit request in OBS do not necessarily refer to a specific revision in the originating package. Means the sources can change after a review passed. Unfortunately changing the sources does not reset reviews and reviews do not record the revision they were done for. That obviously defeats the purpose of source reviews. Accepting a request without revision may therefore lead to accepting something different from what was reviewed (resp staged).
Thanks for publicizing this issue, would it be possible to get a patch to disable this functionality? Doing this per-project basis sounds fine but for some OBS instances the alternative is never desired.
it also breaks functionality like submitting modified link information.
One could enforce it always via an config/options.yml setting, but it has more consequences therefore.
It was always communicated that it is up to the reviewer to decide if he wants to accept this or not (people should be used to this situation as it is also the default on platforms like github). It is nothing new since more then 10 years after all...
This sounds all good and fine but: 1. WebUI does not show this at all, not everyone reviews via osc. Especially outside SUSE. 2. Any reviews done should be obsoleted when a new revision arrives (ala GitHub) but this doesn't happen. Regards, ismail
Hey, On 02.06.21 11:42, İsmail Dönmez wrote:
1. WebUI does not show this at all, not everyone reviews via osc. Especially outside SUSE.
Sounds like you want to create a feature request Kardeş
2. Any reviews done should be obsoleted when a new revision arrives (ala GitHub) but this doesn't happen.
On gihtub this is also a per repository setting which is default off... https://paste.opensuse.org/view/raw/35226649 Henne -- Henne Vogelsang http://www.opensuse.org Everybody has a plan, until they get hit. - Mike Tyson
Hi, On 2021-05-28 16:10:25 +0200, Ludwig Nussel wrote:
tl;dr if you rely on package reviews for your development process in OBS make sure requests have a revision. Only osc shows the information you are looking for.
Note that just relying on the presence of a revision is not "sufficient" because it can point to a link (see below). <SNIP>
AFAIK this feature has been in OBS since the very beginning, it's just well hidden as the official interfaces osc and the webui do add a revision when creating requests. There is no way to turn that off either. Also tools such as bots that use osc as python module automatically generate submit request with revision. This is not because the server enforces it but because the client code does it.
In case of the osc lib, it depends on how you use it. For instance, if you create a request via r = osc.core.Request() r.add_action('submit', src_project='openSUSE:Tools', src_package='osc', tgt_project='home:Marcus_H', tgt_package='abc') r.create(conf.config['apiurl']) no revision is added. However, if you use r.create(conf.config['apiurl'], addrevision=True) the API takes care of adding a revision. <SNIP>
Meanwhile the factory-auto bot was enhanced to decline unversioned requests to Factory (thanks Fabian).
Does it also check if the specified revision points to an expanded file set? For instance, let's assume that - prj/tgt is a plain package (no _link file) - prj/lnk is a link to prj/tgt Now, create a request via r = osc.core.Request() rev = '40e1a6ff74681c68a001adc3ca0c6474' r.add_action('submit', src_project='prj', src_package='lnk', src_rev=rev, tgt_project='prj', tgt_package='bar') r.create(conf.config['apiurl']) where 40e1a6ff74681c68a001adc3ca0c6474 points to an unexpanded file set (that is, it has a _link file). Such a request is displayed like this $> osc rq show 1234 Request: #1234 submit: prj/lnk@40e1a6ff74681c68a001adc3ca0c6474 -> prj/bar ... $> Even if you now run "osc rq show -d 1234", the "expanded" diff is displayed. That is, it is probably not apparent from a reviewer's POV that rev 40e1a6ff74681c68a001adc3ca0c6474 is in fact a link. Now, if a review is accepted, the "attacker" can modify prj/lnk's link target. If the request is eventually accepted, the modified files end up prj/bar. Marcus
participants (6)
-
Adrian Schröter -
Henne Vogelsang -
İsmail Dönmez -
Ludwig Nussel -
Marcus Hüwe -
Michael Ströder