Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CloudEvents Project Security Self-Assessment - Security Pals #1186

Merged
merged 21 commits into from
Jan 17, 2024

Conversation

Igor8mr
Copy link
Contributor

@Igor8mr Igor8mr commented Dec 7, 2023

Created and added the first draft of the CloudEvents Project Security Self-Assessment.

Please feel free to share your thoughts on the security self-assessment.

Copy link

netlify bot commented Dec 7, 2023

Deploy Preview for tag-security canceled.

Name Link
🔨 Latest commit 0d1155d
🔍 Latest deploy log https://app.netlify.com/sites/tag-security/deploys/65a7ee270483c500084bd49c

Signed-off-by: Igor8mr <[email protected]>
Co-authored-by: MatthewZGong <[email protected]>
Co-authored-by: devyani-14 <[email protected]>
Co-authored-by: Kushal-kothari <[email protected]>
@eddie-knight
Copy link
Collaborator

Hi there, and thanks for the hard work you put into creating this self-assessment!

We have plenty more to review on the PR, but as a starter I noticed that you have some notes written in assessments/projects/cloudevents/CE-maintainers-communications.md. Could you move these notes to the PR description, comments, or remove them (as needed)?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two reasons that jump to the front of my mind for why an SBOM isn't needed in this PR...

  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 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eddie-knight, thank you for your comments!
That makes sense, I just removed the notes and the SBOM.
Please, let me know if you have more comments in the future. Thank you!

Signed-off-by: Igor8mr <[email protected]>
Co-authored-by: MatthewZGong <[email protected]>
Co-authored-by: devyani-14 <[email protected]>
Co-authored-by: Kushal-kothari <[email protected]>
@Igor8mr
Copy link
Contributor Author

Igor8mr commented Dec 8, 2023

Communication with the CloudEvents team

Slack Communications

  • Security Pals Involved:
    • Igor Rodrigues (Igor Rodrigues)
  • CloudEvents Team Members Involved:
    • Doug Davis (dug)
Report

Igor Rodrigues (Nov 29th at 4:29:13 PM)

Hello all, I'm a student at NYU involved in the SecurityPal effort from TAG
Security. Our group is conducting a security assessment on CloudEvents, which we
will later submit to the TAG Security Assessments
Repository
. We have completed
an initial
evaluation

of the project and would appreciate your feedback to validate the information we
included. We also want to know if there are additional aspects we should include
in the assessment to correctly represent your project, along with more details
for sections like security issue
resolution

and secure development
practices
.
Please, feel free to share your thoughts here on Slack, on GitHub, or on a call.
Thank you!

Dug (Nov 29th at 8:02:04 PM)

Hi @igor Rodrigues - will take a look. Just curious though, what made you decide
to analyze CloudEvents?

Igor Rodrigues (Nov 29th at 8:36:26 PM)

Hi @dug, thank you. The assessment is one of our assignments for a class we are
taking with Professor Justin Cappos. Each group was assigned to a CNCF project,
and ours was CloudEvents. The project is interesting, so we are trying to do a
bit more than expected. I hope the assessment helps in the future.

Dug (Nov 30th at 10:24:13 AM)

