Refactor build scripts and update devcontainer for FIPS compliance#2
Merged
Conversation
…nce Java version support
…pport, enhance FIPS compliance checks
Updated base image to Ubuntu 22.04 and modified package installations.
📝 WalkthroughWalkthroughAdds a VS Code devcontainer and Dockerfile, installs Python and Buildpacks Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer/VSCode
participant DC as Devcontainer
participant Pack as pack CLI
participant Detect as fips-java-shim/detect (Python)
participant Build as fips-java-shim/build (Python)
participant FS as Filesystem (/opt/jdks, /opt/jres, layers)
participant Registry as Image registry / local daemon
Dev->>DC: open workspace (remoteUser vscode)
DC->>Pack: run `pack build` (via tasks)
Pack->>Detect: invoke detect phase
Detect->>FS: read `BP_JVM_VERSION`, `pom.xml`, `build.gradle`
Detect->>Pack: write `plan.toml` (version metadata)
Pack->>Build: invoke build phase with plan
Build->>FS: compute fingerprints from /opt/jdks /opt/jres
alt fingerprint match
Build-->>FS: reuse existing layer dirs
else fingerprint mismatch
Build->>FS: recreate layer dirs, write TOML metadata
end
Build->>Pack: emit layer/env metadata
Pack->>Registry: publish image (if publish enabled)
Registry-->>Dev: image available
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
build-image/Dockerfile (1)
23-25: Remove duplicatemkdircommand.Lines 23 and 25 both execute
mkdir -p /opt/jdks /opt/jres. Remove one to avoid redundancy.🔧 Proposed fix
RUN pip install --no-cache-dir rich --break-system-packages - -RUN mkdir -p /opt/jdks /opt/jres RUN mkdir -p /opt/jdks /opt/jres🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build-image/Dockerfile` around lines 23 - 25, Remove the duplicate RUN mkdir -p /opt/jdks /opt/jres line in the Dockerfile; locate the two identical RUN commands that create /opt/jdks and /opt/jres and delete one so only a single RUN mkdir -p /opt/jdks /opt/jres remains, keeping the Dockerfile behavior unchanged..devcontainer/Dockerfile (1)
5-11: Add--no-install-recommendsto reduce image size.Per static analysis, adding
--no-install-recommendsprevents installation of unnecessary recommended packages, reducing the image footprint.🔧 Proposed fix
-RUN apt-get update && apt-get install -y \ +RUN apt-get update && apt-get install -y --no-install-recommends \ python3 \ python3-pip \ curl \ git \ sudo \ && rm -rf /var/lib/apt/lists/*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/Dockerfile around lines 5 - 11, The Dockerfile's RUN apt-get install invocation installs recommended packages increasing image size; update the RUN command that currently installs python3, python3-pip, curl, git, sudo (the apt-get update && apt-get install -y \ ... line) to include the --no-install-recommends flag so only essential packages are installed and the image footprint is reduced, keeping the existing rm -rf /var/lib/apt/lists/* cleanup as-is..devcontainer/devcontainer.json (1)
35-35: Redundantrichinstallation and fragile command chain.Two issues:
pip3 install richis redundant -.devcontainer/Dockerfileshould install it if needed (consistency with build-image pattern)- The
|| trueat the end only catches the last command's failure. Earlier failures in the chain won't be suppressed, potentially causing unexpected behavior.Consider splitting into separate commands or using a setup script for better error handling.
🔧 Proposed fix - consolidate pip install in Dockerfile
Add to
.devcontainer/DockerfilebeforeUSER vscode:RUN pip3 install --no-cache-dir richThen simplify postCreateCommand:
-"postCreateCommand": "sudo chmod 666 /var/run/docker.sock && pip3 install rich && sudo chown vscode:vscode /var/run/docker.sock || true" +"postCreateCommand": "sudo chmod 666 /var/run/docker.sock && sudo chown vscode:vscode /var/run/docker.sock || true"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.devcontainer/devcontainer.json at line 35, postCreateCommand currently redundantly installs rich and uses a fragile chained command; move the pip install into the devcontainer image build and simplify the post-create actions: add "pip3 install --no-cache-dir rich" to .devcontainer/Dockerfile before the USER vscode line, then change the postCreateCommand to separate the socket permission steps (chmod 666 /var/run/docker.sock && chown vscode:vscode /var/run/docker.sock) or invoke a small setup script to handle failures explicitly instead of relying on "|| true", keeping references to postCreateCommand, .devcontainer/Dockerfile, USER vscode, and /var/run/docker.sock to locate the edits.fips-java-shim/bin/detect (2)
51-60: Consider supportingbuild.gradle.kts(Kotlin DSL).The Gradle detection only checks for
build.gradle(Groovy DSL). Projects using Kotlin DSL (build.gradle.kts) won't have their Java version detected and will fall back to default.🔧 Proposed fix
# ================================================================= # 3. PRIORITY 3: GRADLE (build.gradle) # ================================================================= - if not version and (app_dir / "build.gradle").exists(): - content = (app_dir / "build.gradle").read_text() + gradle_file = None + if (app_dir / "build.gradle").exists(): + gradle_file = app_dir / "build.gradle" + elif (app_dir / "build.gradle.kts").exists(): + gradle_file = app_dir / "build.gradle.kts" + + if not version and gradle_file: + content = gradle_file.read_text() # Look for Toolchain or Compatibility settings match = re.search(r'(JavaLanguageVersion\.of\(|sourceCompatibility\s*=\s*|targetCompatibility\s*=\s*)[\'"]?([0-9.]+)', content) if match: version = match.group(2) - source = "build.gradle" + source = gradle_file.name🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/detect` around lines 51 - 60, The detection currently only checks for "build.gradle"; update the logic to also check for "build.gradle.kts" in the same PRIORITY 3 block: if version is unset, test (app_dir / "build.gradle.kts").exists() and read its content (same as for build.gradle), run the same regex search for JavaLanguageVersion.of/sourceCompatibility/targetCompatibility, and if found set version and set source to "build.gradle.kts"; ensure the existing (app_dir / "build.gradle") check still runs if the .kts file is absent so the variables version, app_dir, and source are used consistently.
70-71: Edge case: empty version string causesIndexError.If
versionis an empty string (e.g., from an empty XML tag),version.split('.')[0]returns an empty string, which may cause issues downstream. However,.split('.')[0]on empty string returns"", not an error. The real risk is if regex captures whitespace-only content.Consider adding validation:
🔧 Proposed defensive fix
# Map versions: 1.8 -> 8, 17.0.1 -> 17 - clean_version = "8" if "1.8" in version else version.split('.')[0] + clean_version = "8" if "1.8" in version else (version.split('.')[0] or "21")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/detect` around lines 70 - 71, The current clean_version assignment assumes `version` contains non-whitespace content and uses `clean_version = "8" if "1.8" in version else version.split('.')[0]`; add defensive validation by trimming whitespace (e.g., set `version = version.strip()` before use), then check for empty string or whitespace-only and handle it (either log an error and exit or assign a safe default like "unknown" or skip processing), and finally compute `clean_version` using the existing mapping for "1.8" or `version.split('.')[0]` only when `version` is non-empty; update any code paths that rely on `clean_version` to expect the fallback value if validation fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.devcontainer/Dockerfile:
- Around line 16-17: The Dockerfile currently downloads pack pinned to "v0.36.4"
(see the RUN curl line that fetches "pack-v0.36.4-linux.tgz"); either update
that version string and download URL to the current supported release (e.g.,
replace "v0.36.4" with "v0.40.1" in the curl/tar invocation) or add a brief
comment immediately above the RUN line explaining why v0.36.4 is required
(compatibility/testing/certification) so the pin is documented for future
reviewers.
In `@fips-java-shim/bin/build`:
- Around line 135-139: The current try/except around reading
"/sys/fs/cgroup/memory/memory.limit_in_bytes" uses a bare except which can
swallow KeyboardInterrupt/SystemExit; change it to catch specific exceptions
(e.g., FileNotFoundError, PermissionError, ValueError, OSError) or at minimum
except Exception as e to avoid catching system-exiting exceptions, assign the
fallback 1073741824 to total_mem in that handler, and optionally log the caught
exception (referencing the total_mem variable and the file-read block) for
debugging.
- Around line 62-65: The main function currently assumes sys.argv[1] and
sys.argv[2] exist which can raise IndexError; add argument validation at the
start of main() to check len(sys.argv) >= 3 (or >=4 if plan_path required) and
print a clear usage message or exit non-zero if not enough arguments. Update the
code that assigns layers_dir, platform_dir, and plan_path (the variables
layers_dir, platform_dir, plan_path and the main() entry) so you only access
sys.argv[1]/[2]/[3] after validating length, and ensure plan_path still defaults
to Path("") when argv[3] is absent. Ensure the validation path returns/raises
consistent error codes and logs a helpful usage string.
- Around line 134-142: The memory calculation currently reads cgroup v1 at
"/sys/fs/cgroup/memory/memory.limit_in_bytes" (used by total_mem/max_heap) and
thus fails on cgroup v2; update the logic in the block that sets total_mem,
headroom, and max_heap to first attempt reading the cgroup v2 file
"/sys/fs/cgroup/memory.max" (handle the special value "max" as unlimited),
falling back to "/sys/fs/cgroup/memory/memory.limit_in_bytes" if the v2 file is
missing or contains non-numeric content, parse values robustly into integers,
and keep the existing headroom and max_heap calculation using total_mem so
functions/variables total_mem, headroom, and max_heap work unchanged.
- Around line 121-122: The current build writes JAVA_TOOL_OPTIONS.override using
-Xbootclasspath/a via the jdk_boot variable which breaks Java 11+ FIPS
requirements; instead update the logic that builds jdk_boot and the write to
(env_build_dir / "JAVA_TOOL_OPTIONS.override") to point to a dedicated FIPS JAR
directory and use --module-path (or classpath) rather than -Xbootclasspath/a,
i.e. construct options using --module-path /path/to/fips-jars and remove the
-Xbootclasspath/a flag, and ensure you document/configure static providers by
updating java.security to include
security.provider.1=org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider
and security.provider.2=org.bouncycastle.jsse.provider.BouncyCastleJsseProvider
and set keystore.type=BCFKS (keeping the SUN provider entry for SecureRandom
compatibility); locate changes around the jdk_boot variable, the fips_base
usage, and the write to JAVA_TOOL_OPTIONS.override to implement this
replacement.
In `@fips-java-shim/bin/detect`:
- Around line 23-27: The main() function currently reads sys.argv[1] and
sys.argv[2] into platform_dir and plan_path without validating arguments, which
can raise IndexError; update main() to validate len(sys.argv) (or use argparse)
before accessing argv, return a non-zero exit or print a helpful usage message
when arguments are missing, and only set platform_dir, plan_path, and app_dir
after validation; reference the main(), platform_dir, plan_path, app_dir symbols
and ensure any error paths use sys.exit(1) or raise a clear exception so the
script fails safely.
---
Nitpick comments:
In @.devcontainer/devcontainer.json:
- Line 35: postCreateCommand currently redundantly installs rich and uses a
fragile chained command; move the pip install into the devcontainer image build
and simplify the post-create actions: add "pip3 install --no-cache-dir rich" to
.devcontainer/Dockerfile before the USER vscode line, then change the
postCreateCommand to separate the socket permission steps (chmod 666
/var/run/docker.sock && chown vscode:vscode /var/run/docker.sock) or invoke a
small setup script to handle failures explicitly instead of relying on "||
true", keeping references to postCreateCommand, .devcontainer/Dockerfile, USER
vscode, and /var/run/docker.sock to locate the edits.
In @.devcontainer/Dockerfile:
- Around line 5-11: The Dockerfile's RUN apt-get install invocation installs
recommended packages increasing image size; update the RUN command that
currently installs python3, python3-pip, curl, git, sudo (the apt-get update &&
apt-get install -y \ ... line) to include the --no-install-recommends flag so
only essential packages are installed and the image footprint is reduced,
keeping the existing rm -rf /var/lib/apt/lists/* cleanup as-is.
In `@build-image/Dockerfile`:
- Around line 23-25: Remove the duplicate RUN mkdir -p /opt/jdks /opt/jres line
in the Dockerfile; locate the two identical RUN commands that create /opt/jdks
and /opt/jres and delete one so only a single RUN mkdir -p /opt/jdks /opt/jres
remains, keeping the Dockerfile behavior unchanged.
In `@fips-java-shim/bin/detect`:
- Around line 51-60: The detection currently only checks for "build.gradle";
update the logic to also check for "build.gradle.kts" in the same PRIORITY 3
block: if version is unset, test (app_dir / "build.gradle.kts").exists() and
read its content (same as for build.gradle), run the same regex search for
JavaLanguageVersion.of/sourceCompatibility/targetCompatibility, and if found set
version and set source to "build.gradle.kts"; ensure the existing (app_dir /
"build.gradle") check still runs if the .kts file is absent so the variables
version, app_dir, and source are used consistently.
- Around line 70-71: The current clean_version assignment assumes `version`
contains non-whitespace content and uses `clean_version = "8" if "1.8" in
version else version.split('.')[0]`; add defensive validation by trimming
whitespace (e.g., set `version = version.strip()` before use), then check for
empty string or whitespace-only and handle it (either log an error and exit or
assign a safe default like "unknown" or skip processing), and finally compute
`clean_version` using the existing mapping for "1.8" or `version.split('.')[0]`
only when `version` is non-empty; update any code paths that rely on
`clean_version` to expect the fallback value if validation fails.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3d52bd7a-0da0-4f2c-8bca-078dd8f9c499
📒 Files selected for processing (6)
.devcontainer/Dockerfile.devcontainer/devcontainer.json.vscode/tasks.jsonbuild-image/Dockerfilefips-java-shim/bin/buildfips-java-shim/bin/detect
Comment on lines
+16
to
+17
| RUN curl -sSL "https://github.com/buildpacks/pack/releases/download/v0.36.4/pack-v0.36.4-linux.tgz" \ | ||
| | tar -C /usr/local/bin/ --no-same-owner -xzv pack |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the latest version of pack CLI buildpacks?
💡 Result:
The latest version of the pack CLI for Cloud Native Buildpacks is v0.40.1.
Citations:
- 1: https://github.com/buildpacks/pack/releases
- 2: https://github.com/buildpacks/pack
- 3: https://buildpacks.io/docs/for-platform-operators/how-to/integrate-ci/pack
Update pack CLI to a recent version or document the rationale.
The pack CLI is pinned to v0.36.4, which is 3 minor versions behind the latest release (v0.40.1). Either update to the current version or add a comment explaining why v0.36.4 is specifically required.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.devcontainer/Dockerfile around lines 16 - 17, The Dockerfile currently
downloads pack pinned to "v0.36.4" (see the RUN curl line that fetches
"pack-v0.36.4-linux.tgz"); either update that version string and download URL to
the current supported release (e.g., replace "v0.36.4" with "v0.40.1" in the
curl/tar invocation) or add a brief comment immediately above the RUN line
explaining why v0.36.4 is required (compatibility/testing/certification) so the
pin is documented for future reviewers.
Comment on lines
+62
to
+65
| def main(): | ||
| layers_dir = Path(sys.argv[1]) | ||
| platform_dir = Path(sys.argv[2]) | ||
| plan_path = Path(sys.argv[3]) if len(sys.argv) > 3 else Path("") |
There was a problem hiding this comment.
Add argument validation for robustness.
While plan_path has a fallback for missing sys.argv[3], accessing sys.argv[1] and sys.argv[2] without validation can raise IndexError.
🛡️ Proposed fix
def main():
+ if len(sys.argv) < 3:
+ console.print("[bold red]ERROR:[/bold red] Missing required arguments")
+ console.print("Usage: build <layers_dir> <platform_dir> [plan_path]")
+ sys.exit(1)
+
layers_dir = Path(sys.argv[1])
platform_dir = Path(sys.argv[2])
plan_path = Path(sys.argv[3]) if len(sys.argv) > 3 else Path("")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def main(): | |
| layers_dir = Path(sys.argv[1]) | |
| platform_dir = Path(sys.argv[2]) | |
| plan_path = Path(sys.argv[3]) if len(sys.argv) > 3 else Path("") | |
| def main(): | |
| if len(sys.argv) < 3: | |
| console.print("[bold red]ERROR:[/bold red] Missing required arguments") | |
| console.print("Usage: build <layers_dir> <platform_dir> [plan_path]") | |
| sys.exit(1) | |
| layers_dir = Path(sys.argv[1]) | |
| platform_dir = Path(sys.argv[2]) | |
| plan_path = Path(sys.argv[3]) if len(sys.argv) > 3 else Path("") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fips-java-shim/bin/build` around lines 62 - 65, The main function currently
assumes sys.argv[1] and sys.argv[2] exist which can raise IndexError; add
argument validation at the start of main() to check len(sys.argv) >= 3 (or >=4
if plan_path required) and print a clear usage message or exit non-zero if not
enough arguments. Update the code that assigns layers_dir, platform_dir, and
plan_path (the variables layers_dir, platform_dir, plan_path and the main()
entry) so you only access sys.argv[1]/[2]/[3] after validating length, and
ensure plan_path still defaults to Path("") when argv[3] is absent. Ensure the
validation path returns/raises consistent error codes and logs a helpful usage
string.
Comment on lines
+121
to
+122
| jdk_boot = f"{jdk_src}/{trust_dir}/ext/bc-fips.jar:{jdk_src}/{trust_dir}/ext/bcutil-fips.jar:{jdk_src}/{trust_dir}/ext/bctls-fips.jar" if version == "8" else f"{jdk_src}/lib/bc-fips.jar:{jdk_src}/lib/bcutil-fips.jar:{jdk_src}/lib/bctls-fips.jar" | ||
| (env_build_dir / "JAVA_TOOL_OPTIONS.override").write_text(f"{fips_base} -Djavax.net.ssl.trustStore={jdk_trust} -Xbootclasspath/a:{jdk_boot}") |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Java 11+ FIPS Bouncy Castle bootclasspath alternative
💡 Result:
The primary alternative to using -Xbootclasspath/a for Bouncy Castle FIPS (BCFIPS) provider in Java 11+ is to load the required JARs (bc-fips-.jar, bctls-fips-.jar, bcpkix-fips-.jar, bcutil-fips-.jar) via the --module-path JVM option and configure the provider statically in java.security. Key steps: 1. Download latest certified BC-FJA release (e.g., 2.1.2 as of 2025, certified for Java 11+): bc-fips-2.1.2.jar, bcpkix-fips-2.1.10.jar, bctls-fips-2.1.22.jar, bcutil-fips-2.1.2.jar from https://bouncycastle.org/download/bouncy-castle-java-fips. 2. Place JARs in a directory, e.g., /path/to/fips-jars/. 3. Run Java with: java --module-path /path/to/fips-jars -jar your-app.jar - For tools/subprocesses, use export JDK_JAVA_OPTIONS="--module-path /path/to/fips-jars" 4. In java.security (override via -Djava.security.properties or copy/edit $JAVA_HOME/conf/security/java.security): security.provider.1=org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider [config] security.provider.2=org.bouncycastle.jsse.provider.BouncyCastleJsseProvider fips:BCFIPS security.provider.3=sun.security.provider.Sun - [config] example: C:DEFRND[SHA512];ENABLE{ALL}; - Keep SUN provider for SecureRandom/JSSE compatibility. - Set keystore.type=BCFKS for FIPS-approved keystores. Why bootclasspath fails: Classloader issues (null ClassLoader, getResource failures, checksum validation problems) in bc-fips when loaded via bootstrap loader, even after fixes. Other options (less recommended): - Add to app classpath/module-path without static config, install provider dynamically: Security.addProvider(new BouncyCastleFipsProvider); - Avoid jlink (breaks FIPS validation due to repackaging/signature issues). This maintains FIPS 140-3 Level 1 compliance per BC security policy and user guide.
Citations:
- 1: bc-fips 1.0.2 is not compatible with -xbootclasspath/a (Java 11) bcgit/bc-java#873
- 2: Unable to load bouncycastle fips 2.x provider in JDK21 bcgit/bc-java#2000
- 3: https://downloads.bouncycastle.org/fips-java/docs/BC-FJA-UserGuide-2.1.1.pdf
- 4: https://downloads.bouncycastle.org/fips-java/docs/BC-FJA-UserGuide-2.1.0.pdf
- 5: https://bouncycastle.org/download/bouncy-castle-java-fips
Update the bootclasspath approach for Java 11+ FIPS compliance.
Using -Xbootclasspath/a to load Bouncy Castle FIPS causes classloader validation issues (null ClassLoader, getResource failures, checksum problems) and does not maintain FIPS 140-3 Level 1 compliance per BC security policy.
Replace with the recommended approach: use --module-path pointing to the FIPS JARs directory and configure providers statically in java.security:
- Place bc-fips, bcutil-fips, bctls-fips, and bcpkix-fips JARs in a directory (e.g.,
/path/to/fips-jars/) - Pass
--module-path /path/to/fips-jarsor add to classpath - In
java.security, add:security.provider.1=org.bouncycastle.jcajce.provider.BouncyCastleFipsProviderandsecurity.provider.2=org.bouncycastle.jsse.provider.BouncyCastleJsseProvider - Keep SUN provider for SecureRandom compatibility
- Set
keystore.type=BCFKSfor FIPS-approved keystores
Reference: bcgit/bc-java#873, bcgit/bc-java#2000, BC-FJA User Guide 2.1+
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fips-java-shim/bin/build` around lines 121 - 122, The current build writes
JAVA_TOOL_OPTIONS.override using -Xbootclasspath/a via the jdk_boot variable
which breaks Java 11+ FIPS requirements; instead update the logic that builds
jdk_boot and the write to (env_build_dir / "JAVA_TOOL_OPTIONS.override") to
point to a dedicated FIPS JAR directory and use --module-path (or classpath)
rather than -Xbootclasspath/a, i.e. construct options using --module-path
/path/to/fips-jars and remove the -Xbootclasspath/a flag, and ensure you
document/configure static providers by updating java.security to include
security.provider.1=org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider
and security.provider.2=org.bouncycastle.jsse.provider.BouncyCastleJsseProvider
and set keystore.type=BCFKS (keeping the SUN provider entry for SecureRandom
compatibility); locate changes around the jdk_boot variable, the fips_base
usage, and the write to JAVA_TOOL_OPTIONS.override to implement this
replacement.
Comment on lines
+135
to
+139
| try: | ||
| with open("/sys/fs/cgroup/memory/memory.limit_in_bytes", "r") as f: | ||
| total_mem = int(f.read().strip()) | ||
| except: | ||
| total_mem = 1073741824 |
There was a problem hiding this comment.
Avoid bare except: clause.
Bare except: catches all exceptions including KeyboardInterrupt and SystemExit, which can mask critical errors and make debugging difficult.
🔧 Proposed fix
try:
with open("/sys/fs/cgroup/memory/memory.limit_in_bytes", "r") as f:
total_mem = int(f.read().strip())
- except:
+ except (FileNotFoundError, ValueError, PermissionError):
total_mem = 1073741824 📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| with open("/sys/fs/cgroup/memory/memory.limit_in_bytes", "r") as f: | |
| total_mem = int(f.read().strip()) | |
| except: | |
| total_mem = 1073741824 | |
| try: | |
| with open("/sys/fs/cgroup/memory/memory.limit_in_bytes", "r") as f: | |
| total_mem = int(f.read().strip()) | |
| except (FileNotFoundError, ValueError, PermissionError): | |
| total_mem = 1073741824 |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fips-java-shim/bin/build` around lines 135 - 139, The current try/except
around reading "/sys/fs/cgroup/memory/memory.limit_in_bytes" uses a bare except
which can swallow KeyboardInterrupt/SystemExit; change it to catch specific
exceptions (e.g., FileNotFoundError, PermissionError, ValueError, OSError) or at
minimum except Exception as e to avoid catching system-exiting exceptions,
assign the fallback 1073741824 to total_mem in that handler, and optionally log
the caught exception (referencing the total_mem variable and the file-read
block) for debugging.
Comment on lines
+23
to
+27
| def main(): | ||
| # Detect arguments: $1 = platform dir, $2 = plan path | ||
| platform_dir = Path(sys.argv[1]) | ||
| plan_path = Path(sys.argv[2]) | ||
| app_dir = Path.cwd() |
There was a problem hiding this comment.
Add argument validation to prevent crashes.
Accessing sys.argv[1] and sys.argv[2] without validation will raise IndexError if the buildpack lifecycle invokes the script incorrectly or during local testing.
🛡️ Proposed fix
def main():
+ if len(sys.argv) < 3:
+ print("Usage: detect <platform_dir> <plan_path>", file=sys.stderr)
+ sys.exit(1)
+
# Detect arguments: $1 = platform dir, $2 = plan path
platform_dir = Path(sys.argv[1])
plan_path = Path(sys.argv[2])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fips-java-shim/bin/detect` around lines 23 - 27, The main() function
currently reads sys.argv[1] and sys.argv[2] into platform_dir and plan_path
without validating arguments, which can raise IndexError; update main() to
validate len(sys.argv) (or use argparse) before accessing argv, return a
non-zero exit or print a helpful usage message when arguments are missing, and
only set platform_dir, plan_path, and app_dir after validation; reference the
main(), platform_dir, plan_path, app_dir symbols and ensure any error paths use
sys.exit(1) or raise a clear exception so the script fails safely.
There was a problem hiding this comment.
♻️ Duplicate comments (2)
fips-java-shim/bin/build (2)
29-29:⚠️ Potential issue | 🟡 MinorAvoid bare
except:clause.The bare
except: passcatches all exceptions includingKeyboardInterruptandSystemExit. Use specific exceptions to avoid masking critical errors.🔧 Proposed fix
try: if p.exists(): val = p.read_text().strip() if val != "max": return int(val) - except: pass + except (ValueError, PermissionError, OSError): pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/build` at line 29, Replace the bare "except: pass" with a targeted exception handler to avoid catching SystemExit/KeyboardInterrupt; change the clause to "except Exception:" (or list the specific exception types you expect) and handle or log the error instead of silencing it—locate the solitary "except: pass" in the build script and replace it with an explicit exception capture and minimal handling/logging to preserve intended flow while not masking critical interrupts.
87-92:⚠️ Potential issue | 🟠 Major
-Xbootclasspath/a:breaks FIPS 140-3 compliance on Java 11+.Using
-Xbootclasspath/a:to load Bouncy Castle FIPS causes classloader validation issues on Java 11+ (null ClassLoader, getResource failures, checksum problems). For Java 11+, use--module-pathor classpath with static provider configuration injava.security.🔧 Suggested approach for Java 11+
if version == "8": jdk_boot = f"{jdk_src}/{jdk_trust_dir}/ext/bc-fips.jar:{jdk_src}/{jdk_trust_dir}/ext/bcutil-fips.jar:{jdk_src}/{jdk_trust_dir}/ext/bctls-fips.jar" + boot_option = f"-Xbootclasspath/a:{jdk_boot}" else: - jdk_boot = f"{jdk_src}/lib/bc-fips.jar:{jdk_src}/lib/bcutil-fips.jar:{jdk_src}/lib/bctls-fips.jar" + fips_jars = f"{jdk_src}/lib/bc-fips.jar:{jdk_src}/lib/bcutil-fips.jar:{jdk_src}/lib/bctls-fips.jar" + boot_option = f"-cp {fips_jars}" # Or use --module-path - (env_build / "JAVA_TOOL_OPTIONS.override").write_text(f"{fips_base} -Djavax.net.ssl.trustStore={jdk_trust} -Xbootclasspath/a:{jdk_boot}") + (env_build / "JAVA_TOOL_OPTIONS.override").write_text(f"{fips_base} -Djavax.net.ssl.trustStore={jdk_trust} {boot_option}")Additionally, configure static providers in
java.securityfor full compliance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/build` around lines 87 - 92, The current build writes JAVA_TOOL_OPTIONS.override with "-Xbootclasspath/a:{jdk_boot}" which breaks FIPS 140-3 on Java 11+; update the logic around jdk_boot and the write_text call so that when version != "8" you do not use "-Xbootclasspath/a:", but instead add the BouncyCastle FIPS jars via --module-path (or regular classpath) and configure static providers in java.security; specifically change the branch that sets jdk_boot and the line that writes JAVA_TOOL_OPTIONS.override to use a Java 11+ safe invocation (e.g., include --module-path or -cp with the bc-fips jars and ensure the runtime uses a statically configured Security provider rather than -Xbootclasspath/a:), and document that java.security must be updated to register the BCFIPS provider for Java 11+.
🧹 Nitpick comments (4)
fips-java-shim/bin/build (4)
119-124: Simplify redundant conditional logic.Lines 119-124 can be simplified. When
version == "8"andjvm_type == "JRE", it setsjre_trust_dir = "lib". Otherwise whenversion == "8", it setsjre_trust_dir = "jre/lib". For all other versions, it setsjre_trust_dir = "lib". This is clearer as a simpler conditional.♻️ Proposed simplification
- if version == "8" and jvm_type == "JRE": - jre_trust_dir = "lib" - elif version == "8": - jre_trust_dir = "jre/lib" - else: - jre_trust_dir = "lib" + jre_trust_dir = "jre/lib" if (version == "8" and jvm_type != "JRE") else "lib"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/build` around lines 119 - 124, Simplify the conditional that assigns jre_trust_dir: default jre_trust_dir to "lib" and only set it to "jre/lib" when version == "8" and jvm_type != "JRE"; update the assignment around the existing jre_trust_dir, version, and jvm_type variables so the logic is equivalent but not redundant.
68-68: Substring fingerprint match could cause false positives.Using
jdk_fp in jdk_toml.read_text()performs a substring match. While unlikely with the current TOML format, a fingerprint like"abc"would matchfingerprint = "abc123". Consider parsing the TOML or using a stricter match pattern.♻️ Proposed fix using exact pattern match
- if jdk_toml.exists() and jdk_fp in jdk_toml.read_text() and jdk_layer.exists(): + if jdk_toml.exists() and f'fingerprint = "{jdk_fp}"' in jdk_toml.read_text() and jdk_layer.exists():Apply the same pattern for
jre_tomlat line 100.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/build` at line 68, The check uses a substring match (jdk_fp in jdk_toml.read_text()) which can yield false positives; update the logic in the jdk verification (referencing jdk_toml, jdk_fp, jdk_layer) to perform a stricter match by either parsing the TOML and comparing the fingerprint key value exactly or using a regex that matches the full quoted fingerprint (e.g., match r'fingerprint\s*=\s*"{jdk_fp}"'); make the same change for the jre check that uses jre_toml so both use exact fingerprint matching rather than simple substring search.
32-39: Consider including file modification time in fingerprint.The fingerprint only uses filename and size. If a file's content changes but size remains the same, the fingerprint won't detect the change. Including
st_mtimewould improve reliability.♻️ Proposed enhancement
for p in sorted(directory.glob('bin/*')): if p.is_file(): - hasher.update(f"{p.name}{p.stat().st_size}".encode()) + stat = p.stat() + hasher.update(f"{p.name}{stat.st_size}{stat.st_mtime}".encode())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/build` around lines 32 - 39, The get_fingerprint function currently hashes only filename and size, so modify it to also incorporate file modification time (st_mtime) into the hashed input to detect content changes that preserve size; update the loop over directory.glob('bin/*') in get_fingerprint to include p.stat().st_mtime (or an integer/ISO representation) when calling hasher.update so the resulting hasher.hexdigest() reflects modification time changes as well.
42-44:sys.argv[2](platform_dir) is validated but never used.The script validates that at least 3 arguments exist but never uses the
platform_dirvalue fromsys.argv[2]. Either remove the unused argument from validation or assign it if needed for future use.♻️ Proposed fix if platform_dir is not needed
- if len(sys.argv) < 3: sys.exit(1) + if len(sys.argv) < 2: sys.exit(1) layers_dir = Path(sys.argv[1]) plan_path = Path(sys.argv[3]) if len(sys.argv) > 3 else Path("")Note: If this follows Cloud Native Buildpacks specification,
platform_dirmay be required by the interface even if unused—in that case, retain the validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fips-java-shim/bin/build` around lines 42 - 44, The script currently checks for at least three argv entries but never assigns or uses the platform_dir (sys.argv[2]); either remove that requirement or capture the value for future use: change the validation to reflect the actual required count if platform_dir is not needed, or assign platform_dir = Path(sys.argv[2]) alongside layers_dir and plan_path so the variable is available (keep the existing sys.argv checks if this follows CNB spec). Ensure you reference and set platform_dir, sys.argv, layers_dir, and plan_path consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@fips-java-shim/bin/build`:
- Line 29: Replace the bare "except: pass" with a targeted exception handler to
avoid catching SystemExit/KeyboardInterrupt; change the clause to "except
Exception:" (or list the specific exception types you expect) and handle or log
the error instead of silencing it—locate the solitary "except: pass" in the
build script and replace it with an explicit exception capture and minimal
handling/logging to preserve intended flow while not masking critical
interrupts.
- Around line 87-92: The current build writes JAVA_TOOL_OPTIONS.override with
"-Xbootclasspath/a:{jdk_boot}" which breaks FIPS 140-3 on Java 11+; update the
logic around jdk_boot and the write_text call so that when version != "8" you do
not use "-Xbootclasspath/a:", but instead add the BouncyCastle FIPS jars via
--module-path (or regular classpath) and configure static providers in
java.security; specifically change the branch that sets jdk_boot and the line
that writes JAVA_TOOL_OPTIONS.override to use a Java 11+ safe invocation (e.g.,
include --module-path or -cp with the bc-fips jars and ensure the runtime uses a
statically configured Security provider rather than -Xbootclasspath/a:), and
document that java.security must be updated to register the BCFIPS provider for
Java 11+.
---
Nitpick comments:
In `@fips-java-shim/bin/build`:
- Around line 119-124: Simplify the conditional that assigns jre_trust_dir:
default jre_trust_dir to "lib" and only set it to "jre/lib" when version == "8"
and jvm_type != "JRE"; update the assignment around the existing jre_trust_dir,
version, and jvm_type variables so the logic is equivalent but not redundant.
- Line 68: The check uses a substring match (jdk_fp in jdk_toml.read_text())
which can yield false positives; update the logic in the jdk verification
(referencing jdk_toml, jdk_fp, jdk_layer) to perform a stricter match by either
parsing the TOML and comparing the fingerprint key value exactly or using a
regex that matches the full quoted fingerprint (e.g., match
r'fingerprint\s*=\s*"{jdk_fp}"'); make the same change for the jre check that
uses jre_toml so both use exact fingerprint matching rather than simple
substring search.
- Around line 32-39: The get_fingerprint function currently hashes only filename
and size, so modify it to also incorporate file modification time (st_mtime)
into the hashed input to detect content changes that preserve size; update the
loop over directory.glob('bin/*') in get_fingerprint to include
p.stat().st_mtime (or an integer/ISO representation) when calling hasher.update
so the resulting hasher.hexdigest() reflects modification time changes as well.
- Around line 42-44: The script currently checks for at least three argv entries
but never assigns or uses the platform_dir (sys.argv[2]); either remove that
requirement or capture the value for future use: change the validation to
reflect the actual required count if platform_dir is not needed, or assign
platform_dir = Path(sys.argv[2]) alongside layers_dir and plan_path so the
variable is available (keep the existing sys.argv checks if this follows CNB
spec). Ensure you reference and set platform_dir, sys.argv, layers_dir, and
plan_path consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd5c9a09-6739-44d0-9d65-76ac5a3a1cec
📒 Files selected for processing (2)
2-builder/builder.tomlfips-java-shim/bin/build
✅ Files skipped from review due to trivial changes (1)
- 2-builder/builder.toml
Open
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit