Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix redacting sensitive values that uses backslashes #737 #746

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pviniciusfm
Copy link

@pviniciusfm pviniciusfm commented May 15, 2021

Description

Our team has found a security issue on this provider when using field names with backslashes.
Essentially, set_sentitive doesn't work if we use backslashes because the code was splitting any dot character.

The solution was to ignore splitting when a backslash is added, and remove "\" when trying to find the set values.

Go doesn't have a look behind feature in regex. Thus, I opted to write a string tokenizer to ignore "." that are prefixed with "".

Acceptance tests

  • Have you added an acceptance test for the functionality being added?

Release Note

Release note for CHANGELOG:

Fix issue #737 when redacting values that uses backslash characters in field name.

References

#737

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@ghost ghost added the size/S label May 15, 2021
@hashicorp-cla
Copy link

hashicorp-cla commented May 15, 2021

CLA assistant check
All committers have signed the CLA.

@pviniciusfm
Copy link
Author

Any progress on reviewing this?
@camlow325 signed as positive for fixing the issue on their side as well.

@clydetealium
Copy link

@jrhouston @dak1n1, any chance at a review?
cc: @pviniciusfm

@OneMatt
Copy link

OneMatt commented Jun 21, 2023

We're running into this same issue when attempting to deploy the ArgoCD helm chart with an OIDC client secret. The key in question has a dot in it so it's not redacted properly and will show up in metadata. Any chance of getting this merged in the short term?

@phyzical
Copy link

we are facing this with the exact usecase as @OneMatt

any chance this can get nudged along? or is there any alternative that i havn't stumbled across yet?

incase its due to a lack of replication

 set_sensitive {
    name  = "server.config.oidc\\.config"
    type  = "string"
    value = <<-YAML
      some_sensitive
      multi_line_value
    YAML
  }

above results in the following helm result

server:
  config:
    "oidc.config": |
        some_sensitive
        multi_line_value

issue is that its then exposed in the metadata output

@pviniciusfm
Copy link
Author

This issue has been like that for 2 years. I'm wondering if we still have maintainers for this project...

@phyzical
Copy link

It might be that this is a bit of an edgecase as you could argue the helms that need it should just change their map structure to be friendly.

in any case my workaround for the time being is to just use until/if its fixed.

 lifecycle {
    ignore_changes = [metadata]
  }

The output does say it doesn't have any effect but my plan output begs to differ.

@michelzanini
Copy link

michelzanini commented Apr 2, 2024

This issue is really important. Its a security issue.
Why is this sitting here for 3 years and no one from Hashicorp is looking into it?

Any of the top contributors can take a look here?

@arybolovlev
@jrhouston
@BBBmau

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants