Removing inactive Core maintainers to reduce risk

[wfoll...@cloudbees.com]

Hello everyone,

The ‘core’ maintainers team in GitHub grants write permissions to around 50 repos, most notably jenkinsci/jenkins. Several members of this team have been inactive in those repositories for a long time, some of them multiple years. Unused permissions are a (minor, but still) risk to the project, there have been phishing campaigns against GitHub users in the past for example.

For that reason we have decided to remove inactive contributors from the ‘core’ team. What exactly does “inactive” mean? Defining this cutoff is difficult, but we decided that one year without any activity in the affected repositories qualifies as inactive.

Thanks everyone for your past contributions, and you’re of course welcome back any time :)

Best regards
Damien Duportal, Infrastructure officer & Wadeck Follonier, Security officer

PS: this change may take up to 24 hours to occur

[Basil Crow]

On Fri, Jan 27, 2023 at 1:06 AM 'wfoll...@cloudbees.com' via Jenkins
Developers <jenkin...@googlegroups.com> wrote:
>
> For that reason we have decided to remove inactive contributors from the ‘core’ team.

Thanks! Strongly agree that this is a great move.

> we decided

Was this decision made in concert with other core maintainers or unilaterally?

> one year without any activity in the affected repositories qualifies as inactive

Is this one year without any _general_ activity (e.g., opening a PR)
or one year without any _maintainer_ activity (e.g., merging or
closing a PR)? Write permissions are needed for the latter but not the
former. Given the stated goal to reduce the number of people with
unnecessary write access, the latter definition makes more sense to
me.

Basil

[Alexander Brandes]

Thanks for your ongoing efforts to make our repositories secure!

> Was this decision made in concert with other core maintainers

I was not aware of such a change, but I heavily endorse the cleanup :)

Alex

[wfoll...@cloudbees.com]

Thanks for the positive feedback!

>  we decided

Decision was made between Damien and me.

It was discussed with a few other people and as we got mainly just ultra positive response, widen the discussion before the act did not seem necessary. Especially when such a change can be reversed without problems.

Concerning the activities, the remaining people did at least a PR review during the last year. This is not strictly requiring the write permission, but I don't want to do something that could be seen as a witch hunt. 

This kind of risk reduction was rarely done in the past, I don't want to be too aggressive on that. Starting small and increasing the scope over time. CERT membership and VPN/infra access will follow soon.

Wadeck

[Tim Jacomb]

While I’m very much +1 for the change I think it should’ve been discussed on the mailing list publicly

--
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/007096da-aac2-4444-93ec-44466cb3527an%40googlegroups.com.

[Oleg Nenashev]

I agree the cleanup is always useful, though it would be definitely nice to document explicit criteria in https://github.com/jenkinsci/jenkins/blob/master/docs/MAINTAINERS.adoc#roles . Not sure I even count as an active maintainer on my own these days, though my "sabbatical" was publicly announced

For transparency reasons, the following maintainers were removed according to the audit log:

• Michael Neale
• Ryan Campbell
• Andrew Bayer
• Stephen Connolly
• Matt Sicker
• Steven Christou
• Keith Zantow
• Sam van Oort
• Evaristo Gutiérrez
  
• Ramon Leon

All the mentioned contributors are ex CloudBees employees who have already moved on, company wise or project wise. Some of them are still active in the Jenkins community, but I do not see any activities around Jenkins core in the recent 2 years. I would not be too concerned about the removal, especially since the permissions can be easily restored upon return to Jenkins core contributions.

Best regards,

Oleg

[Basil Crow]

On Sun, Jan 29, 2023 at 4:52 AM 'wfoll...@cloudbees.com' via Jenkins
Developers <jenkin...@googlegroups.com> wrote:
> Thanks for the positive feedback!

It was positive feedback about the intention, not necessarily the end result.

> [To] widen the discussion before the act did not seem necessary. Especially when such a change can be reversed without problems.

