Pulp

Extending the content app to make a call during the workflow could be pretty straight forward. This would cause the entire file to be downloaded before any part of it could be served, is that ok?

So the workflow for the content app would be:

1. User requests the file to be downloaded from a repo with, e.g. policy="on_demand"
2. pulpcore-content downloads that file (not streaming any bit to the user yet)
3. Calls out to the virus scanner knowing the file path
4. Reads an "ok" to proceed based on the return code maybe?
5. Serves and saves the file per the on_demand policy.

For steps 3 and 4, I imagined there could be a system-wide config with a script that pulp would call that an admin would configure. The path to the file would be the first positional argument to the script. Then for set 4, the script would return an exit code of 0 would tell pulp to proceed, anything else to not.

@ByteSore what do you think about all this?

Sounds like a plan.

I was thinking..
Since the scanner takes a while to spin up, scan all the files and do it's thing maybe it's a good thing to check if there are multiple files being downloaded. (ie package with all dependencies)
Park them all in a folder or store all the file path's to a textfile which can be used by the scanner.
if you look at clamav, it knows an option to scan files listed in a textfile: --file-list=FILE

Should we implement scanning files on upload too? ...to close all holes.

I've been looking into this issue and came to a few additional conclusions

1. Having to wait for a virus scan result messes up the asynchronous nature of the streamed and on demand policy majorly. I am still unsure how to deal with heavy load on the virus scanner, especially with the streamed policy. With the on demand policy it at least triggers the virus scan, which means that somewhere in the future you can access the artifacts without the synchronous virus scanner call.
2. It would make sense to store the scan result in the database to prevent an infected file from being scanned over and over every time it is requested.
3. A rescan mechanism would be prudent once the basic functionality is implemented to prevent access to infected files which were not yet included in earlier antivirus definitions. Could be a TTL for a scan result or some batch action to periodically execute.
4. A HTTP status code 409 (conflicted resource) makes sense with a message to indicate the nature of the conflict is an infected artifact.

In the attempts to implement the functionality so far it feels like creating two new policies for streamed+scanner and on-demand+scanner. I'll add a PR soon.

Some more food for though: Should we keep a content artifact for infected content or remove it from the repository?

vriesdemichael wrote:

I've been looking into this issue and came to a few additional conclusions

1. Having to wait for a virus scan result messes up the asynchronous nature of the streamed and on demand policy majorly. I am still unsure how to deal with heavy load on the virus scanner, especially with the streamed policy. With the on demand policy it at least triggers the virus scan, which means that somewhere in the future you can access the artifacts without the synchronous virus scanner call.

Agreed. I think we'll have to see in practice how long we can delay clients and prevent timeouts. Some probably have built-in retry behaviors, and others can support receiving an HTTP 429 with maybe a Retry-After header. It's hard though because what I've learned is that each client is different and some just won't work. As an alternative, although it would be as great for users, perhaps systems with virus scanning should not offer on_demand or streamed at all.

2. It would make sense to store the scan result in the database to prevent an infected file from being scanned over and over every time it is requested.

That could be helpful. One of my concerns is storing 'infected' artifacts at all because pulpcore and plugin code directly accesses Artifacts so if they are there they will be "used". For this reason I was thinking it would be best to not save Artifacts that fail virus scanning at all.

3. A rescan mechanism would be prudent once the basic functionality is implemented to prevent access to infected files which were not yet included in earlier antivirus definitions. Could be a TTL for a scan result or some batch action to periodically execute.

Agreed. We have added administrator commands for these kinds of things here. In other cases we've added API endpoints for users to be able to trigger repo-level or system-wide checks, e.g. the repair command here. The former is good if administrators are the actor, the latter is if users are the actor. Getting them to run periodically can be wrapped around either approach.

4. A HTTP status code 409 (conflicted resource) makes sense with a message to indicate the nature of the conflict is an infected artifact.

Sounds good to me.

In the attempts to implement the functionality so far it feels like creating two new policies for streamed+scanner and on-demand+scanner. I'll add a PR soon.

What did you think about disallowing these altogether? I'm ok with keeping them; I'm just worried about clients timing out while virus scanning occurs.

I'd like to help answer any questions you have and see the PR when it comes in to help it get through the process.

Some more food for though: Should we keep a content artifact for infected content or remove it from the repository?

I responded to this top-to-bottom, but similar to what I was pitching earlier, I think actually don't save the Artifact at all. A non-saved-Artifact also cannot be associated with Content and can't be associated with a repository.

Some more food for though: Should we keep a content artifact for infected content or remove it from the repository?

I responded to this top-to-bottom, but similar to what I was pitching earlier, I think actually don't save the Artifact at all. A non-saved-Artifact also cannot be associated with Content and can't be associated with a repository.

Just a thought on this.

If the data is not stored any where, it will make it quite hard to investigate issues after the fact (specifically false positives), if we cant store the data in a quarantined directory, then at least keeping a record of the source URL as well as the hash of the downloaded artifact some where, so that it can be retrieved and compared.

Of course clamav supports the --move switch to be able to quarantine infected files, would this be supported in any way?

Just reply

wibbit wrote:

Some more food for though: Should we keep a content artifact for infected content or remove it from the repository?

I responded to this top-to-bottom, but similar to what I was pitching earlier, I think actually don't save the Artifact at all. A non-saved-Artifact also cannot be associated with Content and can't be associated with a repository.

Just a thought on this.

If the data is not stored any where, it will make it quite hard to investigate issues after the fact (specifically false positives), if we cant store the data in a quarantined directory, then at least keeping a record of the source URL as well as the hash of the downloaded artifact some where, so that it can be retrieved and compared.

Of course clamav supports the --move switch to be able to quarantine infected files, would this be supported in any way?

Just replying to my self, it seems to make the most sense to leave any quarantining of data down to the scanning program, as it can also move the data to a location that pulp would have no access to.

This is certainly some thing that I can see us implementing in due course.