Add content seals and validation for generated CUDA Bindings files#2310
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test |
This comment has been minimized.
This comment has been minimized.
|
I'll come back to review this in a little bit, but let's merge #2316 to catch us up to the |
| f"MISMATCHED cybind-generated file seal in {filepath!r}: " | ||
| f"recorded {recorded_digest.decode()}, computed {computed_digest.decode()}" |
There was a problem hiding this comment.
This message might be a bit confusing for someone who doesn't understand the motivation here. Maybe:
"Manual changes detected in {filepath!r}. It is a generated file and should not be edited directly."
And then show the hashes if you want (though I don't know how useful they are to the user).
There was a problem hiding this comment.
Done. The failure now explains that a manual change was detected in a generated file and that the file should not be edited directly. I kept the recorded and computed hashes as secondary diagnostic information (e.g. if someone accidentally cuts off characters in the seal line, or parsing the seal line goes subtly wrong, that'd be pretty obvious).
| _MARKER_REGEX = re.compile( | ||
| rb"^(?P<prefix>#|\.\.) " | ||
| + re.escape(_TOKEN_BYTES) | ||
| + rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$" |
There was a problem hiding this comment.
I think the \n$ here means it might fail on Windows where the files could have \r\n line endings. Would this work just as well?
| + rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$" | |
| + rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})$" |
There was a problem hiding this comment.
I tested this on Windows, and it's actually fine. (However a bunch of other things in our pre-commit hooks are broken on Windows -- I'll file a separate issue for that).
There was a problem hiding this comment.
I think we should keep the explicit \n. Cybind normalizes generated content to LF before sealing and writes the sealed output as bytes, and cuda-python enforces * text eol=lf in .gitattributes.
Also, because the checker uses fullmatch, removing \n would reject the existing LF-terminated marker lines rather than make the expression line-ending agnostic. Supporting CRLF would require a coordinated change to the marker parsing and content-digest normalization, rather than this regex-only change.
| _MARKER_REGEX = re.compile( | ||
| rb"^(?P<prefix>#|\.\.) " | ||
| + re.escape(_TOKEN_BYTES) | ||
| + rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$" |
There was a problem hiding this comment.
Not strictly necessary because we control both ends, but users would probably expect arbitrary amounts of space between the keys.
| + rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$" | |
| + rb" *format=(?P<format>[0-9]+) *; *content-sha256=(?P<digest>[0-9a-f]{64})\n$" |
There was a problem hiding this comment.
I'd prefer to keep one canonical spelling here. We control both the producer and consumer, and this is a new machine-generated format rather than something people are expected to author manually. Strict spacing catches unexpected format drift early and also keeps ad-hoc tooling, including simple grep-based inspection, straightforward and predictable.
| + re.escape(_TOKEN_BYTES) | ||
| + rb" format=(?P<format>[0-9]+); content-sha256=(?P<digest>[0-9a-f]{64})\n$" | ||
| ) | ||
| _SOURCE_SUFFIXES = frozenset({".py", ".pxd", ".pxi", ".pyx"}) |
There was a problem hiding this comment.
This is basically used to get a comment character for specific file types. To make this more extensible for more filetypes, I'd just be super explicit:
_COMMENT_CHARS = {
".py": b"#",
".pxd": b"#",
".pxi": b"#",
".pyx": b"#",
".pyx.in": b"#",
".pxd.in": b"#",
".pxi.in": b"#",
".rst": b"..",
".c": b"//",
".cpp": b"//",
".h": b"//",
}
| marker_indexes = [index for index, line in enumerate(lines) if _TOKEN_BYTES in line] | ||
|
|
||
| if not marker_indexes: | ||
| if normalized_path in previously_sealed_paths: |
There was a problem hiding this comment.
It's probably not worth fixing, but worth noting that if a user renames and edits a file in the same commit, it will not get flagged.
There was a problem hiding this comment.
As long as the renamed file is staged, a rename followed by an edit is still detected: the seal moves with the file, and the changed content no longer matches its recorded digest.
This pre-commit check is is meant to be an integrity check for ordinary accidental changes. I don't think we should expand the scope beyond that. A rename of a generated file is probably not an accident. Guarding against removing a seal or forging one is also out of scope.
|
|
||
| def load_previously_sealed_paths(): | ||
| process = subprocess.run( # noqa: S603 | ||
| ["git", "grep", "-l", "-I", "-F", GENERATED_FILE_SEAL_TOKEN, "HEAD", "--"], # noqa: S607 |
There was a problem hiding this comment.
No point in grepping outside of cuda_bindings (since the pre-commit config limits it there as well).
| ["git", "grep", "-l", "-I", "-F", GENERATED_FILE_SEAL_TOKEN, "HEAD", "--"], # noqa: S607 | |
| ["git", "grep", "-l", "-I", "-F", GENERATED_FILE_SEAL_TOKEN, "HEAD", "--", "cuda_bindings"], # noqa: S607 |
This reverts commit 94e42e8.
This reverts commit cc82446.
|
|
||
| # <<<< PREAMBLE CONTENT >>>> | ||
|
|
||
| # CYTHON-BINDINGS-GENERATED-DO-NOT-MODIFY-THIS-FILE: format=1; content-sha256=6e7ac86c22e602c08df8250f3b50c135945378aa8ae4ddb4e10174fd979c4aa5 |
There was a problem hiding this comment.
Slightly weird placement here (but not worth blocking on).
There was a problem hiding this comment.
I agree this isn't pretty, but yes, it'll be great to keep to ball rolling.
There was a problem hiding this comment.
FYI — I'm taking another look, because I want to retest anyway (after merging the cybind mainline into MR 460).
There was a problem hiding this comment.
I found a nice solution that's already pushed to MR 460. 6 files are changed here:
- commit 9e91f7a
While working on that, I discovered that grep -l -F CYTHON-BINDINGS-GENERATED-DO-NOT-MODIFY-THIS-FILE matched toolshed/check_generated_file_seals.py, so I fixed that, too:
- commit 89bccba
I'm holding off triggering the CI again until #2323 is merged.
|
|
||
| if [[ "${OS:-}" == "Windows_NT" ]]; then | ||
| nvcc -lib -o "${SCRIPTPATH}/saxpy.lib" "${SCRIPTPATH}/saxpy.o" | ||
| "${NVCC}" -lib -o "${SCRIPTPATH}/saxpy.lib" "${SCRIPTPATH}/saxpy.o" |
There was a problem hiding this comment.
Seems unrelated? Maybe accidentally brought over from another branch?
There was a problem hiding this comment.
See my comment under 2281:
It's a really tiny fix, I hope to avoid creating a separate PR just for that by piggy-backing here.
….0 ../cuda-python Before running cybind: rm $(git grep -l -F CYTHON-BINDINGS-GENERATED-DO-NOT-MODIFY-THIS-FILE)
mdboom
left a comment
There was a problem hiding this comment.
LGTM. Now that the generator side of this has been merged, we should probably regenerate one more time over here (probably a no-op) and then assuming that's clean, let's merge this.
|
Summary
toolshed/check_generated_file_seals.pyto validate seal syntax, format, comment style, and the recorded SHA-256 digest.pre-commitfor text files undercuda_bindings/.Each seal contains a machine-readable generated-file marker and a SHA-256 digest of the complete file content excluding the seal line itself. This makes ordinary manual edits detectable while also providing a consistent marker for generated-file discovery. The digest is an integrity check for accidental changes, not a cryptographic attestation of provenance.
Dependency
The generated files in this PR were produced by cybind MR 460, which implements the producer side of the seal format. This PR implements the corresponding CUDA Python consumer-side validation and should remain a draft until that format is finalized.
Relation to PR 2297
PR 2297 proposes removing the license-era
CI: Restricted Paths Guard; its discussion also asks what should replace that workflow's incidental protection against manual edits to generated files.For that generated-file protection, a separate GitHub Actions workflow is unnecessary. This PR puts the check in the repository's existing pre-commit suite. A manual edit changes the content without updating its seal, the seal hook fails, and the required pre-commit CI check blocks the PR from merging. The same rule applies to every contributor, without employee-detection logic or path-specific review labels.
The seal is intentionally an integrity check, not an authorization boundary. If the project retains a separate policy that only NVIDIA employees may modify particular paths, that policy would require separate enforcement; it is not needed to detect ordinary manual changes to cybind-generated files.
Example failure
After manually modifying a sealed generated file, pre-commit reports:
Unrelated piggy-backed tiny fix (to avoid making a separate PR)