[VC-43403] Add filter for excluding TLS secrets without client certificates

[wallrj-cyberark]

This PR introduces a configurable filter for the k8s-dynamic data gatherer that excludes some Kubernetes TLS secrets;
those which do not contain a client certificate.
This is in support of the CyberArk Discovery and Context service which wants various Secret types, but in the case of X.509 certificates, they only need certificates which allow Client Authentication. So in the interests of least privilege we try to only send data that is strictly needed.

• Introduce ExcludeTLSSecretsWithoutClientCert filter for k8s-dynamic gatherer
• Update config, docs, and examples to support and demonstrate filter usage
• Implement filter logic to exclude TLS secrets lacking client certificates
• Extend tests to cover filtering of TLS secrets based on certificate EKU
• Add logging for filter decisions and error cases

Part of: https://venafi.atlassian.net/browse/VC-43403

Testing

Run the agent and watch it log trace messages as you create TLS secrets

$ go run ./cmd/ark agent --log-level 2 -c examples/machinehub.yaml --output-path data.json -p 1h

$ step certificate create foo foo.crt foo.key --profile self-signed --subtle --no-password --insecure
Your certificate has been saved in foo.crt.
Your private key has been saved in foo.key.

$ kubectl create secret  tls foo --key=foo.key --cert foo.crt
secret/foo created

I0911 22:34:03.464756  162389 cache.go:115] "TLS Secret contains a client certificate" namespace="default" name="foo"
I0911 22:35:08.707640  162389 cache.go:115] "TLS Secret contains a client certificate" namespace="cert-manager" name="foo"

Run the agent in --machine-hub mode and use mitmproxy to examine the snapshot that it uploads:

mitmproxy

$ export HTTPS_PROXY=localhost:8080
$ go run ./cmd/ark agent --log-level 2 -c examples/machinehub.yaml --machine-hub --one-shot

Export the request (via mitmproxy)

$ tail -1 request.txt | jq '.secrets'

  {
    "apiVersion": "v1",
    "data": {
      "tls.crt": "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSUJlakNDQVNDZ0F3SUJBZ0lRSWl2YnBPSzZLL1JOWWs5WVRjR0t5akFLQmdncWhrak9QUVFEQWpBT01Rd3cKQ2dZRFZRUURFd05tYjI4d0hoY05NalV3T1RFeE1qRXpNek01V2hjTk1qVXdPVEV5TWpFek16TTVXakFPTVF3dwpDZ1lEVlFRREV3Tm1iMjh3V1RBVEJnY3Foa2pPUFFJQkJnZ3Foa2pPUFFNQkJ3TkNBQVNFQnpyN3ozSWphMnJnCis0ODJrbUtCZzhWQUY5MElOQzBqYVM5bFZKbElCS2Zja2NhcDEvbHNKRTdCTm9UMDZVS3hTWng2NUwzWTZGTzYKdkRFeGE4cU5vMkF3WGpBT0JnTlZIUThCQWY4RUJBTUNCNEF3SFFZRFZSMGxCQll3RkFZSUt3WUJCUVVIQXdFRwpDQ3NHQVFVRkJ3TUNNQjBHQTFVZERnUVdCQlRSamxjRFdpaTBNNTQvLzRSVmQ1SERFa3RkTFRBT0JnTlZIUkVFCkJ6QUZnZ05tYjI4d0NnWUlLb1pJemowRUF3SURTQUF3UlFJaEFOazFOckNabk1FMHJlSlROU3NHT3N4aGk2ZnUKZ2paOFd1OG02QURJek9YMkFpQkU0V2gvUFhDdngyR3liZ2ltWVNYWXkxY1RNaFhjbXZ2a2JCbDhObnBEeGc9PQotLS0tLUVORCBDRVJUSUZJQ0FURS0tLS0tCg=="
    },
    "kind": "Secret",
    "metadata": {
      "creationTimestamp": "2025-09-11T21:34:03Z",
      "name": "foo",
      "namespace": "default",
      "resourceVersion": "4618",
      "uid": "11bd6a5e-b79b-4117-ac38-c16cb7f911d9"
    },
    "type": "kubernetes.io/tls"
  }

[maelvls]

High‑level: This is a solid addition — the filter is clearly scoped, docs and examples are updated, and tests cover both config parsing and core behaviour for TLS secrets with/without client-auth EKU. Nice work tying this to least‑privilege goals.

Major behaviour issue to address

