Add standalone FIPS protected directory check tool#73
Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the FIPS protected directory change detection out of rolling-release-update.py into a reusable helper in kt/ktlib/ciq_helpers.py, and adds a standalone CLI (check_fips_changes.py) so CI workflows/scripts can run the check independently.
Changes:
- Move FIPS protected directory list + commit-range scanning into
kt/ktlib/ciq_helpers.py. - Update
rolling-release-update.pyto use the shared helper instead of an in-file implementation. - Add
check_fips_changes.pyas a standalone command-line wrapper for CI/workflow usage.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| rolling-release-update.py | Switches FIPS-check logic to the shared helper and adjusts output for detected commits. |
| kt/ktlib/ciq_helpers.py | Introduces shared FIPS protected path list and commit-range scanning helper. |
| check_fips_changes.py | Adds standalone CLI to run the shared FIPS-change detection and exit non-zero on findings (unless overridden). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bmastbergen
force-pushed
the
clk-fips-check
branch
from
b60c564 to
e70378d
Compare
bmastbergen
requested review from
PlaidCat,
kerneltoast,
roxanan1996 and
shreeya-patel98
|
|
||
|
|
||
| FIPS_PROTECTED_DIRECTORIES = [ | ||
| "arch/x86/crypto/", |
There was a problem hiding this comment.
Just a heads up i used bytestring in the original because sometimes parsing the other output from raw git commands leads to a horrible rabbit hole of unsupported or out of range characters. Its something i encountered with the original Rebuild scripts ESPECIALLY when it came to looking at contributor names from across the globe. Bytestring really made this go away and became the standard way I wrote the scripts.
If we not using bytestring elsewhere in the code for processing the output of git we should probably address that in a separate change.
There was a problem hiding this comment.
Gotcha. I've switched back to bytestrings. Things should be much closer to what you did before now.
| ) | ||
| print(f"## Commit {sha}") | ||
| if result.returncode == 0: | ||
| print(result.stdout.decode()) |
There was a problem hiding this comment.
I think this will be the instigator of uncaught exceptions due to authro/committer having character codes outside of UTF-8 and python does not handle this well by default.
Claude suggested the following to replace this with:
result.stdout.decode(errors="replace") or result.stdout.decode("utf-8", "backslashreplace")
● replace substitutes � (U+FFFD) for each bad byte — clean output,
but you lose what was there. Good when the output is for humans
who just need to read it (CI logs, PR comments).
backslashreplace substitutes \xc3\xa9-style escapes — uglier, but
preserves the original byte values. Good when you might need to debug
what the actual encoding was.
I would prefer to see just because the text stuff is such a pain
result.stdout.decode("utf-8", "backslashreplace")
There was a problem hiding this comment.
Gotcha. I've switched back to bytestrings. Things should be much closer to what you did before now.
|
thanks for splitting this out. I just have some landmines i've run into in the past with this stuff that I commented on. |
| if result.returncode != 0: | ||
| raise RuntimeError(f"git log failed for range {start_ref}..{end_ref}: {result.stderr.decode()}") | ||
|
|
||
| shas = [s for s in result.stdout.decode().split("\n") if s.strip()] |
There was a problem hiding this comment.
Maple's .decode() feedback applies here too, all the .decode() calls in this function have the same problem.
| "crypto/", | ||
| "drivers/crypto/", | ||
| "drivers/char/random.c", | ||
| "include/crypto", |
There was a problem hiding this comment.
Just a minor thing :-
This one is missing a trailing slash unlike the other directory entries.
There was a problem hiding this comment.
this just matches what was here when I started. I THINK this is intentional so that it matches include/crypto.h and things like include/cryptographic/
|
|
||
| FIPS_PROTECTED_DIRECTORIES = [ | ||
| "arch/x86/crypto/", | ||
| "crypto/asymmetric_keys/", |
There was a problem hiding this comment.
Umm will not crypto/ on the next line already cover this too?
There was a problem hiding this comment.
yea that might be redundant. again, its whatever was there when I started.
bmastbergen
force-pushed
the
clk-fips-check
branch
from
e70378d to
25ffa3d
Compare
|
|
||
| for sha, dirs in fips_commits.items(): | ||
| result = subprocess.run( | ||
| ["git", "show", "--stat", sha], |
Pulled the FIPS protected directory list and change detection logic out of rolling-release-update.py into kt/ktlib/ciq_helpers.py so it can be reused. Added check_fips_changes.py as a standalone CLI that wraps the shared library function for use by CI workflows and scripts. rolling-release-update.py now imports from ciq_helpers instead of carrying its own copy of the check.
bmastbergen
force-pushed
the
clk-fips-check
branch
from
25ffa3d to
75e8d23
Compare
I appreciate that. I've switched back to bytestrings, so things are much closer to the way they were before I split it out. Thanks! |
Pulled the FIPS protected directory list and change detection logic out of rolling-release-update.py into kt/ktlib/ciq_helpers.py so it can be reused. Added check_fips_changes.py as a standalone CLI that wraps the shared library function for use by CI workflows and scripts.
rolling-release-update.py now imports from ciq_helpers instead of carrying its own copy of the check.
The standalone check_fips_changes.py will be used in CI Workflows like this:
ctrliq/kernel-src-tree@583eb00
So they can produce PR comments like this:
ctrliq/kernel-src-tree#1288 (comment)