[JENKINS-62014] Add build step environment filters

[daniel-beck]

Co-authored-by: Wadeck Follonier [email protected]

See JENKINS-62014.

This PR adds new extension points to filter or change environment variables passed to build steps that support this feature.

They can be configured globally (for which this PR only prepares the infrastructure, no implementations are provided) and for specific build steps. One such filter is provided to filter environment variables. Batch and Shell build steps support this feature.

There are several use cases for this system, for example exercising fine-grained control over environment variables to get more easily reproducible builds, or filtering out extremely long or otherwise broken environment variables that would make some tools misbehave.

Downstream changes

New utility plugin with run matchers to include/exclude only specific builds/jobs on globally configured filters and descriptor matchers to include/exclude build steps based on their type: https://github.com/daniel-beck/environment-filter-utils-plugin

New plugin with generic filters demonstrating use of the new extension point: https://github.com/daniel-beck/generic-environment-filters-plugin

jenkinsci/durable-task-plugin#124 and jenkinsci/workflow-durable-task-step-plugin#135 add support for globally defined build step environment filters to sh, bat, and powershell pipeline steps.

jenkinsci/workflow-basic-steps-plugin#115 implements a retainEnv pipeline step that works somewhat similar to the functionality implemented for Shell/Batch steps here, except for pipeline and any steps. Notably, cannot remove system environment variables; but it does remove characteristic env vars (which aren't used for workflow-durable-task-step anyway, it has its own cookie).

Proposed changelog entries

• RFE: Add the ability to filter out environment variables for Shell and Windows batch build steps
• Developer: RFE: Add new extension points to define build step environment filters (currently in beta)

Proposed upgrade guidelines

N/A

Submitter checklist

• (If applicable) Jira issue is well described
• Changelog entries and upgrade guidelines are appropriate for the audience affected by the change (users or developer, depending on the change). Examples
  • Fill-in the Proposed changelog entries section only if there are breaking changes or other changes which may require extra steps from users during the upgrade
• Appropriate autotests or explanation to why this change has no tests
• [n/a] For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

Maintainer checklist

Before the changes are marked as ready-for-merge:

• There are at least 2 approvals for the pull request and no outstanding requests for change
• Conversations in the pull request are over OR it is explicit that a reviewer does not block the change
• Changelog entries in the PR title and/or Proposed changelog entries are correct
• Proper changelog labels are set so that the changelog can be generated automatically
• If the change needs additional upgrade steps from users, upgrade-guide-needed label is set and there is a Proposed upgrade guidelines section in the PR title. (example)
• If it would make sense to backport the change to LTS, a Jira issue must exist, be a Bug or Improvement, and be labeled as lts-candidate to be considered (see query).

[timja]

I've done an initial look through, and tested the shell step in a freestyle build, using the different options available.

It worked,

A few nits on the UI, nothing blocking.

I've never hit this before as an issue, I would be interested to hear of any issues that users have encountered because of the lack of this feature

