fix: harden Slurm release artifacts - #916
Conversation
Greptile SummaryThe PR hardens Slurm release artifacts and caller-visible command diagnostics.
|
| Filename | Overview |
|---|---|
| packages/data-designer-slurm/src/data_designer/slurm/security.py | Adds centralized credential redaction and resolves the previously reported assignment-boundary and underscore-option gaps. |
| packages/data-designer-slurm/src/data_designer/slurm/launcher/client.py | Redacts normalized scheduler diagnostics before applying the existing caller-visible length bound. |
| scripts/audit_slurm_public_artifacts.py | Adds fail-closed scanning for public files and archives, including byte-exact canonical wheel-license verification. |
| packages/data-designer-slurm/tests/launcher/test_client.py | Covers assignment, option, quoting, separator, authorization, URL, and provider-token redaction through the command-error path. |
| packages/data-designer-slurm/tests/test_public_artifacts.py | Exercises sensitive-content reporting, archive safety, SPDX checks, and truncated-license rejection. |
| packages/data-designer-slurm/pyproject.toml | Includes the canonical Apache-2.0 license in built Slurm distributions. |
Reviews (7): Last reviewed commit: "redact underscore secret options" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _ASSIGNMENT_PATTERN = re.compile( | ||
| r"(?P<prefix>(?P<quote>[\"']?)(?P<name>-{0,2}[A-Za-z][A-Za-z0-9_.-]*)" | ||
| r"(?P=quote)\s*[:=]\s*)(?P<value>\"[^\"]*\"|'[^']*'|[^\s,;]+)" | ||
| ) |
There was a problem hiding this comment.
When scheduler stderr contains a secret assignment such as HF_TOKEN=abc;def or HF_TOKEN=abc,def, the value pattern stops at the delimiter and only redacts the prefix, exposing the remaining credential characters in SlurmCommandError. How this was verified: The assignment regex excludes commas and semicolons while the resulting text is passed directly into caller-visible error details.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/security.py
Line: 12-15
Comment:
**Assignment redaction leaks suffixes**
When scheduler stderr contains a secret assignment such as `HF_TOKEN=abc;def` or `HF_TOKEN=abc,def`, the value pattern stops at the delimiter and only redacts the prefix, exposing the remaining credential characters in `SlurmCommandError`. **How this was verified:** The assignment regex excludes commas and semicolons while the resulting text is passed directly into caller-visible error details.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Addressed in 701b591. Unquoted secret assignments now use whitespace as the value boundary, so comma/semicolon suffixes are redacted while subsequent diagnostic fields remain visible. Regression coverage includes both delimiters and preservation of status=failed. Validation: 52 focused tests passed; full Slurm suite passed (1,205 tests); lint, format, complexity, SPDX, source scan, and wheel-install/artifact audit all passed.
There was a problem hiding this comment.
Addressed on current restacked head in ee11baa. The residual assignment-looking suffix case was valid: unquoted secret values now fail closed through the first whitespace, so HF_TOKEN=secret;part=value and comma variants cannot expose ambiguous suffix bytes. Quoted values retain exact quote boundaries, which preserves following diagnostics when the boundary is explicit. Regression coverage includes secret-first/middle/last, semicolon/comma/whitespace, assignment-looking suffixes, and quoted boundaries. Validation: 56 focused launcher/redaction tests and the full 1,257-test Slurm suite passed; lint, source scan, SPDX audit, diff check, and the isolated wheel workflow are also green. Please re-review current head.
| content = _read_bounded(stream, expected_size=member.file_size) | ||
| except (OSError, ValueError) as error: | ||
| return [AuditFinding(location, str(error))], False, False | ||
| contains_license = is_license and b"Apache License" in content and b"Version 2.0" in content |
There was a problem hiding this comment.
When a wheel contains a truncated or placeholder license with only Apache License and Version 2.0, this condition marks the license as present, causing an incomplete artifact to pass the provenance gate.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/audit_slurm_public_artifacts.py
Line: 222
Comment:
**License check accepts stubs**
When a wheel contains a truncated or placeholder license with only `Apache License` and `Version 2.0`, this condition marks the license as present, causing an incomplete artifact to pass the provenance gate.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Addressed in 701b591. Wheel license validation now requires byte-for-byte equality with the checked-in packages/data-designer-slurm/LICENSE; keyword-only or truncated stubs fail. The valid wheel fixtures now use canonical bytes, and a focused truncated-license regression test was added. Validation: 52 focused tests passed; full Slurm suite passed (1,205 tests); lint, format, complexity, SPDX, source scan, and wheel-install/artifact audit all passed.
| r"(?P<prefix>(?P<quote>[\"']?)(?P<name>-{0,2}[A-Za-z][A-Za-z0-9_.-]*)" | ||
| r"(?P=quote)\s*[:=]\s*)(?P<value>\"[^\"]*\"|'[^']*'|[^\s]+)" | ||
| ) | ||
| _OPTION_PATTERN = re.compile(r"(?P<prefix>(?P<name>--[A-Za-z][A-Za-z0-9.-]*)\s+)(?P<value>\"[^\"]*\"|'[^']*'|[^\s]+)") |
There was a problem hiding this comment.
When scheduler output contains a punctuation-delimited non-secret assignment followed by a secret assignment, such as status=failed;HF_TOKEN=secret, the greedy value match consumes both assignments and _redact_named_value returns them unchanged because status is not secret-bearing, exposing the token in SlurmCommandError.
How this was verified: The assignment pattern consumes through punctuation, while the replacement callback preserves the entire match when its first name is non-secret.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/security.py
Line: 16
Comment:
**Adjacent assignment leaks secret**
When scheduler output contains a punctuation-delimited non-secret assignment followed by a secret assignment, such as `status=failed;HF_TOKEN=secret`, the greedy value match consumes both assignments and `_redact_named_value` returns them unchanged because `status` is not secret-bearing, exposing the token in `SlurmCommandError`.
**How this was verified:** The assignment pattern consumes through punctuation, while the replacement callback preserves the entire match when its first name is non-secret.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Addressed in 413b708. Assignment redaction now uses a left-to-right boundary scanner instead of one overlapping value regex: comma and semicolon end a value only when they introduce another syntactic assignment, while whitespace ends unquoted values and quoted values are scanned as a unit. The regression matrix covers secret-first/middle/last placement across comma, semicolon, whitespace, and spaced punctuation, plus delimiter-bearing secret suffixes, and asserts neighboring diagnostics remain exact. Validation: 48 focused launcher tests and the full 1,214-test Slurm suite passed; lint, format, complexity, SPDX, source scan, and built-wheel isolation/artifact audit all passed.
There was a problem hiding this comment.
Addressed on current restacked head in ee11baa. The unknown-separator masking case was valid: non-secret matches no longer claim or skip their values, so scanning continues from the matched prefix and discovers later secret names in inputs such as status=failed|HF_TOKEN=secret and slash variants. The same small scanner protects secret assignments and options; authorization values fail closed to whitespace. The regression matrix covers secret-first/middle/last, standard and arbitrary separators, compact nested assignments, secret options, and preservation of unrelated diagnostics. Validation: 56 focused launcher/redaction tests and the full 1,257-test Slurm suite passed; lint, source scan, SPDX audit, diff check, and the isolated wheel workflow are also green. Please re-review current head.
413b708 to
e8a4c3c
Compare
| _ASSIGNMENT_START_PATTERN = re.compile( | ||
| r"(?P<prefix>(?P<quote>[\"']?)(?P<name>-{0,2}[A-Za-z][A-Za-z0-9_.-]*)(?P=quote)\s*[:=]\s*)" | ||
| ) |
There was a problem hiding this comment.
When scheduler stderr contains a quoted secret assignment with whitespace, such as HF_TOKEN="quoted secret value", the assignment prefix consumes the opening quote before boundary detection, so only the first word is redacted and secret value" remains visible in SlurmCommandError.
How this was verified: Tracing the quoted assignment through match.end() shows that _find_named_value_end starts on the first value character and stops at the first whitespace.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/security.py
Line: 12-14
Comment:
**Quoted secret suffix leaks**
When scheduler stderr contains a quoted secret assignment with whitespace, such as `HF_TOKEN="quoted secret value"`, the assignment prefix consumes the opening quote before boundary detection, so only the first word is redacted and `secret value"` remains visible in `SlurmCommandError`.
**How this was verified:** Tracing the quoted assignment through `match.end()` shows that `_find_named_value_end` starts on the first value character and stops at the first whitespace.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
This is a false positive on current head ee11baa. _ASSIGNMENT_START_PATTERN ends immediately after = for this input; its optional quote group surrounds a quoted name, not the value. A direct trace yields prefix="HF_TOKEN=", next="\"", so _find_named_value_end sees the opening value quote and delegates to _find_quoted_value_end. The public SlurmCommandClient error-path regression already exercises HF_TOKEN="quoted secret value" and returns exactly HF_TOKEN=<redacted>; the focused launcher suite is green (56 passed). Direct current-head probes also fully redact both quote styles, escaped quotes, and malformed unterminated quoted values. The unchanged ee11baa head previously passed the full Slurm suite (1,257 passed), lint/scans/SPDX, and isolated-wheel validation. No code change is warranted. Please re-review the current head with the value-quote position distinguished from the optional quoted-name group.
There was a problem hiding this comment.
Re-reviewed current head ee11baab and the full helper. You’re correct: for HF_TOKEN="quoted secret value", _ASSIGNMENT_START_PATTERN matches HF_TOKEN= and _find_named_value_end starts at the value’s opening quote, so _find_quoted_value_end consumes the entire quoted value. The optional quote group only surrounds the assignment name (e.g. 'HF_TOKEN'=...). The existing regression coverage also exercises this path, including escaped and unterminated quoted values. This finding is a false positive; no code change is warranted.
Add sanitized credential and infrastructure scans for source, fixtures, runtime archives, logs, and wheels. Include Apache license text in the Slurm wheel and redact sensitive scheduler diagnostics. Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
ee11baa to
1e0b50c
Compare
| _ASSIGNMENT_START_PATTERN = re.compile( | ||
| r"(?P<prefix>(?P<quote>[\"']?)(?P<name>-{0,2}[A-Za-z][A-Za-z0-9_.-]*)(?P=quote)\s*[:=]\s*)" | ||
| ) | ||
| _OPTION_START_PATTERN = re.compile(r"(?P<prefix>(?P<name>--[A-Za-z][A-Za-z0-9.-]*)\s+)") |
There was a problem hiding this comment.
When scheduler diagnostics contain an underscore-form secret option such as --api_key plaintext-secret, _OPTION_START_PATTERN does not match the option and the assignment scanner requires : or =, causing the plaintext value to remain visible in SlurmCommandError.
How this was verified: The option-name character class excludes underscores even though underscore-form names are recognized as secret-bearing.
| _OPTION_START_PATTERN = re.compile(r"(?P<prefix>(?P<name>--[A-Za-z][A-Za-z0-9.-]*)\s+)") | |
| _OPTION_START_PATTERN = re.compile(r"(?P<prefix>(?P<name>--[A-Za-z][A-Za-z0-9_.-]*)\s+)") |
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/data-designer-slurm/src/data_designer/slurm/security.py
Line: 15
Comment:
**Underscore options expose secrets**
When scheduler diagnostics contain an underscore-form secret option such as `--api_key plaintext-secret`, `_OPTION_START_PATTERN` does not match the option and the assignment scanner requires `:` or `=`, causing the plaintext value to remain visible in `SlurmCommandError`.
**How this was verified:** The option-name character class excludes underscores even though underscore-form names are recognized as secret-bearing.
```suggestion
_OPTION_START_PATTERN = re.compile(r"(?P<prefix>(?P<name>--[A-Za-z][A-Za-z0-9_.-]*)\s+)")
```
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Addressed in a633ce3. The finding was valid: the shared option-prefix grammar now accepts underscores as well as the existing hyphen/dot forms, so --api_key plaintext-secret is routed through the same centralized fail-closed value scanner. Public SlurmCommandClient error-path regressions cover underscore and hyphen options, mixed names, double- and single-quoted values, unknown-separator adjacency, punctuation-bearing suffixes, and exact preservation of a non-secret underscore option. Validation passed: 88 focused security/redaction/artifact/package tests, the full 1,301-test Slurm suite, Slurm format/lint, complexity checks, 13-target artifact scan, 752-file SPDX audit, diff check, and isolated wheel workflow. Please re-review current head.
Signed-off-by: Nabin Mulepati <nmulepati@nvidia.com>
1 participant
📋 Summary
Adds the dependency-ready security and provenance hardening slice for the optional Slurm package. This establishes reusable, sanitized scans and closes concrete diagnostic/license gaps without claiming the sealed-artifact release acceptance that remains blocked on the rest of the Slurm delivery.
🔗 Related Issue
Part of #870
🔄 Changes
plans/870/.🔍 Attention Areas
⛓️ Dependencies and remaining blockers
feat/slurm-executionthrough merged PR feat: finalize Slurm shard winners #910 (b98c043e). The four PR-owned commits were restacked onto this base without patch changes.🧪 Testing
1,301 passed88 passedgit diff --check✅ Checklist
Description updated with AI