Add installation instructions for the provider as an RPM in AL2023 - #267
Add installation instructions for the provider as an RPM in AL2023#267derik01 wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #267 +/- ##
=======================================
Coverage 86.35% 86.35%
=======================================
Files 34 34
Lines 9422 9422
Branches 9422 9422
=======================================
Hits 8136 8136
Misses 1153 1153
Partials 133 133 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Claude finished @derik01's task in 45m 57s —— View job Code review: RPM installation instructions for AL2023
Reviewed with The structural problemThere is no RPM packaging anywhere in this repo — no So this section becomes the only specification for behavior nothing in this repo implements or tests, and the Blocking
Should fix
Nits / consistency
What checks out (verified, not flagged)Package/user/group names ( Recommendation: items 1-3 would each break a user following these instructions, so I'd hold the merge on those. Item 1 in particular — the ACM path can't work until No code changes made; this is review feedback only. |
![secrets-manager-ai-code-review[bot]](https://avatars.githubusercontent.com/in/4081740?s=60&v=4){kind=small}
| To enable the Certificate Management capability, run the ACM setup script: | ||
|
|
||
| ```sh | ||
| sudo /opt/aws/workload-credentials-provider/bin/install-acm.sh --config /path/to/config.toml |
There was a problem hiding this comment.
Blocking: install-acm.sh does not exist.
grep -rn install-acm over the whole tree returns exactly one hit — this line. It is also absent from git history (git log --all --diff-filter=A -- '*install-acm*' is empty).
Per commit 1b4b5eb, the AL2023 spec "consumes files from aws_workload_credentials_provider_common/configuration/ directly", and that directory has no such script. The only two files the repo ever places in bin/ are configuration/install:86-87:
install -D -T -m 755 "${PROVIDER_SOURCE_DIR}/${PROVIDER_BIN}" "${PROVIDER_DIR}/bin/${PROVIDER_BIN}"
install -D -T -m 755 "${TOKEN_SCRIPT}" "${PROVIDER_DIR}/bin/${TOKEN_SCRIPT}"So an RPM user who follows this step gets No such file or directory, and Certificate Management is unreachable on the RPM path — there is currently no ACM-only code path anywhere in the repo (install:99-105 installs the ACM unit as part of the single combined install).
The four flags listed just below (202-205) are verbatim the flags of configuration/install (install:20-26), which suggests this section was written against a planned script. Either land that script in configuration/ in this PR, or drop the ACM subsection and point RPM users at the documented acm reload / install path until it exists.
Line 189 ("Installs the binary and helper scripts", plural) reinforces the impression that this ships.
|
|
||
| Log out and back in (or run `newgrp awscreds`) for the group change to take effect\. | ||
|
|
||
| **Note:** To uninstall, run `sudo dnf remove aws-workload-credentials-provider`\. Uninstall stops all services and removes binaries and service units\. The `aws-wcp` user, groups, and logs directory are preserved\. |
There was a problem hiding this comment.
The uninstall note is the opposite of the repo's own teardown, and it omits the one thing that is preserved.
configuration/uninstall does exactly the reverse of what this note promises:
rm -rf "${PROVIDER_DIR}" # :40 — /opt/aws/workload-credentials-provider, which *contains* logs/
userdel "${PROVIDER_USER}" # :47
groupdel "${TOKEN_GROUP}" # :48
groupdel "${PROVIDER_GROUP}" # :49The logs directory lives inside ${PROVIDER_DIR} (setup-permissions.sh:35-37), so it is destroyed, not preserved. Meanwhile uninstall:52 prints Note: ${CONFIG_DIR} was preserved — /etc/aws-workload-credentials-provider/ survives, which this note doesn't mention even though line 193 tells RPM users to hand-author that file. That's the classic %config(noreplace) question and it's the one a reader most needs answered.
Three more things dnf remove cannot do, because they're created after install and so aren't in the RPM manifest:
/etc/sudoers.d/aws-workload-credentials-provider(config/sudoers.rs) — a passwordless-sudo grant left behind on a host where (per this same note) theaws-wcpaccount is preserved/etc/systemd/system/aws-workload-credentials-provider-acm.service.d/cert-paths.conf(config/systemd.rs)- the ACM unit itself, if it was installed by the separate step at line 198 — leaving an enabled unit whose
ExecStartbinary was just deleted, failing203/EXECunderRestart=on-failure
So "stops all services and removes binaries and service units" is not accurate for an ACM-enabled host. Please reconcile this paragraph with the actual %preun/%postun scriptlets, and add a membership-revocation hint (gpasswd -d <APP_USER> aws-wcp-token) since the groups are said to persist.
| ``` | ||
|
|
||
| The ACM setup script accepts the following options: | ||
| - `--config <file>` — Configuration file with ACM certificate entries |
There was a problem hiding this comment.
--config is a destructive copy, not "a configuration file with ACM certificate entries".
The only implementation of this flag in the repo (configuration/install:21 usage text and install:115):
--config <file> Bootstrap config will be copied to ${CONFIG_DIR}
install -T -m 440 -o "root" -g "${PROVIDER_GROUP}" "${CONFIG_FILE}" "${CONFIG_DIR}/config.toml"It overwrites /etc/aws-workload-credentials-provider/config.toml — the exact file line 193 just told the reader to create or edit. Follow this section in order and you silently lose every Secrets Manager setting you wrote nine lines earlier, and the ACM-only file becomes what the SM service reads on its next start.
Passing the default path to dodge that doesn't work either: source and destination become the same file, GNU install refuses, and under #!/bin/bash -e the script aborts partway through. main.rs:149-159 already carries an explicit guard and comment for this same hazard on the acm reload path, so it's a known footgun on this codebase.
At minimum the description should say the file is copied to the configuration directory and must be the complete config, not just ACM entries. Note the pre-existing Option B wording at line 235 already gets this right — the two descriptions of the same flag now contradict each other 33 lines apart.
| - Installs and starts the Secrets Manager and token systemd services | ||
| - Generates a random SSRF token at `/var/run/awssmatoken` | ||
|
|
||
| The Secrets Manager capability is available immediately after install\. When you install with the RPM package, the provider reads configuration options from the default path `/etc/aws-workload-credentials-provider/config.toml`\. To customize the configuration, create or edit that file\. |
There was a problem hiding this comment.
"create or edit that file" is missing two things that each produce a silent failure.
1. No restart step, and Option A never names a unit. aws-workload-credentials-provider-sm.service:15 is ExecStart=... sm start with no --config, and the config is parsed once at startup (main.rs:94-96, resolve_config_path at main.rs:418-439) — there's no watcher (no notify/inotify in any Cargo.toml). Because the RPM already started the service (line 190), an edit here has zero effect until systemctl restart aws-workload-credentials-provider-sm, and that unit name appears nowhere in Option A. Compounding it, README:14/29/728 all instruct --config on the command line, which is inapplicable to a systemd-managed install, and README:852 explicitly says acm reload "does not affect the Secrets Manager process or configuration". Please add a restart step (and ideally a systemctl status verification step — the linked AWS page has one).
2. No ownership/mode guidance. The service runs as User=aws-wcp (sm.service:9) and config loading is a bare read with no permission fixing. Every automated path sets 0440 root:awscreds (install:115; re-applied at main.rs:160-165). A root-created file under the default umask in a 0755 dir is 0644 and happens to work; a sudo install -m 600 or chmod 600 one gives aws-wcp EACCES → main.rs:45-48 exit(1) → crash loop under Restart=on-failure, visible only in journald. Worth stating the expected mode/owner explicitly.
(The rest of this sentence checks out: sm start genuinely works with no config file — resolve_config_path returns None and validate(None) yields SM enabled on 2773.)
| 3. To read provider logs, add your user to the `awscreds` group: | ||
|
|
||
| ```sh | ||
| sudo usermod -aG awscreds <APP_USER> |
There was a problem hiding this comment.
awscreds is the daemon's primary group, not a log-reader group — this grant is much broader than the sentence implies.
setup-permissions.sh:24 creates the service account with awscreds as its primary group:
useradd -r -M -d "${PROVIDER_DIR}" -s /sbin/nologin -g "${PROVIDER_GROUP}" -G "${TOKEN_GROUP}" "${PROVIDER_USER}"so membership also confers:
- read on
/etc/aws-workload-credentials-provider/config.toml, installed0440 root:awscreds(install:115, re-applied atmain.rs:160-165) — deliberately not world-readable, and it holds role ARNs, certificate paths and refresh commands - read on anything the services write, since no unit sets
Group=. That includes ACM output: unixPermissionConfigexposes onlymode, no owner/group (config/types.rs), so a config withkey_permission = { mode = "640" }puts exported TLS private keys in groupawscreds— readable by every member added on this instruction. That contradicts README:962 ("Private key files are written with0600permissions by default").
Two smaller issues in the same step:
- The prose says "add your user" but the command reuses
<APP_USER>from step 2 — different principals. A reader following the command literally puts the application service account into the provider's own file-ownership group, collapsing the token-group/provider-group separation the design depends on. (<APP_USER>is also never defined in Option A; the definition lives at line 242, in Option B, which an RPM user never reads.) - The log path is never given (
/opt/aws/workload-credentials-provider/logs/), so the step isn't actionable.
Suggest scoping this to an operator account, naming the log directory, and — like the comparable passages at README:27, 415 and 722 — linking to Security considerations. Reading logs via sudo avoids the grant entirely.
| 2. To allow your application to read the SSRF token file, add the application's user account to the `aws-wcp-token` group: | ||
|
|
||
| ```sh | ||
| sudo usermod -aG aws-wcp-token <APP_USER> |
There was a problem hiding this comment.
This step is missing the group-activation caveat that the next step gets — and it's the step where it matters.
Line 221 tells the reader to "Log out and back in (or run newgrp awscreds)" after the awscreds grant, but the identical requirement is unstated here, for the grant that actually gates whether the application can call the provider at all (token file is root:aws-wcp-token 0640 per aws-workload-credentials-provider-token:9-11).
For the case this step is about, neither remedy works: usermod -aG doesn't affect running processes, and newgrp is useless for a daemon and impossible for a /sbin/nologin account. The application service must be restarted. As written, a reader adds the user, sees Permission denied on /var/run/awssmatoken, and has nothing to go on.
Also worth carrying over from README:415 — "any user with access to the compute environment and SSRF token can access the secret from the Workload Credentials Provider cache". Operators granting this should know it's full read on every cached secret, including cross-account reach via roleArn.
Separately: the token value is captured once at SM startup (aws_secretsmanager_provider/src/server.rs:54), while the token unit mints a fresh value on every ExecStart and deletes it on ExecStop. The units declare Requires=/After= but not PartOf=/BindsTo=, so restarts don't propagate — any dnf upgrade (or manual token-service restart) that doesn't also restart the SM unit rejects every request with 403 until it is. If upgrade scriptlets restart the token unit, that ordering needs to be handled in the spec and probably noted here.
|
|
||
| The ACM setup script accepts the following options: | ||
| - `--config <file>` — Configuration file with ACM certificate entries | ||
| - `--no-start` — Install the ACM service but don't start it |
There was a problem hiding this comment.
--no-start doesn't keep the service from running — it only skips the immediate start.
configuration/install:142-155 enables all units outside the SKIP_START guard:
systemctl enable "${TOKEN_SCRIPT}" > /dev/null
systemctl enable "${SM_SERVICE}" > /dev/null
systemctl enable "${ACM_SERVICE}" > /dev/null
if [ "${SKIP_START}" = false ]; then
systemctl start ...
else
echo "Services enabled, start skipped (--no-start)"
fiSo the ACM unit starts on the next boot. An operator who uses --no-start to stage an install — before certificates, IAM permissions, or the final config are ready — gets it running unattended after a reboot, writing certificate/key files and executing the configured refresh_command through sudo -n. Worth saying "don't start it now" and noting the unit is still enabled.
Also missing from this subsection: the config must contain [capabilities.acm] with enabled = true (defaults to false, README:840). Without it both setup-config-based-permissions (main.rs:247-252) and acm start (main.rs:55-63) print a skip message and return Ok — and since the unit is Type=simple, that shows as inactive (dead) rather than failed. No sudoers file, no drop-in, no certificate, and no error anywhere. README:715 states the requirement, but Option A never cross-links it.
|
|
||
| **Option A: RPM package (AL2023)** | ||
|
|
||
| 1. Install the RPM package\. The package manager finds the latest version for you: |
There was a problem hiding this comment.
No repository-enablement step, and "finds the latest version for you" doesn't hold on AL2023.
Two gaps before this command can work:
- Where does the package come from? Nothing in this repo builds or publishes an RPM — no
.spec, no%post/%files, no nfpm/fpm/cargo-generate-rpm, andgrep -rn 'rpm\|dnf' .github/workflows/returns nothing.release.ymlandstaging.ymlupload raw per-target binaries to S3. On a stock AL2023 host this isNo match for argument: aws-workload-credentials-providerunless a repo is configured first. Please document the repo-enablement step (or state that the package ships in the AL2023 core repos, if that's the case). - "the latest version" — AL2023 pins repositories to a release snapshot for deterministic upgrades, so
dnf installyields whatever is in the instance's snapshot, not the newest build. That sits oddly next to README:79's advice to pin an exact tag when building from source. Consider dropping the sentence or replacing it withdnf upgrade/--releasever=latestguidance plus a way to check the installed version.
More broadly: since the spec lives outside this repo (per 1b4b5eb, it "consumes files from aws_workload_credentials_provider_common/configuration/ directly"), the four bullets at 187-191 are the only specification for scriptlet behavior, with nothing in CI to keep them honest — the .crux_template.md:97 checklist item ("Changes under configuration/ are RPM-compatible") isn't verifiable by a reviewer who can't see the spec. Worth a sentence saying where the spec lives.
Two precision nits in those bullets:
- Line 189: files go to
/opt/aws/workload-credentials-provider/**bin/**(install:86-87), which line 198 already assumes; and there's one helper script, not "scripts". - Line 191: the token is generated at service start, not by the package (
aws-workload-credentials-provider-token,startcase) — line 240 phrases this correctly as "on startup". - Line 188 could usefully state the group memberships the design depends on (
useradd -g awscreds -G aws-wcp-token), since that's the most spec-verifiable detail and its failure mode is an undiagnosablePermission deniedon the token file.
| The RPM package automatically: | ||
| - Creates the `aws-wcp` service user and `awscreds`/`aws-wcp-token` groups | ||
| - Installs the binary and helper scripts to `/opt/aws/workload-credentials-provider/` | ||
| - Installs and starts the Secrets Manager and token systemd services |
There was a problem hiding this comment.
Option A lists no prerequisites, and the service the RPM auto-starts crash-loops without resolvable AWS credentials.
validate_credentials defaults to true (config/types.rs:107), so sm start performs an STS GetCallerIdentity probe at startup (aws_secretsmanager_provider/src/utils.rs:166-176). A credentials-resolution failure is not classified transient — aws_secretsmanager_caching/src/error.rs:15-21 falls through to _ => false — so the error propagates, main.rs:45-48 exits 1, and the unit restarts forever under Restart=on-failure.
On an EC2 instance with no instance profile, an unresolvable region, or blocked IMDS, "Installs and starts" plus line 193's "available immediately after install" describes something that never comes up, with no diagnostic path offered. The published AWS page this section mirrors lists prerequisites (AWS credentials on the instance, systemd distro); Option A lists none.
Suggest a short prerequisites line before step 1 (instance profile / credentials, IAM permissions for the secrets being fetched, port 2773 free) and a systemctl status aws-workload-credentials-provider-sm verification step so a failed start is discoverable.
To be clear about what does work: starting with no config file at all is fine — resolve_config_path returns None and validate(None) yields Secrets Manager enabled on 2773. The gap is credentials, not config.
|
|
||
| The RPM package automatically: | ||
| - Creates the `aws-wcp` service user and `awscreds`/`aws-wcp-token` groups | ||
| - Installs the binary and helper scripts to `/opt/aws/workload-credentials-provider/` |
There was a problem hiding this comment.
The binary isn't on PATH, and every later command in this README assumes a source checkout.
The binary lands at /opt/aws/workload-credentials-provider/bin/aws-workload-credentials-provider (install:86 + common.sh:3) and nothing symlinks it into PATH. But README:728 and README:856 are written as relative invocations from a build tree:
./aws-workload-credentials-provider sm start --config /path/to/config.toml
sudo ./aws-workload-credentials-provider acm reload --config /path/to/config.toml
An RPM user gets command not found and can't reach the supported acm reload path — which matters, because acm reload is what re-runs setup-config-based-permissions, daemon-reload, and the service restart (main.rs:172-188). Hand-restarting the unit instead leaves the sudoers file and cert-paths.conf stale relative to the new config.
Option A gives an absolute path only for install-acm.sh (line 198). Either state the full binary path here, or have the package drop a symlink in /usr/bin and say so.
|
|
||
| **Note:** To uninstall, run `sudo dnf remove aws-workload-credentials-provider`\. Uninstall stops all services and removes binaries and service units\. The `aws-wcp` user, groups, and logs directory are preserved\. | ||
|
|
||
| **Option B: Install script (any Linux)** |
There was a problem hiding this comment.
"any Linux" overstates it, and Option A now duplicates most of Option B.
The install script hard-requires systemd, useradd/groupadd, and bash: install:142-151 calls systemctl daemon-reload/enable/start, setup-permissions.sh:21-24 calls groupadd/useradd, and both are #!/bin/bash -e with a fixed PATH. On a non-systemd or busybox distro it fails partway through — after creating users and copying binaries. The AWS page this README mirrors states the real constraint ("a Linux instance with systemd"). Suggest "Option B: Install script (systemd-based Linux)".
On duplication — three sets now say the same thing in slightly different ways, which is what will rot first:
- The four flags appear at 202-205, at 235-238, and in
configuration/install:20-25, with--configand--no-startdescribed differently in each. Option A also drops the "(Optional)" markers, implying--configis required. - Lines 209-213 duplicate 240-246 verbatim (
usermod -aG aws-wcp-token <APP_USER>), and 191 duplicates 240 (the token path). Both paths rely on the same token script and group, so this could be one shared "Grant your application access to the SSRF token" subsection placed after both options — which would also fix<APP_USER>being used at 212/218 but only defined at 242. - Option B still has no uninstall instructions, even though
configuration/uninstallexists and (per the comment on line 223) behaves very differently from what's documented for the RPM.
| #### [ Amazon EC2 (Linux) ] | ||
|
|
||
| **To install the Workload Credentials Provider** | ||
| You can install the Workload Credentials Provider using either the RPM package (available for AL2023) or the manual install script\. |
There was a problem hiding this comment.
Adding a second install path leaves three surrounding statements stale, and breaks the Step 1 → Step 2 narrative.
Stale text this diff didn't touch:
- Line 172 (just above): "The install script sets up both the Secrets Manager and Certificate Management capabilities" — false for Option A, which sets up SM only (line 190) and needs a separate script for ACM (line 195).
- Line 27 (Important callout): "The
installscript (Linux) orinstall.ps1script (Windows) configures these permissions automatically and is the recommended setup path". A reader who follows that on AL2023 runssudo ./installon top of an RPM install — re-runningsetup-permissions.shand overwriting packaged unit files with copies in/etc/systemd/system. - Line 962 (Security considerations): "the install script supports
--no-privilegesand--no-sudoersmodes" — now needs to coverinstall-acm.shtoo.
Step 1 is framed as an unconditional prerequisite. "Step 1: Build the Workload Credentials Provider binary" (line 73) precedes "Step 2: Install" (line 170), and this line is the first mention that a prebuilt package exists. Nothing tells RPM users to skip it, so the path now presented first routes them through rustup + cargo build --release for nothing — and they end up with an unmanaged binary in target/release alongside the RPM-managed one. One qualifying sentence at line 75 would fix it.
Deleted procedure heading. This diff removes **To install the Workload Credentials Provider**, making "Amazon EC2 (Linux)" the only Step 2 tab without one — compare line 251 (Windows EC2), 281 (Docker), 353 (Lambda). This README mirrors the generated AWS docs (escaped \., ------ tab delimiters, #### [ Tab ]), where that bold line is the procedure title, so dropping it for one tab breaks parallelism with the published page. Also, Option A numbers 1./2./3. where Option B directly below and the adjacent Windows tab use repeated 1..
Link text nit (line 207): the target page is titled "AWS Workload Credentials Provider", not "Certificate automation" — and notably that page documents only the build-from-source + sudo ./install flow, no RPM. A reader sent there for certificate guidance lands on instructions that contradict this section.
GitHub Actions
Waiting
2 participants
Description
Why is this change being made?
What is changing?
Related Links
Testing
How was this tested?
When testing locally, provide testing artifact(s):
Reviewee Checklist
Update the checklist after submitting the PR
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
If not, why:
Reviewer Checklist
All reviewers please ensure the following are true before reviewing:
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.