[JENKINS-41522] Add better trust checking by allowing non-forked code to be trusted #109

myoung34 · 2017-01-23T17:50:44Z

This resolves:

Better fork detection
Allow PR's from non-fork code. Behave as before for forked code
Allow trusted files inside PR's (Jenkinsfile) from non-fork pull-requests. Let Github handle permissions on pull-requests for code within the same origin.

myoung34 · 2017-01-23T18:10:21Z

@jglick probing because this is holding a lot of people back on the new SCM stuff

ryang1428 · 2017-01-23T19:48:52Z

👍 Thanks for the quick turn around on this issue!

michaelneale · 2017-01-24T01:42:36Z

are there any cases where someone committing to a non forked branch of a github repo may have less access to master? ie less reason to "trust" their Jenkinsfile? (if I Have understood this improvement correctly).

myoung34 · 2017-01-24T03:08:21Z

No, if anything I'm more comfortable trusting people that are able to push to the upstream than I am someone listed as a contributor

michaelneale · 2017-01-24T05:32:06Z

sounds good to me then...

cc @jglick - for consideration, as according to https://issues.jenkins-ci.org/browse/JENKINS-37931, the intention was to allow merged Jenkinsfiles/Jenkinsfiles from origin PRs all along, but it just has very specific requirement on github permissions which isn't quite clear.

myoung34 · 2017-01-25T23:47:33Z

@jglick any updates? I dont want to try to keep resolving conflicts whenever master changes if possible.

ryang1428 · 2017-01-26T03:06:45Z

+1 This is hindering our efforts to get new jobs setup & tested via a PR without merging straight into the base branch

myoung34 · 2017-01-27T14:14:20Z

What if I ping @batmat @Vlatombe @KostyaSha @stephenc

This is really a show stopper for multiple people.

stephenc · 2017-01-27T14:19:13Z

NOTHING is getting merged until https://issues.jenkins-ci.org/browse/JENKINS-41234 is resolved and the releases land in the update center

myoung34 · 2017-01-27T14:20:34Z

Can this be a part of it is what I'm getting towards. Even when those get released this will still be a factor.

I realize 2.0 is in fallout, but this is an easy fix that would make many people jump to it as soon as its solid. Without this even after the fallout we'll still have an issue with adoption.

stephenc · 2017-01-27T14:26:03Z

@myoung34 I have no issues with it being RealSoon™ after the 2.0 fallout, but I am not adding anything new into the code that is currently being soak tested as that would restart the tests

myoung34 · 2017-01-27T14:30:14Z

@stephenc that's fine. This is more about knowing that an issue is actually an issue. Being merged on something both labeled fallout and in beta I don't expect to get any resolution right away.

I really just wanted some sort of acknowlegement that my efforts arent wasted and that there's not a disconnect between what we think is an issue (JENKINS-37931, JENKINS-40652) and something that some might say is either "as intended". Or that this fix does not at least provide a starting place.

I think it's just safe to say we're excited about SCM 2.0 and that as soon as its resolved with some relaxation on trusted files/PR's we'd upgrade faster than you can hit merge.

stephenc · 2017-01-27T16:50:54Z

Minimal fix of regression is in #114 these changes will be reviewed as part of https://issues.jenkins-ci.org/browse/JENKINS-41522

jglick · 2017-01-27T17:47:20Z

AFAICT this fixes no actual issue beyond what #114 addressed.

Cf. #96.

jglick · 2017-01-27T17:49:19Z

the intention was to allow merged Jenkinsfiles/Jenkinsfiles from origin PRs all along

Not just the intention—AFAIK it did do this until that got regressed accidentally in a 2.x version.

but it just has very specific requirement on github permissions

No permission check is necessary for an origin PR. These are trusted by definition.

myoung34 · 2017-01-30T20:14:27Z

@stephenc if none of this pr is needed for port you can close it.

The latest build (beta-6) built my PR as expected...

    Checking pull request #4393
    (not from a trusted source)
    Job name: PR-4393
      ‘Jenkinsfile’ found
      Not mergeable, but will be built anyway
    Met criteria
No changes detected: PR-4393 (still at 6606a5b7b76b45f1eb7fed68e06e33190f1eb6ae)

However it still says not trusted. I'm guessing that log output is not affected by #114

stephenc · 2017-01-31T11:01:23Z

@myoung34 you took the trouble to create a PR, the least I can do is take the due time and attention to review your PR and see if there is any of it that remains appropriate. I do not want to close this PR until I have given your contribution the attention it deserves and I created the JENKINS-41522 issue to ensure that your contribution is not forgotten (even if in the end we may end up not using any of it) and to record the value we place in your time and effort creating this PR in the first place.

#114 is just the minimal fix of the critical regression and the immediate priority is returning the 2.0.x versions of these plugins into the update center so that we can move forward and fix other important (but less critical) issues with these plugins

