[JENKINS-33256] Untrusted PRs #29

jglick · 2016-03-07T20:02:51Z

JENKINS-33256

Downstream of jenkinsci/scm-api-plugin#5 and (implicitly) jenkinsci/pipeline-plugin#244.

implementation sketch
finish implementation
verify that base commit is really in the target branch even for “stacked” PRs
get a PR on PR-demo modifying Jenkinsfile from an outsider: Bad update to Jenkinsfile cloudbeers/PR-demo#6
interactive test

@reviewbybees esp. @stephenc, @recena

recena · 2016-03-07T21:18:14Z

Looks great, waiting for trustedReplacement() implementation 😵

recena · 2016-03-07T22:04:15Z

LGTM

…ead.

jglick · 2016-03-08T00:23:51Z

pom.xml

@@ -50,7 +51,7 @@
        <dependency>
            <groupId>org.jenkins-ci.plugins</groupId>
            <artifactId>github-api</artifactId>
-            <version>1.71</version>
+            <version>1.72</version>


Not really necessary, but somehow @kohsuke seems to have published 1.71 without a *-sources.jar, making it harder to debug issues.

ghost · 2016-03-08T00:40:22Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick · 2016-03-08T00:41:01Z

Now when you build cloudbeers/PR-demo#6 you see a warning in the build log that the PR is untrusted (there is also a matching warning in branch indexing), and Jenkinsfile from the master branch is used…yet this line prints the file size as 13, because it checked out the repository sources as of the PR.

jenkinsadmin · 2016-03-08T01:01:49Z

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

jglick · 2016-03-08T14:13:00Z

src/main/java/org/jenkinsci/plugins/github_branch_source/GitHubSCMSource.java

+     * Quickest is to check whether the author of the PR
+     * <a href="https://developer.github.com/v3/repos/collaborators/#check-if-a-user-is-a-collaborator">is a collaborator of the repository</a>.
+     * By checking <a href="https://developer.github.com/v3/repos/collaborators/#list-collaborators">all collaborators</a>
+     * it is possible to further ascertain if they are in a team which was specifically granted push permission,


Cf. this tip. To check its effectiveness:

if curl -u YOU:ACCESSTOKEN -s -f https://api.github.com/repos/ORG/REPO/collaborators/SOMEONE; then echo trusted; else echo untrusted; fi

Currently gives a false positive for people in a read-only team (which almost certainly means a private organization). I think this is a low risk; presumably such people are known to the administrator of the organization and would be leaving a clear audit trail if they attempted to file a PR with any kind of malicious content.

Since this API does not return a permissions set, the only way to verify that push permission is granted is to retrieve the full collaborators list and search for the user.

👍 @jglick I'd like to review the PR this night, please, give some of time before merge.

Specifically you can use jq

curl -u YOU:ACCESSTOKEN -s https://api.github.com/repos/ORG/REPO/collaborators | jq '.[] | .login, .permissions.push'

but you need to manually follow Link headers with rel="next".

andresrc · 2016-03-10T14:03:57Z

🐝

amuniz · 2016-03-10T14:08:58Z

IIUC for an untrusted PR this is going to build the synthetic merge commit but using the Jenkinsfile at the target branch revision, right? If so, I can not find the code responsible to somehow replace the Jenkinsfile revision. Is it maybe here? Then the target branch revision will be build (for all files, not only Jenkinsfile) when using checkout scm, no?

jglick · 2016-03-10T14:16:08Z

for an untrusted PR this is going to build the synthetic merge commit

From checkout scm, yes.

but using the Jenkinsfile at the target branch revision, right?

GitHub calls it a “base” branch, but yes.

Is it maybe here?

Yes.

Then the target branch revision will be build (for all files, not only Jenkinsfile) when using checkout scm, no?

No.

I actually (tried to) explain this exact point earlier to @kzantow; see above thread.

amuniz · 2016-03-10T14:19:56Z

No.
I actually (tried to) explain this exact point earlier to @kzantow; see above thread.

Ok, in tests we trust :) Although I don't get how it works, I'll try to investigate later by myself 🐝

amuniz · 2016-03-10T14:23:11Z

Ah, ok. SCMVar is used to publish the scm object and SCMBinder is used only to get the content of Jenkinsfile, right? I thought SCMBinder was used for both.

jglick · 2016-03-10T14:25:57Z

right?

Right.

I thought SCMBinder was used for both.

Once the code to define scm was inside SCMBinder.java; jenkinsci/pipeline-plugin#239 changed that, although an analogous differentiation between Jenkinsfile and scm could have been done without that refactoring too.

amuniz · 2016-03-10T14:32:15Z

Everything is clear now, thanks for the explanation!

kzantow · 2016-03-10T14:43:36Z

🐝 yes, much more clear after your explanations, thanks @jglick

jglick · 2016-03-10T14:47:09Z

@reviewbybees done

but deferring a merge until after a proposed plugin release.

recena · 2016-03-14T15:48:03Z

src/main/java/org/jenkinsci/plugins/github_branch_source/UntrustedPullRequestSCMRevision.java

+/**
+ * Revision of a pull request which should load sensitive files from the base branch.
+ */
+class UntrustedPullRequestSCMRevision extends AbstractGitSCMSource.SCMRevisionImpl {


@jglick Why not simplify PullRequestSCMRevision with a field boolean trusted?

There is no such class. Now as previously mentioned, it might be valuable to create such a class to solve things like JENKINS-33161 and JENKINS-33237 as well as correcting the retrieve overload bug, so it might be advisable to do that now to simplify the settings migration. (It is always possible to deprecate this class and readResolve to something else, but leaves behind a small mess in the code.)

recena · 2016-03-14T15:51:50Z

🐝 belated

amuniz · 2016-03-14T16:43:00Z

Getting this exception after branch discovery (branches matching the criteria are discovered successfully but the Jenkinsfile is not run):

[... git checkout logs here ...]

java.lang.VerifyError: Expecting a stackmap frame at branch target 11
Exception Details:
  Location:
    org/jenkinsci/plugins/workflow/cps/CpsThread.isRunnable()Z @4: ifnull
  Reason:
    Expected stackmap frame at this location.
  Bytecode:
    0x0000000: 2ab4 0064 c600 0704 a700 0403 ac       

    at org.jenkinsci.plugins.workflow.cps.CpsScript.<init>(CpsScript.java:63)
    at WorkflowScript.<init>(WorkflowScript)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
    at sun.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
    at sun.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
    at java.lang.reflect.Constructor.newInstance(Constructor.java:422)
    at java.lang.Class.newInstance(Class.java:442)
    at org.codehaus.groovy.runtime.InvokerHelper.createScript(InvokerHelper.java:408)
    at groovy.lang.GroovyShell.parse(GroovyShell.java:743)
    at org.jenkinsci.plugins.workflow.cps.CpsGroovyShell.reparse(CpsGroovyShell.java:106)
    at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.parseScript(CpsFlowExecution.java:376)
    at org.jenkinsci.plugins.workflow.cps.CpsFlowExecution.start(CpsFlowExecution.java:343)
    at org.jenkinsci.plugins.workflow.job.WorkflowRun.run(WorkflowRun.java:212)
    at hudson.model.ResourceController.execute(ResourceController.java:98)
    at hudson.model.Executor.run(Executor.java:381)
Finished: FAILURE

Using scm-api and workflow-aggregator compiled from upstreams noted in the description of this PR and Jenkins 1.609.3 (mvn hpi:run on this plugin after checking out this PR head commit).

jglick · 2016-03-14T16:48:50Z

@amuniz yeah I saw the same error. Cannot imagine how it could be a consequence of this PR, but you never know. I suspect it has something to do with a mixture of plugins built against Java 6 and 7. I will dig into it.

workflow-aggregator compiled from upstreams

Unnecessary, since the 1.15 release should have everything.

amuniz · 2016-03-14T16:57:41Z

Unnecessary, since the 1.15 release should have everything.

Yeah, I realized just now. And scm-api 1.1 is released too, so there is no need to use a SNAPSHOT anymore, just mvn hpi:run on this PR.

BTW I'm unable to manually test this PR because the fail is consistent (at least using hpi:run, I'm going to try it with java -jar jenkins.war and manually upgrading the plugin.

amuniz · 2016-03-14T17:08:44Z

The issue is that this plugin defines its Jenkins baseline as 1.609.3 while it's using workflow-aggregator:1.15 - in test scope - which requires 1.642.1, so hpi:run is starting up an incompatible environment.

amuniz · 2016-03-14T17:19:47Z

When the untrusted PR is filed against a branch that does not have a Jenkinsfile, then this message is shown:

ERROR: [...]/workspace/untrusted-branches/PR-1@script/Jenkinsfile not found
Finished: FAILURE

I guess it is as designed because there is no Jenkinsfile in the base revision, no?

amuniz · 2016-03-14T17:27:13Z

I asked @dariver (thanks!) to create a PR in a repository where he is not registered as collaborator and his PR was correctly untrusted. After that I added him as collaborator and forced a re-index, then his changes on Jenkinsfile where picked up.

In conclusion, I think this is working like a charm, mega-:bee:

Note: about the issue with hpi:run, maybe the Jenkins baseline can be bumped only for that maven goal.

jglick · 2016-03-14T17:28:33Z

hpi:run is starting up an incompatible environment

Ah right.

jglick · 2016-03-14T17:31:35Z

I guess it is as designed

Well…not exactly “designed”, but I suppose an acceptable consequence. Means that if you want to use a PR to add a Jenkinsfile you must be trusted. Perhaps the error message could be improved.

maybe the Jenkins baseline can be bumped only for that maven goal

Possible, or you can just test by other means: make -C demo run-snapshot (not yet implemented), or mvn -f github-organization-folder-plugin hpi:run (after manually linking in snapshot *.jpl; cf. jenkinsci/maven-hpi-plugin#25).

[JENKINS-33256] Untrusted PRs

Initial sketch of getTrustedRevision implementation.

ca27ccb

jglick added the work-in-progress label Mar 7, 2016

Implementing getTrustedRevision to check collaborator API.

8784214

jglick added 4 commits March 7, 2016 17:08

[FIXED JENKINS-33256] Reënabling PRs on public repos.

3e23edf

A bit more logging.

fc6a51d

Test dep on workflow-multibranch for convenience.

c8c1bef

Actually better to keep the baseHash in the SCMRevision, not the SCMH…

3bacc2f

…ead.

jglick reviewed Mar 8, 2016
View reviewed changes

FindBugs

4d9f076

jglick removed the work-in-progress label Mar 8, 2016

jglick reviewed Mar 8, 2016
View reviewed changes

jglick mentioned this pull request Mar 10, 2016

[JENKINS-33256] Calling SCMSource.getTrustedRevision jenkinsci/pipeline-plugin#244

Merged

jglick added 2 commits March 11, 2016 16:11

Merge branch 'master' into getTrustedRevision-JENKINS-33256

1f84c2c

Pipeline 1.15 released.

b039721

Inaccurate comment.

6fa55c1

recena reviewed Mar 14, 2016
View reviewed changes

jglick added a commit that referenced this pull request Mar 14, 2016

Merge pull request #29 from jglick/getTrustedRevision-JENKINS-33256

e1ea086

[JENKINS-33256] Untrusted PRs

jglick merged commit e1ea086 into jenkinsci:master Mar 14, 2016

jglick deleted the getTrustedRevision-JENKINS-33256 branch March 14, 2016 17:33

jglick added a commit that referenced this pull request May 3, 2016

Removing instructions obsolete since #29 (JENKINS-33256).

4970dbc

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-33256] Untrusted PRs #29

[JENKINS-33256] Untrusted PRs #29

jglick commented Mar 7, 2016

recena commented Mar 7, 2016

recena commented Mar 7, 2016

jglick Mar 8, 2016

ghost commented Mar 8, 2016

jglick commented Mar 8, 2016

jenkinsadmin commented Mar 8, 2016

jglick Mar 8, 2016

recena Mar 8, 2016

jglick Mar 8, 2016

andresrc commented Mar 10, 2016

amuniz commented Mar 10, 2016

jglick commented Mar 10, 2016

amuniz commented Mar 10, 2016

amuniz commented Mar 10, 2016

jglick commented Mar 10, 2016

amuniz commented Mar 10, 2016

kzantow commented Mar 10, 2016

jglick commented Mar 10, 2016

recena Mar 14, 2016

jglick Mar 14, 2016

recena commented Mar 14, 2016

amuniz commented Mar 14, 2016

jglick commented Mar 14, 2016

amuniz commented Mar 14, 2016

amuniz commented Mar 14, 2016

amuniz commented Mar 14, 2016

amuniz commented Mar 14, 2016

jglick commented Mar 14, 2016

jglick commented Mar 14, 2016

[JENKINS-33256] Untrusted PRs #29

[JENKINS-33256] Untrusted PRs #29

Conversation

jglick commented Mar 7, 2016

recena commented Mar 7, 2016

recena commented Mar 7, 2016

jglick Mar 8, 2016

Choose a reason for hiding this comment

ghost commented Mar 8, 2016

jglick commented Mar 8, 2016

jenkinsadmin commented Mar 8, 2016

jglick Mar 8, 2016

Choose a reason for hiding this comment

recena Mar 8, 2016

Choose a reason for hiding this comment

jglick Mar 8, 2016

Choose a reason for hiding this comment

andresrc commented Mar 10, 2016

amuniz commented Mar 10, 2016

jglick commented Mar 10, 2016

amuniz commented Mar 10, 2016

amuniz commented Mar 10, 2016

jglick commented Mar 10, 2016

amuniz commented Mar 10, 2016

kzantow commented Mar 10, 2016

jglick commented Mar 10, 2016

recena Mar 14, 2016

Choose a reason for hiding this comment

jglick Mar 14, 2016

Choose a reason for hiding this comment

recena commented Mar 14, 2016

amuniz commented Mar 14, 2016

jglick commented Mar 14, 2016

amuniz commented Mar 14, 2016

amuniz commented Mar 14, 2016

amuniz commented Mar 14, 2016

amuniz commented Mar 14, 2016

jglick commented Mar 14, 2016

jglick commented Mar 14, 2016