search

cncf / tag-security

🔐CNCF Security Technical Advisory Group -- secure access, policy control, privacy, auditing, explainability and more!
https://tag-security.cncf.io
Other
2.03k stars 509 forks source link

Volcano Project Security Self-Assessment - Security Pals #1184

Closed mayank-ramnani closed 9 months ago

mayank-ramnani commented 9 months ago

Created and added first draft of the Volcano Project Security Self-Assessment. Please feel free to share any thoughts on the self-assessment.

netlify[bot] commented 9 months ago

Deploy Preview for tag-security ready!

Name Link
Latest commit 4a7702fae0bff9347ba55d63f273cff6058e6255
Latest deploy log https://app.netlify.com/sites/tag-security/deploys/6576a23bbca4a900081c1bf3
Deploy Preview https://deploy-preview-1184--tag-security.netlify.app
Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

eddie-knight commented 9 months ago

Hi there! I'm just getting started looking at your pull request, and I noticed the DCO check is failing.

You can look at the checks section of the PR (I believe it should always be below the last comment) and look for a red X highlighting the failed check. In this case, you can click Details for more information about how to get that check passing.

Screenshot 2023-12-08 at 8 35 18 AM
eddie-knight commented 9 months ago

I noticed that you included an SBOM along with the self assessment. There are two reasons that jump to the front of my mind for why this isn't needed...

SBOMs should be associated with releases, as the bill of materials is only accurate and useful if it is created at build time and associated to a particular point in the code history. If you need to link to an SBOM for some reason in the self-assessment, you can just provide a link out to the latest build artifacts that contain an SBOM. We still have plenty more to review, but as a starter— could you please remove the SBOM from this PR?

ragashreeshekar commented 9 months ago

I noticed that you included an Open and Secure book along with the self assessment. This is already part of the repository and isn't necessary to include in the self assessment.

To keep the PR/Repo up to date, light and avoid duplication, I would suggest updating the branch by pulling the latest state of the repo for this PR and removing the book.

mayank-ramnani commented 9 months ago

Hi there! I'm just getting started looking at your pull request, and I noticed the DCO check is failing.

You can look at the checks section of the PR (I believe it should always be below the last comment) and look for a red X highlighting the failed check. In this case, you can click Details for more information about how to get that check passing. Screenshot 2023-12-08 at 8 35 18 AM

Hi Eddie! Thank you for working on reviewing the PR. We appreciate your feedback and are looking to improve it and learn in the process as much as possible. Regarding the DCO point, I had already tried following the steps to sign-off all the commits, and it worked. But it had the side effect of having 2 copies of each commit, one with the sign-off and one without. I am trying to figure out how we can fix that and remove the commits which don't have the signoff.

mayank-ramnani commented 9 months ago

I noticed that you included an Open and Secure book along with the self assessment. This is already part of the repository and isn't necessary to include in the self assessment.

To keep the PR/Repo up to date, light and avoid duplication, I would suggest updating the branch by pulling the latest state of the repo for this PR and removing the book.

Hi Ragashree! Thank you for working on reviewing the PR, we appreciate all the feedback! I agree with your point, we should keep the PR as light as possible. That book had been added as a mistake, I've removed it now in the latest commit.

mayank-ramnani commented 9 months ago

I noticed that you included an SBOM along with the self assessment. There are two reasons that jump to the front of my mind for why this isn't needed...

SBOMs should be associated with releases, as the bill of materials is only accurate and useful if it is created at build time and associated to a particular point in the code history. If you need to link to an SBOM for some reason in the self-assessment, you can just provide a link out to the latest build artifacts that contain an SBOM. We still have plenty more to review, but as a starter— could you please remove the SBOM from this PR?

I agree with your point that an SBOM should be associated with a release, however Volcano does not contain a proper SBOM in its build artifacts. It contains go.mod and go.sum files in its releases but from my understanding, they are insufficient as an SBOM on their own. That is the reason I had initially generated one manually using the Github tool.

I've removed the SBOM from this PR currently.

I had generated it from the latest code at that point, would you recommend it be regenerated by going back to the tag of the last release and then we mention the release version in the self-asssessment document?

mayank-ramnani commented 9 months ago

Closing this PR due to Git merging and DCO issues. Tracked in new PR: https://github.com/cncf/tag-security/pull/1205