@igor Rodrigues thanks. Just a few comments from my quick scan:

  • Where do you see ANTRL being used? I'm surprised you didn't include markdown
    in the list despite it not being a "programming language", being a "spec"
    markdown is kind of our "language" 🙂
  • CloudEvents was developed to address the lack of uniformity in event data format... be a bit careful here. While CE does provide a "structured" format,
    that's just there are times when people want the event data and context
    attributes in one doc. In general though CE is NOT trying to define "yet
    another common event format (one format to rule them all)". In particular,
    many people use/prefer "binary" format because it just augments their existing
    events. And even with "structured", the stuff that does into the data
    attribute is wide open - and should be defined by the business. I just don't
    want people to think we're making the same mistake as other folks who tried to
    force one format for all events. Rather CE is about standardizing "where to
    find common metadata about the event w/o having to parse/understand the event
    specific format".
  • Nit: in "Protocol Binding" section it mentions structured-mode but hasn't
    defined that term yet. You may want to define binary vs structured CEs in the
    doc before this section.
  • Not sure what the "trust boundary" is meant to represent in the diagram since
    "trust" is kind of orthogonal to the roles.
  • Goals: may want to tweak some of those based on my comments above. Plus, some
    of those aren't really goals for CE since CE doesn't control them. For
    example, "generate events before consumers are listening" - a good idea, but
    CE doesn't really talk about those in the spec itself. CE is just about the
    format and how they might appear on the transports. With a few exceptions, it
    doesn't get into the protocols themselves or event
    management/subscriptions.....
  • CE is under review for Graduation status right now... hopefully will be
    approved very soon
  • CE doesn't really describe any encryption mechanism or deal with integrity -
    the text you wrote kind of implies CE addresses it. Perhaps say something like
    it's an implementation detail/choice??
  • Ecosystem - might be good to link to the cloudevents.io site
    which includes a list of adopters.
  • The "Security issue resolution" section reads like an SDK specific section -
    perhaps "SDK" should appear in the title to make it clear that the following
    sections apply to the SDK repos and not the spec repo?
  • There's also a new security mailing list people should use to report security
    concerns: https://lists.cncf.io/g/cncf-cloudevents-security/topics
  • There is no "CloudEvents Steering Committee" that's mentioned in the Threat
    Modelling section (typo in Modelling)
  • It might be good to mention that (I think) all of the security issues found by
    Trail of Bits have been addressed

Igor Rodrigues (Nov 30th at 11:58:51 AM)

Hi @dug, Thank you for all the comments! For ANTLR, GitHub marked it as 14.1% of
the CloudEvents spec, so that's why I
added it to the assessment, but I may remove it if it's not very relevant. I'll
also definitely add Markdown, thanks for noticing that. We'll review the doc,
update it with your comments and tell you about the changes. Thank you again!

Igor Rodrigues (Dec 4th at 11:15:26 AM)

Hi @dug, we fixed the comments you provided on the security
assessment
,
along with the comments from the meeting. Here are the new
changes

since then. Please, let me know if there are more parts we could improve. Also,
I wanted to CloudEvents have a public SBOM that we could link, and if you think
there are more aspects we could add to the specification side of the Security
Issue
resolution
.
Thank you for all the help!

Dug (Dec 4th at 11:36:23 AM)

The closest thing we have to a SBOM is:
https://github.com/cloudevents/spec#cloudevents-documents Thanks for the update.
Will look it over in a bit.

Igor Rodrigues (Dec 4th at 11:44:52 AM)

Great, thanks!

Dug (Dec 4th at 12:08:22 PM)

I put just a few minor tweaks as comments on the commit.

Igor Rodrigues (Dec 4th at 12:28:53 PM)

Thanks, I'll fix those soon

Igor Rodrigues (Dec 5th at 8:05:09 AM)

Hi @dug, I forgot to ask this before, but are there any action items you are
currently working on or plan to work on that would solve the concerns mentioned
in the doc or other security concerns? I think it would be good to include those
in the assessment. I remember you mentioned implementing bots to check the SDKs,
do you have a pull request, issue, or any other link to the implementation of
the bots idea? Also, we are willing to help implement one of those solutions to
the concerns if you have some specific things in mind.

Dug (Dec 5th at 11:57:30 AM)

@igor Rodrigues just this one: cloudevents/spec#1235

Dug (Dec 5th at 11:58:19 AM)

