Initial working draft of pluggable scanning proposal and working group #82
Conversation
…ing group. Signed-off-by: Zach Hill <zach@anchore.com>
|
@steven-zou @lizrice and @danielpacak RFC please. This is pretty rough, but outlines my thinking. I have not yet had time to build the actual interfaces or go into implementation details yet, that should be follow-up once we've agreed on overall direction and objectives. Thanks! |
| The plugin interface will support the following high-level capabilities: | ||
|
|
||
| 1. Scanning images and returning a vulnerability listing mapping vulnerabilities to artifacts in the image | ||
| 2. Providing a image check pass/fail recommendation based on the specific capabilities/polices of the scanner (new) |
There was a problem hiding this comment.
Does Harbor already do this kind of policy checking? I would think this is better outsourced to a policy component (e.g. OPA), for separation of concerns.
There was a problem hiding this comment.
The current harbor implementation includes an optional "policyCheck" api interceptor that as I understand it will block image pulls if the image fails the policy check. The current implementation of that check uses the last vuln scan result and uses a simple severity comparison.
My proposal (at least thus far) has intentionally minimal on the impact to the larger Harbor architecture and attempts to bring support for other scanners in with minimal changes to other aspects of the system. I agree there are broader changes possible to support policy-based access gating to images, and the proposal's addition of an explicit image check from the scanner would allow the scanner to provide an input to any such policy evaluation system build into Harbor itself, either by plugin of OPA or another system.
I think what you're suggesting makes sense but could be implemented independently of this proposal and this proposal's changes would fit into that work as an input to the evaluation.
There was a problem hiding this comment.
@zhill is right. Currently, harbor provides an interceptor to do the policy evaluation and checking work. They can be driven by some other tools like OPA in the feature (actually we have started to consider if we should introduce a unique and powerful policy engine to handle the policy related things). But that can be performed in another proposal. In this proposal, as @zhill mentioned, we should define a flexible interface to support the checking process, either by an interceptor or by the OPA, etc..
proposals/new/Pluggable-Scanning.md
Outdated
| The plugin interface will provide: | ||
|
|
||
| 1. Image lifecycle operations: | ||
| 1. Image added to Harbor: execute a scan on the digest |
There was a problem hiding this comment.
When you say "on the digest" do you mean the scanner would just have access to the image digest? I think a full scanner would need access to the image contents (to be able to do things like check for sensitive data). But later you're proposing providing the scanner with a registry credential, so are you thinking that the scanner can pull the image to do its scan?
There was a problem hiding this comment.
Does the scan need to be synchronous? Or does the interface here need to handle an async reponse (so that the UI can display something like "scanning")?
There was a problem hiding this comment.
RE "on the digest": Yes, I was unclear. The expectation is that the scanner driver is given the necessary reference to be able to pass the actual scanner (either a local pieces of code or an external service) the ability to pull the image content using the digest of the manifest and standard registry API operations. I agree I should be more clear on that aspect in the proposal, and I'll make some additions to that effect.
Because Harbor uses the docker registry for image data storage it supports non-local backends so there is no guarantee that image data is available locally to a scanner driver (or a co-located scanner), so I think registry API is the baseline that can be provided to the scanner driver. Each driver implementation may operate on that differently based on the scanner's data access needs.
There was a problem hiding this comment.
RE: synchronous scans... no, the current scan implementation uses an async job that is scheduled and managed by the job service in Harbor. Users can set scan schedules as well as explicitly request a scan. If the admin has configured the vulnerability policy check in the API proxy then the image cannot be pulled by users until the scan is complete and the resulting vulnerability result "passes" the policy check operation. The scan status for an image is tracked explicitly and may have a pending or in-progress state.
Your question brings up another point we'll need to investigate further: does the scanner have to be deployed "internally" to the rest of Harbor deployment, where "internal" means on the same host-local network such that it can access the docker registry directly without going thru the Harbor API proxy that handles external user requests. I think that currently it does, which will be problematic for any scanner that cannot be co-located. Will need to follow up on this question and investigate further.
There was a problem hiding this comment.
Agree to the way of providing enough info to the backend scanner to let it pull the content by itself and then trigger scanning.
There was a problem hiding this comment.
Your question brings up another point we'll need to investigate further: does the scanner have to be deployed "internally" to the rest of Harbor deployment, where "internal" means on the same host-local network such that it can access the docker registry directly without going thru the Harbor API proxy that handles external user requests. I think that currently it does, which will be problematic for any scanner that cannot be co-located. Will need to follow up on this question and investigate further.
internal and communicated via Rest API endpoint should both be considered. In most cases, we only support internally and optionally deploying 1 scanner engine as the default one. Other ones must be deployed and configured into Harbor to take effect by the admins themselves.
proposals/new/Pluggable-Scanning.md
Outdated
| 1. Image added to Harbor: execute a scan on the digest | ||
| 2. Image deleted from Harbor: delete the image in the scanner, configurable to be executed or not, depending on user requirements | ||
| 3. Background scan update: execute a scan on an image previously scanned | ||
| 1. For some scanners this will be the same as 2.1, but for others it may differ in what/if data is retrieved. |
There was a problem hiding this comment.
Yes, correct. Will fix
| * ScanImage | ||
| * Description: The base scan operation to process an image's content and return a list of found vulnerabilities, some scan metadata, and a check result (scanner specific) | ||
| * Params: image digest, registry URL, pull credential, tags applied to the digest | ||
| * Output: |
There was a problem hiding this comment.
Does Harbor use Clair's representation of vulnerability info at the moment?
There was a problem hiding this comment.
Essentially, yes. It saves the listing effectively as-is from Clair, but also creates a summary with counts for each severity category that is used by things like visualization in the UI and the API proxy's policyCheck.
There was a problem hiding this comment.
It does not have to return the scan result, the scan may be async, so how about returning an uri containing all the information to fetch the scan result?
| scan_status: boolean | ||
| vulnerabilities: array of vulnerable artifacts in the image | ||
| scan_metadata: json object, scanner specific | ||
| image_check: boolean (true if image is 'ok' according to scanner implementation and/or policy) |
There was a problem hiding this comment.
Does Harbor currently have this kind of policy decision built in? I am hesitant about it partly because of separation of concerns ("do one thing well") but also because image check policy might be context-dependent. For example,
- the policy might be stricter for a production cluster than for a test cluster
- different teams in an organization might have different requirements around (say) permissible base images, or the vulnerabilities they are allowed to whitelist.
There was a problem hiding this comment.
This field simply to provides a facility for a scanner to indicate a recommendation. What Harbor does with it is open for discussion and mostly for future work. The check result is simply an evaluation of the image content since that is the only context present for a scanner in the base case. I can definitely see a case where the actual admission decision for an image download by a user is based on identity attributes, properties of the project (the namespace for repositories in Harbor), and other user-configurable metadata.
A more robust policy system is an interesting and certainly useful addition, but I think falls outside the scope of this proposal.
There was a problem hiding this comment.
A snapshot of Harbor vulnerability checking which designed as one of the project security settings:
For different environments, you can set different severity levels restriction.
About CVE whitelist, it has been planned to deliver in the next release V1.9 (probably released at end of Aug. ).
| * Output: array of digests to re-scan | ||
|
|
||
| * CheckImage | ||
| * Description: A scanner-specific synchronous check of an image. Provided for optional use in interceptors or other cases where Harbor may want an up-to-date check result from the scanner to make an admission decision. |
There was a problem hiding this comment.
Is this admission from the perspective of "let this image be stored in the registry" or "let this image be deployed to the cluster"?
There was a problem hiding this comment.
The current Harbor implementation is to block API requests in the API proxy component, and those requests are for a download of an image manifest (initiating an image pull) rather than the push of the image. The checks are configurable on a per-project basis in Harbor and also include things like Notary signature checks. See: https://github.com/goharbor/harbor/blob/master/src/core/proxy/interceptors.go
proposals/new/Pluggable-Scanning.md
Outdated
| the mechanisms and inputs to the check decision will be scanner specific and must be handled by the driver implementations themselves. Additional future work in the | ||
| policyChecker or new interceptors can leverage the check recommendations from the scanners or utilize the new CheckImage operation to have control over the staleness of | ||
| the check result. The interceptor implementation(s) can decide to use either the last known recommendation or request a new evaluation during request processing. | ||
|
|
There was a problem hiding this comment.
As mentioned above, I don't currently think that the policy checking should be in scope (but prepared to be convinced otherwise!)
There was a problem hiding this comment.
Per previous comments, I agree that overhauling the existing check capabilities is a larger piece of work than just enabling more scanners than Clair. As it stands, Harbor is using the data from the scanner to implement a very simple policy, which I assume must continue to be supported. I think that scanners that do have policy capabilities based purely on the content of the image itself can contribute useful data into a broader decision in the registry. I think the plugin architecture here should support conveying such information without scope creep into broader policy implementations, which we both agree should be treated as a separate enhancement.
The individual drivers are free to consume much more detailed responses from the actual scanners in order to generate the final decision, or alternatively the plugin interface could support returning a more flexible response structure to be persisted and made available for users and future work on policy-based image access in Harbor.
There was a problem hiding this comment.
OK, that makes sense, thanks. This approach leaves open the option for a scanner to choose to always return true, deferring the policy decision to a later stage (e.g. K8s admission control)
|
Really a nice start! Thanks. Let's work together now to continue the design/architecture work based on this version and push the work forward. Looking forward to making the feature ready in the near feature! @lizrice If you're going to attend the KubeCon Shanghai 2019 at the week of June 24th, maybe we can find some proper time to have a F2F talk and meet the Harbor teams (a pity, @zhill will not be there). To all (@zhill, @lizrice, @danielpacak @jerbia), Again, look forward to working with you all! CC @renmaosheng @michmike @alexvmw for awareness. |
steven-zou
added
the
area/interrogation-service
|
@steven-zou thanks! This PR does include a README.md for a wg-scanning working group, so let me know if that isn't in the right place or you'd prefer to split that into another PR or if you'll just do it for simplicity. I'm open to anything and can modify this PR/branch to remove that document if it helps. @lizrice thanks for the feedback. I'll be making some updates soon to address questions/confusion that you've identified and we can continue discussion as well. |
|
Sorry, I did not notice that file before. Anyway, I suggest submitting the wg-scanning readme in a separate PR because the proposal itself in this PR may be pended for a while to discuss but we can merge the readme earlier. Drafting and reviewing proposals are also works of this new WG ^_^! It has been taken effective now! What do you think? |
Yes, that makes sense. I will remove the file from this PR and open an new one for the WG. |
…nother PR for that process Signed-off-by: Zach Hill <zach@anchore.com>
|
@zhill for the scan results - should the scanning solution provide the vulnerabilities per layer, or an aggregated list of vulnerabilities that are effected per the scanned image? |
The details of the result format are a subject I'd like to discuss and add to the proposal. This proposal content was partly just a brain dump to trigger discussion and move the process forward, so there are definitely large gaps and that is one that needs to be addressed. Specifically, I think the output format (from the driver to Harbor, not a requirement on the scanner itself) should support layer associations with items on the Bill of Materials and the vulnerabilities matched to those entries, but it shouldn't require that level of detail. The layer ID would be an optional component of the artifact identifier struct. Scanners that don't implement per-layer results would omit that field and the semantics are that the result is for the rootfs as present in an image after construction to a container by the runtime (OCI/docker/whatever). I think the final format will look similar to the existing Clair-based result format, but not identical.
The plugin operations identify the content by the image digest (manifest digest) but drivers are free to implement the analysis however makes sense for the scanner. Layers in isolation may not fully contain enough information to perform BoM or vulnerability scans in many cases so the image as an immutable and ordered set of layers should be the entity that drives scan events. However, there is no requirement that a scanner analyze or even fetch all layers on each invocation, that will be implementation specific. Does that make sense? |
|
Overall, this was intended as a starting point and a brain-dump based on the very limited cycles I've been able to give to the work. I'm very happy to add other authors and iterate on the content in a significant way. I've not been able to devote the cycles to this that I had originally intended, but I want to be sure others can get involved at both the design and implementation levels and the work can continue to move forward. Off the top of my head, things to be added and additional detail needed:
I'll add some sections around these issues with callouts for specific discussion points and update the document based on the current feedback as well. Expect those updates next week. |
|
It's interesting that our cyber security architect asks us to integrate Harbor with Anchore service because Anchore is a good adding with Clair as Anchore can scan images with a predefined policy. @zhill I am seeing you are from Anchore, and when do you plan to start the implementation on this PR and how I can get engaged? Thanks |
Hi, @cafeliker . All progress on this PR should be reflected here. My cycles are fairly constrained at the moment, so additional hands for the framework and/or implementing an Anchore driver are greatly appreciated. Joining in for design discussion and/or implementation would be great! Once we (the working group) settle on an acceptable proposal, the next step is to create the issues to track development for the framework and individual drivers. The newly created working group will guide overall progress and your'e more than welcomed to join either officially (PR an update to the README (https://github.com/goharbor/community/tree/master/workgroups/wg-scanning) adding your name) or unofficially by following this PR and later development issues. |
|
@zhill I finally had a proper look into the Harbor's use cases and codebase to review this PR and follow the discussions. All what you've mentioned in the initial draft so far makes perfect sense to me. Especially after I deployed Harbor locally (to minkube) and traced what happens in the Having said that I was wondering how we can split the work of putting more details into the items you mentioned such as defining driver's interface or considering deployment strategies. I'm eager to pick any of those. Just let me know which one you're focusing on so we don't needlessly duplicate efforts. Meanwhile I'm trying to put together a minimal POC of integrating Aqua Security's microscanner by changing the code directly in ClairJob. Obviously it's not the goal of this RFC but just a valuable exercise to quickly identify integration/deployment complexities and later on evaluate more generic/abstract interfaces and data models that we're going to define. |
For collaborating on the proposal itself, I'm not sure what the best mechanism is. One option is to get this draft just good enough to merge as a work-in-progress and then we (anyone interested) can do follow-up PRs to modify it with new content subject to review by the other group members. I can amend the title to this as WIP and then we can merge and continue evolving it until there is a final PR marking it as finalized. That will allow others to contribute on the document directly without me having to be the only editor. I should be able to make progress on that as well as incorporating a good bit of the feedback here this week if that plan is ok with you all. |
I like the idea of smaller follow-up PRs once we merge this one with the feedback collected so far. |
|
@danielpacak where is your POC codes? I am trying to figure out where I can start with. My company has Anchore enterprise license, so I can perform some integration tests with Anchore service. |
I've actually just demo'd it yesterday internally with my team @aquasecurity. It's based on (IMHO) flexible architecture of scanner adapters, i.e. independent microservices implementing well defined API (REST or gRPC?) understandable by Harbor. The API would allow Harbor to submit a container's image and query the corresponding scan results. This effectively means that we can write a generic For the sake of my POC I've hacked directly in
So there is minimum impact on the existing Harbor's data model. The only configuration needed by Harbor in this approach would be Adapter's name and endpoint URL. The scaner/vendor specific configuration/tokens/licenses should be configured and managed by the Adapter itself. I'm putting together more details describing this approach so everyone can comment on that. Also I'll check with @lizrice how we can share the POC. |
|
I'm currently working on updating the doc with some draft interface specs and incorporating the feedback here. Should have that available very soon |
|
As we have set up the scanning WG, do you think if we need to run regular meetings to sync what we're doing and plan the next step/follow-ups? Maybe at the beginning bi-weekly is enough. |
|
|
||
| 1. Scanning images and returning a vulnerability listing mapping vulnerabilities to artifacts in the image | ||
| 2. Providing a image check pass/fail recommendation based on the specific capabilities/polices of the scanner (new) | ||
| 3. Cleanup of data in the scanner when an image is deleted from Harbor (for image lifecycle completeness) (new) |
There was a problem hiding this comment.
Cleanup is questionable as the scanner may very likely be shared by different services? if it has the knowledge that an image with digest sha256:xxxx is vulnerable, we may not need to remove it if this image is removed from Harbor?
There was a problem hiding this comment.
Agreed that the logic can be complex, and I think that can be handled in the plugin configuration, since that has the plugin-specific context, and/or the overall configuration that can be deployment specific. The intent is simply to provide a facility to allow the scanner to be told that an image is no longer in Harbor and thus may be GC'd if the scanner does have some state for the image and if the user wants it to be kept in sync with the registry. Such behavior is optional and a specific plugin may choose to implement the method as a no-op.
There was a problem hiding this comment.
@zhill How about Harbor send a webhook to scanner? I hope the scanner adapter or plugin should focus on enabling Harbor to scan artifacts, cleanup the data on the scanner may be out of the scope?
| The plugin interface will support the following high-level capabilities: | ||
|
|
||
| 1. Scanning images and returning a vulnerability listing mapping vulnerabilities to artifacts in the image | ||
| 2. Providing a image check pass/fail recommendation based on the specific capabilities/polices of the scanner (new) |
There was a problem hiding this comment.
As long as there's a vulnerability list provided by scanner, the pass/fail is not really necessary from scanner?
Is this a case we think valuable when different scanners report different pass/fail recommendations under same conditions (e.g. same vulnerability list)?
There was a problem hiding this comment.
In my experience, a vulnerability listing is the very bare minimum of useful information about an image's content, and often not sufficient for production security practices. Clair provides a very simple scan capability, but others like Anchore and Aqua provide much deeper analysis and capabilities such as secret detection, provenance, configuration checks, and metadata checks on the image content which all factor into acceptance criteria.
As I said in comments previously, a scanner may or may not implement this and Harbor/its users are free to ignore/disable such capabilities as desired, but I think it is important to have support for more sophisticated scanners than just vulnerability mappings and returning lists.
I will certainly update the doc with more clarity on this position and the central idea behind including this function to facilitate a decision on inclusion.
There was a problem hiding this comment.
I agree with @zhill . Our company purchased anchore enterprise because we wanted to detect the secret in any docker images as well; meanwhile, it also gives the cyber security team the ability to define some custom policy.
There was a problem hiding this comment.
@cafeliker @zhill I see this is a valuable use case. That means we need to make Harbor's security policy flexible to support different attributes for different scanners?
I'd love to have such a meeting scheduled to coordinate the progress and review some parts of the design. Most suitable time for me is 9:00 - 18:00 CEST. Both weekly and bi-weekly work for me. |
@zhill I'm looking forward for your update. In the meantime I've submitted already mentioned opinionated proposal of using scanner adapters #90 That's the approach I took to implement the POC with Microscanner. |
I think that is a good idea. I am PDT time-zone am available 17-18:00 CEST most days (8-9AM PDT). Today (July 4) is a holiday but I'm available July 5 or the following week. Thursdays are best, but other options are open as well. |
Signed-off-by: Zach Hill <zach@anchore.com>
|
Updated for a bit more clarity. I think at this point we focus on merging the two proposals into a single document. |
|
Besides cc @danielpacak for awareness. |
@steven-zou and @danielpacak Given the wide range of times to coordinate (Central Europe, China and US Pacific), it's going to be a tricky schedule. I can't do anything earlier than 5:00AM PDT. How about 6:00 PDT/15:00 CEST/21:00 China? |
Works for me |
|
Send meeting invitation to your Gmail box as I don't have your work email, please check. Thanks! |
| * New image pushed | ||
| * Scheduled scan interval | ||
| * User API call | ||
| * Notification (POST) from Clair to _/service/notifications/clair_ indicating an update available |
| The new ImageScannerAdapter interface provides: | ||
|
|
||
| 1. Execute a scan on the image data, retrievable by the digest of the image manifest document (aka the "image digest") (same as current impl) | ||
| 1. Notify the scanner that the image is removed from Harbor. Depending on scanner and configuration this may be a no-op. (new) |
There was a problem hiding this comment.
My assumption is that the adapter should be minimal and focus on the general workflow for all scanners.
So this is probably not part of it.
| 1. Execute a scan on the image data, retrievable by the digest of the image manifest document (aka the "image digest") (same as current impl) | ||
| 1. Notify the scanner that the image is removed from Harbor. Depending on scanner and configuration this may be a no-op. (new) | ||
| 1. Execute a rescan of an image previously scanned (Note: For some scanners this will be the same as #1, but for others it may differ in what/if data is retrieved.) (new, but largely identical to current impl) | ||
| 1. Execute a "check" operation for an image against the scanner, if implemented, to provide an evaluation and pass/fail (new) |
There was a problem hiding this comment.
Could the result of "check" part of the scan result?
So that the adapter will only support two actions "TriggerScan" and "GetResult".
| that implements access control for external Harbor users. This allows the scanners to fetch image data even when external users cannot retrieve image data due to | ||
| access restriction caused by the scan status and/or vulnerability status of the image as configured by the API policyChecker component. As such, either a given | ||
| scanner to be integrated with Harbor must have access to internal network interfaces not exposed to external users, or else must implement a proxy that can run | ||
| within that network that is exposed externally (presumably with authc/authn) to ensure access to the data regardless of the access status for Harbor users. |
There was a problem hiding this comment.
We can implement that via "service account"?
| 1. Notify the scanner that the image is removed from Harbor. Depending on scanner and configuration this may be a no-op. (new) | ||
| 1. Execute a rescan of an image previously scanned (Note: For some scanners this will be the same as #1, but for others it may differ in what/if data is retrieved.) (new, but largely identical to current impl) | ||
| 1. Execute a "check" operation for an image against the scanner, if implemented, to provide an evaluation and pass/fail (new) | ||
|
|
There was a problem hiding this comment.
The scannerAdapter may also need to provide capability to describe itself, to help Harbor consume the heterogeneous scan result? Or is can be encapsulate in scan_result_metadata ?
Proposal is a work in progress to be discussed and improved by members of working group and maintainers.