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

New label for officially announced CVEs by SRC #23428

Merged
merged 2 commits into from
Dec 1, 2021

Conversation

PushkarJ
Copy link
Member

@PushkarJ PushkarJ commented Aug 31, 2021

This PR adds a new label official-cve-feed for CVEs announced by SRC

  • Currently, it is not possible to filter for issues and PRs that are related to CVEs found in Kubernetes
  • It will allow filtering and automation to create a CVE feed for Kubernetes
  • This is a restricted label that can be added by SRC and Tooling Lead
  • Limited to k/k repo for clarity of scope

Related Issues and PRs:

/sig contribex security

/cc @kubernetes/sig-contributor-experience-pr-reviews

@k8s-ci-robot k8s-ci-robot added sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/label_sync Issues or PRs related to code in /label_sync sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Aug 31, 2021
@PushkarJ
Copy link
Member Author

PushkarJ commented Sep 1, 2021

/retest-required

@PushkarJ PushkarJ force-pushed the new-label-cves branch 2 times, most recently from 2b05571 to 203aabd Compare September 1, 2021 18:38
@justaugustus
Copy link
Member

This would be a great label to filter on. AFAIK, we currently use area/security or area/release-eng/security (for RelEng).
Would love to see this as a supported label for all repos, not just kubernetes/kubernetes.

cc: @kubernetes/release-engineering @kubernetes/security-response-committee @kubernetes/sig-security

@PushkarJ
Copy link
Member Author

PushkarJ commented Sep 1, 2021

This would be a great label to filter on. AFAIK, we currently use area/security or area/release-eng/security (for RelEng).
Would love to see this as a supported label for all repos, not just kubernetes/kubernetes.

cc: @kubernetes/release-engineering @kubernetes/security-response-committee @kubernetes/sig-security

Thank you @justaugustus, refactored the label now to make it available to all repos. To your point, was hoping to use area/security or area/release-eng/security for the purposes of filtering for CVEs but realized that it was a very coarse grained filter, which is what led to this idea about creating a new label :-)

@cblecker
Copy link
Member

cblecker commented Sep 1, 2021

/hold

So what I don't see on https://github.com/kubernetes/community/issues/5923 is consensus on this. Has this been brought up and discussed with sig-security and the security response committee? If they are on board with the plan/label, then the other thing we'd need to do is give notice to the community. Adding this label globally will add to all repos in all orgs.. we don't want anyone to be surprised.

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2021
@PushkarJ
Copy link
Member Author

PushkarJ commented Sep 3, 2021

@cblecker You are right, I definitely missed adding evidence of consensus between SIG Security and SRC. We discussed this in SIG Security on 07/29 and one of the action items was to get input from SRC and address any concerns. As its been a while since then, I have added this to the agenda for our next SIG Security meeting on 09/09

Also something that I did not clarify earlier, even though the label will be available for all repos, the intent is to create automation only for kubernetes/kubernetes repo.

@tabbysable
Copy link
Member

(with my sig security hat on)
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 23, 2021
@puerco
Copy link
Member

puerco commented Sep 23, 2021

Building on this idea, I think we could even tag the actual CVE pull requests with the new label automatically from the data that the SRC hands over to release engineering every time a CVE is fixed in a patch release. I've added an issue to have Release Engineering work on this once this merges:

kubernetes/release#2264

From RelEng:
/lgtm

@tallclair
Copy link
Member

