Use on-demand activation for low-traffic services - #688
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors the ACME backend: local backend bound to 127.0.0.1:port+1, socket-activated systemd proxy exposes the external ACME port, gocryptfs mounts a TPM-sealed encrypted Badger DB declaratively, and preparatory/permission rules and packages are added. ChangesACME Backend and Encryption Infrastructure
Sequence Diagram(s)sequenceDiagram
participant Client as ACME Client
participant Socket as step-ca-proxy.socket
participant Proxy as step-ca-proxy
participant Backend as Step CA Backend (127.0.0.1:port+1)
participant gocryptfs as gocryptfs mount (dbPath)
participant Badger as Badger DB (dbPath)
Client->>Socket: connect to external ACME port
Socket->>Proxy: socket-activates proxy
Proxy->>Backend: forward requests (systemd-socket-proxyd)
Proxy->>Backend: ExecStartPre polls backend port (nc)
Backend->>gocryptfs: access decrypted dbPath
gocryptfs->>Badger: read/write Badger DB
Proxy->>Client: proxied responses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai regarding the auto-generated description:
Actually, This makes sense because:
|
|
I'll regenerate the summary with this context. The PR summary needs correction: (1) ✏️ Learnings added
✅ Actions performedSummary regeneration triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
modules/tags/acme.nix (1)
17-37: ⚡ Quick winAvoid materializing the unsealed DB key on disk.
The current
dbExtpasswrites the unsealed key into amktempfile under/runand thencats it. Even on tmpfs this is unnecessary disk exposure (and a SIGKILL leaves the file behind until reboot).tpm_unsealdatawrites to stdout by default, so the helper can be a singleexecthat streams the key directly without touching the filesystem.♻️ Proposed refactor
dbExtpass = pkgs.writeShellApplication { name = "step-ca-db-extpass"; runtimeInputs = with pkgs; [coreutils tpm-tools]; text = '' set -euo pipefail if [[ ! -s ${dbKeySealed} ]]; then echo "sealed DB key is missing (${dbKeySealed})" >&2 exit 1 fi - key="$(mktemp /run/step-ca-db-key.XXXXXX)" - cleanup() { - rm -f "$key" - } - trap cleanup EXIT - - tpm_unsealdata -z -i ${dbKeySealed} -o "$key" - cat "$key" + exec tpm_unsealdata -z -i ${dbKeySealed} ''; };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/tags/acme.nix` around lines 17 - 37, The helper currently materializes the unsealed DB key to a temporary file; update the dbExtpass script to avoid creating a temp file by removing mktemp, cleanup(), trap, and cat, and instead directly exec tpm_unsealdata -z -i ${dbKeySealed} so the command streams the key to stdout (retain the initial existence check and set -euo pipefail); reference dbExtpass, dbKeySealed and tpm_unsealdata when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@modules/tags/acme.nix`:
- Around line 17-37: The helper currently materializes the unsealed DB key to a
temporary file; update the dbExtpass script to avoid creating a temp file by
removing mktemp, cleanup(), trap, and cat, and instead directly exec
tpm_unsealdata -z -i ${dbKeySealed} so the command streams the key to stdout
(retain the initial existence check and set -euo pipefail); reference dbExtpass,
dbKeySealed and tpm_unsealdata when making this change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 54a90818-1a97-4e3c-a5a2-dca50b3e5e68
📒 Files selected for processing (3)
docs/pki.mdmodules/tags/acme.nixmodules/tags/all.nix
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
modules/tags/acme.nix (1)
96-96: 💤 Low value
pathon these services is unused.For
step-ca-db-prepare, the script invokes gocryptfs vialib.getExe' pkgs.gocryptfs "gocryptfs"(absolute), sopath = [gocryptfs]doesn't contribute. Forstep-ca-proxy,ExecStartPreis a self-containedwriteShellApplicationwith its ownruntimeInputs, andExecStartuses the absolute${pkgs.systemd}/lib/systemd/systemd-socket-proxydpath — nothing consults the unit's$PATH. These can be dropped to reduce noise, or kept as a deliberate safety net if you prefer.♻️ Optional cleanup
- path = with pkgs; [gocryptfs]; unitConfig.StopWhenUnneeded = true;- path = with pkgs; [coreutils netcat-openbsd]; - serviceConfig = {Also applies to: 159-165
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@modules/tags/acme.nix` at line 96, The `path = with pkgs; [gocryptfs]` entries are dead/unused for the services `step-ca-db-prepare` and `step-ca-proxy` because `step-ca-db-prepare` calls gocryptfs via an absolute lib.getExe pkgs.gocryptfs invocation and `step-ca-proxy` uses a writeShellApplication with its own runtimeInputs and ExecStart pointing to ${pkgs.systemd}/lib/systemd/systemd-socket-proxyd, so remove the `path = [gocryptfs]` lines (or explicitly document why you keep them) from the service definitions to reduce noise; look for the `path` attribute in the `step-ca-db-prepare` and `step-ca-proxy` service blocks and delete it (ensure there are no other references to `path` in those units such as relying on unit $PATH or runtimeInputs before removing).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@modules/tags/acme.nix`:
- Around line 86-96: The hardcoded before = ["var-lib-step\\x2dca-db.mount"] in
the step-ca-db-prepare block is brittle and redundant because the mount unit
name is derived from dbPath and the mount unit already Requires/After
step-ca-db-prepare.service; remove the entire before attribute from the
step-ca-db-prepare declaration to avoid breakage when dbPath changes.
Alternatively, if you really want explicit bidirectional ordering, add utils to
the module args and change the before value to ["${utils.escapeSystemdPath
dbPath}.mount"] so the unit name is generated from dbPath (but prefer removing
the redundant before).
---
Nitpick comments:
In `@modules/tags/acme.nix`:
- Line 96: The `path = with pkgs; [gocryptfs]` entries are dead/unused for the
services `step-ca-db-prepare` and `step-ca-proxy` because `step-ca-db-prepare`
calls gocryptfs via an absolute lib.getExe pkgs.gocryptfs invocation and
`step-ca-proxy` uses a writeShellApplication with its own runtimeInputs and
ExecStart pointing to ${pkgs.systemd}/lib/systemd/systemd-socket-proxyd, so
remove the `path = [gocryptfs]` lines (or explicitly document why you keep them)
from the service definitions to reduce noise; look for the `path` attribute in
the `step-ca-db-prepare` and `step-ca-proxy` service blocks and delete it
(ensure there are no other references to `path` in those units such as relying
on unit $PATH or runtimeInputs before removing).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
1 participant
Summary by CodeRabbit
Documentation
New Features
Refactor