[ASCII-1152] Refreshing secrets creates a refresh-audit.json file

[dustmop]

What does this PR do?

Add to a refresh-audit.json file whenever a secret is refreshed. Scrub the value of the secret, retaining the last 5 chars for secrets that appear to be API or App Keys. Prune any entries older than 90 days.

Motivation

Part of the API Key Refresh initiative. Allows some record keeping about when secrets get refreshed, without exposing any information about what the secret values were.

Additional Notes

Possible Drawbacks / Trade-offs

Describe how to test/QA your changes

Behavior covered by unit tests.

Reviewer's Checklist

• [x] If known, an appropriate milestone has been selected; otherwise the Triage milestone is set.
• [ ] Use the major_change label if your change either has a major impact on the code base, is impacting multiple teams or is changing important well-established internals of the Agent. This label will be use during QA to make sure each team pay extra attention to the changed behavior. For any customer facing change use a releasenote.
• [x] A release note has been added or the changelog/no-changelog label has been applied.
• [x] Changed code has automated tests for its functionality.
• [x] Adequate QA/testing plan information is provided. Except if the qa/skip-qa label, with required either qa/done or qa/no-code-change labels, are applied.
• [x] At least one team/.. label has been applied, indicating the team(s) that should QA this change.
• [ ] If applicable, docs team has been notified or an issue has been opened on the documentation repo.
• [ ] If applicable, the need-change/operator and need-change/helm labels have been applied.
• [ ] If applicable, the k8s/<min-version> label, indicating the lowest Kubernetes version compatible with this feature.
• [ ] If applicable, the config template has been updated.

[pr-commenter[bot]]

Bloop Bleep... Dogbot Here

Regression Detector Results

Run ID: 351ab9ad-cd22-4ef8-baa7-d0838a01208d Baseline: 39eea0d6bce393e259882133d83548563da0979e Comparison: 059e7d91725860b540594f33e1119ac2422530a8

Performance changes are noted in the perf column of each table:

• ✅ = significantly better comparison variant performance
• ❌ = significantly worse comparison variant performance
• ➖ = no significant change in performance

No significant changes in experiment optimization goals

Confidence level: 90.00% Effect size tolerance: |Δ mean %| ≥ 5.00%

There were no significant changes in experiment optimization goals at this confidence level and effect size tolerance.

Experiments ignored for regressions

Regressions in experiments with settings containing erratic: true are ignored.

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	+1.88	[-4.73, +8.49]

Fine details of change detection per experiment

perf	experiment	goal	Δ mean %	Δ mean % CI
➖	file_to_blackhole	% cpu utilization	+1.88	[-4.73, +8.49]
➖	process_agent_standard_check	memory utilization	+0.76	[+0.72, +0.80]
➖	uds_dogstatsd_to_api_cpu	% cpu utilization	+0.40	[-2.66, +3.47]
➖	trace_agent_msgpack	ingress throughput	+0.03	[+0.01, +0.05]
➖	trace_agent_json	ingress throughput	+0.01	[-0.04, +0.06]
➖	tcp_dd_logs_filter_exclude	ingress throughput	-0.00	[-0.05, +0.05]
➖	uds_dogstatsd_to_api	ingress throughput	-0.00	[-0.06, +0.06]
➖	idle	memory utilization	-0.36	[-0.41, -0.31]
➖	otel_to_otel_logs	ingress throughput	-0.49	[-1.12, +0.15]
➖	process_agent_real_time_mode	memory utilization	-0.57	[-0.61, -0.52]
➖	process_agent_standard_check_with_stats	memory utilization	-0.64	[-0.69, -0.60]
➖	pycheck_1000_100byte_tags	% cpu utilization	-0.79	[-6.00, +4.41]
➖	tcp_syslog_to_blackhole	ingress throughput	-0.84	[-0.92, -0.75]
➖	basic_py_check	% cpu utilization	-1.51	[-3.80, +0.78]
➖	file_tree	memory utilization	-2.59	[-2.67, -2.51]

Explanation

A regression test is an A/B test of target performance in a repeatable rig, where "performance" is measured as "comparison variant minus baseline variant" for an optimization goal (e.g., ingress throughput). Due to intrinsic variability in measuring that goal, we can only estimate its mean value for each experiment; we report uncertainty in that value as a 90.00% confidence interval denoted "Δ mean % CI".

For each experiment, we decide whether a change in performance is a "regression" -- a change worth investigating further -- if all of the following criteria are true:

1. Its estimated |Δ mean %| ≥ 5.00%, indicating the change is big enough to merit a closer look.

2. Its 90.00% confidence interval "Δ mean % CI" does not contain zero, indicating that if our statistical model is accurate, there is at least a 90.00% chance there is a difference in performance between baseline and comparison variants.

3. Its configuration does not mark it "erratic".

[dustmop]

I’m not sure that opening the log file, JSON-unmarshaling its whole content and marshalling it back and writing the whole file each time we refresh secrets is efficient. Do we have any strong guarantee that the audit log file size is bounded to a size small enough that this will not be an issue ?

The audit file should only be added to when a secret is actually reloaded with a new value, this functionality wasn't done correctly in the initial PR. If someone refreshed the API Key once a day, which is quite often, this file holds only 90 days of data, so it shouldn't grow too large.

[GustavoCaso]

@dustmop regarding your last comment

The audit file should only be added to when a secret is actually reloaded with a new value, this functionality wasn't done correctly in the initial PR. If someone refreshed the API Key once a day, which is quite often, this file holds only 90 days of data, so it shouldn't grow too large.

Right now we only support api_key but in the future that might not be the case. Could it be that holding 90 days of data when api_key is not the only secret we allow to refresh might cause some issues when loading and parsing the audit log?

Maybe an append only file that we truncate to certain size when the size reaches certain threshold might be a possible solution to avoid the parsing of old data as @L3n41c suggested might be a good compromise.

WDYT?

[dustmop]

Maybe an append only file that we truncate to certain size when the size reaches certain threshold might be a possible solution to avoid the parsing of old data as @L3n41c suggested might be a good compromise.

WDYT?

Security recommends we retain at least 90 days of records, so simply truncating the file size isn't enough. Rather, we need to read entries at the start to prune old entries, and shift or re-write the file to do so, so this isn't strictly append-only.

But most of all, I think it's going to be a challenge to grow this file very large. Let's say a user were to update 10 secret values every hour, which means rotating keys, passwords, tokens, etc all of the time while the agent is running (restarting the agent with updated config values doesn't add to this audit file). Each json row takes up about 100 bytes. That means the whole file will take up 10 * 100 * 24 * 90 (number of changes, bytes, hours per day, days to retain) = 2,160,000 so just over 2MB. That's on the order of 10s of milliseconds to parse the json.

In line with this comment, I think instead we should keep the implementation simple, and add a config option that limits the filesize when reading. This both prevents OOM attempts and also guarantees that the file is small enough that it won't seriously impact performance.

[GustavoCaso]

@dustmop

In line with https://github.com/DataDog/datadog-agent/pull/22752#discussion_r1486497197, I think instead we should keep the implementation simple, and add a config option that limits the filesize when reading. This both prevents OOM attempts and also guarantees that the file is small enough that it won't seriously impact performance.

That makes sense. I think is a good approach to limit the filesize when reading it to avoid OOM.

One thing that is missing is adding a test case to the secret component that hit that case and validate that we do not append a new entry. Also, we should add tests for the ReadFileWithSizeLimit function in pkg/util/filesystem

Once we have tests for those cases I'm happy to add the approval ✅ 😄

[dustmop]

Tests added: both of the audit-file limit getting reached when Refreshing secrets, and also for the new filesystem.ReadFileWithSizeLimit function.

[dustmop]

Hi @L3n41c and @hush-hush, I have addressed your earlier review comments. Please when you get a chance, have a another look at this PR. Thanks in advance!

[dustmop]

I have updated this PR to use a new facility "RotatingNDRecords". This will efficiently append to an ndjson (newline deliminated json) file, as described, but there's a few differences in my implementation:

1. The file is only opened when it needs to be written to, instead of opening the handle at agent startup, as that would have issues with Windows.

2. The file is synced to disk immediately when new data is added. This is because, as has been mentioned before, data is not being added to this file often. The most often we have heard of users wanting to reload their API key is every hour, and it's not even possible to reload more than once a second (using the secret_refresh_interval config setting).

3. Old entries are still trimmed from the front of the file if they pass the 90 day threshold, but now that is being done with io.Copy instead of loading everything into memory.

We currently have a number of files that we create and include in the flare, see the list here: https://github.com/DataDog/datadog-agent/blob/199d74cb775d8bd3d8cc015e1a522ee760c03e24/test/new-e2e/tests/agent-subcommands/flare/flare_files.go#L9 . I'm not sure how these interact with containerized environments, but since they are being written to the "run_path" value of the config, I assume they are setup properly with the container to use persistent storage. I can investigate more in order to validate this assumption.

[dustmop]

/merge

[dd-devflow[bot]]

:steam_locomotive: MergeQueue

This merge request is not mergeable yet, because of pending checks/missing approvals. It will be added to the queue as soon as checks pass and/or get approvals. Note: if you pushed new commits since the last approval, you may need additional approval. You can remove it from the waiting list with /remove command.

Use /merge -c to cancel this operation!

[dd-devflow[bot]]

:steam_locomotive: MergeQueue

Added to the queue.

This build is next! (estimated merge in less than 26m)

Use /merge -c to cancel this operation!