container-registry / backstage-plugin-harbor-backend

MIT License
5 stars 18 forks source link

Fix bug not displaying vulns #110

Closed locomoco28 closed 9 months ago

locomoco28 commented 1 year ago

Closes https://github.com/container-registry/backstage-plugin-harbor/issues/239 in combination with https://github.com/container-registry/backstage-plugin-harbor/pull/244

locomoco28 commented 1 year ago

I'm a little over exhausted and did a few mistakes that I just corrected.

Please carefully review, but I'm feeling confident with the changes now :)

locomoco28 commented 12 months ago

Besides a wedding tomorrow I should be free over this weekend. I'm going over the whole artifacts.ts script and write it cleaner with better typings. Drafting the PR in the mean time.

Gonna take care of either MIME type depending on which returns

locomoco28 commented 12 months ago

Was busier than expected yesterday as there was also an engagement

I just wanted to mention, I feel like parsing dates on the server-side is not good practice. Imo, the date should be forwarded as we receive it from harbor which is in ISO format and then use the browser's native date APIs to display the correct date using locale aware APIs (toLocaleDateString). That would also remove the dependency of MomentJS.

Is it fine if I remove MomentJS in this PR aswell, or should I open an issue to discuss this suggestion?

locomoco28 commented 12 months ago

My local installation of harbor does not return the application/vnd.security.vulnerability.report; version=1.1 MIME type, can someone test the other mime type? Not sure how to switch which MIME type to use

Vad1mo commented 12 months ago

@locomoco28, I would recommend testing against demo.goharbor.io.

locomoco28 commented 12 months ago

I'm also only getting application/vnd.security.vulnerability.report; version=1.1 from the demo harbor instance

sonarcloud[bot] commented 12 months ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Vad1mo commented 12 months ago

@locomoco28 should we raise this upstream wiht the Harbor team?

locomoco28 commented 12 months ago

should we raise this upstream wiht the Harbor team?

What exactly does that mean? @Vad1mo

Vad1mo commented 12 months ago

@locomoco28 I scanned for vulnerabilities on demo.goharbor.io, so there should be some reports now.

can you also try with https://github.com/goharbor/harbor/issues/18253#issuecomment-1436941076

If you think that this is a problem in upstream Harbor, we should address that.

locomoco28 commented 12 months ago

Running

APP_CONFIG_harbor_baseUrl=https://demo.goharbor.io APP_CONFIG_harbor_username=locomoco APP_CONFIG_harbor_password=blabla yarn start

And calling localhost:7007/artifacts?project=test-syafiq&repository=customimages/ansiblewithsshpass

Returns me

[
  {
    "id": "50latest2023-09-26T07:29:32.434Z",
    "projectID": 50,
    "tag": "latest",
    "artifactDigest": "sha256:b1f9f3759120f3cfdf0d0dde31fd1b4facf7fbc2558d3eb0abc3681a67410806",
    "size": 110.56,
    "repoUrl": "https://demo.goharbor.io/harbor/projects/50/repositories/customimages%2Fansiblewithsshpass",
    "vulnerabilities": {
      "count": 356,
      "severity": "Critical",
      "critical": 3,
      "high": 21,
      "medium": 211,
      "low": 121,
      "none": 0
    },
    "pullTime": "2023-09-26T09:50:36.703Z",
    "pushTime": "2023-09-26T07:29:32.434Z"
  }
]

It works fine (just like with my local harbor instance)

I just always get the same MIME type

image

Which should be fine imo. That seems to me like it's the default MIME type for harbor now, and previously it was the other MIME type. It should handle both of them though

https://github.com/container-registry/backstage-plugin-harbor-backend/pull/110/files#diff-dd0a57cec23efa084ec6ed775a1b92606a9fca0abc0cd69f1776ddd9113a2ec5R76-R106

locomoco28 commented 12 months ago

I would assume that in the past, Harbor defaulted to application/vnd.scanner.adapter.vuln.report.harbor+json; version=1.0 if no accept mime type header was provided, and now it uses application/vnd.security.vulnerability.report; version=1.1.

locomoco28 commented 12 months ago

Sorry for my last comment, that was a mistake, I deleted it

https://demo.goharbor.io/api/v2.0/projects/test-syafiq/repositories/customimages%252Fansiblewithsshpass/artifacts/sha256:b1f9f3759120f3cfdf0d0dde31fd1b4facf7fbc2558d3eb0abc3681a67410806/additions/vulnerabilities

This is the vulnerabilities href that you get here

https://github.com/container-registry/backstage-plugin-harbor-backend/blob/master/src/service/artifact.ts#L52

if you call that URL in your browser, you can see that that mime type is missing. The Root type should have had this key optional

https://github.com/container-registry/backstage-plugin-harbor-backend/blob/master/src/service/artifact.ts#L156

And the request should add the X-Accept-Vulnerabilities header to require application/vnd.scanner.adapter.vuln.report.harbor+json; version=1.0 from harbor

Otherwise, Harbor only responds with application/vnd.security.vulnerability.report; version=1.1.


This PR would skip that second fetch and get the vuln scan results from the first request, and it would be able to handle either MIME type. I personally feel like that's more elegant rather than the current approach of doing 2 fetches and only handling one MIME type. I'll leave the choice up to you if you want that, or keep the current implementation and only add the X-Accept-Vulnerabilities header to the fetch on artifact.ts#L55. Either seems to work

I really have made things more complicated than they were, but it took me a while to figure out how everything works lol

knowacki23 commented 9 months ago

Hello! :) Is there a possibility of merging this PR? Thank you! :smiley: