Implementing proper permission checks during silo publish

[Łukasz Zemczak]

This has been raised by the DMB as a potential archive-security issue.

Currently the CI Train allows anyone with access to the right jenkins jobs publishing silos, so in fact publishing packages to the Ubuntu archive. This means that every train operator basically has the same power as a core-dev.
The CI Train should check the permission levels of the currently signed in trainguard and only allow him/her to publish packages that he/she is allowed to, e.g. MOTUs - universe, core-dev - everything, PPU - selected packages.

[Steve Langasek (vorlon)]

For the record, note that there are only a small number of people in the set of "train operators" (as opposed to "train users") who are not also core-devs; these people are all aware of the policy that requires core-dev sign-off for package changes; and they have commit access to the actual train code itself. While there is opportunity to improve and streamline the CI Train in this respect, I would argue that the current situation is no different than the fact that people who are not Ubuntu core-devs work for Canonical IS, or have commit access to Launchpad.

[Steve Langasek (vorlon)]

Robert asked me to open a separate bug for this, but after thinking it over I'm convinced this is all part of the same problem so I'm adding a follow-up comment instead of opening a new bug.

The things that need to be fixed are:

1) The CI Train does not currently enforce that the packaging ack is done by someone with upload rights on the package in question. The train operators who publish uploads know that the policy is that packaging changes must be signed off by a core-dev; however the signoff should be done by the core-dev directly in the train.
2) Signoff permissions should track the archive permissions for a package: respecting archive components (ubuntu-core-dev vs. motu) and per-package uploaders.
3) Packages which Canonical is not upstream for should always require an Ubuntu developer's signoff for publication. The only case where an Ubuntu dev ack is not required is when the train user is an upstream developer for the package in question and there are no packaging changes.

[Robert Bruce Park (robru)]

Right, this is all within the realm of possibility, the only challenge is
identifying who has what permissions in lplib.

I'll work to get the spreadsheet replacement online first and then focus on
this later.
On Jun 11, 2015 11:50 AM, "Steve Langasek" <email address hidden>
wrote:

> Robert asked me to open a separate bug for this, but after thinking it
> over I'm convinced this is all part of the same problem so I'm adding a
> follow-up comment instead of opening a new bug.
>
> The things that need to be fixed are:
>
> 1) The CI Train does not currently enforce that the packaging ack is done
> by someone with upload rights on the package in question. The train
> operators who publish uploads know that the policy is that packaging
> changes must be signed off by a core-dev; however the signoff should be
> done by the core-dev directly in the train.
> 2) Signoff permissions should track the archive permissions for a package:
> respecting archive components (ubuntu-core-dev vs. motu) and per-package
> uploaders.
> 3) Packages which Canonical is not upstream for should always require an
> Ubuntu developer's signoff for publication. The only case where an Ubuntu
> dev ack is not required is when the train user is an upstream developer for
> the package in question and there are no packaging changes.
>
> --
> You received this bug notification because you are subscribed to CI
> Train [cu2d].
> https://bugs.launchpad.net/bugs/1459186
>
> Title:
> Implementing proper permission checks during silo publish
>
> Status in CI Train [cu2d]:
> In Progress
>
> Bug description:
> This has been raised by the DMB as a potential archive-security issue.
>
> Currently the CI Train allows anyone with access to the right jenkins
> jobs publishing silos, so in fact publishing packages to the Ubuntu
> archive. This means that every train operator basically has the same power
> as a core-dev.
> The CI Train should check the permission levels of the currently signed
> in trainguard and only allow him/her to publish packages that he/she is
> allowed to, e.g. MOTUs - universe, core-dev - everything, PPU - selected
> packages.
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/cupstream2distro/+bug/1459186/+subscriptions
>

[Steve Langasek (vorlon)]

On Thu, Jun 11, 2015 at 06:55:58PM -0000, Robert Bruce Park wrote:
> Right, this is all within the realm of possibility, the only challenge is
> identifying who has what permissions in lplib.

ubuntu = lp.distributions['ubuntu']
archive = ubuntu.main_archive
# replace with upload target
series = ubuntu.getSeries(name_or_version= 'wily')
# replace with source package name
src = 'qtbase-opensource-src'
# replace with uploader
uploader = lp.people['timo-jyrinki']

sourcepub = archive.getPublishedSources(distro_series = series,
                                        exact_match = True,
                                        status = 'Published',
                                        pocket = 'Release',
                                        source_name = src)[0]

archive.checkUpload(pocket='Proposed', component = sourcepub.component_name,
                    person = uploader, sourcepackagename = src,
                    distroseries = series)

https://launchpad.net/+apidoc/1.0.html#archive
https://launchpad.net/+apidoc/1.0.html#person
https://launchpad.net/+apidoc/1.0.html#source_package_publishing_history

[Robert Bruce Park (robru)]

Great, thanks.

[Robert Bruce Park (robru)]

meetign with steve:

must enforce checkupload for merges with diffs, all manual sources, and NEW.

[PS Jenkins bot (ps-jenkins)]

Fix committed into lp:cupstream2distro at revision 1065, scheduled for release in cupstream2distro, milestone Unknown

[Łukasz Zemczak (sil2100)]

Is the DMB happy with the current approach? If I understand the announcement correctly, currently only manual sources and packaging changes need to be published by people with the right permissions. I originally thought that they wanted ALL publishings to require specific permissions (which, IMO, isn't really required). Just want to make sure that the DMB is happy with the current approach.

[Steve Langasek (vorlon)]

The issue that was raised by the DMB was that the train was not enforcing the agreed-upon upload requirements, namely:
 - an Ubuntu dev (with relevant upload rights) needs to sign off on packaging changes
 - an Ubuntu dev (with relevant upload rights) needs to sign off on changes to package for which Canonical is not the upstream
 - an Ubuntu dev needs to sign off on new source package additions
 - existing packages that Canonical is upstream for, and that introduce no packaging changes, can be signed off by the Canonical upstream developers.

Since this bug was filed, a member of the DMB has questioned whether this is the correct policy. That is something that the door is always open for revisiting, but is out of scope for this bug. This bug is only about enforcing the agreed policy.

With Robert's recent changes, the policy is *nearly* correctly enforced. The remaining problems that I'm aware of are:
 - When an Ubuntu dev signoff is not required, the publication is done by the trainguards, not by the upstream developers. This is not a security hole since the trainguards only publish upon request of the uploaders, who are authorized. But the process should be improved so that upstream devs can publish directly and the trainguards don't have to be in the middle. I believe Robert is working towards this.
 - The CI Train *assumes* that if the package is in a silo by way of an MP, this package is one Canonical is upstream for. This is sufficient to avoid accidental publication of packages without signoff, which is the case that has arisen in practice. It does not address the case of a malicious train user constructing a pair of branches in the correct format and an MP between them for the sole purpose of bypassing the permissions checks.

I think both of these remaining problems should be dealt with as separate bugs.

[Robert Bruce Park (robru)]

On Sep 14, 2015 4:31 PM, "Steve Langasek" <email address hidden>
wrote:
> With Robert's recent changes, the policy is *nearly* correctly enforced.
The remaining problems that I'm aware of are:
> - When an Ubuntu dev signoff is not required, the publication is done by
the trainguards, not by the upstream developers. This is not a security
hole since the trainguards only publish upon request of the uploaders, who
are authorized. But the process should be improved so that upstream devs
can publish directly and the trainguards don't have to be in the middle. I
believe Robert is working towards this.

This is basically ready to go, all that's left is to change the permissions
on the Jenkins publish job to allow train-users team to run the job
(basically publish permissions should match build permissions). I didn't
include this in the branch with the checkUpload implementation simply
because i wanted to phase the changes in over time. There's no technical
issue holding it back at all.

Basically i just wanted to see checkUpload in action for a bit, until i
grew to trust that it prevented me from uploading things correctly, before
extending publish permissions to all train users. I'm happy with
checkUpload, so I'm ready for this now, just on vacation.

> - The CI Train *assumes* that if the package is in a silo by way of an
MP, this package is one Canonical is upstream for. This is sufficient to
avoid accidental publication of packages without signoff, which is the case
that has arisen in practice. It does not address the case of a malicious
train user constructing a pair of branches in the correct format and an MP
between them for the sole purpose of bypassing the permissions checks.

This is consistent with my understanding of the train. Not having a
whitelist of things that we are upstream of was considered a feature in the
early days of the train (which was a welcome change from
lp:cupstream2distro-config if anybody remembers that).

I think the idea of a malicious user making a fake mp to trick the train
into uploading something it shouldn't is probably not a serious concern, as
the train requires some nontrivial modifications to the packaging in order
to even build the source package, you'd find that anybody trying to do this
would get stopped by the packaging ack check anyway.

> I think both of these remaining problems should be dealt with as
> separate bugs.

Fine with me.