michaelneale · 2017-02-02T08:20:01Z

The 2.0 stuff hopefully may be out tomorrow (well depends on your timezone) so would be a good time to revisit that after the dust settles.

myoung34 · 2017-02-02T13:37:10Z

My only real contribution from this ticket may be a takeaway more than code, and the Jenkinsfile resolves it at a minimum, but as someone who ran into this, here's my thoughts:

Always trust a non-fork. This lets github do its own thing, and it means that your assumption is that if they're part of the org, they can make trusted changes. This is the most important and safest assumption in my opinion because it fits the org/private repo model. If they can push a change and create a PR on that remote, just short circuit and let them do their thing.
If it's a fork, add a checkbox to set the contributor logic. Perhaps it's a private repo and the "contributor" is a contractor who can fork/push but needs to be trusted. Making the assumption to trust them implicitly might be bad, but a checkbox for this would be perfect.
Be careful of the number of API calls. I noticed there are some singletons, but github has an API limit of 5000 req/hr for authenticated requests and 60/hr for unauth. If the remote has a lot of branches, pull requests and contributors and it uses a scan poll instead of remote trigger you can eat up your request limit verrry quickly. I ran into this by running this plugin on two masters and having them look at branches.

michaelneale · 2017-02-03T07:32:26Z

@myoung34:

#1: Agree - should always trust a non fork, I can't see why not.

#2: well I would think if there is a way to think about not having an option - and think really hard what default should be. Only as a last resort, add another confusing hard to explain option.

#3: the new SCM api changes help that by a LOT. I think there is more to do but it is in progress.

8000

ekimia · 2017-02-23T21:06:26Z

any updates on this? Would love to see my PRs building again...

myoung34 · 2017-02-23T21:08:33Z

What's the issue? They filed an MVC pr in #114 that has had me able to build since

jones2026 · 2017-03-31T21:10:32Z

Anyone know who we can pester to get this merged? This is stopping a lot of work!

stephenc · 2017-04-07T11:51:22Z

This will be considered after https://issues.jenkins-ci.org/browse/JENKINS-43426 has been delivered (though I suspect that the full acceptance criteria for that UX refactoring will include a fix for this issue through the ability to control the trust of forks)

stephenc · 2017-04-21T14:10:53Z

Here is a preview of the new UI. Hopefully this will explain why I have been blocking any new PRs until this work gets completed:

stephenc · 2017-04-21T14:12:02Z

BTW the Trust drop-down in "Discover pull requests from forks" is an extension point so that plugins can provide their own trust determination alogorithms

ewhauser · 2017-04-23T01:56:05Z

@stephenc Having just done the SCM 2.0 upgrade, I can tell you that this configuration looks great and is much more intuitive than the current settings.

Shouldn't Trust also be an option for Discover pull requests from origin as well? It does not seem limited to just forks. For our private organization, it doesn't make since to add people who have access to the repo as collaborators. We just want to trust everyone by default.

stephenc · 2017-04-23T08:05:54Z

If you can write to the origin repo, you are trusted. So there is no need to set trust on the origin repo PRs.

It's only fork PRs that need trust

i386 · 2017-05-04T06:42:42Z

@stephenc @recampbell

Allow PR's from non-fork code. Behave as before for forked code

FWIW recently this has come up as a point of confusion for new users of Blue Ocean and Pipeline

jglick · 2017-06-12T15:01:19Z

Is this not solving the same issue as #134?

i386 · 2017-07-13T04:23:54Z

@stephenc is there something in the Traits work that will cover this?

9E5F

stephenc · 2017-07-13T16:30:12Z

I think this is essentially covered by JENKINS-43507, there are potentially some cases where the existing trust extension points are not sufficient for some use cases, but I think those should be handled in a fresh PR

myoung34 force-pushed the bugfix/trust_unforked_201 branch from efac180 to e9bac8f Compare January 23, 2017 18:05

Add better trust checking by allowing non-forked code to be trusted

c00a3d6

myoung34 force-pushed the bugfix/trust_unforked_201 branch from e9bac8f to c00a3d6 Compare January 23, 2017 18:13

Merge branch 'master' into bugfix/trust_unforked_201

9534960

Add targetOwner into new constructor

e0a566f

stephenc changed the title ~~Add better trust checking by allowing non-forked code to be trusted~~ [JENKINS-41522] Add better trust checking by allowing non-forked code to be trusted Jan 27, 2017

jglick mentioned this pull request Jun 12, 2017

[JENKINS-40652] origin pr builds not treated as trusted - fix for regression in org repos origin PRs #134

Closed

stephenc closed this Jul 13, 2017

[JENKINS-41522] Add better trust checking by allowing non-forked code to be trusted #109

[JENKINS-41522] Add better trust checking by allowing non-forked code to be trusted #109

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!