• Filters only apply on Add events. In newDataGathererWithClient the filter is passed to onAdd(...) but not enforced on updates. Today onUpdate(...) will (re)insert the object into the cache even if it would have been excluded by the filter, because it doesn’t check filters and will add the object when not present.
  • Impact: a TLS secret excluded at create time can be added later on any Update event (e.g., label change) even if it still lacks ClientAuth EKU; conversely, if a secret becomes acceptable later (e.g., cert change), we want it to be included — but this should be done by evaluating the filter on the new object.
  • Suggested fix:
    • Apply c.Filters in the Update handler as well, using the new object. If any filter returns true, remove the entry from the cache if present and return; otherwise, update it. For example:
      • In the informer registration: UpdateFunc: func(oldObj, newObj interface{}) { onUpdate(log, oldObj, newObj, dgCache, c.Filters...) }
      • Adjust onUpdate(...) to accept the variadic filters; if excluded, dgCache.Delete(string(uid)) and return; else proceed to set.
    • Consider evaluating filters in Delete only for completeness (likely not necessary) but Update needs it.
  • Please also add a test that verifies: (1) a non‑client TLS secret is excluded on add; (2) an Update still keeps it excluded; (3) flipping the certificate to one with ClientAuth EKU on Update makes it appear in cache.

Logging/UX

• The filter logs at V(4) (e.g., "TLS Secret contains a client certificate" / "does not contain a client certificate"). The PR body suggests --log-level 2 to observe them. Confirm that log level 2 surfaces V(4) in your logger wiring; if not, either drop to a lower V level (e.g., V(2)) or adjust the docs to match the effective verbosity.

Config/Marshalling

• Using an internal []cacheFilterFunction in ConfigDynamic and mapping from YAML strings in UnmarshalYAML is clean. Returning an error for unknown filter names is good for fail‑fast configuration.

Minor nits

• Typo in a comment: "eliptic" -> "elliptic" in test helper docstring.
• Consider a slightly clearer name for the helper (hasClientAuthEKU or similar), but isClientCertificate is acceptable given the docstring.

Unrelated but noticed while here (optional follow‑up PR)

• updateCacheGatheredResource(...): the condition
  if deletedAt.IsZero() && !deletedAt.IsZero() { cacheObject.DeletedAt = deletedAt }
  can never be true; I believe the intent was to copy a non‑zero DeletedAt from the existing cache entry. Probably should be if !deletedAt.IsZero() { cacheObject.DeletedAt = deletedAt }.

Overall this looks close — the main functional gap is filtering on Update. Happy to take another look after that change and the accompanying tests.

[maelvls]

Shouldn't the filter also apply the the update func? I'm not sure, since I don't know how these AddFunc, UpdateFunc, DeleteFunc relate to the underlying client-go informer.
If it's not required, would be nice to have a comment explaining why the filtering only applies to AddFunc.

Also a test for that?

[maelvls]

Shouldn't this be at level 1 or 2? The Helm chart says:

# The log levels are: 0=Info, 1=Debug, 2=Trace.

[maelvls]

Thanks for the thorough implementation and tests — this is a nice addition. I left a few inline comments:

• Guard a possible nil dereference in the filter’s error logging.
• Apply filters on updates as well (and add tests) so cache stays consistent when Secret EKUs change.
• Config shape nit: prefer storing filter names on the config and mapping to funcs at construction for easier logging/marshalling.
• Small docs fix for --log-level to use numeric values.

Happy to iterate on any of these!

[maelvls]

Filters are applied on add events but not on updates. If a TLS Secret’s EKU changes (e.g., client → server-only), the cache will retain an entry that should now be excluded.

Proposal:

• Pass filters to onUpdate as well. If any filter excludes newObj, remove it from the cache; if it was previously excluded but now passes, add/replace it.
• Add tests for both transitions (client→non-client and non-client→client) to ensure cache consistency.

[maelvls]

Config shape nit: Filters on ConfigDynamic stores function pointers. That works after unmarshalling, but it makes the config awkward to marshal/log and couples config with runtime state. Consider storing []string on the config (the names), and mapping them to functions during construction (e.g., in NewDataGatherer).

[maelvls]

Minor doc fix: the project uses numeric --log-level (0=Info, 1=Debug, 2=Trace; 6–9 HTTP trace). Suggest rewording to something like: "set --log-level=1 (Debug)" so users don’t try --log-level=debug.

[maelvls]

possible nil dereference here: when parsePEMCertificateChain returns (nil, nil) (or len(certs) == 0), err is nil and err.Error() will panic

[wallrj-cyberark]

After chatting to @inteon and @maelvls we decided that its safer to do the "minimization" in the cyberark client, just before upload.

Supplanted by: #714

• [VC-43403] Minimize snapshot by filtering non-clientauth TLS secrets #714

I've addressed those code review points in #714 where they still apply.