onedr0p / sops-pre-commit

Sops pre-commit hook
MIT License
42 stars 17 forks source link

false positive? #24

Open AskAlice opened 2 years ago

AskAlice commented 2 years ago

Check for unencrypted Kubernetes secrets in manifests.......................Failed

Unencrypted Kubernetes secret detected in file: tmpl/cluster/cluster-secrets.sops.yaml

# yamllint disable
apiVersion: v1
kind: Secret
metadata:
    name: cluster-secrets
    namespace: flux-system
stringData:
    SECRET_DOMAIN: ${BOOTSTRAP_CLOUDFLARE_DOMAIN}
    SECRET_CLOUDFLARE_EMAIL: ${BOOTSTRAP_CLOUDFLARE_EMAIL}
    SECRET_PILOT_TOKEN: ${BOOTSTRAP_TRAEFIK_PILOT_TOKEN}

git diff image

onedr0p commented 2 years ago

Unfortunately there's no great way to detect what secrets should be encrypted if not already. It would be to be either the filename (e.g. something.sops.yaml) or the way this checks is by checking the file content for ^kind:\ssecret$ AND contains ENC.AES256.

The work around is to make sure your files are encrypted and use the --no-verify flag on git commit

almereyda commented 2 years ago

Here we could reuse the values of the path_regex keys within the items of the creation_rules array in .sops.yaml (at a repository's root) to distinctively identify the files that are required to be encrypted.

tuxpeople commented 2 years ago

I do have another false positive, which I was able to commit using @onedr0p's --no-verify hint:

---
apiVersion: v1
kind: Secret
type: kubernetes.io/service-account-token
metadata:
  name: kube-vip
  namespace: kube-system
  annotations:
    kubernetes.io/service-account.name: kube-vip
---
[...]

Well, from the perspective of the check, it's not really a false positive. It is a Secret and it isn't encrypted. But from de DevOps perspective, it's a false positive because there is no unencrypted data in it.

May I suggest to either check if there is data or stringData specify (would solve my issue) or add a disable-comment like for xamllint in above screenshot? That would solve my and @AskAlice's issue.

denis-ev commented 2 years ago

I do have another false positive, which I was able to commit using @onedr0p's --no-verify hint:

---
apiVersion: v1
kind: Secret
type: kubernetes.io/service-account-token
metadata:
  name: kube-vip
  namespace: kube-system
  annotations:
    kubernetes.io/service-account.name: kube-vip
---
[...]

Well, from the perspective of the check, it's not really a false positive. It is a Secret and it isn't encrypted. But from de DevOps perspective, it's a false positive because there is no unencrypted data in it.

May I suggest to either check if there is data or stringData specify (would solve my issue) or add a disable-comment like for xamllint in above screenshot? That would solve my and @AskAlice's issue.

I agree like the # yamllint disable just # forbid-secrets disable maybe.

Cheers, thanks for the good work.

onedr0p commented 2 years ago

I am definitely open to PRs on either implementation, but heads up this repo might get migrated over to my own name in the future.

denis-ev commented 2 years ago

I am definitely open to PRs on either implementation, but heads up this repo might get migrated over to my own name in the future.

Maybe this will help someone to implement it https://github.com/adrienverge/yamllint/blob/5ac3ed44901bf57036d610d23c4b7cf6e9446977/yamllint/linter.py#L89

denis-ev commented 2 years ago

this is how I currently exclude some files

- repo: https://github.com/k8s-at-home/sops-pre-commit
    rev: v2.1.1
    hooks:
      - id: forbid-secrets
        exclude: |
          (?x)^(
            cluster\/apps\/kube-system\/kube-vip\/rbac.yaml|
            tmpl\/.*.sops\.ya?ml
            )$
onedr0p commented 2 years ago

Here we could reuse the values of the path_regex keys within the items of the creation_rules array in .sops.yaml (at a repository's root) to distinctively identify the files that are required to be encrypted.

This seems like the best path forward but would be tricky since you need to parse the .sops.yaml and basically re-implement logic in Python that is already done in the SOPS Go code :/

Ideally it would be great if the SOPS maintainers were actually active, as this PR would just let me check via a sops CLI flag if the file should be encrypted or not.

https://github.com/mozilla/sops/pull/545

almereyda commented 2 years ago

Beautiful finding. Thanks for getting back to the idea. It's always encouraging when encryption- and thus security-related repositories have a lack of maintenance and cannot respond properly to PRs which are open since three years. Should we revive the discussion over there to eventually get a review of the current state?

onedr0p commented 2 years ago

I posted Bump in that PR in January, SOPS is really struggling to find maintainers.

More people asking for an update here the better. https://github.com/mozilla/sops/discussions/927

almereyda commented 2 years ago

There was a response from Mozilla shortly after you linked the discussion.

onedr0p commented 2 years ago

I saw, there probably won't be much movement on this repo until changes happen over there. Any option to try and glue together a solution will be near the current implementation and that's lacking as is.