Skip to content

feat: Redact env secrets from output and logs#20691

Merged
kodiakhq[bot] merged 6 commits intomainfrom
feat/redact-env-secrets
May 6, 2025
Merged

feat: Redact env secrets from output and logs#20691
kodiakhq[bot] merged 6 commits intomainfrom
feat/redact-env-secrets

Conversation

@maaarcelino
Copy link
Copy Markdown
Contributor

This adds functionality to redact secrets (aka environment variables) in cloud sync environments. This redacts them in the logs and in the output of all the CLI commands.

@maaarcelino maaarcelino requested review from a team and przste-go May 6, 2025 09:52
@cq-bot cq-bot added the area/cli label May 6, 2025
for _, v := range env {
parts := strings.SplitN(v, "=", 2)
if len(parts) == 2 && len(parts[1]) > 0 {
s.secrets[parts[1]] = parts[0]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we consider adding some ignore logic here, to prevent replacing shorter, day to day (1, yes, 17...) env vars that could be in regular, useful logs? Anything that's less than maybe 4 chars (maybe more?) shouldn't be considered a secret, maybe? Would still redact things like user, root, true and false...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this is one downside to the current approach. Env vars with values that may appear in legitimate logs would be redacted, e.g. if the value is test or any of the ones you mentioned. Not sure how to exclude these because legitimate secrets might also be short.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least they are only used on cfg.Environment, so shouldn't pose much of a problem. Not all of env is being redacted, only if it starts with __KIND_PLUGINNAME__... so we can be sure the env vars were intended for the plugin itself anyway.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned in the comment below, that should prevent these usecases since users would not have a reason for setting non-secrets as sync environment variables.
#20691 (comment)

@murarustefaan
Copy link
Copy Markdown
Member

Since it's mostly internal usecases that require secret redaction (general usage of the CLI does not require that), I think the better option would be toggling this sort of behaviour opt-in, trough configuration (probably an environment variable is the best choice).

@maaarcelino
Copy link
Copy Markdown
Contributor Author

Since it's mostly internal usecases that require secret redaction (general usage of the CLI does not require that), I think the better option would be toggling this sort of behaviour opt-in, trough configuration (probably an environment variable is the best choice).

Technically this is already opt-in through the CQ_CLOUD environment variable. Adding a separate new environment variable to our current pipeline will make this change much bigger and I don't think that's necessary. WDYT?

@murarustefaan
Copy link
Copy Markdown
Member

Ah ok I missed that it's already behind CQ_CLOUD. If that's the case then it should be good to go.

@maaarcelino maaarcelino added the automerge Automatically merge once required checks pass label May 6, 2025
@kodiakhq kodiakhq bot merged commit a913f10 into main May 6, 2025
18 checks passed
@kodiakhq kodiakhq bot deleted the feat/redact-env-secrets branch May 6, 2025 11:07
kodiakhq bot pushed a commit that referenced this pull request May 6, 2025
🤖 I have created a release *beep* *boop*
---


## [6.19.0](cli-v6.18.2...cli-v6.19.0) (2025-05-06)


### Features

* Redact env secrets from output and logs ([#20691](#20691)) ([a913f10](a913f10))


### Bug Fixes

* **deps:** Update module github.com/cloudquery/plugin-sdk/v4 to v4.79.1 ([#20692](#20692)) ([50f909e](50f909e))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/cli automerge Automatically merge once required checks pass

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants