Unable to decrypt a secret via SOPS

[kamialie]

With the recent upgrade to kustomize controller to v1.5.0 I'm facing the following issue during reconciliation:

Warning  BuildFailed              2m40s              kustomize-controller  decryption failed for 'foo-token': failed to decrypt and format 'default/foo-token' Secret data: error decrypting sops tree: Error walking tree: Could not decrypt value: Input string glagent-xxx does not match sops' data format

I have a secret resource which is set to empty string in the base layer, and there is a patch that contains actual secret data. Both of them are encrypted using SOPS (and AWS KMS key, but I think that's irrelevant here). To my surprise kustomize controller shows the secret value itself (in the logs!), and that's probably the hint as well why it fails. I suspect kustomize is trying to decrypt the secret twice, so on the second attempt SOPS obviously complains that the passed in value doesn't match the regex it expects.

I confirmed that my GitOps repo sucessfully reconciles with kustomize controller v1.4.0, and does not with v1.5.0. I believe the cullprit is here, but I'm lacking background knowledge and local setup to confirm though.

[kamialie]

Going through code I guess the assumption at the moment is that base layer is not encrypted, while the patch is. The latest version of the controller would basically simply decrypt earlier. The reason I encounter the error is probably due to base layer containing SOPS metadata, so controller thinks it needs to decrypt that source file, where patch (with already decrypted data) has been applied to. If my assumptions are correct, I can see how I can immediately fix the issue by just removing SOPS metadata from base layer, since encrypting empty string barely serves any purpose, but I can see how this can still not work for everyone. For instance, what if I simply want to rotate a shared secret across environments: my current value would be in the base layer (to avoid duplication), and I'm introducing new value in dev environment.

[matheuscscp]

Hi @kamialie I'm investigating this bug right now, can you please share more details of how your config is structured? Possibly the exact thing except for the actual secret values of course. Much appreciated! 🙏

Also pinging @vlasov-y, your two PRs #1283 and #1286 seem to be the only ones touching decryption in the change log for kustomize-controller v1.5.0, if you could please shed some light here 🙏

[vlasov-y]

Both of them are encrypted using SOPS

@kamialie That is a very important point 😄
Check this https://github.com/fluxcd/website/pull/2107/files.

We have faced this problem during the PR development. The issue is in the SOPS itself. If your patches bring some plaintext values to keys that match SOPS'es encrypt-regex - that will make SOPS to fail. You are right, now we decrypt patches, then merge them, then decrypt the result manifest right before the apply.
Just move all your secrets to patches and the issue will be resolved.

[kamialie]

Hi @matheuscscp, yes of course! Please, let me know, if the structure/file contents below make sense:

# ./infra/base/agent/kustomization
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - secret.yaml

# ./infra/base/agent/secret.yaml
apiVersion: v1
kind: Secret
metadata:
    name: token
type: Opaque
stringData:
    token: ""
sops:
    kms:
        - arn: arn:aws:kms:eu-central-1:1234567890:key/...
          <...>
    encrypted_regex: ^(data|stringData)$
    version: 3.8.0

# ./infra/base/kustomization
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: addons
resources:
  - agent

# ./infra/dev/kustomization
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - ../base
patches:
  - path: agent.secret.patch.yaml

# ./infra/dev/agent.secret.patch.yaml
apiVersion: v1
kind: Secret
metadata:
    name: token
type: Opaque
stringData:
    token: ENC[AES256_GCM,data:foobar,type:str]
sops:
    kms:
        - arn: arn:aws:kms:eu-central-1:1234567890:key/...
          <...>
    encrypted_regex: ^(data|stringData)$
    version: 3.8.0

Also, I just noticed, if I set value to empty string, SOPS doesn't even bother to encrypt it (rightfully), but I sort of doubt that changes much in our context.

[kamialie]

@vlasov-y thanks for providing the link to updated doc, somehow I missed it. While I sort of agree with the solution, I still wonder, if this is handled correctly. It is very easy to make this mistake, encrypt both resource and patch, and the result would be controller spitting out the plaintext value in the logs. Also, I still stand by the use case of having a current secret defined in the base layer (resource), and introducing an update in particular env via patch.

I also wonder about a use case, where a patch would bring one more key/value pair instead of overwriting something defined in the resource.

[vlasov-y]

@vlasov-y thanks for providing the link to updated doc, somehow I missed it. While I sort of agree with the solution, I still wonder, if this is handled correctly. It is very easy to make this mistake, encrypt both resource and patch, and the result would be controller spitting out the plaintext value in the logs. Also, I still stand by the use case of having a current secret defined in the base layer (resource), and introducing an update in particular env via patch.

I also wonder about a use case, where a patch would bring one more key/value pair instead of overwriting something defined in the resource.

No wonder you have missed that information in documentation since I could not find it either, but I remember we have merged it.
I would also prefer to avoid that pain, we have even opened an issue in SOPS getsops/sops#1680, but as you can see, it is not possible to change SOPS behavior and make it just skip fields that mach regex but contain a plain data

[matheuscscp]

and the result would be controller spitting out the plaintext value in the logs

We are already addressing this part here: #1372

[vlasov-y]

@kamialie I would do it this way

#!/usr/bin/env bash

mkdir -p infra/base/agent
cat >infra/base/agent/kustomization.yaml <<EOF
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
secretGenerator:
  - name: token
    envs:
      - token.sops.env
    options:
      disableNameSuffixHash: true
EOF

mkdir -p infra/base/agent
cat >infra/base/agent/token.sops.env <<EOF
token=value
EOF

mkdir -p infra/base
cat >infra/base/kustomization.yaml <<EOF
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
namespace: addons
resources:
  - agent
EOF

mkdir -p infra/dev
cat >infra/dev/kustomization.yaml <<EOF
apiVersion: kustomize.config.k8s.io/v1beta1
kind: Kustomization
resources:
  - ../base
secretGenerator:
  - name: token
    envs:
      - token.sops.env
    behavior: merge
    options:
      disableNameSuffixHash: true
    
EOF

mkdir -p infra/dev
cat >infra/dev/token.sops.env <<EOF
token=overridden
EOF

Also, update your .sops.yaml to manage *.sops.env, so your token files will be encrypted. That should work.

[matheuscscp]

No wonder you have missed that information in documentation since I could not find it either, but I remember we have merged it.

Changes to the fluxcd/website are only sent live if we add the label to backport the PR to the current minor version of flux e.g. backport/v2-5. We didn't do that in your PR at the time it was merged, but your change is live in the website now: https://fluxcd.io/flux/guides/mozilla-sops/#sops-encrypted_regex-conflict

When the website change is only relevant for the next minor version of flux, we don't add the backport label, because it has to go live only when the next minor version is released.

You are right, now we decrypt patches, then merge them, then decrypt the result manifest right before the apply.
Just move all your secrets to patches and the issue will be resolved.

So I'm understanding that we basically shipped a breaking change instead of a bug? We should avoid doing that on GA APIs like Kustomization, which is already v1 (GA).

[kamialie]

I kept postponing introducing secretGenerator, guess now is the time 🙈. My only concern is it somehow prevent my current usage with more obvious error mesage or something.

You are right, now we decrypt patches, then merge them, then decrypt the result manifest right before the apply.

Not sure I agree that the issue is with SOPS in this case either though. I wonder, why Flux merges plaintext with encrypted file in the first place. Wouldn't it be more appropriate to merge only plaintext files? If resource (where patch should be applied to) is encrypted, decrypt it, then merge the patch on top of it, and finally store resulting manifest somewhere to be applied later. Did I understand the workflow wrong? Or is there some limitation that doesn't allow that?

[vlasov-y]

Wouldn't it be more appropriate to merge only plaintext files?

That was my primary implementation, I had all resources files being decrypted separately as well, but unfortunately it was not approved since it would significantly raise memory consumption.

So I'm understanding that we basically shipped a breaking change instead of a bug? We should avoid doing that on GA APIs like Kustomization, which is already v1 (GA).

Before 1.5.0 patches could not be encrypted at all, so this problem did not exist. If people start encrypting patches because it is possible then issue appears, but old stacks should work without errors.

[kamialie]

That was my primary implementation, I had all resources files being decrypted separately as well, but unfortunately it was not approved since it would significantly raise memory consumption.

I see

Before 1.5.0 patches could not be encrypted at all, so this problem did not exist. If people start encrypting patches because it is possible then issue appears, but old stacks should work without errors.

Actually I'm on 1.4.0, and just recently upgraded entire Flux setup from 2.2 to 2.4, and we had this encrypted patch before. As far as I can see, it was working before. I encountered the initial error during upgrade to 2.5 testing.

[vlasov-y]

Actually I'm on 1.4.0, and just recently upgraded entire Flux setup from 2.2 to 2.4, and we had this encrypted patch before. As far as I can see, it was working before. I encountered the initial error during upgrade to 2.5 testing.

Then it worked after flux merged together encrypted value from two different encrypt sessions. Since most of us use one key for all secret files - it worked, but that is an implicit benefit. Anyway, I would agree that it appeared to be a breaking change

[matheuscscp]

By the way @kamialie, thanks for reporting the vulnerability related to printing the secret values in the logs, much appreciated 🙏 In the future please use the correct channel, described here: https://fluxcd.io/security/#report-a-vulnerability