By the same logic, we could dispense with core PR reviews because any
commit can be reverted without problems. Such an approach would appear
to go against the consensus-driven nature of the project.

> Concerning the activities, the remaining people did at least a PR review during the last year.

That would make those people eligible to be members of the
core-pr-reviewers team, which has triage permissions but not write
permissions. Eligible members of the core team, which has write
permissions, would be those who have merged or closed a PR during the
last year. Can you see a flaw in my reasoning?

> This is not strictly requiring the write permission, but I don't want to do something that could be seen as a witch hunt.

I do not see how it could be seen as that given the explicitly stated
goal (reducing the risk of security exposure), which has been
discussed transparently in this public thread.

> This kind of risk reduction was rarely done in the past, I don't want to be too aggressive on that.

On the contrary, the fact that this kind of risk reduction was rarely
done in the past is all the more reason to act deliberately,
conciliarly, and thoroughly in the present: past precedent shows that
our decision is likely to stick for a long time.

[wfoll...@cloudbees.com]

>  By the same logic, we could dispense with core PR reviews because any commit can be reverted without problems

You're going on the extreme there. A PR merged in a release can be installed. A missing permission is not at that kind of level of impact.

> [...] witch hunt [...]

To explicit my thinking process there that will also reply to the other comments.

I do not want to put a clock ticking above the heads of any role, that would notify you "hey you have not merged a PR since 11 months, in 30 days you will be kicked". That's neither the goal nor the intent.

> [...] I think it should’ve been discussed on the mailing list publicly

For the CERT list, I will post a message in the mailing list before removing the inactive users.

[Daniel Beck]

On Sun, Jan 29, 2023 at 8:53 PM Basil Crow <m...@basilcrow.com> wrote:

By the same logic, we could dispense with core PR reviews because any
commit can be reverted without problems. Such an approach would appear
to go against the consensus-driven nature of the project.

If there were votes taken for release readiness, rather than a weekly release train that ships whatever is on the default branch, maybe. As it is, it's not a good comparison.