(the jira issue doesn't have much detail although there is some more on the PR)

[fqueiruga]

I think the UI is a bit confusing. TBH looking at that form field I didn't understand what it was supposed to do, or which use-cases it solves. No idea how to fix though, maybe add specific wording regarding what the retention means. Maybe also call them "Available envvars" or something like that

[alecharp]

I'm wondering how this configuration could be done with CasC. I guess some test-only global rule and a CasC test would be great. This could also be a way to document how to set those global rule.

[rsandell]

Looks good to me. But I would like to see a test for the global rules as well making sure they work and can intermingle with local rules.

[jglick]

Is there any integration with durable-task needed?

[daniel-beck]

@jglick I have some stuff prepared, including that and a demo global filter, and was waiting for an incremental to be deployed (but forgot that was broken). I hope it'll be resumed now and I can publish some downstream changes.

[fqueiruga]

My 2 cents: being somebody who had no context of what this PR was about, looking at the form I had no idea whatsoever what this configuration would do.

I suggest improve the description to make it clear on the form what the purpose of this is. Maybe switch from saying "retain variables" to "variables whitelist", as it is a common term that can help put users in context.

[daniel-beck]

Maybe switch from saying "retain variables" to "variables whitelist", as it is a common term that can help put users in context.

If has basically the same function as the Collection#retainAll method in Java collections. I'm not sure "whitelist" or related terms make a lot of sense (what happens to non-whitelisted ones?), and "remove all environment variables unless specified in the configuration below" seems unwieldy.

[jsoref]

Only keep specified items

[daniel-beck]

I updated the PR description referencing two plugins that combined demonstrate the new global extension point (which has no implementation in core):

New utility plugin with run matchers to include/exclude only specific builds/jobs on globally configured filters and descriptor matchers to include/exclude build steps based on their type: https://github.com/daniel-beck/environment-filter-utils-plugin

New plugin with generic filters demonstrating use of the new extension point: https://github.com/daniel-beck/generic-environment-filters-plugin

They are not hosted in the Jenkins project and need to be built manually to test them. Both need a build of this PR to work.

Three global filters are currently implemented in the latter plugin:

• Add/Set an environment variable (very minimal)
• Remove an environment variable (very minimal)
• Match an environment variable based on its value with a regex and act on it:
  • Remove it, replace it with a different value, or fail the build.
  • Only do this for specific types of build steps, or all of them that support this feature
  • Exclude jobs based on their name: specific jobs or all jobs matching a regular expression

Screenshot

It's not particularly pretty, but it demonstrates multiple notable features/behaviors, including the interplay between "Retain environment variables" defined on a per build step basis, and "Fail the build" on a global rule -- a job configured to not retain matching environment variables will not have its builds failed. Additionally, it demonstrates how to only apply rules to certain kinds of build steps.

Remember Shellshock? The features in the regex sample filter should be enough to prevent build parameters from being used to exploit this vulnerability in old versions of bash: Configure a global regex filter only acting on shell build steps that fails the build when an environment variable starting with something like () { shows up. Now, nobody (hopefully) uses such old releases anymore, but it's still a nice example what could be done with this. (In the screenshot I just match environment variables 100+ characters long, not an actual shellshock exploit.)

[daniel-beck]

I changed the label based on feedback, and make the process environment variables handling an enum selection rather than a checkbox, since it's a very different option from the "retain characteristic env vars" one. Unfortunately this change loses inline help and form validation for that field.

Screenshot

[jeffret-b]

Just to clarify about the two new plugins under the "daniel-beck" organization: These are just for demonstration purposes and you're not intending to actually release these or anything. Is that correct?

[daniel-beck]

Just to clarify about the two new plugins under the "daniel-beck" organization: These are just for demonstration purposes and you're not intending to actually release these or anything. Is that correct?

Could go either way. I've written them so that they could be released. Right now they exist to validate this feature.

[jeffret-b]

OK. I guess there isn't anything to approve or review there specifically at the moment, just in context of this one.

[jeffret-b]

This looks good and has value. I've long been surprised at how much data can flow through environment variables to a Jenkins job without any way of controlling it. This provides the framework for useful controls and also includes some initial controls on a project for reducing environment variable transmission.

I'm almost ready to approve, but there are a few outstanding concerns that need to be addressed first.

[jeffret-b]

I'm going to let others with more UI expertise weigh in here, but I'm finding the layout a little weird. The variablesLabel has a fair bit of text and then there is also the additional help text. They variables label is wordier than I would expect. It's not clear why some of the text goes in the title to be always displayed and some gets separated out to the additional help.

The same thing also happens for the retainCharacteristicEnvVarsLabel, but in that case there is a bit of content duplication between the two different UI documentation areas.

[jeffret-b]

Oh, I also did some testing of the user functionality provided here. Everything I tried worked as expected.

[jeffret-b]

There are a few items I don't think have been clarified or resolved yet.

[jvz]

Looks good other than the nits already mentioned by Jeff. I already tested this out.

[jeffret-b]

Looks good. I'm in favor of shipping this and getting these new controls out to users.

I've read through the latest code changes and they all look good. I ran through all of the new UI. I've been a bit critical of the UI in previous iterations of this capability. Now it all looks good. This capability is kind of complicated but this does a reasonable job of conveying it.

[jeffret-b]

There are a few other people who have commented or reviewed, but I don't see any outstanding requests for change or blocking discussions. The maintainer checklist appears to be satisfied.

If anyone has any further concerns, please respond. Given the involved discussion that has transpired here, I plan on giving it and additional day. Tomorrow I'll mark it ready-for-merge unless concerns are raised before then.

[jeffret-b]

Marking as ready-for-merge. This will start the 24-hour waiting period. If no concerns or questions are raised during this period, this may be merged at that time.