Add delete and add methods for parsers in global configuration

[meiswjn]

I have a Jenkins instance that multiple teams work on at the same time. Since it requires admin permissions to add a new parser via the UI, we allow users to add them via Groovy. However, the function ParserConfiguration#setParsers is much more prone to accidentally deleting all other parsers.

With this PR, single parsers can be added and deleted from within pipelines.

Testing done

-> Unit Tests and tested in localhost instance

Submitter checklist

Preview Give feedback

1. Make sure you are opening from a topic/feature/bugfix branch (right side) and not your main branch!
2. Ensure that the pull request title represents the desired changelog entry
3. Please describe what you did
4. Link to relevant issues in GitHub or Jira
5. Link to relevant pull requests, esp. upstream and downstream changes
6. Ensure you have provided tests - that demonstrates feature works or fixes the issue

[KalleOlaviNiemitalo]

What is going to call these?

Does @DataBoundSetter support addFoo and deleteFoo methods? Its javadoc mentions only setFoo.

[meiswjn]

Thanks for your input @KalleOlaviNiemitalo!

In our case, the methods are called directly from pipelines. We want to allow users to create their own parsers without using setParsers, because it is much more prone to accidentially delete all other parsers.

-> We also don't need DataBoundSetter, thanks!

[uhafner]

I have a Jenkins instance that multiple teams work on at the same time. Since it requires admin permissions to add a new parser via the UI, we allow users to add them via Groovy. However, the function ParserConfiguration#setParsers is much more prone to accidentally deleting all other parsers.

With this PR, single parsers can be added and deleted from within pipelines.

Can you please show how you plan to invoke these methods? How do you get the config in your script?

[meiswjn]

Sure.

Here is a real example (edited actual values) that I grabbed from a user. In this example, they delete all other parsers. Of course they could simply extend the list, but they have to know that.

def InitParser(){
    def config = io.jenkins.plugins.analysis.warnings.groovy.ParserConfiguration.getInstance()
    config.setParsers([]) // Here, the user accidentially deleted all other parsers.

    def newParser = new io.jenkins.plugins.analysis.warnings.groovy.GroovyParser(
        'some_id', 
        'some_name', 
        'my_regex', 
        '...script...',
        '...',
        "..."
        )
    config.setParsers([newParser]) // Same thing: All other parsers deleted by accident.
}
InitParser()

With this PR, you can do (the script that I tested myself with the new changes):

def InitParser(){
    def config = io.jenkins.plugins.analysis.warnings.groovy.ParserConfiguration.getInstance()

    def newParser = new io.jenkins.plugins.analysis.warnings.groovy.GroovyParser(
        'some_id', 
        'some_name', 
        'my_regex', 
        '...script...',
        '...',
        "..."
        )
    config.addParser(newParser)
}
InitParser()

[uhafner]

Here is a real example (edited actual values) that I grabbed from a user. In this example, they delete all other parsers. Of course they could simply extend the list, but they have to know that.

Interesting. Which permissions do users have to execute such code that normally requires admin permission?

[meiswjn]

No special permission is needed. However, the method must be allowed via Script Approval. Because of this, we would also be able to prevent users from using #setParsers and exclusively allow the add, get and delete method.

[uhafner]

Because of this, we would also be able to prevent users from using #setParsers and exclusively allow the add, get and delete method.

It definitely is better than allowing them to directly use set. But I'm not sure if your approach is a good thing from security perspective: this basically allows your users to completely erase an agent disk without further approval.

[meiswjn]

How would they be able to do that with those two methods? From my understanding they are only able to make changes to the ParserConfiguration which does not relate to the agent disk.

[KalleOlaviNiemitalo]

I guess @uhafner means the Groovy code in the parser runs without a Groovy sandbox and has the same access to files as the Jenkins agent process. (I'm not sure whether parsers run in agents or in the Jenkins controller.)

warnings-ng-plugin/plugin/src/main/java/io/jenkins/plugins/analysis/warnings/groovy/GroovyExpressionMatcher.java

Lines 23 to 25 in 610b02f

	* This class does not use any sandboxing mechanisms to parse or run the Groovy
	* script. Instead, only users with Overall/Run Scripts permission are able to
	* configure parsers that use custom Groovy scripts.

Apart from that, I think there might be a conflict if multiple pipelines attempt to register the parser with the same ID in parallel.

[meiswjn]

I guess @uhafner means the Groovy code in the parser runs without a Groovy sandbox and has the same access to files as the Jenkins agent process. (I'm not sure whether parsers run in agents or in the Jenkins controller.)

warnings-ng-plugin/plugin/src/main/java/io/jenkins/plugins/analysis/warnings/groovy/GroovyExpressionMatcher.java

Lines 23 to 25 in 610b02f

	* This class does not use any sandboxing mechanisms to parse or run the Groovy
	* script. Instead, only users with Overall/Run Scripts permission are able to
	* configure parsers that use custom Groovy scripts.

Apart from that, I think there might be a conflict if multiple pipelines attempt to register the parser with the same ID in parallel.

The code runs sandboxed. I was also surprised by this, but it is the case.

The conflict chance is not increased by this PR, I think. It should be the same as with #setParsers.

[KalleOlaviNiemitalo]

The code runs sandboxed. I was also surprised by this, but it is the case.

I don't see where the sandbox is set up. How did you test it?

The conflict chance is not increased by this PR, I think. It should be the same as with #setParsers.

If setParsers is only called during Jenkins startup or when an administrator edits the settings, but addParser is intended to be called from pipelines, then that increases the conflict chance. If a multibranch pipeline hardcodes the parser ID, and the parser script is edited in one branch but the ID is not changed, then it is not clear which version of the parser will be used.

If addParser were only called from hook groovy scripts and not from pipelines, then that would prevent conflicts but would not let non-administrators add parsers.

[uhafner]

I guess @uhafner means the Groovy code in the parser runs without a Groovy sandbox and has the same access to files as the Jenkins agent process. (I'm not sure whether parsers run in agents or in the Jenkins controller.)

warnings-ng-plugin/plugin/src/main/java/io/jenkins/plugins/analysis/warnings/groovy/GroovyExpressionMatcher.java

Lines 23 to 25 in 610b02f

	* This class does not use any sandboxing mechanisms to parse or run the Groovy
	* script. Instead, only users with Overall/Run Scripts permission are able to
	* configure parsers that use custom Groovy scripts.

Apart from that, I think there might be a conflict if multiple pipelines attempt to register the parser with the same ID in parallel.

The code runs sandboxed. I was also surprised by this, but it is the case.

The code runs without sandboxing on an agent (agents do not use the sandbox, this is a controller concept only). And running Groovy parsers on the controller is not allowed.

[meiswjn]

The code runs sandboxed. I was also surprised by this, but it is the case.

I don't see where the sandbox is set up. How did you test it?

In the pipeline itself:

If setParsers is only called during Jenkins startup or when an administrator edits the settings, but addParser is intended to be called from pipelines, then that increases the conflict chance. If a multibranch pipeline hardcodes the parser ID, and the parser script is edited in one branch but the ID is not changed, then it is not clear which version of the parser will be used.

If addParser were only called from hook groovy scripts and not from pipelines, then that would prevent conflicts but would not let non-administrators add parsers.

You are correct in your scenario, but I am comparing the scenarios where a user calls setParsers vs addParser directly from the pipeline. In my scenario, the conflict chance is the same for both.

[uhafner]

The code runs sandboxed. I was also surprised by this, but it is the case.

I don't see where the sandbox is set up. How did you test it?

In the pipeline itself:

This does not affect the Groovy parser, as this parser is invoked on the agent. (See my comment above.)

[meiswjn]

I guess @uhafner means the Groovy code in the parser runs without a Groovy sandbox and has the same access to files as the Jenkins agent process. (I'm not sure whether parsers run in agents or in the Jenkins controller.)

warnings-ng-plugin/plugin/src/main/java/io/jenkins/plugins/analysis/warnings/groovy/GroovyExpressionMatcher.java

Lines 23 to 25 in 610b02f

	* This class does not use any sandboxing mechanisms to parse or run the Groovy
	* script. Instead, only users with Overall/Run Scripts permission are able to
	* configure parsers that use custom Groovy scripts.

Apart from that, I think there might be a conflict if multiple pipelines attempt to register the parser with the same ID in parallel.

The code runs sandboxed. I was also surprised by this, but it is the case.

The code runs without sandboxing on an agent (agents do not use the sandbox, this is a controller concept only). And running Groovy parsers on the controller is not allowed.

But you would still need to allow the function in the script approval on a per-function basis. From what I understand, the mentioned ParserConfiguration functions are not able to erase an agents disk.

Users are not able to run any other commands except for #deleteParser, #addParser and #getParsers and #ParserConfiguration.getInstance().

[uhafner]

...script...

This is not true. Users can run any groovy script that is passed in the parameter ...script.... Hopefully this value is hard coded in your pipelines so that you can review it, but you need to be aware that this code can be misused.

[KalleOlaviNiemitalo]

The sh pipeline step can run arbitrary software on an agent. Although that software cannot directly call Java methods within the Jenkins agent process, I suppose it could attach a debugger to that process and manipulate it that way. If the Warnings NG plugin allowed a pipeline to define an unsandboxed Groovy parser for that pipeline's own use, would that feature be more dangerous than sh?

OTOH, if a pipeline can add an unsandboxed Groovy parser to the global configuration and this parser ends up being executed by other pipelines on other agents, then that seems a privilege escalation risk.

[KalleOlaviNiemitalo]

I wonder if the parsers could be set up as local to the pipeline, without changing the global configuration. Similar to how the withChecks step works. Then the parsers would not affect any other Job, and not even any other Run of the current Job.

withWarningsGroovyParsers([[
    id: 'some_id',
    name: 'some_name',
    regexp: 'my_regex',
    script: '...script...', // or perhaps readTrusted('my_parser.groovy') for easier editing
    example: '...']])
{
    // Use the recordIssues step here.
}

And running Groovy parsers on the controller is not allowed.

Is that restriction specific to Groovy parsers, or does it just follow from how the recordIssues and scanForIssues pipeline steps work?

[meiswjn]

I wonder if the parsers could be set up as local to the pipeline, without changing the global configuration. Similar to how the withChecks step works. Then the parsers would not affect any other Job, and not even any other Run of the current Job.

withWarningsGroovyParsers([[
    id: 'some_id',
    name: 'some_name',
    regexp: 'my_regex',
    script: '...script...', // or perhaps readTrusted('my_parser.groovy') for easier editing
    example: '...']])
{
    // Use the recordIssues step here.
}

And running Groovy parsers on the controller is not allowed.

Is that restriction specific to Groovy parsers, or does it just follow from how the recordIssues and scanForIssues pipeline steps work?

-> But that would not resolve this concern, if I am correct?

OTOH, if a pipeline can add an unsandboxed Groovy parser to the global configuration and this parser ends up being executed by other pipelines on other agents, then that seems a privilege escalation risk.

[uhafner]

And running Groovy parsers on the controller is not allowed.

Is that restriction specific to Groovy parsers, or does it just follow from how the recordIssues and scanForIssues pipeline steps work?

All parsers run on the agent since they might need access to other local files.

[uhafner]

I wonder if the parsers could be set up as local to the pipeline, without changing the global configuration. Similar to how the withChecks step works. Then the parsers would not affect any other Job, and not even any other Run of the current Job.

withWarningsGroovyParsers([[
    id: 'some_id',
    name: 'some_name',
    regexp: 'my_regex',
    script: '...script...', // or perhaps readTrusted('my_parser.groovy') for easier editing
    example: '...']])
{
    // Use the recordIssues step here.
}
  
> And running Groovy parsers on the controller is not allowed.

Is that restriction specific to Groovy parsers, or does it just follow from how the `recordIssues` and `scanForIssues` pipeline steps work?

-> But that would not resolve this concern, if I am correct?

BTW: this issue is tracked as https://issues.jenkins.io/browse/JENKINS-52237.

[uhafner]

-> But that would not resolve this concern, if I am correct?

Just to make sure: I have no concern about this PR, it just simplifies something that is already possible (and documented in https://github.com/jenkinsci/warnings-ng-plugin/blob/master/doc/Documentation.md#creating-support-for-a-custom-tool).

I just wanted to make you aware that this might introduce a security problem.

[meiswjn]

Thanks for your input @uhafner, I appreciate it!

I will follow this up, if more features are required I would open new PRs.

As you stated that you do not have any problems with this PR in particular, I would still appreciate this PR being reviewed/merged :)

-> Should I adapt the documentation?
-> Should we continue the discussion in the issue?

[uhafner]

As @KalleOlaviNiemitalo mentioned, your code is not thread safe and might produce problems in the way you are intending to use it. But I think we can ignore that right now and change that later if users complain.

Optional enhancement: add an integration test that shows your approach in code, example

warnings-ng-plugin/plugin/src/test/java/io/jenkins/plugins/analysis/warnings/steps/StepsITest.java

Line 799 in 51cca6b

void shouldShowWarningsOfGroovyParserWhenScanningConsoleLogWhenThatIsPermitted() throws IOException {

.

[meiswjn]

Thanks for your review, @uhafner. I implemented the changes and one integration test.

[codecov]

Codecov Report

Merging #1599 (f8d6e14) into master (51cca6b) will increase coverage by 0.15%.
The diff coverage is 84.61%.

@@             Coverage Diff              @@
##             master    #1599      +/-   ##
============================================
+ Coverage     81.85%   82.00%   +0.15%     
- Complexity     1404     1411       +7     
============================================
  Files           249      249              
  Lines          5339     5352      +13     
  Branches        396      397       +1     
============================================
+ Hits           4370     4389      +19     
+ Misses          850      847       -3     
+ Partials        119      116       -3     

Files	Coverage Δ
.../analysis/warnings/groovy/ParserConfiguration.java	78.57% <84.61%> (+15.78%)	⬆️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[uhafner]

Thanks!