Security approval required on UI-related PRs in Jenkins core

[wfoll...@cloudbees.com]

Today the Jenkins project released a security version that contains several high severity vulnerabilities. Five vulnerabilities from Jenkins core were introduced very recently during UI improvement work.

Such security issues discovered after a merge implies that we are investing a lot of energy/time to correct it and providing all the necessary data in terms of vulnerability management. The difference between finding them during review and after a release is really huge.

For this reason, as the security officer and effective as of today, I want to block the merge of any UI-related PRs until they have received at least one approval from someone in CERT.

To set expectations, if a PR is approved but then substantial change is committed, the approval must be dismissed and re-requested. The second approval is expected to be quicker.

This process is expected to provide better security coverage of the upcoming changes and thus, reducing the likelihood of introducing vulnerabilities.

In order to not be a blocker for the UI improvement project, I will assign more people from my team to review the PRs. The job done by the UI team is amazing and should continue.

This new policy will be revised over time and ideally removed in the mid-term.

Do you have any concerns related to this?

Wadeck Follonier

Security Officer

[Tim Jacomb]

Hi Wadeck

We can monitor this in the short term.

My concern would be around responsiveness and turn around time.

RPU hosting requests already can take a fair amount of time if there’s a few at a time

 It already takes a long time to get some of the UI pull requests in. As long as the security team are in early and actively reviewing, (and that means looking at all the pull requests currently active) too then fine.

We already have a big backlog of pull requests that we were asked to hold off on the last few weeks due to the security release, so if these could be looked at ASAP then that would be great.

Let’s add this as an agenda item for the next UX sig meeting to review how it’s going

Thanks

Tim

--
You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/7846d76d-2bc0-4829-a4a2-d9035e10592fn%40googlegroups.com.

[Alexander Brandes]

Hey Wadeck,

> until they have received at least one approval from someone in CERT.

Could we add a "security" team with read permissions on jenkinsci/jenkins, core maintainers can request a review upon if a PR touches UI components? I'm aware that Basil (?) created a label, but GitHub honors submitted reviews from team members, if requested.

Not every contributor may know every CERT member, but feedback submitted on behalf of a team gets special attribution, see https://github.com/jenkinsci/packaging/pull/320 for an example, to differentiate between regular reviews and explicit reviews.

Cheers

Alex

[wfoll...@cloudbees.com]

Great idea Alex =>  @jenkinsci/core-security-review created

Thanks for the feedback and yes Tim, I will allocate more people to those reviews, compared to the hosting requests that were mainly out-of-order stuff we are doing.

[Daniel Beck]

On Wed, Jun 22, 2022 at 9:26 PM 'wfoll...@cloudbees.com' via Jenkins Developers <jenkin...@googlegroups.com> wrote:

Great idea Alex =>  @jenkinsci/core-security-review created

Thanks for the feedback and yes Tim, I will allocate more people to those reviews, compared to the hosting requests that were mainly out-of-order stuff we are doing.

I would like to retain the ability to review core PRs without those reviews automatically counting towards security review, so please be mindful in the handling of this reviewer group. (In particular, me requesting changes for other reasons should not carry the same weight as rejecting a PR for security reasons.)

[wfoll...@cloudbees.com]

The team got "triage" permission, so that they can add the newly created label "security-approved", so that it's easier to understand when it's good to go. That will also "solve" Daniel's concern about regular review ;-)

[Tim Jacomb]

I'd suggest another label for security review complete but needs fixes.

There's currently 12 PRs showing as blocked by needing security review (1 or 2 of these may need a fix and a label could make that clearer)

https://github.com/jenkinsci/jenkins/pulls?q=is%3Apr+is%3Aopen+sort%3Aupdated-desc+label%3Aneeds-security-review

Cheers

Tim

--

You received this message because you are subscribed to the Google Groups "Jenkins Developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to jenkinsci-de...@googlegroups.com.

To view this discussion on the web visit https://groups.google.com/d/msgid/jenkinsci-dev/cdc1485d-e683-4033-aff5-3b1410e10481n%40googlegroups.com.

[Alexander Brandes]

+1, labels indeed help to make it more clear what still needs to be done.

We could add something like "needs-security-fix" next to the "needs-fix" label.

[Basil Crow]

On Wed, Jun 22, 2022 at 10:37 AM 'wfoll...@cloudbees.com' via Jenkins
Developers <jenkin...@googlegroups.com> wrote:
> For this reason, as the security officer and effective as of today, I want to block the merge of any UI-related PRs until they have received at least one approval from someone in CERT.

[…]

> Do you have any concerns related to this?

-1 from me. I lack confidence that the CERT team will acknowledge and
respond to all questions asked during the security review process.