I also see this less as a step to remove maintainers who are not doing any maintenance (which goes beyond what the officers' mission is) and more limiting risk (which is in scope). IMO if one of the affected folks were to show up tomorrow and resume activity in core reviews etc., I would be happy with a quick restoration of access, different from how it'd work for a newcomer.

Similarly, if any of the folks affected actually did have recent activity and it was just missed, access can easily be restored as well with minimal inconvenience to those affected.

That would make those people eligible to be members of the
core-pr-reviewers team, which has triage permissions but not write
permissions. Eligible members of the core team, which has write
permissions, would be those who have merged or closed a PR during the
last year. Can you see a flaw in my reasoning?

There's a discussion to be had about what activities count as core maintainer activities. While that's probably useful for us to have at some point, it'd block removing access from folks who _clearly_ are no longer involved.

If Wadeck and Damien had removed or downgraded permissions of folks who're still actively submitting or reviewing PRs (perhaps just not clicking the "Merge" button), I could see the problems with this approach, as we've never defined what counts as maintaining. As is, arguments seem to be more about a hypothetical "what if" than what was actually done. How would you feel if any of the folks Oleg listed suddenly started to merge or close PRs? Would you really not be surprised, and "think this is fine"?

[Daniel Beck]

On Mon, Jan 30, 2023 at 12:27 PM Daniel Beck <db...@cloudbees.com> wrote:

I also see this less as a step to remove maintainers who are not doing any maintenance (which goes beyond what the officers' mission is) and more limiting risk (which is in scope). IMO if one of the affected folks were to show up tomorrow and resume activity in core reviews etc., I would be happy with a quick restoration of access, different from how it'd work for a newcomer.

We could even use a separate team to represent this, like jenkinsci/inactive-core-maintainers, which would include folks whose permissions were removed due to inactivity, but who are welcome back if they want (while we don't have a real process to remove maintainer status).

(While the jenkinsci/alumni team exists, its description indicates willingness to review things, which is probably not a given here.)

[Basil Crow]

On Mon, Jan 30, 2023 at 3:19 AM 'wfoll...@cloudbees.com' via Jenkins
Developers <jenkin...@googlegroups.com> wrote:
> A PR merged in a release can be installed. A missing permission is not at that kind of level of impact.

A compromised account with unnecessary commit access could very well
have that level of impact if it is used to introduce malicious content
into a release.

> To [make] explicit my thinking process there that will also reply to the other comments.

I do not see how it could reply to the other comments, because my
point about membership in the core-pr-reviewers group being more
appropriate remains unaddressed.

> I do not want to put a clock ticking above the heads of any role, that would notify you "hey you have not merged a PR since 11 months, in 30 days you will be kicked". That's neither the goal nor the intent.

The stated goal and intent is to eliminate unnecessary
security-related exposure, accomplished by removing unneeded commit
access. Just as an individual who has not reviewed a PR in over a year
is creating unnecessary exposure by holding onto unneeded commit
access, so also an individual who has not merged or closed a PR in
over a year is creating unnecessary exposure by holding onto unneeded
commit access. Therefore, accomplishing the stated goal and intent
necessitates removing the unnecessary exposure created by both groups.

On Mon, Jan 30, 2023 at 3:27 AM 'Daniel Beck' via Jenkins Developers
<jenkin...@googlegroups.com> wrote:
> How would you feel if any of the folks Oleg listed suddenly started to merge or close PRs? Would you really not be surprised, and "think this is fine"?

Since I often receive feedback from individuals who have not been
active in a long time and who decline to provide acknowledgement after
I address their feedback, nothing would surprise me at this point.

On Sun, Jan 29, 2023 at 6:15 AM Oleg Nenashev <o.v.ne...@gmail.com> wrote:
> it would be definitely nice to document explicit criteria in https://github.com/jenkinsci/jenkins/blob/master/docs/MAINTAINERS.adoc#roles

On Mon, Jan 30, 2023 at 5:06 AM 'Daniel Beck' via Jenkins Developers
<jenkin...@googlegroups.com> wrote:
> We could even use a separate team to represent this

I agree that the criteria should be explicitly documented in
MAINTAINERS.adoc for the reasons explained in
https://producingoss.com/en/written-rules.html and that alumni should
be recognized in a separate team or web page in order to publicly
honor their past contributions. In the absence of documentation and an
alumni group, this endeavor lacks the thoroughness described in my
previous post.

[Daniel Beck]

On Mon, Jan 30, 2023 at 7:26 PM Basil Crow <m...@basilcrow.com> wrote:

A compromised account with unnecessary commit access could very well
have that level of impact if it is used to introduce malicious content
into a release.

Exactly, which is why this was done.

As a reminder, you originally responded to an explanation why this wasn't discussed more widely before this was implemented.

[wfoll...@cloudbees.com]

> [...] jenkinsci/inactive-core-maintainers [...]
+1 for the idea

And thanks for providing additional rationales behind the approach.

For Basil:

As it's about reducing the risk and not eliminating it, the approach was voluntarily not aggressive.

If you want to have a stricter / more aggressive approach, from my PoV it's another topic with harder consensus to find.

[Basil Crow]

On Tue, Jan 31, 2023 at 7:24 AM 'wfoll...@cloudbees.com' via Jenkins
Developers <jenkin...@googlegroups.com> wrote:
> As it's about reducing the risk and not eliminating it

I do not see a reason why we should accept continued exposure.
Software supply chain attacks have been increasing in recent years,
highlighting the need for a thorough response. As industry leaders in
this space, we ought to be role models rather than doing the bare
minimum.

> from my PoV it's another topic with harder consensus to find.

I see no evidence that consensus would be hard to find. It would be
one thing if concrete objections had been raised in this discussion,
but so far nobody has provided a cogent counterargument to the points
made in my last post.