feat: update ScannerCapability, enabled_capabilities, HarborSbomRepor… #17

zyyw · 2024-01-12T08:20:42Z

…t and sbom data spec

zyyw · 2024-01-12T08:37:10Z

Hi @knqyf263 we have some updates on the scanner spec 1.2. Please help to review it. Thanks!

Summary of change points:

add additional_attributes in ScannerCapability of ScannerCapability returned by sending a request to /metadata. Here the /metadata will also return the supported_media_types for the type of SBOM
add accept_media_types for parameter of enabled_capabilities to indicate accept_media_types for the type of SBOM, when sending a request to /scan
in HarborSbomReport, the media_type and sbom field are wrapped into as an array of SbomData object of data, when sending a request to /scan/{scan_request_id}/report

cc: @wy65701436 , @stonezdj

zyyw · 2024-01-12T08:41:12Z

cc: @chlins

knqyf263 · 2024-01-12T10:09:59Z

api/spec/scanner-adapter-openapi-v1.2.yaml

+        data:
+          type: array
+          items:
+            $ref: '#/components/schemas/SbomData'
+          additionalProperties: true
+          description: 'The raw data of the sbom generated by the scanner and its format.'


It is not easy for us to implement this API. Currently, each report is stored separately. Trivy doesn't generate both SPDX and CycloneDX together. We prefer returning one report at /scan/{scan_request_id}/report. Or it is better to return all reports (vulnerability and SBOM) together.

This suggestion is as follows:

Vulnerability and SBOM reports are separately returned

SPDX and CycloneDX reports are aggregated.

It looks inconsistent. I'd suggest

Vulnerability, SPDX SBOM and CycloneDX SBOM are returned separately.

Or

Vulnerability, SPDX SBOM and CycloneDX SBOM are returned together

What do you think?

The API doesn't require the backend implementation to store the information with requestID, you could stored each report with the key <requestID>:<media_type>. then you can store whatever vulnerability, sbom/spdx or sbom/cyclonedx

@knqyf263 , the /scan/{scan_request_id}/report is identified by scan_request_id and Accept header (example: application/vnd.security.vulnerability.report; version=1.1 or application/vnd.security.sbom.report+json; version=1.0). That's the reason why SPDX SBOM report and CycloneDX SBOM report are aggregated.
We do understand that "Trivy doesn't generate both SPDX and CycloneDX together". For example, when /scan is requested like below:

{ "enabled_capabilities": [ { "type": "sbom", "produces_mime_types": [ "application/vnd.security.sbom.report+json; version=1.0" ], "accept_media_types": [ "application/spdx+json", "application/vnd.cyclonedx+json" ] } ] }

trivy-adapter may need to run trivy CLI twice passing different parameters to generate different format of SBOM report.

The API doesn't require the backend implementation to store the information with requestID, you could stored each report with the key :<media_type>. then you can store whatever vulnerability, sbom/spdx or sbom/cyclonedx

They all have different statuses, like spdx: completed, cyclonedx: in progress, but the API forces scanners to aggregate results. The specification should define what should be returned in that case. There are more considerations to take into account, so it is closer to a new API rather than an extension of the current API.

That's the reason why SPDX SBOM report and CycloneDX SBOM report are aggregated.

We can do that, but it complicates implementation. If you pass the media type to /scan/{scan_request_id}/report, it simply returns one report. Does Harbor benefit from this specification? If you have a big advantage on this API, I'm ok, but it makes implementation more complicated for scanners and brings more considerations as mentioned above.

@knqyf263 if the /scan request asks for both SPDX sbom and Cyclonedx sbom, like below:

{ "enabled_capabilities": [ { "type": "sbom", "produces_mime_types": [ "application/vnd.security.sbom.report+json; version=1.0" ], "accept_media_types": [ "application/spdx+json", "application/vnd.cyclonedx+json" ] } ] }

and if we pass the Accept-Media-Type header (application/spdx+json or application/vnd.cyclonedx+json) to /scan/{scan_request_id}/report so that it simply returns one report at one http request to /scan/{scan_request_id}/report, there will be one {scan_request_id} used in two requests to /scan/{scan_request_id}/report. Can the one {scan_request_id} be used two times?

scan_request_id is already used multiple times for vulnerabilities and SBOM.

If I understand correctly, the current suggestion is as follows (Option 1). The same scan_request_id is used twice.

scan_request_id (e.g. ABCDE12345) + mime_type

ABCDE12345 + application/vnd.security.vulnerability.report+json; version=1.1

Return one report

ABCDE12345 + application/vnd.security.sbom.report+json; version=1.0

Return two reports (SPDX and CycloneDX) in one request

Would the following approach (Option 2) not work?

scan_request_id (e.g. ABCDE12345) + mime_type (+ media_type)

ABCDE12345 + application/vnd.security.vulnerability.report+json; version=1.1

Return one report

ABCDE12345 + application/vnd.security.sbom.report+json; version=1.0 + application/spdx+json

Return one report (SPDX SBOM)

