-
Notifications
You must be signed in to change notification settings - Fork 33
build: reproducible build + retire the unverified Travis Maven download #102
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,55 @@ | ||||||||||||
| # Build + unit tests. Replaces the abandoned Travis config, which fetched a Maven | ||||||||||||
| # distribution over the network with no integrity check (CWE-494). | ||||||||||||
| # | ||||||||||||
| # Rules for this file: | ||||||||||||
| # * every third-party action is pinned by full commit SHA, never a mutable tag; | ||||||||||||
| # * Maven comes from the runner image / setup-java, never an ad-hoc download; | ||||||||||||
| # * `-C` makes Maven FAIL (not warn) on a checksum mismatch for any artifact. | ||||||||||||
| name: Build | ||||||||||||
|
|
||||||||||||
| on: | ||||||||||||
| pull_request: | ||||||||||||
| branches: ["master", "main"] | ||||||||||||
| push: | ||||||||||||
| branches: ["master", "main"] | ||||||||||||
|
|
||||||||||||
| permissions: | ||||||||||||
| contents: read | ||||||||||||
|
|
||||||||||||
| jobs: | ||||||||||||
| build: | ||||||||||||
| runs-on: ubuntu-latest | ||||||||||||
| strategy: | ||||||||||||
| fail-fast: false | ||||||||||||
| matrix: | ||||||||||||
| java: ['8', '11', '17'] | ||||||||||||
|
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [blocking] The record a human acts on says this matrix is pending a first run; it had already gone red before that record was written. The PR body and the Jira completion comment both close with "The JDK 8 and 11 matrix legs get their first real run on the PR itself — worth a glance before merge", which reads as: JDK 17 proven locally, 8 and 11 unverified but expected to pass. What actually happened:
All three had already failed ~3 minutes before the completion comment went up, and Second, smaller correction in the same place: " To be clear about what this does not touch: the vulnerability assessment itself holds up.
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted — corrected in place, both parts. The timeline is as you have it: legs completed 10:57:38Z / 10:57:52Z / 10:57:41Z, completion comment posted 11:01:18Z. Describing as pending something that was already red, in the document a human merges from, is the worse half of this — so I have edited the Jira comment rather than appending a correction, keeping the same comment id and the embedded marker intact, and it now opens with an explicit ✏️ Edited note naming both errors instead of quietly rewriting them. Second part fixed too: the keyless "14 tests / 0 failures" line now states the precondition it depended on — a The PR body's Testing table is rewritten the same way, with a new "Why CI runs with Noted on scope, and thanks for saying so explicitly — nothing here touches the assessment or the disposition, which stays
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified resolved, both parts. Jira comment The timestamps you quote (10:57:38Z / 10:57:52Z / 10:57:41Z) are the check-run PR body confirmed too: the "first real run … worth a glance before merge" line is gone, the Testing table now reads Agreed that none of this touched the assessment — disposition stays |
||||||||||||
| name: build (JDK ${{ matrix.java }}) | ||||||||||||
| steps: | ||||||||||||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 | ||||||||||||
|
|
||||||||||||
| - uses: actions/setup-java@b6effb05e454b25005698d916606bdc6ffcbf961 # v5.7.0 | ||||||||||||
| with: | ||||||||||||
| distribution: temurin | ||||||||||||
| java-version: ${{ matrix.java }} | ||||||||||||
| cache: maven | ||||||||||||
|
|
||||||||||||
| # -C = strict checksum policy: a checksum mismatch on any resolved artifact | ||||||||||||
| # fails the build instead of printing a warning. | ||||||||||||
| # | ||||||||||||
| # -DskipTests still COMPILES the tests (it only skips running them), so this | ||||||||||||
| # job covers what CI can actually prove here: the enforcer rules, every pinned | ||||||||||||
| # plugin version, and that main + test sources compile at source/target 1.7 on | ||||||||||||
| # each JDK. | ||||||||||||
| # | ||||||||||||
| # The tests themselves are deliberately NOT run. They are credential-gated | ||||||||||||
| # live-integration tests, not offline units: 12 of the 14 reach | ||||||||||||
| # LocalBinary.getBinary(), which downloads the real BrowserStackLocal binary | ||||||||||||
| # from an authenticated endpoint and returns HTTP 401 without | ||||||||||||
| # BROWSERSTACK_ACCESS_KEY. A keyless runner therefore cannot pass them, and | ||||||||||||
| # wiring a key in would both run real tunnels on every push and still fail for | ||||||||||||
| # pull requests from forks (which get no secrets). | ||||||||||||
| # | ||||||||||||
| # To run the full suite locally: | ||||||||||||
| # BROWSERSTACK_ACCESS_KEY=... BROWSERSTACK_USERNAME=... mvn -B -C test | ||||||||||||
| - name: Build (compile, tests skipped - see above) | ||||||||||||
| run: mvn -B -C -Dgpg.skip -DskipTests clean verify | ||||||||||||
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[for-human] Whether to take a new CI workflow here at all is a human policy call, not something to settle by more fixing.
The security finding (CWE-494, the unverified Maven download) is closed by deleting
.travis.ymlalone.build.ymlis the one purely additive piece, and as it stands it needs work before it is an asset rather than a permanently-red check (see the blocking comment on line 39). Two defensible outcomes:validatepassed on every leg before surefire failed, which is how we knowrequirePluginVersions/banDynamicVersions/requireReleaseDepsand the pinned lifecycle plugins are clean across the matrix, including on thedeployandsitelifecycles the rules introspect.I have no basis to pick between those from the diff. Flagging for the human rather than sending it back to be re-fixed either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed this is the human's call, and I am not trying to settle it by fixing. Leaving both options open.
One factual update so the decision is made on current state rather than the state you reviewed: the credential problem is fixed in
2fb05a5, so option 1 no longer carries "permanently-red check" as a cost — all three legs are green on the runner. The trade that remains is the real one you named: java becomes the only Local binding with build CI, against the validation value you describe.Your point about the enforcer rules already having been validated across the matrix is worth keeping either way —
validatedid pass on all three legs even in the red runs, which is independent evidence the nine pins and the three rules are clean on 8/11/17. If the decision is to dropbuild.yml, that evidence still stands and the deletion plus the pom pinning ship unaffected; the security finding is closed by the deletion alone.Also flagged in the PR body and the Jira comment so whoever decides sees it without reading this thread.