feat: Support plaintext secrets #1102
Conversation
MartinAltmayerTMH
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1102 +/- ##
==========================================
- Coverage 71.80% 68.71% -3.09%
==========================================
Files 3 3
Lines 461 489 +28
==========================================
+ Hits 331 336 +5
- Misses 91 112 +21
- Partials 39 41 +2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Right now the integ tests can be a bit painful, they should self mutate in pipeline. Don't take asset hashes to serious. I can fix them If we merge this :-) |
| @@ -269,22 +270,36 @@ func (a AWS) syncSopsToSecretsmanager(ctx context.Context, event cfn.Event) (phy | |||
| } | |||
| } | |||
|
|
|||
| if resourceProperties.ConvertToJSON == "" { | |||
There was a problem hiding this comment.
maybe the diff is wrong, but for me it looks like you are replacing ConvertToJSON? I think we should keep this option :-)
There was a problem hiding this comment.
This is indeed an artifact from the diff. I did not change the ConvertToJson logic, except for adding one level of indentation.
Here is the new code:
if resourceProperties.Plaintext == "" {
resourceProperties.Plaintext = "false"
}
resourcePropertiesPlaintext, err := strconv.ParseBool(resourceProperties.Plaintext)
if err != nil {
return tempArn, nil, fmt.Errorf("failed to parse bool:\n%v", err)
}
if resourcePropertiesPlaintext {
decryptedContent, err = convertToPlainText(finalInterface)
if err != nil {
return tempArn, nil, fmt.Errorf("failed to convert to plaintext: \n%v", err)
}
} else {
... ConvertToJson and ConvertToYaml logic goes here ...
} | func convertToPlainText(input interface{}) ([]byte, error) { | ||
| v := reflect.ValueOf(input) | ||
| key := reflect.ValueOf("data") | ||
| value := v.MapIndex(key) | ||
|
|
||
| if !value.IsValid() { | ||
| return nil, fmt.Errorf("if plaintext is set to true, secret must contain exactly one key '%s'", key.Interface()) | ||
| } | ||
|
|
||
| switch v := value.Interface().(type) { | ||
| case string: | ||
| return []byte(v), nil | ||
| case int: | ||
| return []byte(strconv.Itoa(v)), nil | ||
| default: | ||
| return nil, fmt.Errorf("failed to convert value to bytes: %v", v) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If I understand correctly, your implementation checks for the existence of a "data" key. If it exists, we use its value; otherwise, we throw an error. This approach seems fine to me.
However, I'm wondering if we need to distinguish the type of the interface. Can't we just write the bytes we receive without converting them? I am referring to this issue. Perhaps it is possible to handle any "type" in the interface as a byte array?
I'll need to investigate what SOPS produces for "binary files."
There was a problem hiding this comment.
Alright, we need to handle binaries when passing them to the AWS Secrets Manager API. In such cases, we should set the content to the SecretBinary field instead of the SecretString field. However, I believe we should consider this here as well. I just checked with random binary data; it will also be encoded into the YAML file with the data key.
So what do you think, can we just handle the interface as byte[], without the switch statement? Than it would be more of a removeDataKey method.
There was a problem hiding this comment.
It sounds like a good idea to use the same approach for binary secrets (though I'd like to keep the actual support for binary, i.e. using SecretBinary, out of this PR).
I don't see how a generic conversion to byte[] could work, though: Currently, when my implementation starts, it already has an interface (read from the JSON/Yaml file), so the value below data might be anything and there is no obvious way to convert this to byte[]. For example, I could use json.Marshal. But this would encode the string 12341234 as "12341234" and thus change the secret content.
There was a problem hiding this comment.
Will look into this tomorrow. Maybe you can give me access to your fork?
Fixes #1096.
This PR adds a new option
plaintext. If set, we will expect the SOPS file to contain just a single keydataon toplevel and will sync only the value below this key to the secret. Note that SOPS files always contain a toplevel object, so it was previously impossible to sync just a string to the secret.Open points:
databe configurable? Or should we just require that there is exactly one key, no matter its name?convertToJsonandconvertToYamloptions? Currently I ignore those, ifplaintextis true.@markussiebert Are you ok with the general direction of this PR? If so, I would add some tests for the new functionality.