Repository navigation
Discover and patch Java certificate blocklists with shared Python script - #117
Conversation
Centralize the eMudhra mitigation after Java setup so repositories can protect their actual JVM without relying on vendor patch availability. Run dependency-free TypeScript with Bun, preserve vendor blocklists, and fail closed when protection cannot be verified. Add offline certificate fixtures, unit tests, and a cross-vendor/OS JVM verification matrix. Allow local root-level actions in the immutable-ref check so the new composite action can test itself.
Apply the approved Crit review: derive fingerprints from one PEM bundle, use Bun file and shell APIs, and keep support scoped to Linux and macOS. Guard the entrypoint on import, trim documentation, and leave the shared immutable-reference validator unchanged. Retain actual JVM rejection tests and add a separate Bun-native unit job.
Share one dependency-free patcher between developer machines and CI. Inventory known JVM, IDE, version-manager, Gradle, and runner-cache locations, with explicit extra roots and dry-run/apply modes. Preserve vendor blocklists, deduplicate shared runtimes and files, and report partial failures without claiming complete filesystem coverage. Discover as the user and elevate only file updates. Replace Bun setup with a thin Python composite step and add stdlib compatibility tests.
roryabraham
changed the title
Add function and class docstrings and follow the Python style guide's single quotes, multiline layout, argument conventions, and line limit. Keep the patching and discovery behavior unchanged.
Put opening and closing triple quotes on separate lines and leave a blank line after each docstring, following the explicit style-guide examples rather than conventional Python docstring formatting.
Separate completed conditionals and loops from subsequent operations while keeping related branches and exception handlers together. Whitespace only; the Python AST is unchanged.
|
@codex review |
|
@MelvinBot review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b4c4a806a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if os.geteuid() == 0: | ||
| parser.error('Run as your normal user, not with sudo; only individual file updates will be elevated.') |
There was a problem hiding this comment.
When the composite action runs in a Linux job container or self-hosted runner whose job user is UID 0, this unconditional check exits before inspecting the explicitly selected JAVA_HOME. Because action.yml always invokes this entry point, such jobs cannot use the action at all, even though selected-home mode performs no broad discovery and already has permission to update the file; permit this mode as root while retaining the restriction for --all if executing discovered JVMs is the concern.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I don't think this will be a real issue. Though I don't know if we really need to error if it's run as sudo 🤷🏼
| def certificateFingerprints(bundle: str) -> list[str]: | ||
| """ | ||
| Derive uppercase SHA-256 certificate fingerprints from the reviewed PEM bundle. |
There was a problem hiding this comment.
Could we save the precomputed fingerprints instead in compromised-java-certificates.pem, then skip this step? NAB
There was a problem hiding this comment.
The Java piece of the test that ensures that the JVM actually rejects the invalid certificates needs the whole .pem file, so saving the precomputed fingerprints separately would create a second source of truth.
Use literal property separators and Java major-version prefix removal instead of regexes. Keep required-field validation and significant path whitespace, with regression tests for malformed versions and paths.
Resolve targets into a path set before any writes, without inode maps or a special shared-file status. Keep runtime inventory separate from per-file update results so shared paths do not inflate entry counts.
Remove locking, the second pre-write read, inode guards, and metadata assertions. Append missing entries normally and keep final fingerprint verification, with a regression test for missing post-write entries.
Replace privileged Python re-execution and its private CLI mode with sudo tee -a on permission-denied appends. Feed only missing entries on stdin, suppress tee output, and verify file contents afterward. Keep dry-runs and reads unprivileged. Cover prompt policy, failed sudo, and successful commands that fail to persist the expected entries.
Derive fingerprints from the trusted PEM bundle without a fixed count or ad hoc DER checks. Keep an empty-policy guard and validate the checked-in data in tests. Make test counts follow the bundle size.
Always append through sudo -n after a permission-denied write. Remove the interactive-mode parameter and CLI flag, and update the action, workflow, documentation, and tests to match.
Build the ASCII additions with an f-string and use explicit ASCII text I/O for normal and sudo appends. Keep original vendor-file reads binary so existing bytes and line endings are not reinterpreted or rewritten.
Return immutable JavaRuntime and BlocklistUpdate dataclasses instead of anonymous dictionaries. Type successful update statuses with Literal while keeping JSON values plain strings and avoiding an enum wrapper.
Move stdlib tests to tests/python and give them a dedicated workflow. Rename the certificate integration workflow and keep its path filters focused on the action, implementation, and actual-JVM test fixture.
Linux/macOS runners already default to bash. Document why the local action test checkout lives under .github without changing the shared action-reference validator.
Allow sudo to request authorization in interactive non-CI sessions, preceded by a clear permission message on stderr. Keep sudo -n for CI and headless execution, preserving clean JSON output and write-only elevation. Update the prompt-policy tests and usage documentation.
|
pushed a bunch of simplifications |
Handle missing, inaccessible, and non-directory roots inside the walk instead of duplicating the root check before entering it. Preserve optional-root reporting and add root-validation regression coverage.
Use canonical directory paths instead of device/inode tuples to avoid revisiting aliases and symlink cycles. Reuse resolved runtime homes and report resolution failures on both older and newer Python versions.
Accept an iterable in run and pass Discovery.homes directly. Keep the single candidate deduplication/sort in run, with coverage for sets and one-pass iterators.
Move JSON and text rendering out of main so CLI orchestration and exit status handling remain separate from presentation. Cover output streams, report contents, and apply-mode reminders without launching Java.
Replace string-valued root kinds with an isHomebrew boolean. Preserve the top-level Java-package filter and cover default/custom prefixes, nested Java layouts, and ordinary unfiltered roots.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6166330657
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| SearchRoot(Path(environment.get('JENV_ROOT', str(home / '.jenv'))) / 'versions'), | ||
| SearchRoot(Path(environment.get('GRADLE_USER_HOME', str(home / '.gradle'))) / 'jdks'), | ||
| ] | ||
| if platform == 'darwin': |
There was a problem hiding this comment.
When --all runs on a Linux developer machine using Linuxbrew, this platform guard prevents HOMEBREW_PREFIX/opt from ever being searched. Keg-only JDKs such as Homebrew OpenJDK may not appear in PATH, JAVA_HOME, or /usr/lib/jvm, so they are silently omitted despite the documented Homebrew discovery coverage; add the configured Homebrew prefix on Linux as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Nathan is 1/2 developers with a linux workstation. Do you use Linuxbrew?
Drop refactor-specific spies and redundant synthetic version checks. Consolidate coverage around file safety, discovery, sudo behavior, CLI outcomes, and partial failures. Keep real-JVM verification unchanged.
Details
This eMudhra certificate-blocklist mitigation is intended to be used across CI and developer laptops. It's one, standard-library-only Python 3.9+ script.
Add
scripts/blockCompromisedJavaCertificates.pyand a thinExpensify/GitHub-Actions/block-compromised-java-certificatescomposite action:actions/setup-javausingJAVA_HOMEor itsjava-homeinput.--alldiscovery across configured/PATH runtimes, the macOS registry, standard JDK directories, Homebrew, version managers, IDE bundles, Gradle JDKs, and runner caches. Allow custom locations with--search-root.--applyto write. Support JSON inventory/results, deduplicate runtime aliases and resolved blocklist paths, and continue processing other installations while returning failure for any search or patch errors.Supports Linux/macOS JVMs with OpenJDK-style blocklists. Discovery is bounded and reports its scope; custom paths, inaccessible directories, other users, containers/VMs, and future downloaded toolchains may require separate runs. Restart existing JVMs/IDEs/Gradle daemons after patching and rerun after installations/updates. This PR does not migrate consumers, patch employee installations, or deploy anything.
Rollout plan
My intention is that this script and action can be used:
Related Issues
Remediation plan: https://expensify.slack.com/docs/T03SC9DTT/F0CBWHPE856
Manual Tests
--all --dry-run, then--all --applyacross all 5 discovered JVMs, adding 16 blocklist entries to each.Linked PRs
None.