Best-effort detection of potentially secret variables #665
Conversation
code/go/internal/validator/spec.go
Outdated
| if substringInSlice(e.Error(), redundant) { | ||
| continue | ||
| } | ||
| processedErrs = append(processedErrs, e) |
There was a problem hiding this comment.
Unrelated fix required for this change, but that I could move to a different PR.
There was a problem hiding this comment.
Created also separate PR so we merge this fix in any case #666
| anyOf: | ||
| - properties: | ||
| name: | ||
| pattern: "(password|api_key|access_key|token)" |
There was a problem hiding this comment.
@kpollich @nimarezainia this is the regexp pattern for variable names that we would detect as candidates to be secrets, is there any other string do you think we should add here?
Should we match with exact values instead of with substrings?
There was a problem hiding this comment.
We could add the substring secret here as it will catch things like an AWS secret_access_key value. I think using substring matching is good to keep this generic.
There was a problem hiding this comment.
Added secret and passphrase, the latter is sometimes used for encrypted TLS keys.
There was a problem hiding this comment.
We should juste be cautious with values like "secret_path" which might be caught by this check though.
There was a problem hiding this comment.
We should juste be cautious with values like "secret_path" which might be caught by this check though.
Yep, in these cases they will need to set secret: false.
| "manifest.yml", | ||
| []string{ | ||
| "field vars.0: variable identified as possible secret, secret parameter required to be set to true or false", | ||
| "field vars.1: variable identified as possible secret, secret parameter required to be set to true or false", |
There was a problem hiding this comment.
This is the kind of errors package developers will see when upgrading to Package Spec 3.0.2.
There was a problem hiding this comment.
This is the kind of errors package developers will see when upgrading to Package Spec 3.0.2.
Isn't this an enhancement? I'm surprised a new feature would fall into a patch version.
I'm also hesitant to jump right into requiring secret when it's still new functionality (sure, a dev can set secret: false but the way the error's presented, we're encouraging to set secret: true). Based on a recent issue I ran into trying to set secret: true (see elastic/kibana#154715 (comment)), I think it's worth adopting on a smaller set of integrations first.
Should using secret be encouraged, especially with new integrations? Definitely! But required? I'm not so sure.
There was a problem hiding this comment.
Hey @ebeahan,
I'm surprised a new feature would fall into a patch version.
Starting on Package Spec 3 we are not fully following semver. I understand this can be surprising. We use minors now for compatibility with the stack, and this change doesn't affect the compatibility with the stack, it only enhances validations. Secrets were already supported in the spec since 2.9.0.
It is an enhancement in the sense that it can prevent security risks. We have also considered as enhancements other "breaking" validations that we have introduced in the past to avoid problematic situations. We can reconsider this in any case if needed.
a dev can set
secret: falsebut the way the error's presented, we're encouraging to setsecret: true
I am open to suggestions regarding the error message.
Based on a recent issue I ran into trying to set secret: true (see elastic/kibana#154715 (comment))
This is an issue found in an snapshot build, that was fixed. We can still delay the introduction of this change though, 3.0.2 hasn't been released yet.
There was a problem hiding this comment.
Starting on Package Spec 3 we are not fully following semver. I understand this can be surprising. We use minors now for compatibility with the stack, and this change doesn't affect the compatibility with the stack, it only enhances validations. Secrets were already supported in the spec since 2.9.0.
Updating versioning guidelines in the README taking recent changes into account #667
💚 Build Succeeded
History
cc @jsoriano |
| ExcludeChecks: []string{"SVR00004"}, | ||
| ExcludeChecks: []string{ | ||
| // Allow to test unreleased features in "good" packages. | ||
| "PSR00001", |
What does this PR do?
Fails validation on variables that look like secrets and don't declare if they are secrets or not with the
secretproperty.This detection is based on two things:
password,api_keyortoken.password.Why is it important?
To encourage adoption of this feature to secure secrets in package policies.
Checklist
test/packagesthat prove my change is effective.spec/changelog.yml.Related issues
secret: trueelastic-package#1554