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

Cloud Custodian Joint Review markdown #786

Merged
merged 57 commits into from
Feb 1, 2022

Conversation

sunstonesecure-robert
Copy link
Contributor

needs review and final content

Copy link
Collaborator

@lumjjb lumjjb left a comment

Choose a reason for hiding this comment

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

Initial housekeeping comments

| Security Provider | Yes, however many users implement Cloud Custodian for non security purposes, i.e., cost optimization, governance |
| Languages | Python, YAML |
| Incubation PR | https://github.com/cncf/toc/pull/480 |
| SIG-Security Assessment Request | https://github.com/cncf/sig-security/issues/307 |
Copy link
Collaborator

@lumjjb lumjjb Sep 8, 2021

Choose a reason for hiding this comment

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

Suggested change
| SIG-Security Assessment Request | https://github.com/cncf/sig-security/issues/307 |
| TAG-Security Assessment Request | https://github.com/cncf/tag-security/issues/307 |

@TheFoxAtWork TheFoxAtWork changed the title Cloud Custodian Joint Review markdown WIP Cloud Custodian Joint Review markdown Sep 8, 2021
adding overview matrix
add missing matrix
Markdown linter.

Consolidating unordered list styles (i.e. `*` versus `-` to silence
linter; trailing period style, etc.).

Swapping out instances of "Custodian" and "c7n" with "Cloud Custodian"
(where appropriate).

Updating TOC via CLI TOC-generator tool.
Adding reviewers/participant section.
[CNCF-STAG-786] WIP Cloud Custodian Joint Review markdown
assessments/projects/custodian/joint-review.md Outdated Show resolved Hide resolved
assessments/projects/custodian/joint-review.md Outdated Show resolved Hide resolved

### Case Studies

TODO: 2-3 scenarios of real-world use cases.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will be resolved before we merge this correct ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will ping Jorge and @kapilt

Copy link
Collaborator

Choose a reason for hiding this comment

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

@sunstonesecure-robert @castrojo This is still outstanding. Please add 2-3 real world use cases before we can merge in, otherwise everything is good to go.

@sunstonesecure-robert
Copy link
Contributor Author

I'm not sure how to get past the spelling check issue - these are expected words we want in the content?

I have fixed the markdown formatting issues.

@TheFoxAtWork
Copy link
Collaborator

@sunstonesecure-robert i've got an outstanding PR to correct some of the larger spelling items (add to the linter). i'm waiting on one of the leadership team to review/approve

Copy link
Collaborator

@TheFoxAtWork TheFoxAtWork left a comment

Choose a reason for hiding this comment

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

Case Studies/Use cases are still outstanding. Please add them and then we can merge it in 🥳


### Case Studies

TODO: 2-3 scenarios of real-world use cases.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@sunstonesecure-robert @castrojo This is still outstanding. Please add 2-3 real world use cases before we can merge in, otherwise everything is good to go.

@castrojo
Copy link
Member

@sunstonesecure-robert
Copy link
Contributor Author

added @castrojo content with formatting fixups

@TheFoxAtWork
Copy link
Collaborator

@ashutosh-narkar please review

Copy link
Collaborator

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

Overall lgtm. Some minor comments.

Comment on lines +15 to +17
security flaws. As a utility tool, the review team feels the primary threat to
the project is compromise of the code itself, and packaging and distribution of
the tool or its dependencies - rather than an attack on individual c7n
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm curious what led to this conclusion since we haven't done a code review or tried out a sample deployment.

## Hands-on Review

Although some of the reviews have previously used c7n in AWS, both as cli and
in Lambda, Cloud Custodian did not receive a join hands-on review from
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nit: joint

| | | | | | |
| | | | | | |
+----------------------+------------------+----------+--------------------------+---------------+--------------------------------+
```
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need the HIGH vulnerabilities to be resolved or at least require the c7n team to open issues for them and link them here ?

Copy link

Choose a reason for hiding this comment

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

These are operating system library level considerations not per se the project's responsibility to fix, but the distribution vendor, ie a libc high issue is something i would expect the os vendor to triage and fix as needed. This is also specific to a distribution channel, and immaterial for those that install the project's software directly into their own OS / container base. For direct application dependency graphs concerns we already address those pro actively (dependabot alerts etc).

Choose a reason for hiding this comment

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

these were from the Github action docker scan - if the project is releasing/maintaining a docker image I think the user expectation is they should be fixing high security risks in the image as good hygiene.

maybe best to just do source releases? or clearly document that the image is not secure?

Copy link

Choose a reason for hiding this comment

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

This feels like a misunderstanding of operating system docker images, and what severity levels correspond to a need to immediate triage. If we take any production docker image out there using an operating system, they all have these sorts of cve nits wrt to high severity scoring. Take anything from redis to postgresql, (trivy image redis:latest etc), that doesn't make them insecure or high risk (trivy output for both here https://gist.github.com/cbdd697ce7f8ad0a40896c0a081c1a7c). The most relevant cves are those deemed critical, but all of operating system cves are triaged and dealt with by the security team of the respective distros which in many cases are professional security teams focused on exactly this topic. Moreover unlike postgresql or redis, we're talking about software in custodian that doesn't have an open port, its literally a cli, not a daemon or service running in a container.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My original comment was related to direct dependancies of c7n. @kapilt I think you mentioned you have a process to handle those iiuc.

Copy link

Choose a reason for hiding this comment

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

for custodian's dependency graph, we rely on dependabot alerts https://docs.github.com/en/code-security/supply-chain-security/managing-vulnerabilities-in-your-projects-dependencies/about-alerts-for-vulnerable-dependencies against our poetry.lock files which contain the version/hash dependency pins for the project. that alert is a prompt for us to update our dependency graph. our dep graph update process is automated and documented https://cloudcustodian.io/docs/developer/packaging.html and done regularly independent of vulnerabilities as a prelude to our release process.

Copy link

Choose a reason for hiding this comment

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

also to be specific there are zero cves extant on our application dependency graph at the moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for the clarification.

Copy link
Collaborator

@TheFoxAtWork TheFoxAtWork left a comment

Choose a reason for hiding this comment

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

@ashutosh-narkar are we good to merge this in?

Copy link
Collaborator

@ashutosh-narkar ashutosh-narkar left a comment

Choose a reason for hiding this comment

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

LGTM

@ashutosh-narkar
Copy link
Collaborator

@sunstonesecure-robert can you please rebase and we can merge this. Thanks.

@sunstonesecure-robert
Copy link
Contributor Author

@sunstonesecure-robert can you please rebase and we can merge this. Thanks.

try now

@ashutosh-narkar
Copy link
Collaborator

@sunstonesecure-robert can you please rebase and we can merge this. Thanks.

try now

Thanks. Can you please also squash your commits.

@TheFoxAtWork TheFoxAtWork merged commit 803e3ff into cncf:main Feb 1, 2022
@chasemp
Copy link
Contributor

chasemp commented Feb 1, 2022

It happened!

Michael-Susu12138 pushed a commit to Michael-Susu12138/tag-security that referenced this pull request Dec 12, 2023
* Create joint-review.md

* Update joint-review.md

* Update joint-review.md

* Update joint-review.md

* Update joint-review.md

* Consistency and refinement for initial land

* Headers capitalized 
* Some grammar updates

Various opinionated changes for readability and consistency.

* Add files via upload

attack paths

* Delete CloudCustodian DFD.drawio.png

* Add files via upload

attack paths with source

* Update joint-review.md

adding some attack paths and diagram

* Add files via upload

adding overview matrix

* Add files via upload

fix colors

* Update joint-review.md

add missing matrix

* Squashing paragraphs to 80-column line-length to appeasease `mdl`
Markdown linter.

Consolidating unordered list styles (i.e. `*` versus `-` to silence
linter; trailing period style, etc.).

Swapping out instances of "Custodian" and "c7n" with "Cloud Custodian"
(where appropriate).

Updating TOC via CLI TOC-generator tool.

* Updating TOC.

Adding reviewers/participant section.

* Update joint-review.md

adding additional CNCF recommendation for linter and/or locked down policy

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* add reviewer table per template

both those who contributed on the first round and the most recent cycles

* typo correction

fix grammar

* fix typo

misspelling

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* update reviewers table

last but NOT least

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* capitalize

consistently

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Emily Fox <[email protected]>

* use consistent YAML

the official docs use uppercase

* What: non spelling linter corrections

* Update joint-review.md

fixing linter issues for markdown

* What: ignore cSpell for GH handles

* What: general spelling corrections

* fixing my mistake

* Update assessments/projects/custodian/joint-review.md

* added case studies 

verbatim from https://gist.githubusercontent.com/castrojo/9c6058bc170cef075075b8f1f90b7515/raw/9a261a45589770fe266396192a7b46d407ffd9f8/gistfile1.txt but with formatting changes for linter happiness

* What: add ignore for JPMC

* What: fix lint

* Update assessments/projects/custodian/joint-review.md

Co-authored-by: Chase Pettet <[email protected]>
Co-authored-by: Matthew Giassa <[email protected]>
Co-authored-by: Emily Fox <[email protected]>
Co-authored-by: Emily Fox <[email protected]>
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.

9 participants