VC-36950: It is now possible to exclude labels and annotations #614
Conversation
maelvls
changed the title
maelvls
force-pushed
the
VC-36950-add-exclude-annots-labels
branch
from
dfcfa3c to
9f8f724
Compare
| dynDg, isDynamicGatherer := newDg.(*k8s.DataGathererDynamic) | ||
| if isDynamicGatherer { | ||
| dynDg.ExcludeAnnotKeys = config.ExcludeAnnotationKeysRegex | ||
| dynDg.ExcludeLabelKeys = config.ExcludeLabelKeysRegex | ||
| } |
There was a problem hiding this comment.
This is super hacky... I tried finding a cleaner way of "injecting" the ExcludeAnnotKeys and ExcludeLabelKeys, but gave up.
maelvls
force-pushed
the
VC-36950-add-exclude-annots-labels
branch
from
9f8f724 to
4097047
Compare
There was a problem hiding this comment.
@maelvls I read the code but didn't test it yet.
- The unit tests are failing
- The example regex patterns don't seem very realistic. Why would a user put "secrets" in the annotations of labels?
- And if it is realistic, shouldn't the venafi-kubernetes-agent redact labels containing those terms by default...like a log sanitizer or like GitHub actions attempts to do to prevent accidentally leaking these things.
- The original Jira ticket says: "Note: sensitive data could be something like organizational structure or team ownership of a resource, which could be defined through labels." but does not give any real world examples. I imagine something like
^.*\.example\.com/.*to filter out all the labels with the well known organization prefix.
| # If you would like to exclude annotations keys that contain the word | ||
| # `secret`, use the regular expression `.*secret.*`. The leading and ending .* | ||
| # are important if you want to filter out keys that contain `secret` anywhere | ||
| # in the key string. |
There was a problem hiding this comment.
| # If you would like to exclude annotations keys that contain the word | |
| # `secret`, use the regular expression `.*secret.*`. The leading and ending .* | |
| # are important if you want to filter out keys that contain `secret` anywhere | |
| # in the key string. | |
| # If you would like to exclude annotation keys that contain the word | |
| # `secret`, use the regular expression `.*secret.*`. The leading and ending .* | |
| # are important if you want to filter out keys that contain `secret` anywhere | |
| # in the key string. |
pkg/datagatherer/k8s/dynamic.go
Outdated
| annotsRaw, ok, err := unstructured.NestedFieldNoCopy(resource.Object, "metadata", "annotations") | ||
| if err != nil { | ||
| return fmt.Errorf("wasn't able to find the metadata.annotations field: %w", err) | ||
| } |
There was a problem hiding this comment.
I don't think this should return an error, because the object might not have any annotations. I guess it depends whether the metadata.annotations field has an omitempty marker.
There was a problem hiding this comment.
You are right. I will rework this and add more unit tests to feel a little more confident.
There was a problem hiding this comment.
I've removed this error and added unit tests in 120453f. PTAL
|
@wallrj FYI, the documentation for this feature is being worked on in https://gitlab.com/venafi/vaas/ua/clouddocs/-/merge_requests/1220. |
maelvls
force-pushed
the
VC-36950-add-exclude-annots-labels
branch
from
eecf784 to
365d7a1
Compare
|
I've fixed the unit tests. The issue was each test case focused on a single data gatherer, and one data gatherer only ever deals with one resource at a time. And the test case in question was set up with the The tests are now passing. You can go ahead and try the feature out. Thanks! @wallrj |
You are right. To give a realistic regex in this test case, let’s use the example of the Kapp project that uses four annotations that all start with annotations:
kapp.k14s.io/original: |
{"apiVersion":"v1","kind":"Secret","spec":{"data": {"password": "cGFzc3dvcmQ=","username": "bXl1c2VybmFtZQ=="}}}
kapp.k14s.io/original-diff: |
- type: test
path: /data
value:
password: cygpcGVyUzNjcmV0UEBhc3N3b3JkIQ==
username: bXl1c2VybmFtZQ==In this case, I'd suggest using the following exclusion setting in config:
excludeAnnotationKeysRegex:
- kapp\.k14s\.io/original.*^ Note that the regex doesn't need to start with
Good point, that'd be perfect. I don't know how to get there though, so until we get there, I like the idea of providing a way of letting customers exclude specific annotations.
I think your example is sensible, and I'll change my examples in code and documentation to match what you proposed. The config:
excludeLabelKeysRegex:
- .*\.company\.com/.*It seems like It reminds me that I need to clarify a few things in the documentation:
|
There was a problem hiding this comment.
@maelvls I tested it locally and it does strip the labels and annotations from the metdata of objects.
But it misses the labels and annotations that are added to PodTemplate in Deployment, StatefulSet, Job etc.
So I don't think it's going to satisfy users who want to prevent the sharing of sensitive labels and annotations.
Happy to approve this if you consider it to be a step in the right direction, but I think a general solution which scrubs all sensitive fields, will look different to this.
I suppose it might be applied to all keys in the venafi-kubernetes-agent JSON output,
regardless of which DataGatherers have been used.
helm template deploy/charts/venafi-kubernetes-agent \
--show-only templates/configmap.yaml \
--set fullnameOverride=venafi-kubernetes-agent \
--set config.excludeAnnotationKeysRegex={'.*'} \
--set config.excludeLabelKeysRegex={'.*'} \
| yq '.data."config.yaml"
| @yamld
| .organization_id |= "example.com"
| .cluster_id |= "cluster-1.example.com"' \
> examples/venafi-kubernetes-agent.yaml
go run ./ agent \
--one-shot \
--api-token should-not-be-required \
--install-namespace venafi \
--output-path /dev/stdout \
--agent-config-file examples/venafi-kubernetes-agent.yaml 2>/dev/null \
| grep -E -C 5 -e '"(labels|annotations)": {$'
...
--
}
},
"template": {
"metadata": {
"creationTimestamp": null,
"labels": {
"app.kubernetes.io/instance": "venafi-enhanced-issuer",
"app.kubernetes.io/managed-by": "Helm",
"app.kubernetes.io/name": "venafi-enhanced-issuer",
"app.kubernetes.io/version": "v0.14.0",
"helm.sh/chart": "venafi-enhanced-issuer-v0.14.0",
"pod-template-hash": "d7496dc68"
},
"annotations": {
"kubectl.kubernetes.io/default-container": "venafi-enhanced-issuer"
}
},
"spec": {
"containers": [
maelvls
force-pushed
the
VC-36950-add-exclude-annots-labels
branch
from
689b311 to
120453f
Compare
I totally forgot about these. I think it's worth investigating this further in a separate PR, but I'd be OK shipping the next version of Venafi Kubernetes Agent with the feature as presented in this PR. |
maelvls
force-pushed
the
VC-36950-add-exclude-annots-labels
branch
2 times, most recently
from
42a4e34 to
0e609ff
Compare
There was a problem hiding this comment.
Thanks @maelvls
Thanks for making the new unit tests so easily readable.
You've acknowledged that this only catches the labels in the object metadata,
not in the PodTemplates of Deployment, StatefulSet etc..
But we've agreed that you'll address that in another PR.
The user-facing configuration makes sense and is very well documented.
But we've also acknowledged that it is going to be quite difficult for the user to know whether their supplied exclusion regular expressions are actually working.
They don't have an easy way to see the data that is being collected by the agent before it is posted to the Venafi API.
I imagine that users may ask for a label / annotation (or even a general field) whitelist feature in future, so that they can declare exactly which fields and values are shared with Venafi....but hopefully this new feature will help us to validate that.
/approve
/lgtm
I've also reduced the size of the documentation in values.yaml; it now only contains the essential information.
Co-authored-by: Richard Wall <[email protected]>
maelvls
force-pushed
the
VC-36950-add-exclude-annots-labels
branch
from
0e609ff to
8f99daa
Compare
Now being worked on in https://venafi.atlassian.net/browse/VC-37240
|
Some annotations and labels contain sensitive information. A well-known example is the
kubectl.kubernetes.io/last-applied-configurationannotation. We already exclude this annotations from Secret resources, but we have found that customers use other sensitive annotations and labels that they would like to not be sent to the Venafi Control Plane API.A realistic example lies in the Kapp project. The Kapp project uses four annotations that all start with
kapp.k14s.io/original*as seen in 1. These annotations are similar tokubectl.kubernetes.io/last-applied-configurationin that they may contain sensitive information. The annotations would look like this:In this case, customers will be suggested to use the following exclusion setting in
values.yamlto exclude bothkapp.k14s.io/originalandkapp.k14s.io/original-diff:For labels, it looks like this:
The documentation for this feature is being worked on in https://gitlab.com/venafi/vaas/ua/clouddocs/-/merge_requests/1220.
Ref: VC-36950
Why regex, why not fixed string search with wildcards?
We have chosen to go with regexes instead of fixed-string search or wildcards. We know that using regexes will be confusing, but we don't expect this feature to be used so often, and since we don't know exactly the type of filtering that customers will want, we have decided to use the most versatile one : regexes.
Manual Testing
Although I've added a couple of unit tests, I haven't had time to add an automated end-to-end test. In particular, my PR is lacking a smoke test to check that the Helm chart value
excludeAnnotationKeysRegexdoes what it claims it does.Here is the two manual tests that make me confident that this feature works as expected.
Test 1: Using
--output-path+ fake Jetstack Secure API tokenI used an empty Kind cluster for this test.
First, run this:
Then:
openssl req -x509 -nodes -days 365 -newkey rsa:2048 -keyout tls.key -out tls.crt -subj "/CN=mydomain.com/O=myorganization" kubectl create secret tls my-tls-secret --cert=tls.crt --key=tls.key kubectl annotate secret my-tls-secret some-secret-apikey=1234 kubectl label secret my-tls-secret some-secret-password=1234 kubectl create ns my-ns kubectl annotate ns my-ns some-secret-apikey=1234 kubectl label ns my-ns some-secret-password=1234Finally, run the agent in one-shot mode. You should see no output when running this:
Before this PR, the output would look like this:
Test 2: Real API + Venafi Cloud Key Pair Service Account auth
In this test, I check that agent's HTTP request doesn't contain the annotations and labels using mitmproxy.
For this test, I've used the tenant https://ven-tlspk.venafi.cloud/. To access the API key, use the user
[email protected]and the password is visible in the page Production Accounts (private to Venafi). Then go to the settings and find the API key, and set it as an env var:Create the service account and key pair:
Now, make sure to have
127.0.0.1 mein your/etc/hosts.Then, run mitmproxy with:
Run this:
Now, don't forget to create a Kind cluster. Then:
openssl req -x509 -nodes -days 365 -newkey rsa:2048 -keyout tls.key -out tls.crt -subj "/CN=mydomain.com/O=myorganization" kubectl create secret tls my-tls-secret --cert=tls.crt --key=tls.key kubectl annotate secret my-tls-secret some-secret-apikey=1234 kubectl label secret my-tls-secret some-secret-password=1234 kubectl create ns my-ns kubectl annotate ns my-ns some-secret-apikey=1234 kubectl label ns my-ns some-secret-password=1234Finally, run the Agent with:
Look at the mitmproxy logs and look for the entry that has the path
Open it (enter) and type
/and 1234. You should not see any instance of "1234", proving that the annotations and labels have been filtered out.When looking at the JSON blob, all of the annotations and labels containing
apikeyandpasswordare gone:Here is the output that I got without this PR: