SLCORE-2239 Add on-demand plugin signature verification#1921
SLCORE-2239 Add on-demand plugin signature verification#1921Krosovok wants to merge 1 commit intofeature/on-demand-analyzersfrom
Conversation
SummaryThis PR implements PGP signature verification for on-demand plugins to ensure downloaded C-family and C# analyzers haven't been tampered with. It adds a new What reviewers should knowStart here: Review Key decisions:
Test coverage: The test class is well-designed with 8 scenarios covering the happy path, invalid signatures, missing/corrupt files, and edge cases like compressed signatures and unknown keys. Test resources include a real test JAR and various signature formats. Minor change: The
|
...ain/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandPluginSignatureVerifier.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Adds a PGP-based signature verification utility intended to validate on-demand downloaded plugin JARs against a SonarSource public key, with accompanying test fixtures.
Changes:
- Introduce
OnDemandPluginSignatureVerifierusing BouncyCastle OpenPGP APIs to verify detached.ascsignatures. - Add SonarSource public key to main resources and a test-only key/signature + test JAR fixture for verification tests.
- Add JUnit tests covering valid signature, tampered JAR, and missing signature scenarios.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/core/src/main/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandPluginSignatureVerifier.java | New verifier implementation for detached PGP signatures. |
| backend/core/src/main/resources/sonarsource-public.key | Adds the SonarSource public key used for verification in production. |
| backend/core/src/test/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandPluginSignatureVerifierTest.java | Adds unit tests for the verifier behavior. |
| backend/core/src/test/resources/sonarsource-public.key | Test-only public key used to validate the test signature. |
| backend/core/src/test/resources/sonar-cpp-plugin.jar.asc | Detached signature fixture used by tests. |
| backend/core/src/test/resources/sonar-cpp-plugin-test.jar | Test JAR fixture whose content matches the detached signature. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
...ain/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandPluginSignatureVerifier.java
Show resolved
Hide resolved
...ain/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandPluginSignatureVerifier.java
Show resolved
Hide resolved
...ain/java/org/sonarsource/sonarlint/core/plugin/ondemand/OnDemandPluginSignatureVerifier.java
Outdated
Show resolved
Hide resolved
3ee607b to
3e86f2b
Compare
There was a problem hiding this comment.
Conclusion: Solid, focused PR. The PGP verification logic is correct, the BouncyCastle provider is passed explicitly rather than registered globally (the right call), the compressed/uncompressed signature format handling is thorough, and the test setup using a separate test key pair is well-structured. No logic duplication, no missed bugs.
6b58dba to
8582fe5
Compare
8582fe5 to
921523f
Compare
|
|
|
||
| return signature.verify(); | ||
| } | ||
| } catch (IOException | PGPException e) { |
There was a problem hiding this comment.
Every other failure path in this method logs at ERROR before returning false — missing signature, unknown key, parse exception — but the most security-sensitive case does not: when signature.verify() itself returns false the call site receives false with zero log output. An operator whose JAR has been tampered with would see nothing in the logs to indicate why verification failed.
Add an explicit error log before returning:
boolean valid = signature.verify();
if (!valid) {
LOG.error("PGP signature verification failed for plugin '{}' — file may have been tampered", pluginKey);
}
return valid;The pluginKey is available in the enclosing verifyPgpSignature parameter, so no extra plumbing is needed.
- Mark as noise
| <!-- More recent versions require Java 21 --> | ||
| <jooq.version>3.19.15</jooq.version> | ||
| <flyway.version>11.20.3</flyway.version> | ||
| <bouncycastle.version>1.78.1</bouncycastle.version> |
There was a problem hiding this comment.
I think you can use the latest version
| @@ -0,0 +1,2 @@ | |||
| cfamily.version=${cfamily.version} | |||
There was a problem hiding this comment.
WDYT about moving these resource files in a sub folder?



No description provided.