[procmgr] Secret backend resolution for config gates - #54734
[procmgr] Secret backend resolution for config gates#54734jose-manuel-almaza wants to merge 4 commits into
Conversation
jose-manuel-almaza
added
the
qa/done
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa9c11ac11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let resolved = secrets::resolve_config_string(text, agent_yaml); | ||
| if secrets::is_enc(&resolved) { | ||
| return None; |
There was a problem hiding this comment.
When a configured backend is unavailable, times out, or omits a handle, returning None here removes that source from consideration, so best_config_layer can select a lower-priority fleet, environment, YAML, or default value. For example, an unresolved DD_PROCESS_CONFIG_PROCESS_COLLECTION_ENABLED=ENC[...] with true in the YAML starts the process, whereas the core Agent returns an error from LoadDatadog when any pre-fleet secret cannot be decrypted. Preserve the unresolved source as a blocking result instead of falling through, or procmgr can auto-start services while the Agent rejects the same configuration.
Useful? React with 👍 / 👎.
| let wait_result = wait_for_child(); | ||
| let output = reader | ||
| .join() | ||
| .map_err(|_| anyhow::anyhow!("secret backend stdout reader panicked"))??; |
There was a problem hiding this comment.
If a backend forks a child that inherits stdout, killing or observing the direct backend process does not close the pipe, yet this code unconditionally joins the reader. A backend such as a shell script that launches a background helper can therefore hang config-gate evaluation indefinitely—even after the configured timeout—because both platform runners terminate only the direct process. Run the backend in a killable process group/job and terminate all descendants, or otherwise bound the reader join.
Useful? React with 👍 / 👎.
| unsafe fn drop_to_agent_user(uid: Uid, gid: Gid) -> std::io::Result<()> { | ||
| nix::unistd::setgid(gid).map_err(io_error)?; | ||
| nix::unistd::setuid(uid).map_err(io_error)?; |
There was a problem hiding this comment.
When the Unix supervisor runs as root, setgid followed by setuid leaves the supervisor's supplementary groups intact; a typical root service can therefore execute the secret backend as UID dd-agent while retaining membership in privileged groups such as GID 0. Clear or initialize supplementary groups for dd-agent before changing GID/UID so the backend actually runs with the Agent account's privileges.
Useful? React with 👍 / 👎.
| timeout: std::time::Duration, | ||
| max_output_bytes: usize, | ||
| _skip_acl_check: bool, |
There was a problem hiding this comment.
On Unix, the ACL-check parameter is ignored and every custom secret_backend_command is executed directly. The core Agent's filesystem.CheckRights rejects executables with group/other permissions unless secret_backend_command_allow_group_exec_perm permits the restricted group case; without the equivalent check, procmgr will execute a world-writable backend that the Agent refuses, allowing a local user to run code as dd-agent and potentially open config gates.
Useful? React with 👍 / 👎.
|
🎯 Code Coverage (details) 🔗 Commit SHA: 01af0ac | Docs | Datadog PR Page | Give us feedback! |
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
Empty secret-backend values are accepted as resolved secrets, so they receive higher priority than Fleet policy and can suppress a valid Fleet-enabled gate. The core Agent rejects empty resolved values; this path should preserve the unresolved value instead of promoting an empty string.
🤖 Datadog Autotest · Commit fa9c11a · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| entry | ||
| .get("value") | ||
| .and_then(Value::as_str) | ||
| .map(|value| normalize_secret_value(value, remove_trailing_line_break)) |
There was a problem hiding this comment.
Customers using an empty or newline-only secret can have Fleet-enabled process-manager features silently not start.
Assertion details
- Input: A local config gate contains ENC[mode], the backend returns {"mode":{"value":""}}, and Fleet policy enables the same string-backed setting (for example infrastructure_mode: end_user_device).
- Expected:
An empty resolved secret is treated as unresolved, matching the core Agent, so the valid Fleet value remains effective. - Actual:
parse_secret_response returns Ok("") and promote_secret_string assigns Secret priority to that empty value. The empty local secret therefore wins over the Fleet value and can disable the derived system-probe gate.
| entry | |
| .get("value") | |
| .and_then(Value::as_str) | |
| .map(|value| normalize_secret_value(value, remove_trailing_line_break)) | |
| entry | |
| .get("value") | |
| .and_then(Value::as_str) | |
| .map(|value| normalize_secret_value(value, remove_trailing_line_break)) | |
| .filter(|value| !value.is_empty()) | |
| .with_context(|| format!("secret backend response missing value for {handle}")) |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
fa9c11a to
4f70c7b
Compare
b11d137 to
f8f5bdb
Compare
919ce8d to
59cf619
Compare
ac7bfde to
7153cf9
Compare
7153cf9 to
d57c842
Compare
d57c842 to
8ec8ebc
Compare
8ec8ebc to
805a702
Compare
805a702 to
d8b67e1
Compare
8b0a427 to
02e8ade
Compare
02e8ade to
13aa0d1
Compare
df88247 to
d554bba
Compare
Move writeProcessesDYamlContent and process-owner helpers here where the agent-profile E2E tests use them, after dropping them from spawn-profiles.
Resolve ENC[...] gate values via secret_backend_command and native backends, matching Agent precedence and spawning backends under the core Agent account.
Invalidate secret-backend and Windows SCM env caches before reloading processes.d so config gates re-evaluate with fresh values.
d554bba to
01af0ac
Compare
1 participant
What does this PR do?
Resolves
ENC[...]values during config gate evaluation so dd-procmgr matches Agent secret handling.Adds:
config_gate/secrets.rs: resolve handles viasecret_backend_command, nativesecret_backend_type, andmulti_secret_backends(same precedence as the core Agent)CreateProcessAsUserWunder the Agent account; Unix setuid when procmgr runs as root for Privileged children)secret_backend_exec.rs: shared spawn, timeout, stdout drain, and response parsingDD_SECRET_BACKEND_*env (including core Agent SCMEnvironmenton Windows) overdatadog.yamlFleet policy
ENC[...]values stay unresolved here, matching AgentMergeFleetPolicyrunning after secret resolution.Stack context: PR 3/4 split from #53249. Builds on #54732 (
jose/procmgr-config-gates), which in turn builds on #54731. Process-agent dual-mode integration lands in PR 4 (jose/procmgr-windows-process-agent).Motivation
PR 2 config gates read YAML, env, and fleet policy, but many customers gate features with secret-backed settings (
ENC[api_key], secret-backed booleans, etc.). Without secret resolution, gates would mis-evaluate and auto-start behavior would diverge from the Agent.Describe how you validated your changes
pkg/procmgr/rust/src/config_gate/secrets.rsand config gate integration tests (serialized env to avoid cross-test leakage)Additional Notes
secret-generic-connectorwhen no customsecret_backend_commandis configured.