If someone knows how to setup the bots and wants to submit a PR to add them...
that would be great! Or even just a list of instructions for an admin to follow
(if it's more than just a PR) that would be great too.

Igor Rodrigues (Dec 5th at 12:12:57 PM)

Great, thanks! We are taking a look here

CloudEvents Team Meeting

  • Security Pals Involved:
    • Igor Rodrigues
  • CloudEvents Team Members Involved:
    • Doug Davis
    • Tommy
    • Erik
    • David B
    • Jon
    • Calum
    • Jem
    • Clemens

The team joined the CloudEvents public team meeting on November 30th, 2023, which was recorded on YouTube.

@duglin
Copy link

duglin commented Dec 9, 2023

Can we please not call this a "self-assessment" since this was not done by the CE project itself. I'm not suggesting anything is incorrect with the assessment but it's not accurate to imply that the CE team did it. This was done as a school project (at the request of their teacher) and while the CE team did look it over, it was done so just to make sure there weren't any superficial problems with it. If this is meant to be some kind of formal CNCF review/analysis then more work/review should be done so it can have a more official standing.

For the TAG-Security team... what's the purpose of these docs? What are they used for? Do they have any official standing within the CNCF? I've looked over https://github.com/cncf/tag-security/tree/main/assessments but what's not clear to me is why a project should create one of these docs if they've already done something like had a Trail of Bits review and added that analysis to their own docs.

@JustinCappos
Copy link
Collaborator

Can we please not call this a "self-assessment" since this was not done by the CE project itself.

STAG Security Assessment Facilitator here.. I understand the potential for confusion. The name "self-assessment" was created back when we made the original process because it was actually done by the projects. The Security Pals process was added because some CNCF projects hadn't done their self assessments and these Pals are meant to help spur it along. I totally get where you're coming from, we will certainly consider this feedback.

I'm not suggesting anything is incorrect with the assessment but it's not accurate to imply that the CE team did it. This was done as a school project (at the request of their teacher) and while the CE team did look it over, it was done so just to make sure there weren't any superficial problems with it. If this is meant to be some kind of formal CNCF review/analysis then more work/review should be done so it can have a more official standing.

Yes, please do this! This is the goal!

For the TAG-Security team... what's the purpose of these docs? What are they used for? Do they have any official standing within the CNCF? I've looked over https://github.com/cncf/tag-security/tree/main/assessments but what's not clear to me is why a project should create one of these docs if they've already done something like had a Trail of Bits review and added that analysis to their own docs.

So they are part of the assessment process in the TAG Security repo ( https://github.com/cncf/tag-security/tree/main/assessments ). At different times the TOC has required / recommended these for projects. We have heard from end user adopters, etc. that they find these valuable in getting another perspective on a project. This is different than an audit from ToB, etc. which tends to be a more "point-in-time" security assessment of specific components of a project.

I'd be happy to discuss any of this process, etc. further either in a TAG Security meeting (I am usually on the Wednesday call), on the CNCF slack, or via a separate issue dedicated to revising the assessment process, etc. on the TAG Security issue tracker.

@Igor8mr
Copy link
Contributor Author

Igor8mr commented Dec 9, 2023

Thank you for your comments explaining the purpose of the project @JustinCappos! I would also be happy to discuss the assessment with you and the CloudEvents team on a call, on Slack, or here.

@duglin, please, feel free to add more comments here or on Slack with more suggestions for the assessment if you can. We want to represent the project in the most accurate way possible, so we are definitely open to your feedback. Thank you again for all the feedback you have given me so far.

Comment on lines 327 to 331
As of the latest security assessment, CloudEvents does not explicitly document
compliance with specific security standards such as PCI-DSS, COBIT, ISO, GDPR,
etc. Current efforts are focused on evaluating compliance with these standards
and ensuring that CloudEvents adheres to industry best practices in security and
privacy.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the documentation, can you remove the 2nd sentence as it can be misconstrued.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for reviewing the assessment. Sounds good, I removed the sentence now.

Copy link
Contributor

@ragashreeshekar ragashreeshekar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR @Igor8mr and team, appreciate the efforts.
I have completed first pass of review. 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.


#### The SDK management teams may implement vulnerabilities while implementing SDKs

Trail Of Bits was able to identify 7 different security concerns regarding the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the work outside of the scope of security pals activity. I would recommend introducing it in a few sentences and providing its reference instead of copying the same content here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. I shortened this sentence and added a reference link there. Please, let me know if I should make more changes there.

and reliable development, testing, and deployment of applications using
CloudEvents.

### [Action Item 4] Enhanced Encryption and Data Validation Mechanisms
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This title is not relevant to the content, as it implies project needs to implement enhanced encryption and data validation mechanisms. Something on the lines of document project scope and user responsibilities might a better fit.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for noticing that, I changed the title to Emphasize goals, non-goals and user responsibilities.


## Action Items

### [Action Item 1] Setup Snyk for SDKs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kindly keep the recommendations to reflect the task instead of suggesting particular tool. In this case, the task is to establish SAST, SCA and/or secret scanning services. The project team is free to choose the tool of their choice to accomplish this.

In addition, pointer to associated task/process documentation could be useful, however I would suggest removing the steps to accomplish it embedded in this document.

Copy link
Contributor Author

@Igor8mr Igor8mr Dec 10, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the comment! I changed the action item to make it broader now. I also linked this to a GitHub issue with the task/process documentation and removed the steps from the documentation. Please, let me know if I should make more changes to this.

CloudEvents SDK, not the specification. All of the security issues found by
Trail of Bits have already been addressed.

Below are listed the findings by Trail Of Bits with their descriptions.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the work outside of the scope of security pals activity. I would recommend introducing it in a few sentences and providing its reference instead of copying the same content here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback. We removed this part and replaced it with a summary and reference.

@duglin
Copy link

duglin commented Dec 11, 2023

So they are part of the assessment process in the TAG Security repo ( https://github.com/cncf/tag-security/tree/main/assessments ). At different times the TOC has required / recommended these for projects. We have heard from end user adopters, etc. that they find these valuable in getting another perspective on a project. This is different than an audit from ToB, etc. which tends to be a more "point-in-time" security assessment of specific components of a project.

Thanks but I'm not sure this really answers my question. TOB did a review from a security perspective and we include that in our repo. This PR doesn't introduce much (any?) new 'security' related information and actually spends most of its time talking about the project from a process or governance perspective. Not bad things but it feels a bit odd to be part of a "security" review document. Yes the TOB review is a point in time review, but so is this PR... or any assessment. I'm just trying to understand the impact/benefit of this piece of work when it feels like just about all of the info is already part of our repo (or should be as that's the source of truth for all things CE) so why duplicate it here, and are we then on the hook to keep it up to date since the minute the PR is merged it could be out of date?

Sorry if this sounds a bit like I'm pushing back, but I have a very strong negative reaction to things that feel like unnecessary bureaucracy - especially if it then requires an on-going commitment.

Comment on lines 46 to 62
* [Action Items](#action-items)
* [[Action Item 1] Setup a system for automatically reviewing
SDKs](#action-item-1-setup-a-system-for-automatically-reviewing-sdks)
* [[Action Item 2] Regular SDK Security
Audits](#action-item-2-regular-sdk-security-audits)
* [[Action Item 3] Enforce governance rules throughout SDK
development](#action-item-3-enforce-governance-rules-throughout-sdk-development)
* [[Action Item 4] Obtain a Silver or Gold CII Best Practices
Badge](#action-item-4-obtain-a-silver-or-gold-cii-best-practices-badge)
* [[Action Item 5] Improve the documentation on the Security Response
Processes](#action-item-5-improve-the-documentation-on-the-security-response-processes)
* [[Action Item 6] Emphasize goals, non-goals and user
responsibilities](#action-item-6-emphasize-goals-non-goals-and-user-responsibilities)
* [[Action Item 7] Documentation and Knowledge
Sharing](#action-item-7-documentation-and-knowledge-sharing)
* [[Action Item 8] Community Engagement and
Feedback](#action-item-8-community-engagement-and-feedback)
Copy link
Collaborator

@eddie-knight eddie-knight Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following discussions from the review team, we are asking that Security Pals please remove any recommendations from the scope of this document (unless they were written and included under guidance from the maintainer team). You may optionally communicate Security Pals recommendations for projects in a more accessible location, such as in a project community channel or GitHub Issue. Consider linking to the content from the Appendix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @eddie-knight, thank you for the feedback on this. I removed all the action items, except the Setup a system for automatically reviewing SDKs, which is something we discussed with the maintainer team, as you mentioned not to remove those. They are also already working on implementing this and we provided some guidance to help them get started. I also moved this action item to the appendix now as you mentioned.

Please, let me know if I should still change this section more, or remove this last action item. Thank you again!

Comment on lines 1053 to 1063
## References

* [CloudEvents Website](https://cloudevents.io/)
* [CloudEvents GitHub](https://github.com/cloudevents)
* [CloudEvents Security Assessment by Trail of
Bits](https://github.com/cloudevents/spec/blob/main/docs/CE-SecurityAudit-2022-10.pdf)
* [Security Assessment Guide](https://github.com/Rana-KV/ISP)
* [Sample Security
Assessment](https://github.com/Rana-KV/tag-security/blob/main/assessments/projects/karmada/self-assessment.md#threat-modeling-with-stride)
* [Open and Secure
Book](https://github.com/cncf/tag-security/blob/main/assessments/Open_and_Secure.pdf)

This comment was marked as resolved.

Copy link
Contributor Author

@Igor8mr Igor8mr Dec 12, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your comment, they are mostly the same thing. I merged it into the appendix now.

Igor8mr and others added 3 commits December 11, 2023 23:27
@JustinCappos
Copy link
Collaborator

So they are part of the assessment process in the TAG Security repo ( https://github.com/cncf/tag-security/tree/main/assessments ). At different times the TOC has required / recommended these for projects. We have heard from end user adopters, etc. that they find these valuable in getting another perspective on a project. This is different than an audit from ToB, etc. which tends to be a more "point-in-time" security assessment of specific components of a project.

Thanks but I'm not sure this really answers my question. TOB did a review from a security perspective and we include that in our repo. This PR doesn't introduce much (any?) new 'security' related information and actually spends most of its time talking about the project from a process or governance perspective. Not bad things but it feels a bit odd to be part of a "security" review document. Yes the TOB review is a point in time review, but so is this PR... or any assessment. I'm just trying to understand the impact/benefit of this piece of work when it feels like just about all of the info is already part of our repo (or should be as that's the source of truth for all things CE) so why duplicate it here, and are we then on the hook to keep it up to date since the minute the PR is merged it could be out of date?

Sorry if this sounds a bit like I'm pushing back, but I have a very strong negative reaction to things that feel like unnecessary bureaucracy - especially if it then requires an on-going commitment.

Sure, feel free to hop into a TAG Security meeting or to raise an issue there to discuss.

I will say that your perspective has not been uncommon for folks at the beginning of the process. I invite you to talk about folks who completed it to get their perspective. Their candid feedback was that an assessment helped their project's security in a way that an audit did not.


## Self-assessment use

This self-assessment is created by the CloudEvents team to perform an internal
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we modify this to more accurately state who created it? It's fair to add that the CE team has reviewed it but the CE team did not create it. There might be other spots in here that might need similar tweaking.

I'd also like to have something added about how the "source of truth" for all things CE related can be found in the CE project repos and this is a point-in-time summary of the project.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback on this. That makes sense, I changed it now to be more precise on who created and reviewed it.

This security assessment was created by Igor Rodrigues, Matthew Gong, Kushal Kothari and Devyani Bairagya to perform an internal analysis of the CloudEvents project. The document was also reviewed by CloudEvents maintainers and members of the CNCF Security Technical Advisory Group (TAG). It is meant to provide a current summary of the project and its security-related aspects. It is not intended to provide a security audit of CloudEvents, or function as an independent assessment or attestation of CloudEvents's security health. For the latest state of the project please check the CloudEvents GitHub repository.

I also added the paragraph below to the beginning of the document to mention where we extracted this information from:

The information contained in this assessment was extracted from the CloudEvents GitHub Repository, and other channels maintained by the project, such as its website and Slack workspace.

Please, let me know if you want us to change these more or change other parts of the documents. Thank you for the help!

## Self-assessment use

This self-assessment is created by the CloudEvents team to perform an internal
analysis of the project's security. It is not intended to provide a security
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd change this to something like:
This assessment was created by ..., and has been reviewed by the CloudEvents team. It is meant to provide a point-in-time summary the state of the project and summarize its security related aspects. It is not intended to provide a security audit of CloudEvents, or function as an independent assessment or attestation of CloudEvents' security health. For the latest state of the project please see the CloudEvents github repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the suggestion, I incorporated it into the assessment with some modifications. Please, let me know if you want to make more changes to it.

Copy link
Collaborator

@JustinCappos JustinCappos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A solid Security Pals assessment! LGTM

@JustinCappos JustinCappos merged commit 6b1e182 into cncf:main Jan 17, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants