Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-53752

Block PRs from forks from untrusted users

    XMLWordPrintable

Details

    Description

      The plugin currently has no way to block untrusted users from making a PR from a fork and having this PR built by Jenkins. The GitHub Pull Request Builder does have this feature which is very useful for open source projects to protect the build system from malicious changes. The documentation on the GitHub Pull Request Builder wiki page says to move from the GHPRB plugin to the GitHub Branch source plugin which causes the user to lose this extremely useful functionality.

      Attachments

        Issue Links

          Activity

            markewaite Mark Waite added a comment -

            Isn't the option to "Discover pull requests from forks" what you are seeking, with the setting "From Users with Admin or Write permission"?

            markewaite Mark Waite added a comment - Isn't the option to "Discover pull requests from forks" what you are seeking, with the setting "From Users with Admin or Write permission"?
            directhex Jo Shields added a comment -

            No. That's the point. That setting determines whether pull requests should use Jenkinsfile from origin/ or from the fork - it has no functionality to block pull requests from users under any circumstance.

            directhex Jo Shields added a comment - No. That's the point. That setting determines whether pull requests should use Jenkinsfile from origin/ or from the fork - it has no functionality to block pull requests from users under any circumstance.
            abayer Andrew Bayer added a comment -

            Yeah, this is a missing feature - I'm trying to figure out if it's missing by design for some reason.

            abayer Andrew Bayer added a comment - Yeah, this is a missing feature - I'm trying to figure out if it's missing by design for some reason.
            abayer Andrew Bayer added a comment -

            Preliminary PR up at https://github.com/jenkinsci/github-branch-source-plugin/pull/188 - we'll see what the reviewers think of it.

            abayer Andrew Bayer added a comment - Preliminary PR up at https://github.com/jenkinsci/github-branch-source-plugin/pull/188 - we'll see what the reviewers think of it.

            Any word on the status of this?

            I would add one more feature to allow those with write or perhaps just admin privileges to approve "untrusted" PRs.

            brianjmurrell Brian J Murrell added a comment - Any word on the status of this? I would add one more feature to allow those with write or perhaps just admin privileges to approve "untrusted" PRs.

            Will any further work be done on this or should this issue be closed?

            brianjmurrell Brian J Murrell added a comment - Will any further work be done on this or should this issue be closed?

            Isn't this option:

            supposed to achieve what is being asked for in this ticket?

            brianjmurrell Brian J Murrell added a comment - Isn't this option: supposed to achieve what is being asked for in this ticket?

            brianjmurrell, no, it isn't because it blocks only Jenkinsfile changes (it will be taken from PR's target branch, not source) and still executes it.  Therefore any user who can open a PR in your repository can easily modify build scripts/CMake files and gain access to your build systems

            oxygenxo Andrey Babushkin added a comment - brianjmurrell , no, it isn't because it blocks only Jenkinsfile changes (it will be taken from PR's target branch, not source) and still executes it.  Therefore any user who can open a PR in your repository can easily modify build scripts/CMake files and gain access to your build systems

            oxygenxo That's not at all how the item description or help text reads.  It very specifically says it will only build a change request / pull request ...

            brianjmurrell Brian J Murrell added a comment - oxygenxo That's not at all how the item description or help text reads.  It very specifically says it will only build a change request / pull request ...

            I'm sorry brianjmurrell, It seems I've just screwed the config of my GitHub Organization folder. I've set "Build strategies" like on the picture you've provided and "Trust" to "Nobody". Jenkins creates jobs for PRs opened by untrusted persons, but doesn't run them. That's exactly what I've needed, thank you

            oxygenxo Andrey Babushkin added a comment - I'm sorry brianjmurrell , It seems I've just screwed the config of my GitHub Organization folder. I've set "Build strategies" like on the picture you've provided and "Trust" to "Nobody". Jenkins creates jobs for PRs opened by untrusted persons, but doesn't run them . That's exactly what I've needed, thank you
            bitwiseman Liam Newman added a comment -

            This is fixed and the feature provided by a plugin

            bitwiseman Liam Newman added a comment - This is fixed and the feature provided by a plugin

            bitwiseman Could you provide some more details?  Which plugin, at least.

            brianjmurrell Brian J Murrell added a comment - bitwiseman Could you provide some more details?  Which plugin, at least.

            bitwiseman Perhaps you are referring to [#188| https://github.com/jenkinsci/github-branch-source-plugin/pull/188]. If so I would direct you to the last comment there about JENKINS-58618 and JENKINS-58683, neither of which have even been triaged.

            brianjmurrell Brian J Murrell added a comment - bitwiseman Perhaps you are referring to [#188| https://github.com/jenkinsci/github-branch-source-plugin/pull/188] . If so I would direct you to the last comment there about JENKINS-58618 and JENKINS-58683 , neither of which have even been triaged.
            jglick Jesse Glick added a comment -

            I do not see any evidence that this is fixed.

            github-branch-source will automatically ignore Jenkinsfile modifications from an untrusted forked PR, for security reasons, but it will let the build proceed. I am not aware of any way to disable that other than suppressing discovery of all forked PRs, including those filed by authors who would have had write permission to the origin repository anyway.

            jglick Jesse Glick added a comment - I do not see any evidence that this is fixed. github-branch-source will automatically ignore Jenkinsfile modifications from an untrusted forked PR, for security reasons, but it will let the build proceed. I am not aware of any way to disable that other than suppressing discovery of all forked PRs, including those filed by authors who would have had write permission to the origin repository anyway.

            Some of our repos were attacked yesterday by malicous PRs from Forks that modified our POM files (not the Jenkinsfile) to download and run a rogue shell script.  We used the default settings yet the PR build ran anyway.  

            The recommended setting should be "nobody" until this is fixed...

            rhpatrick Robert Patrick added a comment - Some of our repos were attacked yesterday by malicous PRs from Forks that modified our POM files (not the Jenkinsfile) to download and run a rogue shell script.  We used the default settings yet the PR build ran anyway.   The recommended setting should be "nobody" until this is fixed...
            jglick Jesse Glick added a comment -

            Ouch. Out of curiosity, if you are comfortable sharing—was the damage limited to stealing compute resources (e.g., for coin mining) or did the scripts attempt to use or export something sensitive from the environment?

            This seems too important to leave to an extension plugin like basic-branch-build-strategies, especially if JENKINS-58618 & JENKINS-58683 are accurate. Coming up with a good default policy which balances security of builds running on public repos with usability is not trivial, though. (Compare JENKINS-46795.) GitHub Actions has gone through a few revisions here. Ideally a repository maintainer could quickly approve a new forked PR for CI without needing to navigate to the Jenkins GUI, but TBD whether all subsequent commits to the same PR should also be built automatically, or all subsequent PRs from the same author, etc.

            jglick Jesse Glick added a comment - Ouch. Out of curiosity, if you are comfortable sharing—was the damage limited to stealing compute resources (e.g., for coin mining) or did the scripts attempt to use or export something sensitive from the environment? This seems too important to leave to an extension plugin like basic-branch-build-strategies , especially if JENKINS-58618 & JENKINS-58683 are accurate. Coming up with a good default policy which balances security of builds running on public repos with usability is not trivial, though. (Compare JENKINS-46795 .) GitHub Actions has gone through a few revisions here. Ideally a repository maintainer could quickly approve a new forked PR for CI without needing to navigate to the Jenkins GUI, but TBD whether all subsequent commits to the same PR should also be built automatically, or all subsequent PRs from the same author, etc.

            Unfortunately, I am not allowed to share the details.  

            I agree this is critical functionality for Jenkins.  It seems that other related open issues are only focused on the Jenkinsfile.  I think that is short-sighted.  Build files like POMs, Makefiles, and others are just as vulnerable as Jenkinsfiles.

            rhpatrick Robert Patrick added a comment - Unfortunately, I am not allowed to share the details.   I agree this is critical functionality for Jenkins.  It seems that other related open issues are only focused on the Jenkinsfile.  I think that is short-sighted.  Build files like POMs, Makefiles, and others are just as vulnerable as Jenkinsfiles.
            jglick Jesse Glick added a comment -

            Potentially; it depends on how your system is structured.

            The reasoning behind restricting modifications only to Jenkinsfile is to prevent an unauthorized user from running, say, withCredentials to retrieve secrets in scope. The assumption is that the Jenkinsfile is written so as to assume malicious intent from a PR on the part of any external processes it forks, which would be run on an agent computer that is itself not trusted (container or VM), so a rogue process could waste time but not do much else. (You can choose to readTrusted other configuration files which would structurally affect how a build uses Jenkins resources, such as choice of agent labels or credentials or notification targets.)

            A particular project may not have been set up with this threat model in mind, though, or PR builds might require certain credentials (such as for a database) just to run tests, and in some environments just running unexpected build processes even on an isolated computer could be a threat, so we ought to have something that is easier to secure. See also JENKINS-45970.

            jglick Jesse Glick added a comment - Potentially; it depends on how your system is structured. The reasoning behind restricting modifications only to Jenkinsfile is to prevent an unauthorized user from running, say, withCredentials to retrieve secrets in scope. The assumption is that the Jenkinsfile is written so as to assume malicious intent from a PR on the part of any external processes it forks, which would be run on an agent computer that is itself not trusted (container or VM), so a rogue process could waste time but not do much else. (You can choose to readTrusted other configuration files which would structurally affect how a build uses Jenkins resources, such as choice of agent labels or credentials or notification targets.) A particular project may not have been set up with this threat model in mind, though, or PR builds might require certain credentials (such as for a database) just to run tests, and in some environments just running unexpected build processes even on an isolated computer could be a threat, so we ought to have something that is easier to secure. See also JENKINS-45970 .

            You aren’t thinking this through.  Suppose, for example, you are running a Maven command within a withCredentials block…same for running Make, ant, etc.  change the build file being run and now you can grab the environment variables in any rogue script…

            rhpatrick Robert Patrick added a comment - You aren’t thinking this through.  Suppose, for example, you are running a Maven command within a withCredentials block…same for running Make, ant, etc.  change the build file being run and now you can grab the environment variables in any rogue script…
            jglick Jesse Glick added a comment -

            Right, which is why you must not run a Maven command inside a withCredentials block unless you are on a known (origin) branch.

            jglick Jesse Glick added a comment - Right, which is why you must not run a Maven command inside a withCredentials block unless you are on a known (origin) branch.
            rhpatrick Robert Patrick added a comment - - edited

            jglick We understood the risk of auto-building a fork PR but trusted the Jenkins GitHub Branch Source Plugin to do what it said when we set Trust to "From users with Admin or Write permission"...that was our mistake.  I am appalled by the fact that this issue is 4+ years old and no one seems to care. 

            rhpatrick Robert Patrick added a comment - - edited jglick We understood the risk of auto-building a fork PR but trusted the Jenkins GitHub Branch Source Plugin to do what it said when we set Trust to "From users with Admin or Write permission"...that was our mistake.  I am appalled by the fact that this issue is 4+ years old and no one seems to care. 

            To be frank, we were pretty shocked also to find that PRs were not being blocked from untrusted sources amongst all of this, particularly given the screenshot in https://issues.jenkins.io/browse/JENKINS-53752?focusedCommentId=371866&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-371866.

            We just ended up not allowing anything from forks.  Our users are unhappy about that.  Or repo has nearly a thousand branches now because we can only run PRs opened in our organization.  The whole thing is a mess.

            Our path forward is probably going to be to trigger Jenkins jobs from GitHub Actions since GitHub Actions has some at least half-way sane security settings for when PRs can run actions and when they cannot.  No offence to anyone, but we simply cannot trust Jenkins in this regard any more.

            brianjmurrell Brian J Murrell added a comment - To be frank, we were pretty shocked also to find that PRs were not being blocked from untrusted sources amongst all of this, particularly given the screenshot in https://issues.jenkins.io/browse/JENKINS-53752?focusedCommentId=371866&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-371866 . We just ended up not allowing anything from forks.  Our users are unhappy about that.  Or repo has nearly a thousand branches now because we can only run PRs opened in our organization.  The whole thing is a mess. Our path forward is probably going to be to trigger Jenkins jobs from GitHub Actions since GitHub Actions has some at least half-way sane security settings for when PRs can run actions and when they cannot.  No offence to anyone, but we simply cannot trust Jenkins in this regard any more.

            I think Jenkins is starting to show its age.  When CloudBees first got a involved, I was hopeful that the gaps and quality issues would improve and, for awhile, I think that they did.  However, right now, it feels as bad as it did pre-CloudBees.  I would love to help but it feels like the entire project is suffering from a lack of leadership.  😞

            rhpatrick Robert Patrick added a comment - I think Jenkins is starting to show its age.  When CloudBees first got a involved, I was hopeful that the gaps and quality issues would improve and, for awhile, I think that they did.  However, right now, it feels as bad as it did pre-CloudBees.  I would love to help but it feels like the entire project is suffering from a lack of leadership.  😞

            People

              Unassigned Unassigned
              roguishmountain Sam Schwarz
              Votes:
              6 Vote for this issue
              Watchers:
              12 Start watching this issue

              Dates

                Created:
                Updated: