mirror of
https://github.com/rustfs/rustfs.git
synced 2026-05-22 14:36:57 +08:00
docs: add security advisory lessons skill (#2801)
Signed-off-by: 安正超 <anzhengchao@gmail.com> Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
113
.agents/skills/security-advisory-lessons/SKILL.md
Normal file
113
.agents/skills/security-advisory-lessons/SKILL.md
Normal file
@@ -0,0 +1,113 @@
|
||||
---
|
||||
name: security-advisory-lessons
|
||||
description: Apply RustFS security lessons distilled from repository GitHub Security Advisories. Use when making or reviewing RustFS code changes, doing security checks, handling PR review for auth/authz, IAM, storage, RPC, logging, CORS, console/browser, encryption, policy, or endpoint changes, and when deciding which security regression tests are required.
|
||||
---
|
||||
|
||||
# RustFS Security Advisory Lessons
|
||||
|
||||
Use this skill as a RustFS-specific security lens before changing or approving code. For the current advisory snapshot and full pattern map, read [advisory-patterns.md](references/advisory-patterns.md).
|
||||
|
||||
When currentness matters, refresh the advisory inventory first:
|
||||
|
||||
```bash
|
||||
gh api repos/rustfs/rustfs/security-advisories --paginate \
|
||||
--jq '.[] | {ghsa_id,state,severity,summary,updated_at,html_url}'
|
||||
```
|
||||
|
||||
For the full pattern map, read [advisory-patterns.md](references/advisory-patterns.md).
|
||||
|
||||
## Workflow
|
||||
|
||||
### 1. Scope the change
|
||||
- Identify touched routes, handlers, storage paths, credentials, logs, browser surfaces, CI/release code, and policy checks.
|
||||
- Treat these paths as security-sensitive by default: `rustfs/src/admin/`, `rustfs/src/storage/`, `rustfs/src/auth.rs`, `rustfs/src/server/layer.rs`, `crates/iam/`, `crates/policy/`, `crates/credentials/`, `crates/ecstore/src/rpc/`, `crates/rio/`, and console preview/auth code.
|
||||
|
||||
### 2. Map to advisory classes
|
||||
- Read [advisory-patterns.md](references/advisory-patterns.md) for matching GHSA lessons.
|
||||
- Do not rely on advisory titles alone. Confirm whether the issue is authentication, authorization, input validation, storage invariant, browser isolation, logging, or operational hardening.
|
||||
|
||||
### 3. Verify fail-closed behavior
|
||||
- Check that unauthenticated, wrong-permission, cross-user, cross-bucket, malformed-input, and default-config cases fail explicitly.
|
||||
- Prefer exact action/permission checks over broad helper calls or inferred ownership.
|
||||
- Confirm lower storage/RPC layers do not bypass checks done in upper layers.
|
||||
|
||||
### 4. Require regression evidence
|
||||
- For behavior changes, add focused negative tests that reproduce the advisory class.
|
||||
- For sensitive fixes, include tests for the bypass form, not only the happy path.
|
||||
- If a test is impractical, explain the residual risk and provide a manual verification command.
|
||||
|
||||
### 5. Report clearly
|
||||
- Lead with concrete findings and file/line evidence.
|
||||
- Separate proven vulnerabilities from hardening risks.
|
||||
- Avoid exaggerating unauthenticated impact when the code actually rejects unauthenticated requests but allows a low-privileged authenticated bypass.
|
||||
|
||||
## Advisory-Derived Guardrails
|
||||
|
||||
### Auth and admin authorization
|
||||
- Every admin or diagnostic route needs an explicit authn and authz story. Route registration, router whitelist, and handler-level authorization must agree.
|
||||
- Match the admin action to the operation exactly. Copy-paste action constants are a known RustFS vulnerability class.
|
||||
- Avoid authentication-only helpers for state-changing admin APIs; use `validate_admin_request` or the established equivalent with the right `AdminAction`.
|
||||
- Do not assume admin-action `Resource` scoping constrains blast radius unless the policy engine actually enforces resources for that action.
|
||||
|
||||
### IAM and service accounts
|
||||
- Treat imported IAM payload fields as attacker-controlled: `parent`, `claims`, `accessKey`, `secretKey`, status, policy names, and groups.
|
||||
- For service account create/update/import, prove parent ownership or root/admin authority before writing credentials or claims.
|
||||
- Do not let `deny_only` or "no explicit deny" become an allow decision that skips required allow checks.
|
||||
- Test cross-user list/update/import flows with wrong, correct, self, parent, and root identities.
|
||||
|
||||
### S3 copy, multipart, and presigned POST
|
||||
- Multipart copy must enforce source `GetObject` and destination `PutObject` semantics equivalent to `CopyObject`, including copy-source and policy conditions.
|
||||
- Do not let `CreateMultipartUpload`, `UploadPartCopy`, `CompleteMultipartUpload`, or `AbortMultipartUpload` return success without authorization.
|
||||
- Presigned POST policies are server-side contracts. Enforce `content-length-range`, key prefix, exact metadata/content-type, and all signed policy conditions.
|
||||
|
||||
### Paths, object keys, and filesystem access
|
||||
- Never join untrusted bucket/object/RPC path strings onto filesystem roots without normalization and boundary checks.
|
||||
- Reject or safely handle `..`, absolute paths, URL-encoded traversal, platform separators, empty components, and paths that canonicalize outside the intended root.
|
||||
- Validate both S3 object-key paths and internode/RPC disk paths; storage helpers can bypass S3 authorization if they trust already-parsed paths.
|
||||
|
||||
### Secrets, default credentials, and crypto
|
||||
- Do not ship hard-coded shared tokens, HMAC secrets, private keys, or production test keys.
|
||||
- Defaults for internode/RPC auth must fail closed for network-reachable deployments or require explicit opt-in with loud warnings.
|
||||
- License or token validation must use signatures with embedded public/verifying keys only; do not use private-key decryption as authenticity.
|
||||
- Plan key rotation and key IDs when removing exposed keys.
|
||||
|
||||
### Logging and debug output
|
||||
- Logs must never include access keys beyond safe identifiers, secret keys, session tokens, JWT claims, HMAC secrets, expected signatures, license secrets, or raw response bodies containing credentials.
|
||||
- Treat `Debug` implementations, `?value` tracing, merged config dumps, and dependency-level HTTP body logging as leak surfaces.
|
||||
- Add log-capture tests or targeted unit tests for redaction wrappers when changing credential structs or response bodies.
|
||||
|
||||
### RPC, parsing, and panic safety
|
||||
- Treat all RPC payload bytes as attacker-controlled. Replace `unwrap`, `expect`, and panic-prone deserialization with typed errors.
|
||||
- Malformed request tests should cover empty bytes, truncated MessagePack/protobuf, invalid enum values, stale timestamps, and invalid signatures.
|
||||
- RPC authentication must be independently strong; do not depend on S3 admin credentials unless the fallback is explicit and safe.
|
||||
|
||||
### Browser, CORS, and console surfaces
|
||||
- Do not reflect arbitrary `Origin` while also allowing credentials. Default CORS should be no CORS unless explicitly configured.
|
||||
- Do not render user-controlled object content in a same-origin iframe with console credentials available to JavaScript.
|
||||
- Prefer origin separation for object preview/download, `nosniff`, CSP, strict content-type handling, and avoiding durable credentials in `localStorage`.
|
||||
- License/version-like metadata endpoints should expose only coarse public data unless authenticated.
|
||||
|
||||
### Profiling, debug, and health endpoints
|
||||
- Profiling and debug endpoints are not health checks. They require admin auth, opt-in enablement, rate limiting, and safe responses.
|
||||
- Do not return absolute filesystem paths or other deployment layout in unauthenticated or low-privilege responses.
|
||||
- Ensure health endpoint allowlists cannot accidentally include expensive diagnostics.
|
||||
|
||||
### Trusted proxy and network identity
|
||||
- Only honor `X-Forwarded-For` or `X-Real-IP` when the request came from a configured trusted proxy.
|
||||
- Direct clients must use the socket peer address for `aws:SourceIp` and policy condition evaluation.
|
||||
- Add tests for direct spoofed headers and trusted-proxy headers.
|
||||
|
||||
### SSE and storage invariants
|
||||
- Encryption metadata is not proof that bytes were encrypted on disk.
|
||||
- When touching reader/writer wrappers such as hashing, encryption, compression, or warp readers, verify wrapper order and inspect stored bytes in regression tests.
|
||||
- Avoid helper shortcuts that unwrap nested readers and accidentally bypass encryption or integrity layers.
|
||||
|
||||
## Review Prompts
|
||||
|
||||
Use these prompts while reviewing a diff:
|
||||
|
||||
- Could a low-privileged authenticated user reach this path with the wrong action, parent, bucket, or source object?
|
||||
- Does a public/default/empty config change security behavior from fail-closed to fail-open?
|
||||
- Is any attacker-controlled value later used as a path, policy condition, credential identity, log field, URL, Origin, or response body?
|
||||
- Is the same operation implemented in multiple paths, such as `CopyObject` vs `UploadPartCopy`, and do all paths enforce the same security contract?
|
||||
- Does the test prove the exploit form is denied, or only that the intended form still works?
|
||||
@@ -0,0 +1,4 @@
|
||||
interface:
|
||||
display_name: "Security Advisory Lessons"
|
||||
short_description: "Apply advisory lessons in reviews."
|
||||
default_prompt: "Review code changes against past RustFS security advisory lessons and report concrete risks, missing tests, and recommended fixes."
|
||||
@@ -0,0 +1,85 @@
|
||||
# RustFS Advisory Pattern Map
|
||||
|
||||
Snapshot source: `gh api repos/rustfs/rustfs/security-advisories --paginate` on 2026-05-05. It included 23 advisories: 8 triage, 13 published, and 2 closed.
|
||||
|
||||
Refresh this file when new advisories appear or when an advisory changes state materially.
|
||||
|
||||
## Pattern Index
|
||||
|
||||
### Admin authorization and route exposure
|
||||
|
||||
- `GHSA-pfcq-4gjr-6gjm` published: notification target endpoints accepted authenticated users but skipped admin authorization. Lesson: distinguish authn from authz; admin target CRUD must call the operation-specific admin authorization path.
|
||||
- `GHSA-mm2q-qcmx-gw4w` published: `ListServiceAccount` used `UpdateServiceAccountAdminAction`, while update lacked target ownership checks. Lesson: exact action constants and ownership checks are both required; information disclosure can chain into secret rotation and takeover.
|
||||
- `GHSA-vcwh-pff9-64cc` published: `ImportIam` checked `ExportIAMAction` for an import/write operation. Lesson: every admin handler must authorize the action it actually performs.
|
||||
- `GHSA-jqmc-mg33-v45g` triage and `GHSA-8784-9m7f-c6p6` triage: `/profile/cpu` and `/profile/memory` were whitelisted from auth and allowed expensive diagnostics plus path disclosure. Lesson: profiling/debug endpoints need admin auth, opt-in, rate limits, and non-sensitive responses.
|
||||
- `GHSA-x5xv-223c-8vm7` triage: console license metadata endpoint was public. Lesson: public metadata endpoints should be coarse or authenticated.
|
||||
|
||||
### IAM import, service accounts, and privilege boundaries
|
||||
|
||||
- `GHSA-566f-q62r-wcr8` triage: `ImportIam` accepted attacker-controlled service account `parent`, `claims`, `accessKey`, and `secretKey`, enabling persistent backdoor accounts under root. Lesson: imported IAM payloads are untrusted data and must be validated against privilege boundaries.
|
||||
- `GHSA-xgr5-qc6w-vcg9` published: `deny_only=true` skipped allow checks and let restricted service accounts mint unrestricted children. Lesson: deny-only logic must never become implicit allow for privilege creation.
|
||||
- `GHSA-mm2q-qcmx-gw4w` published: leaked service account access keys plus update-without-ownership formed an escalation chain. Lesson: service-account identifiers are security-sensitive because update APIs consume them.
|
||||
|
||||
### S3 copy, multipart, and upload policy validation
|
||||
|
||||
- `GHSA-mx42-j6wv-px98` published: `UploadPartCopy` missed source authorization and allowed cross-bucket object exfiltration. Lesson: multipart copy must enforce the same source and destination contract as `CopyObject`.
|
||||
- `GHSA-wfxj-ph3v-7mjf` triage: `UploadPartCopy` checked source and destination independently but missed destination copy-source policy constraints. Lesson: source read and destination write checks are not sufficient when policy constrains allowed copy sources.
|
||||
- `GHSA-w5fh-f8xh-5x3p` published: presigned POST accepted uploads without enforcing signed policy conditions. Lesson: parse and enforce all POST policy constraints server-side, including size, key prefix, and content type.
|
||||
|
||||
### Filesystem paths and object key traversal
|
||||
|
||||
- `GHSA-pq29-69jg-9mxc` published: RPC `read_file_stream` joined untrusted paths under a volume directory without canonical boundary checks. Lesson: `PathBuf::join` plus length checks are not path security.
|
||||
- `GHSA-8r6f-hmq2-28rg` closed: object keys containing traversal sequences bypassed bucket/object authorization when mapped to filesystem paths. Lesson: reject traversal at object-key parsing and verify final storage paths remain under the expected bucket/key root.
|
||||
|
||||
### Secrets, defaults, and cryptographic misuse
|
||||
|
||||
- `GHSA-h956-rh7x-ppgj` published: gRPC used the hard-coded token `rustfs rpc` on both client and server. Lesson: source-visible shared tokens are authentication bypasses.
|
||||
- `GHSA-r5qv-rc46-hv8q` triage: internode RPC HMAC secret fell back to the public default `rustfsadmin`. Lesson: RPC/internode auth must fail closed instead of silently using public defaults.
|
||||
- `GHSA-923g-jp7v-f97f` triage: license verification embedded a production RSA private key and used private-key decryption as authenticity. Lesson: ship verifying/public keys only and use real signature verification.
|
||||
|
||||
### Sensitive logging and debug output
|
||||
|
||||
- `GHSA-r54g-49rx-98cr` published: STS credentials were logged at info level. Lesson: generated credentials must never be logged in plaintext.
|
||||
- `GHSA-8cm2-h255-v749` triage: debug logs leaked session tokens, secret keys, JWT claims, and raw STS response bodies. Lesson: redaction must cover custom `Debug` implementations and dependency response-body logging.
|
||||
- `GHSA-333v-68xh-8mmq` published: invalid RPC signature logging included the shared HMAC secret and expected signature. Lesson: error paths often leak secrets; never log raw secrets or derived authenticators.
|
||||
|
||||
### RPC input validation and panic safety
|
||||
|
||||
- `GHSA-gw2x-q739-qhcr` published: malformed gRPC `GetMetrics` payloads reached `unwrap()` on deserialization and caused remote DoS. Lesson: every network/RPC deserialization failure returns an error, not a panic.
|
||||
- `GHSA-h956-rh7x-ppgj` published and `GHSA-r5qv-rc46-hv8q` triage: weak RPC auth increased reachability of otherwise internal handlers. Lesson: panic bugs become more severe when internode auth is weak or defaulted.
|
||||
|
||||
### Browser, CORS, and console isolation
|
||||
|
||||
- `GHSA-v9fg-3cr2-277j` published: object preview rendered attacker-controlled HTML in a same-origin iframe, exposing console credentials stored in `localStorage`. Lesson: user content must be origin-isolated from the console and protected with `nosniff`, CSP, and strict content-type handling.
|
||||
- `GHSA-x5xv-223c-8vm7` triage: default CORS reflected arbitrary origins with credentials. Lesson: never combine reflected origins with `Access-Control-Allow-Credentials: true`; default should be fail-closed.
|
||||
|
||||
### Trusted proxy and source IP conditions
|
||||
|
||||
- `GHSA-fc6g-2gcp-2qrq` published: `aws:SourceIp` trusted client-supplied `X-Forwarded-For` or `X-Real-IP`. Lesson: forwarded IP headers are valid only behind configured trusted proxies; direct clients use socket peer IP.
|
||||
|
||||
### SSE and on-disk storage invariants
|
||||
|
||||
- `GHSA-xrrf-67jm-3c2r` closed: SSE metadata reported encryption while reader composition bypassed `EncryptReader` and stored plaintext. Lesson: test actual bytes on disk and wrapper order, not only API metadata.
|
||||
|
||||
## Useful Search Seeds
|
||||
|
||||
Use these targeted searches when a diff touches security-sensitive code:
|
||||
|
||||
```bash
|
||||
rg -n "validate_admin_request|check_permissions|AdminAction::|deny_only|is_allowed" rustfs crates
|
||||
rg -n "UploadPartCopy|upload_part_copy|CompleteMultipart|PostObject|content-length-range|starts-with" rustfs crates
|
||||
rg -n "PathBuf::join|canonicalize|\\.\\.|x-forwarded-for|x-real-ip|SourceIp" rustfs crates
|
||||
rg -n "DEFAULT_SECRET|DEFAULT_ACCESS|TEST_PRIVATE_KEY|rustfs rpc|RUSTFS_RPC_SECRET" rustfs crates
|
||||
rg -n "debug!|trace!|info!|error!|\\?resp|\\?merged_config|session_token|secret_key" rustfs crates
|
||||
rg -n "HashReader|EncryptReader|SSE|server-side encryption|Access-Control-Allow-Credentials|Origin" rustfs crates
|
||||
```
|
||||
|
||||
## Minimum Regression Test Expectations
|
||||
|
||||
- Authz fixes: include unauthenticated, valid low-privilege, wrong-action, correct-action, owner, non-owner, and root/admin cases as applicable.
|
||||
- IAM fixes: include import/update/list service-account cases with attacker-controlled parent, claims, access key, secret key, and policy.
|
||||
- Copy/upload fixes: include cross-bucket, cross-user, source-denied, destination-denied, copy-source-condition, and multipart completion cases.
|
||||
- Path fixes: include encoded traversal, absolute path, nested traversal, valid object keys that resemble traversal text but should be rejected, and canonical boundary checks.
|
||||
- Logging fixes: assert redacted output for structs and response bodies that may contain credentials.
|
||||
- Browser/CORS fixes: assert no credentials on reflected/default origins, correct behavior for explicit allowlists, and no same-origin script execution for previewed object content.
|
||||
- SSE fixes: inspect stored bytes and verify API metadata, read-back behavior, and on-disk ciphertext together.
|
||||
Reference in New Issue
Block a user