If the plan is to use this label as input for an automated list of Kubernetes CVEs, then we should try to keep it constrained (at least on the issue side) to only the "official" CVE issues that get linked from the announcements / CVE reference. Alternatively, I could also see this used to track follow up requests (e.g. kubernetes/kubernetes#95086). Either would be useful, but we should be clear on the expectations.

A related question: the SRC sometimes handles things that are security incidents but not CVEs. These don't usually get filed as issues on k/k, but if there was an issue filed would you expect it to be labeled with area/cve?

@tabbysable
Copy link
Member

I'm unfamiliar with what's available in GitHub - is there a way to restrict who can apply a label? Agreed that it would be good to restrict it to prevent accidental pollution of the search results if we're using it to catalog the CVE issues. Perhaps to the robot + SRC?

I think labeling the non-CVE issue disclosures with area/cve would be good, despite it not quite matching up to the name (or it could be area/vuln, area/security-advisory, or something like that if we really wanted it to be strictly correct in its name)

@PushkarJ
Copy link
Member Author

PushkarJ commented Sep 23, 2021

If the plan is to use this label as input for an automated list of Kubernetes CVEs, then we should try to keep it constrained (at least on the issue side) to only the "official" CVE issues that get linked from the announcements / CVE reference.

@tallclair : yes that is the plan and agree on scoping to the ones officially announced as CVEs from SRC

I think labeling the non-CVE issue disclosures with area/cve would be good, despite it not quite matching up to the name (or it could be area/vuln, area/security-advisory, or something like that if we really wanted it to be strictly correct in its name)

@tabbysable: My suggestion would be to continue to use area/security for any issue that does not fix a vulnerability which has been given a CVE identifier by SRC (i.e. the scope above)

EDIT: Sorry for not previewing my message earlier

@PushkarJ
Copy link
Member Author

I'm unfamiliar with what's available in GitHub - is there a way to restrict who can apply a label? Agreed that it would be good to restrict it to prevent accidental pollution of the search results if we're using it to catalog the CVE issues. Perhaps to the robot + SRC?

@tabbysable : Yes agree that we should restrict who can put this label. It seems like the labels can be authorized based on different kinds of membership as per this doc

Various other plugins manage labels, milestone, and issue state based on /foo style commands from authorized users. Authorization may be based on org membership, GitHub team membership, or OWNERS file membership.

I will dig more to find out how to exactly achieve this. But wanted to put this out in case anyone exactly knows how to do this.

@PushkarJ PushkarJ changed the title New label for CVEs New label for officially announced CVEs by SRC Sep 27, 2021
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 28, 2021
@PushkarJ
Copy link
Member Author

@tabbysable , @tallclair Good news!! Thanks to pointers from @alvaroaleman (Links for which are now added in the PR description) found a way to add a label based on Github memberships.

New commit which also passed all the checks, does the following changes:

  • Renames label from area/cve to official-cve-feed to clarify intent and expected usage
  • Restricts scope where label can be applied from all repos and orgs to only k/k to keep it simple as we are using restricted labels within Kubernetes Org for the first time
  • Limits who can add the label i.e. SRC Github team and Me (SIG Security Tooling Lead).
  • As Tooling sub-project grows in future, we can consider adding a Github team for it too instead of pointing to just me

@puerco for kubernetes/release#2264 you may need to add a team from release engg. to allow it to auto-label in-scope Issues

@PushkarJ
Copy link
Member Author

PushkarJ commented Oct 5, 2021

/assign tabbysable tallclair

For /lgtm or further input

Copy link
Member

@tallclair tallclair 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 follow-up!

label_sync/labels.yaml Show resolved Hide resolved
@sftim
Copy link
Contributor

sftim commented Oct 27, 2021

Do we also need to reconfigure the label plugin?

See https://github.com/kubernetes/test-infra/pull/24058/files for an example.

@PushkarJ
Copy link
Member Author

PushkarJ commented Oct 30, 2021

Do we also need to reconfigure the label plugin?

See https://github.com/kubernetes/test-infra/pull/24058/files for an example.

@sftim Looking at this block of code, it seems like additional labels is a separately handled from restricted labels: https://github.com/alvaroaleman/test-infra/blob/c37ab31c679b722b9371dfff87017db343f8bb17/prow/plugins/label/label.go#L169-L178

Example usage of restricted labels also does not seem to need additional labels yaml block here: https://github.com/openshift/release/blob/7bb7822854861b87d30c9c09c77c77b33b5dfafe/core-services/prow/02_config/openshift/cluster-baremetal-operator/_pluginconfig.yaml#L6-L13

Let me know if I am missing something or if I misunderstood the way labels are being managed by the plugin

- Currently, it is not possible to filter for
  issues and PRs that are related to CVEs found
  in Kubernetes

- It will allow filtering and automation to create
  a CVE feed for Kubernetes

- This is a restricted label that can be added by SRC
  and Tooling Lead

- Limited to k/k repo for clarity of scope
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2021
@PushkarJ
Copy link
Member Author

PushkarJ commented Nov 2, 2021

/label tide/merge-method-squash

@k8s-ci-robot k8s-ci-robot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Nov 2, 2021
@PushkarJ
Copy link
Member Author

PushkarJ commented Dec 1, 2021

@cblecker : The new changes have addressed the concerns raised here: #23428 (comment) to remove /hold as follows:

  • @tabbysable and @tallclair as SRC and SIG Security members have +1 OR /lgtm on this. Last commit erased the /lgtm from New label for officially announced CVEs by SRC #23428 (comment) . Confirmed from @tabbysable on slack that both of them are good with the current state of the changes
  • The label is restricted i.e. can be only applied by SRC and SIG Security Tooling Lead and is applied only to k/k repo. So there is no risk of drive through labelling of unrelated issues or it being a surprise to the community. There will also be a KEP created soon on how this label is consumed, that will allow us to get more visibility into this from community. But this label is a pre-requisite for that KEP.

Happy to hear any additional concerns or questions that still need addressing. Thank you for asking to do this due diligence. I believe we have a well thought out labelling process because of it now.

/assign @cblecker

@cblecker
Copy link
Member

cblecker commented Dec 1, 2021

/lgtm
/approve
/hold cancel

@PushkarJ Thanks so much for jumping through the hoops! Glad we were able to figure out a great solution for this :)

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 1, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker, PushkarJ

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 1, 2021
@k8s-ci-robot k8s-ci-robot merged commit 5f63b9f into kubernetes:master Dec 1, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.24 milestone Dec 1, 2021
@k8s-ci-robot
Copy link
Contributor

@PushkarJ: Updated the following 2 configmaps:

  • plugins configmap in namespace default at cluster test-infra-trusted using the following files:
    • key plugins.yaml using file config/prow/plugins.yaml
  • label-config configmap in namespace test-pods at cluster test-infra-trusted using the following files:
    • key labels.yaml using file label_sync/labels.yaml

In response to this:

This PR adds a new label official-cve-feed for CVEs announced by SRC

  • Currently, it is not possible to filter for issues and PRs that are related to CVEs found in Kubernetes
  • It will allow filtering and automation to create a CVE feed for Kubernetes
  • This is a restricted label that can be added by SRC and Tooling Lead
  • Limited to k/k repo for clarity of scope

Related Issues and PRs:

/sig contribex security

/cc @kubernetes/sig-contributor-experience-pr-reviews

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/config Issues or PRs related to code in /config area/label_sync Issues or PRs related to code in /label_sync cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants