feat: Add handling for a optional configurable delay in days to prevent package downloads #142
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
| this(path, testResult, ignores, Optional.empty()); | ||
| } | ||
|
|
||
| public MonitoredArtifact(String path, TestResult testResult, Ignores ignores, Optional<Instant> createdDate) { |
There was a problem hiding this comment.
As far as I'm aware, passing Optional as parameters is still generally considered bad practice. It's much better as a return type than as a parameter.
Better may just be to make this Instant then make getCreatedDate():
public Optional<Instant> getCreatedDate() {
return Optional.ofNullable(createdDate);
}
There was a problem hiding this comment.
This has been changed to be just Instant. The constructor will default the value to null if its not passed.
| Instant now = Instant.now(); | ||
| long daysSinceCreation = ChronoUnit.DAYS.between(createdDate.get(), now); | ||
|
|
||
| if (daysSinceCreation < delayDays) { |
There was a problem hiding this comment.
What exactly is the createdDate for an artifact? Is it the instant it was published to some base repo (e.g. maven central or npmjs registry)? Or is it the instant it was published/downloaded to this particular artifactory?
If the latter, I am concerned about how this will behave for artifacts when the artifactory is operating as a mirror.
If they want to update from [email protected] to [email protected], will the first attempt succeed due to not having createdDate` information yet, then subsequent attempts fail for X days? Or will the first attempt download the artifact to the repo, then deny the user access for X days?
Neither seems particularly good to me
There was a problem hiding this comment.
According to the docs, the createdDate for an artifact is when it was published to a particular Artifactory instance. My understanding is that the artifact will already be present in Artifactory when the Gatekeeper Plugin runs, which will prevent it from being downloaded out of Artifactory by a first-party user. Any downloads will be prevented for the duration of that delay parameter.
There was a problem hiding this comment.
This has been changed to use the lastModifiedDate from Artifactory PAPI.
core/src/main/java/io/snyk/plugins/artifactory/scanner/ScannerModule.java
Outdated
Show resolved
Hide resolved
core/src/main/java/io/snyk/plugins/artifactory/scanner/ScannerModule.java
Outdated
Show resolved
Hide resolved
|
I think I'd want to see some additional tests exercising this particular behaviour if possible |
…annerModule to use local date. Add unit test for created date delay
… preliminary unit test
|
^ This is really cool but I think it has a mini-oversight. Artifactory likely has both internal and external packages. The age delays make perfect sense for external packages, but could actually be disruptive to users for internal packages. I think that should be configurable independently to make this production safe for everyone. |
|
|
||
| LOG.debug("Found repository type: {}", repoType); | ||
|
|
||
| return repoType == "remote"; |
There was a problem hiding this comment.
| return repoType == "remote"; | |
| return repoType.equals("remote"); |
MMC requested the ability to introduce a delay in days that prevents packages from being downloaded (by throwing a 403) if the package creation date is newer than the current time minus the delay period. This change is in response to the SHA1-Halud malicious package suite of vulns. By introducing this delay period, there is time for security researchers to identify malicious packages before they are being used by the customer's developers.