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

add SECURITY_CONTACTS to repo template #2062

Merged
merged 1 commit into from
May 22, 2018

Conversation

jessfraz
Copy link
Contributor

This adds a list of contacts for the repo that the Product Security Team can
reach out to for triaging and handling of incoming issues, please try to keep
this list small as the contact can then further decide who "needs to know" to
complete the fix.

@k8s-ci-robot k8s-ci-robot added 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. labels Apr 20, 2018
@k8s-ci-robot k8s-ci-robot added area/contributor-guide Issues or PRs related to the contributor guide sig/contributor-experience Categorizes an issue or PR as relevant to SIG Contributor Experience. labels Apr 20, 2018
@jessfraz
Copy link
Contributor Author

@cblecker
Copy link
Member

cc: @cjwagner @BenTheElder @fejta @spiffxp (for comment on the OWNERS file spec itself)

@jessfraz: are you looking to have any automation consume this, or is this primarily for humans on the PST to use?

/area security test-infra

@jessfraz
Copy link
Contributor Author

jessfraz commented Apr 20, 2018 via email

@BenTheElder
Copy link
Member

I will let @cjwagner confirm but I believe the existing tooling will only inspect known fields. Makes sense for this to be in OWNERS to me :-)

@idvoretskyi
Copy link
Member

/hold

for the discussion keep going

@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 Apr 20, 2018
@@ -47,6 +51,8 @@ reviewers:
- alice
- carol # this is another comment
- sig-foo # this is an alias
security:
- alice
Copy link
Member

Choose a reason for hiding this comment

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

It's usually easy enough to track someone down via their github handle, but not always (email hidden, different slack handle). Do we want to require contact information 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.

oh good point we probably only want emails, but then idk if people want their email in a repo...

Copy link
Member

Choose a reason for hiding this comment

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

We have had privacy/legal issues previously around putting personal information (such as e-mail addresses) inside repos. You need explicit consent from anyone who you do this for. Not impossible, but can be difficult to enforce and audit.

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, there were issues with requiring personal info about people be published on github

Copy link
Member

Choose a reason for hiding this comment

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

@liggitt
Copy link
Member

liggitt commented Apr 20, 2018

see also #1808 (comment) for rationalizing how OWNERS files map to repos/sigs/subprojects... we probably wouldn't want this field populated in deeply nested folders or at multiple levels

@jessfraz
Copy link
Contributor Author

true we only need it in the root, is there a way to designate that or should i just add to the definition?

@jessfraz jessfraz force-pushed the security-owners branch 2 times, most recently from d0121a5 to 21f8a9a Compare April 20, 2018 20:59
@liggitt
Copy link
Member

liggitt commented Apr 20, 2018

is there a way to designate that or should i just add to the definition?

no formal way I know of, probably just a comment

@cjwagner
Copy link
Member

Why does this belong in the OWNERS files? It seems like it should not for a few reasons:

  • OWNERS files are scoped to files/directories and can apply recursively and cumulatively to directories beneath them. This list of contacts is simply scoped at the repo level.
  • Existing OWNERS file automation wouldn't be applicable to this field because it is only allowed to be specified at root.
  • It doesn't sound like this field would require any related automation.

I think putting this information in something like a SECURITY_CONTACTS (or a more general CONTACTS) file at the repo root would serve the same function without conflating the function of OWNERS files.

@cblecker
Copy link
Member

Putting it in OWNERS may allow for future parsing, and I can also see some use cases where you'd want nested information.. mono repos like k/k would probably have multiple levels of security contacts, where as most repos would only need this at the root.

@tpepper
Copy link
Member

tpepper commented Apr 20, 2018

The role description (and the guidance to put in root of the sig’s repos?) should also be added to https://github.com/kubernetes/community/blob/master/committee-steering/governance/sig-governance-template-short.md

@cblecker
Copy link
Member

@cjwagner It sounds like though, that there are no technical issues with adding this. The existing automation will ignore it, correct?

If so, then I'd pose the question back to the PST: Is there a future intention of using the OWNERS system/automation with these contacts, or will it for the foreseeable future just be a human contact list? If there are future intentions of using the OWNERS system (such as prow plugins/tools), then I'm supportive of expanding the specification. If there is no intention of using any of that logic in the future, then perhaps a separate file that the PST controls the spec of is better. I'm okay letting you folks decide.

@jessfraz
Copy link
Contributor Author

I mean it might be nice to have automation in the future but as of right now I was just going to maybe make us a tool to do the lookups and translate to an email.

@cblecker
Copy link
Member

Then I'd say, working under the assumption that this will not break any existing automation/tooling, my opinion would be that we expand the spec to include security contacts. There is already code available to parse the OWNERS file if we use github handles (https://git.k8s.io/test-infra/prow/plugins/approve/approvers), and within the project people know where to go to the OWNERS file to look for things.

@cjwagner
Copy link
Member

@cblecker Yeah, it shouldn't break anything.

@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Apr 23, 2018
@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 17, 2018
@jessfraz
Copy link
Contributor Author

sorry for the delay...

opened kubernetes/kubernetes-template-project#18

and updated here

@jessfraz jessfraz changed the title owners: add security to owners spec add SECURITY_CONTACTS to repo template May 17, 2018
@cblecker
Copy link
Member

/lgtm
/assign @spiffxp @philips

This works for me. Assigning to Aaron/Brandon for approval.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 18, 2018
- *MUST* be a contact point for the Product Security Team to reach out to for
triaging and handling of incoming issues
- *MUST* accept the [Embargo Policy](https://github.com/kubernetes/sig-release/blob/master/security-release-process-documentation/security-release-process.md#embargo-policy)
- Defined in [SECURITY_CONTACTS] files, this is only relevant to the root file in
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want backticks instead of brackets here? Althernatively, could you add a reference like this (which currently 404s, but that's a separate issue)? If you add the reference, you'll want a SECURITY_CONTACTS in this repo (probably not a bad idea anyway).

@jessfraz
Copy link
Contributor Author

jessfraz commented May 21, 2018 via email

@jessfraz
Copy link
Contributor Author

jessfraz commented May 21, 2018 via email

@wking
Copy link
Contributor

wking commented May 21, 2018

Also the line above with OWNERS has brackets I merely copied the formatting

Look at the formatted rendering, which currently shows a link for OWNERS and a bracketed [SECURITY_CONTACTS]. The difference is the reference definition for OWNERS, which allows GitHub's renderer to interpret [OWNERS] an implicit reference-style link. Without a local SECURITY_CONTACTS to link, I think you'll just want to replace the SECURITY_CONTACTS brackets with backticks.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2018
triaging and handling of incoming issues
- *MUST* accept the [Embargo Policy](https://github.com/kubernetes/sig-release/blob/master/security-release-process-documentation/security-release-process.md#embargo-policy)
- Defined in `SECURITY_CONTACTS` files, this is only relevant to the root file in
the repository
Copy link
Contributor

@wking wking May 21, 2018

Choose a reason for hiding this comment

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

Do you want to define the syntax here (one GitHub username per line, with blank lines and # comments ignored)? Or just link to the template once kubernetes/kubernetes-template-project#18 lands and assume the sytax is self-evident? Without at least one of those, examples in the wild may not be consistent enough for machine parsing.

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 do

Signed-off-by: Jess Frazelle <acidburn@microsoft.com>
@jessfraz
Copy link
Contributor Author

this should be all good now and inline with kubernetes/kubernetes-template-project#18

@cblecker
Copy link
Member

@philips Would you mind reviewing this and kubernetes/kubernetes-template-project#18 as @spiffxp is out?

@cblecker
Copy link
Member

kubernetes/kubernetes-template-project#18 has merged

/lgtm
/approve
/hold cancel

@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 May 22, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cblecker

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 May 22, 2018
@k8s-ci-robot k8s-ci-robot merged commit 4571dbe into kubernetes:master May 22, 2018
@jessfraz jessfraz deleted the security-owners branch May 22, 2018 23:17
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/contributor-guide Issues or PRs related to the contributor guide area/security area/test-infra cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. committee/steering Denotes an issue or PR intended to be handled by the steering committee. 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. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.