-
Notifications
You must be signed in to change notification settings - Fork 350
feat: quarantine based on scanner malicious verdict #1991
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: main
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 |
|---|---|---|
|
|
@@ -50,6 +50,23 @@ public String extractString(String response, String format, String path) throws | |
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Extract a boolean value from a response. Accepts JSON booleans and string forms ("true"/"false"). Returns null | ||
| * when the path is missing or the value is not a boolean. | ||
| */ | ||
| public Boolean extractBoolean(String response, String format, String path) throws ScannerException { | ||
| if (response == null || path == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return switch (format.toLowerCase()) { | ||
| case "json" -> extractJsonBoolean(response, path); | ||
| case "xml" -> throw new UnsupportedOperationException("XML extraction not yet implemented"); | ||
|
Member
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. question: if not supported "yet" why don't just make it fall under default?
Contributor
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. yeah, remove all unsupported exceptions and let it fall through to the default case
Contributor
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. Can remove them |
||
| case "text" -> throw new UnsupportedOperationException("Text extraction not yet implemented"); | ||
| default -> throw new IllegalArgumentException("Unsupported format: " + format); | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Extract a list of objects from a response. | ||
| */ | ||
|
|
@@ -79,6 +96,26 @@ private String extractJsonString(String json, String jsonPath) throws ScannerExc | |
| return node != null && !node.isMissingNode() ? node.asString(null) : null; | ||
| } | ||
|
|
||
| private Boolean extractJsonBoolean(String json, String jsonPath) throws ScannerException { | ||
| JsonNode node = pickFirst(parseJson(json), jsonPath); | ||
| if (node == null || node.isMissingNode() || node.isNull()) { | ||
| return null; | ||
| } | ||
| if (node.isBoolean()) { | ||
| return node.asBoolean(); | ||
| } | ||
| if (node.isString()) { | ||
| String value = node.asString(null); | ||
| if ("true".equalsIgnoreCase(value)) { | ||
| return true; | ||
| } | ||
| if ("false".equalsIgnoreCase(value)) { | ||
| return false; | ||
| } | ||
| } | ||
| return null; | ||
| } | ||
|
|
||
| private List<Map<String, Object>> extractJsonList(String json, String jsonPath) throws ScannerException { | ||
| JsonNode root = parseJson(json); | ||
| List<JsonNode> nodes = evaluateJsonPath(root, jsonPath); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,7 @@ | |
|
|
||
| import java.io.File; | ||
| import java.util.ArrayList; | ||
| import java.util.Collections; | ||
| import java.util.HashMap; | ||
| import java.util.List; | ||
| import java.util.Map; | ||
|
|
@@ -405,26 +406,33 @@ private Scanner.Result parseResult( | |
| responseConfig.getSummaryPath()); | ||
| } | ||
|
|
||
| // Extract explicit malicious verdict, if configured. A configured path that fails to resolve is treated as | ||
|
Member
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. thought (non-blocking): given that the scanner result parsing method it's growing it might be worth considering to break it down into some easy to digest methods.
Contributor
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. I’ll give it light refactor to make it more readable |
||
| // a scan failure | ||
| Boolean maliciousVerdict = null; | ||
| if (responseConfig.getMaliciousPath() != null) { | ||
| maliciousVerdict = responseExtractor.extractBoolean( | ||
| response, | ||
| responseConfig.getFormat(), | ||
| responseConfig.getMaliciousPath()); | ||
| if (maliciousVerdict == null) { | ||
| throw new ScannerException( | ||
|
Contributor
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. If a malicousPath is defined but could not be extracted that could be due to various reasons:
Failing the scan if the verdict is not present is oth not desirable. If no verdict is present then we should continue with the normal threats path imho to make that more failure resiliient.
Member
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. aren't we already failing for similar reasons above?
Contributor
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. I had made the behavior fail closed rather than just warn since a misconfiguration for receiving verdicts from a provider felt significant enough to raise immediate concerns. Otherwise the configured integration may be silently (or quietly) allowing publishing of extension with malicious verdicts due to a misconfiguration or API change of the underlying provider. We do still fallback to the legacy behavior where any findings block publishing, so if instead its easier to setup monitoring we could just log a warning |
||
| "Scanner '" + scannerName + "' has malicious-path '" + responseConfig.getMaliciousPath() | ||
| + "' configured, but it did not resolve to a boolean in the response"); | ||
| } | ||
| } | ||
|
|
||
| // Extract threats | ||
| String threatsPath = responseConfig.getThreatsPath(); | ||
| if (threatsPath == null) { | ||
| // No threats path - assume clean | ||
| return Scanner.Result.clean(summary); | ||
| List<Scanner.Threat> threats = Collections.emptyList(); | ||
| if (threatsPath != null) { | ||
| List<Map<String, Object>> threatObjects = responseExtractor.extractList( | ||
| response, | ||
| responseConfig.getFormat(), | ||
| threatsPath); | ||
| threats = mapThreats(threatObjects, responseConfig); | ||
| } | ||
|
|
||
| List<Map<String, Object>> threatObjects = responseExtractor.extractList( | ||
| response, | ||
| responseConfig.getFormat(), | ||
| threatsPath); | ||
|
|
||
| // Map threats | ||
| List<Scanner.Threat> threats = mapThreats(threatObjects, responseConfig); | ||
|
|
||
| if (threats.isEmpty()) { | ||
| return Scanner.Result.clean(summary); | ||
| } else { | ||
| return Scanner.Result.withThreats(threats, summary); | ||
| } | ||
| return Scanner.Result.of(threats, summary, maliciousVerdict); | ||
| } | ||
|
|
||
| /** | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -473,6 +473,12 @@ public static class ResponseConfig { | |
| // Surfaced verbatim in ScanCheckResult.summary when present | ||
| private String summaryPath; | ||
|
|
||
| /** | ||
| * JSONPath to a boolean malicious verdict on the result response (e.g. "$.verdictData.isMalicious"). When set, | ||
| * quarantine is driven by this verdict rather than "any findings present". | ||
| */ | ||
| private String maliciousPath; | ||
|
Member
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. question (non-blocking): if this points to a
Contributor
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. yeah lets rename that to maliciousVerdictPath to make it clear
Contributor
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. Will rename 👍 |
||
|
|
||
| // For error detection | ||
| private String errorPath; // Path to error message | ||
| private String errorCondition; // Expression to detect errors | ||
|
|
@@ -527,6 +533,13 @@ public void setSummaryPath(String summaryPath) { | |
| this.summaryPath = summaryPath; | ||
| } | ||
|
|
||
| public String getMaliciousPath() { | ||
| return maliciousPath; | ||
| } | ||
| public void setMaliciousPath(String maliciousPath) { | ||
| this.maliciousPath = maliciousPath; | ||
| } | ||
|
|
||
| public String getErrorPath() { | ||
| return errorPath; | ||
| } | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,33 +67,51 @@ class Result { | |
| private final boolean clean; | ||
| private final List<Threat> threats; | ||
| private final String summary; | ||
| private final Boolean maliciousVerdict; | ||
|
|
||
| private Result(boolean clean, List<Threat> threats, String summary) { | ||
| private Result(boolean clean, List<Threat> threats, String summary, Boolean maliciousVerdict) { | ||
| this.clean = clean; | ||
| this.threats = new ArrayList<>(threats); | ||
| this.summary = summary; | ||
| this.maliciousVerdict = maliciousVerdict; | ||
| } | ||
|
|
||
| @NonNull | ||
| public static Result clean() { | ||
| return new Result(true, Collections.emptyList(), null); | ||
| return new Result(true, Collections.emptyList(), null, null); | ||
| } | ||
|
|
||
| @NonNull | ||
| public static Result clean(@Nullable String summary) { | ||
| return new Result(true, Collections.emptyList(), summary); | ||
| return new Result(true, Collections.emptyList(), summary, null); | ||
| } | ||
|
|
||
| @NonNull | ||
| public static Result withThreats(@NonNull List<Threat> threats) { | ||
| return new Result(false, threats, null); | ||
| return new Result(false, threats, null, null); | ||
| } | ||
|
|
||
| @NonNull | ||
| public static Result withThreats(@NonNull List<Threat> threats, @Nullable String summary) { | ||
| return new Result(false, threats, summary); | ||
| return new Result(false, threats, summary, null); | ||
| } | ||
|
|
||
| /** | ||
| * Builds a result directly from a scanner's findings and its explicit malicious verdict (e.g. Argus | ||
| * verdictData.isMalicious). Prefer this over clean()/withThreats() whenever a verdict is available: naming a | ||
| * malicious result via "clean" reads as contradictory, since findings and the safety verdict are independent | ||
| * signals here. "Clean" (no findings) is derived from the threats list itself. | ||
| */ | ||
| @NonNull | ||
| public static Result of(@NonNull List<Threat> threats, @Nullable String summary, @Nullable Boolean malicious) { | ||
| return new Result(threats.isEmpty(), threats, summary, malicious); | ||
|
Contributor
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. Is this really correct? If there are no threats but the scan would still bear a maliciousVerdict the result would still be clean. I have seen cases were the threat score was high but there was no specific threat identified and the scan was marked clean. I think the maliciousVerdict should also act as a gate keeper: if its true, the scan should not be marked as clean at all.
Contributor
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. I see in the code above the 2 states are treated differently:
however that is also a bit error prone as you need to take that always into account when doing changes to this code
Contributor
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. so a result can be
Contributor
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. Yeah clean was originally assigned to scans which returned no findings, which was associated with a clean verdict but in doing so conflated the meaning. Will see what I can do to see if we can strictly use that to refer to verdicts and potentially drop clean as an association with the number of findings |
||
| } | ||
|
|
||
| /** | ||
| * True when there are no findings recorded. This reflects the threats list only — it is not a safety verdict. A | ||
| * malicious verdict with zero findings is still isClean() == true; check {@link #hasMaliciousVerdict()} for the | ||
| * actual verdict. | ||
| */ | ||
| public boolean isClean() { | ||
| return clean; | ||
| } | ||
|
|
@@ -108,6 +126,25 @@ public List<Threat> getThreats() { | |
| public String getSummary() { | ||
| return summary; | ||
| } | ||
|
|
||
| /** | ||
| * Explicit malicious verdict from the scanner response, if configured. null means the scanner does not expose | ||
| * this signal. | ||
| */ | ||
| @Nullable | ||
| public Boolean isMalicious() { | ||
| return maliciousVerdict; | ||
| } | ||
|
|
||
| /** True when the scanner explicitly returned a malicious verdict. */ | ||
| public boolean hasMaliciousVerdict() { | ||
| return Boolean.TRUE.equals(maliciousVerdict); | ||
| } | ||
|
|
||
| /** True when the scanner explicitly returned a non-malicious verdict. */ | ||
| public boolean hasBenignVerdict() { | ||
| return Boolean.FALSE.equals(maliciousVerdict); | ||
| } | ||
| } | ||
|
|
||
| /** | ||
|
|
||
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.
This functionality is intended to allow open vsx maintainers to override known false positives returned from a provider, given the file hash of the matched behavior is exactly the same.