Allow better tuning of SecretsUsedInArgOrEnv check
#5403 complained about the SecretsUsedInArgOrEnv check being overly strict. The example given was PUBLIC_KEY being flagged as a secret. #5410 fixed #5403 too literally, by allowing secrets if they contained the token "public". The current check:
https://github.com/moby/buildkit/blob/bd6820ae0d855510f30ef2b22e70e35396f6e66a/frontend/dockerfile/dockerfile2llb/convert.go#L2539-L2561
is both buggy and still too inflexible.
- It has a bug. Although it claims to "Check for either full value or first/last word", it in fact flags values containing any of the tokens anywhere (using underscore as the token separator). For example, it flags
KMS_KEY_ALIAS,AWS_ACCESS_KEY_ID, andAWS_SHARED_CREDENTIALS_FILEas secrets. - By design it flags things like
KEY_NAME,KEY_ALIAS,KEY_ENABLED,CREDENTIAL_MANAGER,REQUIRE_MFA_AUTH,PASSWORD_PROMPT_STRINGand so on, which are not secrets.
As a general rule, I think a check like this is impractical to tune correctly. I would prefer in not be enabled by default. I would also like a workaround that, like with other linters, would allow some kind of annotation in the Dockerfile that the ARG or ENV is not a secret.
At a minimum, I would like it to live up to its claim to only check the full value or first/last word. I think this would work:
pattern := `(?i)^(?:(` + strings.Join(secretTokens, "|") + `)(?:_.*)?|.*_(` + strings.Join(secretTokens, "|") + `))$`
Secret detection is hard, just a regex is not enough. Please allow disabling this per entry and not just per Dockerfile.
E.g. I came here because we jet another false positive to process and disabling this rule for the whole Dockerfile feels .. icky:
ENV CARGO_REGISTRIES_SOME_REGISTRY_NAME_CREDENTIAL_PROVIDER='cargo:token-from-stdout cat /run/secrets/codeartifact_token'
This is not a secret, it is explicitly avoiding exposing a secret by using the docker secrets infrastructure.
The intention behind this check is admirable, but the number of false positives is crazy and far exceeds the usefulness of the check. Like others have said the ability to ignore individual false positives inline seems like the only reasonable solution.
Even if the logic were magically 100% perfect, the ability to ignore this check for a single instance (instead of the whole file) would still be useful.
+1 to being able to specify an ignore list