Conversation
bdemeo12
left a comment
There was a problem hiding this comment.
Looks great !
One tiny general nit, there are a lot of comments in this PR. You could move the final design, and reasons as to why you chose to do xyz into a confluence doc, and clean up a lot of these comments!
bgardiner
left a comment
There was a problem hiding this comment.
looks good. a few questions within about the implementation
lib/analyzer/java-runtime/parser.ts
Outdated
| if (!content || content.trim().length === 0) { | ||
| return null; | ||
| } | ||
| if (content.length > MAX_CONTENT_LENGTH) { |
There was a problem hiding this comment.
How did you determine MAX_CONTENT_LENGTH and MAX_LINE_COUNT?
Is the objective of these checks to mitigate dealing with large files that don't happen to be release files? If so, would it be better to use getContentAsBuffer within getJavaRuntimeReleaseContent instead of getContentAsString (i.e., only load up to MAX_CONTENT_LENGTH into memory)? Maybe not... just trying to understand your reasoning,
There was a problem hiding this comment.
yes - my original intention was to block reading really long files in that are not actually release files. We don't however do that for our other parsers within the repo so it might be best to leave out. I agree it would make sense to enforce this when we are reading the file into a buffer rather than a post-process check. But I'm leaning towards removing these constraints altogether since it is possible for the java modules list to be quite long. It would be hard to estimate reasonable values for MAX_CONTENT_LENGTH and MAX_LINE_COUNT without making too many assumptions about what the customers might have in the release file. 🤔
There was a problem hiding this comment.
Interestingly, streamStoString has a streamSize property, but it appears to be completely ignored:
snyk-docker-plugin/lib/stream-utils.ts
Line 15 in d9a0b98
streamToJson: snyk-docker-plugin/lib/stream-utils.ts
Lines 74 to 95 in d9a0b98
Ideally we would be more defensive here and not read a potentially unbounded file contents into memory, but it's not unprecedented in the repo for other static file analysis (like the os package databases), so we can just keep it for now. However, I think it might be good to create a ticket to track this improvement (not just for release file parsing but other instances of file parsing).
|
TODO: update test snapshots as well |
92c7007 to
e816533
Compare
PR Reviewer Guide 🔍
|
lib/analyzer/java-runtime/parser.ts
Outdated
| if (!content || content.trim().length === 0) { | ||
| return null; | ||
| } | ||
| if (content.length > MAX_CONTENT_LENGTH) { |
There was a problem hiding this comment.
Interestingly, streamStoString has a streamSize property, but it appears to be completely ignored:
snyk-docker-plugin/lib/stream-utils.ts
Line 15 in d9a0b98
streamToJson: snyk-docker-plugin/lib/stream-utils.ts
Lines 74 to 95 in d9a0b98
Ideally we would be more defensive here and not read a potentially unbounded file contents into memory, but it's not unprecedented in the repo for other static file analysis (like the os package databases), so we can just keep it for now. However, I think it might be good to create a ticket to track this improvement (not just for release file parsing but other instances of file parsing).
e816533 to
f3d9e76
Compare
| } | ||
|
|
||
| export interface BaseRuntime { | ||
| type: string; |
There was a problem hiding this comment.
Since the only current value is "java" would narrowing this to a literal type (or a discriminated union) provides compile-time safety:
For example:
export interface BaseRuntime {
type: "java";
version: string;
}| export const getJavaRuntimeReleaseAction: ExtractAction = { | ||
| actionName: "java-runtime-release", | ||
| filePathMatches: (filePath) => | ||
| filePath === normalizePath("/opt/java/openjdk/release"), |
There was a problem hiding this comment.
Claude suggests that:
/opt/java/openjdk/release is the Eclipse Adoptium/Temurin convention. Many widely-used JVM base images use different paths:
- eclipse-temurin → /opt/java/openjdk/release ✓
- openjdk (official Docker) → /usr/local/openjdk-/release
- Debian/Ubuntu default-jdk → /usr/lib/jvm/java--openjdk-/release
- Oracle → /usr/java//release
Should we at least expand to a few common other release file locations:
filePathMatches: (filePath) =>
filePath === normalizePath("/opt/java/openjdk/release") ||
filePath.startsWith(normalizePath("/usr/local/openjdk-")) ...
Goal: Expand support for JVM vuln scanning by parsing the release file.
Jira Ticket:
Discovered it is possible to parse the release file generated when Java is loaded in an image.
An example release file:
This PR extracts the Java version from the release file and sends the version to registry to be scanned for vulns.
We have a separate scan result, keyBinariesHashes, that discovers java binaries within the image and will send a hash of the binary to registry. In registry, these hashes are attempted to be resolved to a package (named 'openjdk-jre') and version with a call to
hash-lookup. A depGraph is generated from the matched java packages, with the package manager is "upstream". These naming conventions are set in order to match vulns with our db.In registry, the extracted java version will be added to the keyBinariesHashes flow to be added to the depGraph and scanned for vulnerabilities. Since package names in the depGraph need to be unique, there can only be one 'openjdk-jre' package within the tree. This restraint means that we only add the java version parsed from the release file if there were no successfully matched hash-lookup binaries. In other words, this feature only supports one version of Java to be scanned.
Overview of Changes Within Snyk-docker-plugin: