fix: implicitly base64 encode base64 secret values

[teejaded]

Description

If a secret is successfully decoded from base64, assume it needs to be re-encoded. This allows for secret values with multiple placeholders or concatenation with other strings.

Fixes: #124

Checklist

Please make sure that your PR fulfills the following requirements:

• Reviewed the guidelines for contributing to this repository
• The commit message follows the Conventional Commits Guidelines.
• Tests for the changes have been updated
• Docs have been added / updated

Type of Change

• Bugfix
• Feature
• Code style update (formatting, local variables)
• Refactoring (no functional changes, no api changes)
• New tests
• Build/CI related changes
• Documentation content changes
• Other (please describe)

Other information

[werne2j]

If a secret is successfully decoded from base64, assume it needs to be re-encoded.

Are we sure about this assumption?

[jkayani]

Recap of how the plugin works now:

• stringData, placeholder alone: replace as-is, works

• stringData, placeholder part of a larger plain string: replace as-is, works since entire result is plain-text

• data, placeholder alone: fails as expected since replacement is plain-text. Workaround is to use stringData or base64 data in Vault.

• data, placeholder part of a larger string: fails as expected since entire result is plain-text. Workaround is to use stringData or load in Vault the larger string pre-replaced, base64-encoded and then only keep the placeholder in the data.

• data, placeholder with base64 modifier: base64 the replacement, works

• data, placeholder with base64 modifier pre-encoded as base64: decode placeholder, base64 replacement, works

• data, placeholder with base64 modifier part of a larger plain string pre-encoded as base64: fails, since we only base64 the replacement and not the whole string. The current workaround would be to put the entire plain string as desired to appear in data in Vault and just keep a placeholder with the base64 modifier. Or to put the entire string as desired to appear in base64 format in Vault, and just keep a placeholder alone in data.

This PR addresses the last one so that data placeholders (possibly part of a larger string) pre-encoded as base64 will be replaced and the entire result is re-encoded as base64. You wouldn't need the base64 modifier here at all I think.

I think it's a good feature to have but it will be inconsistent with how the plugin works in the 3rd and 4th scenarios

[jkayani]

So if we're going down this route then to make 3rd and 4th scenarios consistent, we'd have to change the rules so that AVP will re-encode into base64 the result of any replacements of strings in data (making the base64 modifier unnecessary). That would a breaking change

[codecov]

Codecov Report

Merging #138 (4f7b023) into main (36291c4) will increase coverage by 0.23%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #138      +/-   ##
==========================================
+ Coverage   57.88%   58.11%   +0.23%     
==========================================
  Files          16       16              
  Lines         539      542       +3     
==========================================
+ Hits          312      315       +3     
  Misses        187      187              
  Partials       40       40              

Impacted Files	Coverage Δ
pkg/kube/util.go	85.71% <100.00%> (+0.52%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36291c4...4f7b023. Read the comment docs.

[jkayani]

Talked to Jake some more about it and Jake pointed out that the 3rd and 4th scenarios I brought up would only happen if a YAML outside of someone's control (so from an upstream chart) did something like:

data:
  credentials: |
     [block]
      password: {{ .values.password }}

and just expected that to work, no base64'ing done anywhere. That's unreasonable.

If the user did have control over the YAML, this is easily solved by swapping data for stringData. So there is nothing "inconsistent" about what this PR does and I approve!

Sorry about the delay :)

[werne2j]

@teejaded Can you add documentation to the readme about how the encoding/decoding will work?