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

OpenKruise Project Security Self-Assessment - Security Pals #1174

Closed bradcush closed 8 months ago

bradcush commented 10 months ago

Created and added first draft for OpenKruise Project Security Self-Assessment. Please feel free to share your feedback on the security self-assessment.

netlify[bot] commented 10 months ago

Deploy Preview for tag-security canceled.

Name Link
Latest commit 8f66e8a4a0927e7c90be736c5c1b99ce1d21a326
Latest deploy log https://app.netlify.com/sites/tag-security/deploys/65a969272a87190008dde23b
eddie-knight commented 9 months ago

Hi there, thanks for the work you did on this self-assessment!

I'm just now cracking this open for review, and the first thing I noticed is 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...

  1. SBOMs should be associated with releases, as the bill of materials is only accurate and useful if it is created at build time and only known to be associated to a particular point in the code history.
  2. 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?

bradcush commented 9 months ago

@eddie-knight Thanks for starting the review and for the initial comments!

I've included a manually generated SBOM with the assessment for reference at the time the security assessment was completed because the project itself does not generate on during the build or release process. They only have go.mod files that are associated with the dependencies of the project. Do you still think we should link nothing there in the meantime until the project has one?

bradcush commented 9 months ago

Thanks for the good work @bradcush and team, appreciate the efforts. I have completed first pass of review and there are two things that need your attention. Please feel free to reach out here or on slack for any questions and clarifications. Along with addressing the comments, kindly update the PR branch with the latest content in the repo as this branch is out-of-date with the base branch.

Thanks @ragashreeshekar for a timely first review. I'll let you know as soon as these issues are fixed and we're ready for another review. I'll also make sure to get clarification on some of the points raised regarding the STRIDE thread modeling.

bradcush commented 9 months ago

@ragashreeshekar I think we're ready for another look when you have time. I've addressed two out of your three comments and rebased the branch on top of the latest main. I've made them in the same commit, in order to keep things squashed in preparation for merging, hopefully that's ok. I'll leave resolve the comments up to you.

We still have the open question about how to handle SBOMs which the project doesn't generate currently. (This is actually recommended as part of our assessment for that reason) I'll defer to you on what we should do next for our threat modeling also but I've responded to your concern.

eddie-knight commented 9 months ago

Hey @bradcush, thanks for the explanation!

The reviewer team had a chat regarding the SBOMs, since this was a recurring thing we saw from other Security Pals contributions.

We should absolutely be highlighting known gaps in the self assessment, such as this, by stating that it is not currently addressed. This will help projects know gaps that they should address before continuing on to a joint assessment.

bradcush commented 9 months ago

We should absolutely be highlighting known gaps in the self assessment, such as this, by stating that it is not currently addressed. This will help projects know gaps that they should address before continuing on to a joint assessment.

Ok great, thanks for looking into it @eddie-knight. As we have it listed the creation of an SBOM as a recommendation already in the self-assessment, should I go ahead and remove the local one I have generated or should we keep that until the project officially generates one as part of their build/release? Sorry for the back and forth, just want to be clear here.

eddie-knight commented 9 months ago

just want to be clear here

Always appreciate back-and-forth for clarity. Please pull out the SBOM that you created, and adjust the SBOM metadata value to OpenKruise does not currently generate an SBOM on release.

Raga and I are still going through the rest of our review checklist for all of the recent contributions, but this PR seems to be very close to being ready for merge. We'll let you know if there is anything else that needs addressed. Thanks for continuing to work with us on this!

bradcush commented 9 months ago

Always appreciate back-and-forth for clarity. Please pull out the SBOM that you created, and adjust the SBOM metadata value to OpenKruise does not currently generate an SBOM on release.

Ok great, thanks! I've removed the SBOM and added the above line.

bradcush commented 9 months ago

@eddie-knight Thanks again for the thorough review. I've gone ahead and addressed all of the comments and pushed the changes in another separate commit so they are easily visible. I can squash commits again before it's ready to merge if needed. I've still kept all conversations unresolved. Feel free to give it another look when you have time to make sure you're aligned with the changes I've made. Thanks!

bradcush commented 9 months ago

I can squash commits again before it's ready to merge if needed. I've still kept all conversations unresolved.

I've also just squashed the commits that were making changes based on the feedback that's been provided during to review to make sure everything is as ready as possible to merge when it's time.

ragashreeshekar commented 9 months ago

Thank you for the updates. Largely looks good to me. Just one from my end on extracting threat model to a separate file within the same folder.

Tagging @eddie-knight for your review, and @JustinCappos both for your opinion on including maintainers for their ack and review of this work, followed by chair review (as applicable) and merging them to the repo.

JustinCappos commented 9 months ago

Okay, as for merging, let's talk about a process in the upcoming TL and TAG Security meetings. I think having a small group that goes and completes this process would be the best way to move forward. We should be able to get most of these merged with a few hours of effort, I think.

On Tue, Jan 2, 2024 at 5:31 PM Raga @.***> wrote:

Thank you for the updates. Looks good to me. Tagging @eddie-knight https://github.com/eddie-knight for your review, and @JustinCappos https://github.com/JustinCappos both for your opinion on including maintainers for their ack and review of this work, followed by chair review (as applicable) and merging them to the repo.

— Reply to this email directly, view it on GitHub https://github.com/cncf/tag-security/pull/1174#issuecomment-1874644273, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAGROD4VIKB4IQDDP7GQRADYMSDEZAVCNFSM6AAAAABAJYIRT6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZUGY2DIMRXGM . You are receiving this because you were mentioned.Message ID: @.***>

bradcush commented 9 months ago

Thank you for the updates. Largely looks good to me. Just one from my end on extracting threat model to a separate file within the same folder.

Thanks for taking a look @ragashreeshekar. I've extracted the STRIDE Threat Model into a a new file named threat-model.md and linked to that file from the same section of the self assessment where it was previously inline.

eddie-knight commented 8 months ago

LGTM