Redaction of secrets should only happen for non expression like inputs

[keevan]

This could be debatable, but consider the following situation

1. I have many s3 steps.
2. All of which require inputs to be set for the s3 connection credentials (bucket, key, secret, etc).
3. To reduce the amount of configuration copied for each step, I use expressions to de-duplicate where they are configured.
4. I would like to ultimately export this when it is ready, and import it into an environment for real usage.
5. Once imported, I would only need to tweak any fields that were redacted.

So, because expressions in secret inputs are redacted. Instead of needing to add the secret in once. I now need to edit N steps to fill in the missing secret. Even if the value was actually an expression referencing the secret.

Proposal:

• [ ] "Secret" inputs won't redact values containing expressions. This may be slightly more "risky" if this pointed to a credential store and you can use it to determine the key store structure, but at the moment, this is not the case.
• [ ] #613
• [ ] A security setting to "always redact secret fields even if they contain expressions" (add this as a configuration option), default to OFF.

The setting might not need to happen if there is no big reason to have it. But this would improve the test, import and exporting process, in that there will be less things that need to be altered.

[brendanheywood]

In that scenario above I’d go one step further and reference a global var so you never set it in the flow in the first place. You should not need to set anything after a flow re-import.

Redaction in my mind means hiding it when shown, I’m border line about redaction in exporting. It sounds like this is complicating things and we could just export it. If you need the security then use a global var

[keevan]

In my real example it does live in a global var. Problem then is that it is dumped in the output logs.

I vaguely recall this being mentioned as unexpected behaviour?

[keevan]

Sorry I meant dataflow var. There is no global var setting yet afaik.

[brendanheywood]

Ok, but treat this as an output problem with the logs leaking a secret rather than a deeper problem with the export leaking a secret which is not a bug. Note that we cannot protect against an admin writing a flow which exfiltrates config, if a flow can use a secret then it must be able to use it, eg we must be able to give it to s3 in order to work at all. So we shouldn't waste too many brain cycles here hiding stuff in all cases especially if it is causing knock on complexity

[keevan]

Okay, so brain dumping out the scenarios where we currently output information. Then determine comparing with current behaviour if we should:

• output it at all,
• redact if it's sensitive (e.g. a secret)
• or show it by default.
• and whether it shows the original value or expressed value (resolved expression)

The changes proposed are based on the previous suggestion, so for the following scenarios:

• Dataflow details page admin/tool/dataflows/view.php?id=123 Current: shows all unexpressed config in YAML, redacts secrets. Proposal: show all unexpressed config in YAML, including secrets. Pros: only admins should be able to author flows. Easier to verify value of secret field. Cons: anyone viewing the screen (passerby or screensharing) can leak secrets unintentionally. A better solution might be to have the ability similar to the password mask, where it can be shown on demand as needed, but quickly.
• Dataflow run (execution logs) Current: Steps should not intentionally output fields considered secret, at the end of the run, a dump of the state is shown. Proposal: shouldn't dump state here. Link to run history instead.
• Dataflow run history (dumped state in the run history) Current: outputs the raw unexpressed configuration and state changes (variables). This does redact secrets. Proposal: Unchanged. The less places secrets can exist, the better.
• Dataflow export Current: Exports all dataflow and step config, unexpressed, redacts secrets Proposed: Exports all dataflow and step config, unexpressed, does not redact secrets. People sharing dataflows will now need to pay more careful attention to what information in the flow they want to share. The alternative here is to add the extra complexity but add it as another option (e.g. export without secrets, skipping expressions)

As I write the above, I'm thinking main reason for this complexity currently is because we are handling "secrets" as part of working with dataflows. If this was abstracted into an integration or plugin which interfaces with a credential store e.g. vault, then this might be a bit easier to manage, and we could get away with calling it in an expression e.g. ${{ secrets.path.to.my.secret }} or ${{ secret('path/to/my/secret.field') }}. Then most controls here could be removed and it could be exposed as is, as the information would mainly be config (steps would not natively not expose secrets). Access to the credential store would be controlled at the site level or at the plugin level.

If that's too far out of scope, maybe a plugin level "secrets" store could be added, and would act like a simplified version of the would-be aforementioned integration. All fields marked as secrets in step types should be added as expressions, then they can just be exposed and stored and logged everywhere without actually exposing the underlying value.

A side thought is how far you want secrets management to go, whether it is write only and only steps can read and use it during execution, no where else, or same as previous but admins can read/write on some edit page.

[brendanheywood]

I'm happy with those 4 points, but I'd strongly lean towards breaking these up, deferring most of them, and doing only the bare minimum only when we actually need it. The only one which might be causing an issue right now which needs fixing is point 4.