ABCDE12345 + application/vnd.security.sbom.report+json; version=1.0 + application/vnd.cyclonedx+json

Return one report (CycloneDX SBOM)

The same ID can be used three times.

Or aggregate all reports (Option 3). scan_request_id will be unique.

scan_request_id (e.g. ABCDE12345)

ABCDE12345

Return three reports (vuln, SPDX and CycloneDX)

I'm just curious Option 1 has any advantages over Option 2 and Option 3. Option 2 is simpler from the implementation perspective (it is already done in aquasecurity/harbor-scanner-trivy#422). But again, if you see benefits in Option 1, I'm ok with the approach.

@knqyf263 if we go with the Option 2 you mentioned above, but having media_type (application/spdx+json or application/vnd.cyclonedx+json) as optional query parameter in /scan/{scan_request_id}/report, for example /scan/{scan_request_id}/report?media_type=application/spdx+json (the / in application/spdx+json might be encoded), is it okay with you?

/scan/{scan_request_id}/report

return one report for vulnerabilities

/scan/{scan_request_id}/report?media_type=application/spdx+json

return one report of SPDX SBOM

/scan/{scan_request_id}/report?media_type=application/vnd.cyclonedx+json

return one report of CycloneDX

Once you confirm it, i'll update the PR.
Thanks

Yes, it works. I tried to express "optional" by (+ media_type). But all of the above options work. You can decide it. I just tried to understand the pros/cons.

@knqyf263 , Thank you for the confirmation! We think your suggestion of Option 2 makes sense and it makes the process of getting scan report request much simpler. Updated the PR accordingly.
Please help to review it. Appreciated!

wy65701436

lgtm

stonezdj · 2024-01-15T08:56:22Z

api/spec/scanner-adapter-openapi-v1.2.yaml

              }
            ]
        properties:
          $ref: "#/components/schemas/ScannerProperties"
      description: |
-        Represents metadata of a Scanner Adapter which allows Harbor to lookup a scanner capable
+        Represents metadata of a Scanner Adapter which allows Harbor to lookup a scanner capabilities


knqyf263

LGTM

chlins

lgtm

zyyw · 2024-01-16T09:53:54Z

@knqyf263 for awareness, made a few subtle changes after your approval on this PR:

https://github.com/goharbor/pluggable-scanner-spec/compare/1cbaa544b6e6eacae9f548d36f2e544768a357d2..776efab89d73ffd2da977615740dc12c4063b787

and add /scan/{scan_request_id}/report 400 response code Signed-off-by: Shengwen Yu <[email protected]>

wy65701436

lgtm

knqyf263 · 2024-01-18T06:39:32Z

After I started implementing this spec, another question came to my mind.

additional_attributes is defined in #/components/schemas/ScannerCapability is defined as below.

pluggable-scanner-spec/api/spec/scanner-adapter-openapi-v1.2.yaml

Lines 314 to 323 in 776efab

    
                   additional_attributes: 
        
                     type: object 
        
                     descriptions: The additional attributes for scanner capabilities. If the type is sbom, then it returns supported media types of the SBOM format. 
        
                     example: | 
        
                       { 
        
                         "sbom_media_types": [ 
        
                           "application/spdx+json", 
        
                           "application/vnd.cyclonedx+json" 
        
                         ] 
        
                       }

Is there any reason another parameter name (parameters) is used here? (additional_attributes vs parameters)

pluggable-scanner-spec/api/spec/scanner-adapter-openapi-v1.2.yaml

Lines 359 to 368 in 776efab

    
                         parameters: 
        
                           $ref: '#/components/schemas/SbomParameters' 
        
                           description: The additional parameters for the scan request, for the SBOM type, harbor will carry with `sbom_media_types` to specify the expected formats for SBOM content. 
        
                           example: |  
        
                             { 
        
                               "sbom_media_types": [ 
        
                                 "application/spdx+json", 
        
                                 "application/vnd.cyclonedx+json" 
        
                               ] 
        
                             }

It is not a big deal, but I feel like using the same name is more straightforward because their relationship is "supported capabilities" and "enabled capabilities". They refer to essentially the same thing. What do you think? @zyyw

zyyw · 2024-01-18T06:56:07Z

Hi @knqyf263 , regarding additional_attributes and parameters, they do essentially refer to the same thing:

{ 
 "sbom_media_types": [ 
   "application/spdx+json", 
   "application/vnd.cyclonedx+json" 
 ] 
}

The main reason of using two different terms refer to the same value is that additional_attributes serves as a part of the response data of a request to /metadata, while parameters serves as a part of request parameter to ScanRequest of /scan and it's not a response data indicating some additional attributes of an entity.

knqyf263 · 2024-01-18T07:04:25Z

Yes, it makes sense. parameters might sound weird for /metadata, but this is because the request uses the term parameters. What if finding a better name that can be used for both, like additional_properties? For example, produces_mime_types is used in request and response. If I understand correctly, your answer is based on the current naming, additional_attributes and parameters, and it's not opposed to using the same name, right?

zyyw · 2024-01-18T09:17:27Z

the additional_attributes is to wrap up all extra attributes of the response from a request to /metadata, while parameters indicating parameters of a request of ScanRequest to /scan. Currently, they are essentially referring the same value. But as the scanner capabilities extends in the future, additional_attributes and parameters might refer to different things.

knqyf263 · 2024-01-18T09:58:15Z

Thanks for your explanation. OK, I'll implement it.

zyyw assigned stonezdj and wy65701436 Jan 12, 2024

zyyw assigned chlins Jan 12, 2024

knqyf263 reviewed Jan 12, 2024

View reviewed changes

zyyw force-pushed the update-spec-1.2 branch 3 times, most recently from 31db260 to 1cbaa54 Compare January 16, 2024 03:14

wy65701436 previously approved these changes Jan 16, 2024

View reviewed changes

stonezdj previously approved these changes Jan 16, 2024

View reviewed changes

knqyf263 approved these changes Jan 16, 2024

View reviewed changes

chlins previously approved these changes Jan 16, 2024

View reviewed changes

zyyw dismissed stale reviews from chlins, stonezdj, and wy65701436 via 776efab January 16, 2024 09:52

zyyw force-pushed the update-spec-1.2 branch from 1cbaa54 to 776efab Compare January 16, 2024 09:52

stonezdj previously approved these changes Jan 17, 2024

View reviewed changes

feat: update ScannerCapability, enabled_capabilities, HarborSbomReport,

9028fef

and add /scan/{scan_request_id}/report 400 response code Signed-off-by: Shengwen Yu <[email protected]>

zyyw dismissed stonezdj’s stale review via 9028fef January 17, 2024 07:21

zyyw force-pushed the update-spec-1.2 branch from 776efab to 9028fef Compare January 17, 2024 07:21

wy65701436 approved these changes Jan 17, 2024

View reviewed changes

stonezdj approved these changes Jan 17, 2024

View reviewed changes

zyyw merged commit 7aad47c into goharbor:master Jan 17, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: update ScannerCapability, enabled_capabilities, HarborSbomRepor… #17

feat: update ScannerCapability, enabled_capabilities, HarborSbomRepor… #17

zyyw commented Jan 12, 2024

zyyw commented Jan 12, 2024

zyyw commented Jan 12, 2024

knqyf263 Jan 12, 2024

stonezdj Jan 15, 2024 •

edited

Loading

zyyw Jan 15, 2024

knqyf263 Jan 15, 2024

zyyw Jan 15, 2024

knqyf263 Jan 15, 2024 •

edited

Loading

zyyw Jan 15, 2024 •

edited

Loading

knqyf263 Jan 15, 2024 •

edited

Loading

zyyw Jan 16, 2024

wy65701436 left a comment

stonezdj Jan 15, 2024

knqyf263 left a comment

chlins left a comment

zyyw commented Jan 16, 2024

wy65701436 left a comment

knqyf263 commented Jan 18, 2024 •

edited

Loading

zyyw commented Jan 18, 2024

knqyf263 commented Jan 18, 2024

zyyw commented Jan 18, 2024 •

edited

Loading

knqyf263 commented Jan 18, 2024

feat: update ScannerCapability, enabled_capabilities, HarborSbomRepor… #17

feat: update ScannerCapability, enabled_capabilities, HarborSbomRepor… #17

Conversation

zyyw commented Jan 12, 2024

zyyw commented Jan 12, 2024

zyyw commented Jan 12, 2024

knqyf263 Jan 12, 2024

Choose a reason for hiding this comment

stonezdj Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

zyyw Jan 15, 2024

Choose a reason for hiding this comment

knqyf263 Jan 15, 2024

Choose a reason for hiding this comment

zyyw Jan 15, 2024

Choose a reason for hiding this comment

knqyf263 Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

zyyw Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

knqyf263 Jan 15, 2024 • edited Loading

Choose a reason for hiding this comment

zyyw Jan 16, 2024

Choose a reason for hiding this comment

wy65701436 left a comment

Choose a reason for hiding this comment

stonezdj Jan 15, 2024

Choose a reason for hiding this comment

knqyf263 left a comment

Choose a reason for hiding this comment

chlins left a comment

Choose a reason for hiding this comment

zyyw commented Jan 16, 2024

wy65701436 left a comment

Choose a reason for hiding this comment

knqyf263 commented Jan 18, 2024 • edited Loading

zyyw commented Jan 18, 2024

knqyf263 commented Jan 18, 2024

zyyw commented Jan 18, 2024 • edited Loading

knqyf263 commented Jan 18, 2024

stonezdj Jan 15, 2024 •

edited

Loading

knqyf263 Jan 15, 2024 •

edited

Loading

zyyw Jan 15, 2024 •

edited

Loading

knqyf263 Jan 15, 2024 •

edited

Loading

knqyf263 commented Jan 18, 2024 •

edited

Loading

zyyw commented Jan 18, 2024 •

edited

Loading