@jglick @svanoort @dwnusbaum could you please review the PR

• make TaskListenerDecorator.Factory (global pipeline level) apply earlier than step level decorator (steps call BodyInvoker.mergeConsoleLogFilters)
• use default lookup order (instead of reversed) for plugins implementing TaskListenerDecorator.Factory, consistent with ConsoleLogFiler

cc @car-roll

There is a reason the reverse is there. I am not sure I recall it at this point (it has been years), but I suspect this is not right. Check downstream tests in workflow-durable-task-step.

From my perspective, the proper fix for the reported problem (ordering between withCredentials and the Splunk log redirector) is to use LogStorage, as in jenkinsci/splunk-devops-plugin#17. If there are minor enhancements needed to JEP-210 to make that practical then they should be explored.

(Even if TaskListenerDecorator.Factory were the best way to implement the Splunk use case, for compatibility reasons I would still find this patch too risky. The factory could be given an optional flag to apply on the opposite side of mainDecorator so that the behavior of existing code would be unchanged.)

@jglick When I discussed with Ted Xiao here https://issues.jenkins.io/browse/SECURITY-2401 he cited LogStorage being exclusive, where the first one loaded wins, as being a possible problem. If different plugins implement different LogStorages, they may conflict.

The splunk secret leakage has been a growing problem on our end, and we'd like to work with your team to see about getting this fixed.

If different plugins implement different LogStorages, they may conflict.

True, though that seems pretty hypothetical as

• Currently there is just one public implementation, for AWS CloudWatch Logs, which is really intended more as a reference implementation than as a practical feature.
• The logging system in Splunk sounds offhand like a natural alternative to that or to other plausible log storages. (I know nothing about this product, though.)

If and when this becomes a real problem, there is a simple solution: a plugin offering a multiplexing or “tee” storage implementation that lets you select any number of sinks (including the built-in log file as one option), picking exactly one as the authoritative source for read operations from within Jenkins.

I think log plugins (splunk, logstash, datalog) implemented TaskListenerDecorator.Factory instead of LogStorages for various reasons. IMHO some log systems separate write and read (search, report, alert), log plugins only cares about the write part, but LogStorages requires plugin to implement read part also. if LogStorages is easy to adopt, most log plugins would already switched to it in last 3 years.
I tested this on workflow-durable-task-step but no test failure, I have no idea why reverse required.
Anyway, added a flag to restore the behavior

for various reasons

Timing and/or ignorance, I suspect.

@jglick @fengxx What is the current status of this change? And has a design decision been made moving forward?

@jglick @fengxx Checking in again to see if there's been any progress made in this effort? We have a current workaround, but this is posing as a security risk to our team.

I am not a maintainer.

I am not a maintainer.

Understood, but you and @fengxx were having discussions around how his implementation broke backwards compatibility and if moving towards the use of LogStorage was the better design. So just wanted to check in and see if there had been any progress there.

how his implementation broke backwards compatibility

I think that has been resolved.

if moving towards the use of LogStorage was the better design

A matter of opinion I guess.

@fengxx Can you confirm that the backward compatibility issues that @jglick previously brought up have been resolved?

I had updated the PR per review comments and there is no breakage of backward compatibility

@dwnusbaum are you the maintainer of the plugin? could you please review the PR

Not sure who claims to maintain this plugin. @bitwiseman? @olamy?

@jenkinsci/workflow-api-plugin-developers

Checking in again to see if there's been any progress made here

rebased on master branch, request for review

@fengxx @timja Do we have a point of contact for who owns this plugin so we can get this PR reviewed?

These are the members of the GitHub team:

@dwnusbaum @bitwiseman @kshultzCB @car-roll Can someone please review this?

Hi @dwnusbaum @bitwiseman @car-roll @svanoort @joseblas @rsandell @jglick @abayer @olamy,

Sorry for the wide casting blast, but we are trying to get the ball moving on this PR so we can hopefully fix a long running issue with sensitive data not being masked properly in the https://plugins.jenkins.io/splunk-devops/ plugin.

Please let us know if we should be working with anyone specific on this or if there is anything else needed for this PR. We (Intuit) are very interested in this change.

Thanks,
-Mike

Thanks for jumping in and approving @olamy 👍 . What's the next step in order to get this merged and released?

Thanks for jumping in and approving @olamy 👍 . What's the next step in order to get this merged and released?

let's wait few more days if someone else has comments then I will merge that.

I am sorry, but the effect of using the new mode is not immediately clear to me, and I do not have time to investigate to understand the exact problem and the effects of the new mode, so I have no plans to review this PR.

In terms of general advice, I would recommend marking the new API method @Restricted(Beta.class) just to indicate that it is not recommended for general use. Also, just as a matter of course we prefer not to introduce new APIs without a linked PR to another plugin that consumes the API, so I would recommend filing a linked PR against Splunk that consumes this PR and adds tests showing that it fixes the issue. You can depend on workflow-api 1137.v36a_8153e5c69, which is an incremental build of this PR, see here for details.

My personal feeling is that something along the lines of jenkinsci/splunk-devops-plugin#17 that uses LogStorage would be a preferable approach, although perhaps TeeConsoleLogStorageFactory should be defined either here in workflow-api or in some new plugin (maybe with an associated GUI or other settings). If LogStorage is not currently suitable for whatever Splunk needs to do, I think we should probably see if there is some way to improve it so that it is suitable.

@dwnusbaum @fengxx I'm okay with marking the API as beta. As long as it's exposed in some way that we can use it downstream in the splunk-devops-plugin

We have validated a proof of concept fix in the Splunk devops here plugin using this incremental version of the workflow-api plugin

We have validated a proof of concept fix in the Splunk devops here plugin using this incremental version of the workflow-api plugin

That's great @pyieh!

@dwnusbaum and @olamy - Can this PR be merged now that we have a linked PR with the Splunk Devops Plugin consuming the change?

We have validated a proof of concept fix in the Splunk devops here plugin using this incremental version of the workflow-api plugin

That's great @pyieh!

@dwnusbaum and @olamy - Can this PR be merged now that we have a linked PR with the Splunk Devops Plugin consuming the change?

Hi @dwnusbaum @bitwiseman @car-roll @svanoort @joseblas @rsandell @jglick @abayer @olamy - Can this change be merged in now that @pyieh has verified it on the Splunk Devops Plugin here - jenkinsci/splunk-devops-plugin#20?

@dwnusbaum @olamy Wanted to check in again to see if this PR can be merged so we can begin consuming the official version in the splunk-devops plugin

Hi @dwnusbaum @bitwiseman @car-roll @svanoort @joseblas @rsandell @jglick @abayer @olamy @NotMyFault @basil Circling back on this. We are hoping to get this PR merged asap so we can start consuming the changes as part of the Splunk Devops Plugin (jenkinsci/splunk-devops-plugin#20) which will help to close a credentials leaking issue

Hi @dwnusbaum @bitwiseman @car-roll @svanoort @joseblas @rsandell @jglick @abayer @olamy @NotMyFault @basil Circling back on this. We are hoping to get this PR merged asap so we can start consuming the changes as part of the Splunk Devops Plugin (jenkinsci/splunk-devops-plugin#20) which will help to close a credentials leaking issue

Pinging this

Thanks everyone 👍