Add security_contacts field to OWNERS spec

[sfowl]

This supports #56

I don't believe similar changes are also needed to https://github.com/kubernetes/test-infra/blob/master/prow/repoowners/repoowners.go as no automation depends on this new additional field. Similarly, there seems to be no code references in test-infra for emeritus* keys, so I imagine adding new keys to OWNERS should not cause any issues.

EDIT: Made a PR for test-infra changes: kubernetes/test-infra#22075

[k8s-ci-robot]

Welcome @sfowl!

It looks like this is your first PR to kubernetes/community 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/community has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @sfowl. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

[sfowl]

Removed WIP status, need to merge this before proceeding with bulk PRs to all OWNERS files and remove SECURITY_CONTACTS files.

[sfowl]

Mail thread for more context: https://groups.google.com/g/kubernetes-sig-architecture/c/0MUJBDGf3jg/m/wYrr8i1tBQAJ

[tpepper]

/ok-to-test

Maybe a dumb question, but why's this one the pre-req for bulk changes? Is it just to ensure the docs at contributors/guide/owners.md are right and then the bulk changes are implementing what's documented?

I ask mostly because I worry about what might break in this change. Is there anybody out there possibly using SECURITY_CONTACTS as it is an API? The current files aren't highly structured, but one could guess the "schema"? I don't see this as a blocker though. PSC has agreed a reasonable path in kubernetes/committee-security-response#56 which includes removal of the SECURITY_CONTACTS and you're implementing that. And it looks like you've done some archeology to try to confirm none of our things were using the files.

I've got one question about the new schema data around PII (see inline comments).

[tpepper]

Are we "allowed" to publish these? I always hear this type of public identification mapping is somehow not allowed by GitHub ToS?

Will all three of {github,email,slack} be required?

[mrbobbytables]

I'm pretty sure we can publish those - github/email is mapped already via cncf gitdm from commits, and we have github -> slack mappings for slack user-groups in the slack-config.

Many people publish that info themselves for their personal website as well.

[sfowl]

Will all three of {github,email,slack} be required?

No, I updated the descriptions to be clearer that only GitHub username is required. The other fields are optional.

I'm pretty sure we can publish those - github/email is mapped already via cncf gitdm from commits, and we have github -> slack mappings for slack user-groups in the slack-config.

That was my feeling as well. I also reached out to GitHub's privacy team for some input but I'm not sure if this is within their scope. I'll share any reply I get.

Also, I think the intention is for individuals to add update their own email/slack IDs. The bulk updates that add security_contacts to OWNERS will have email and slack set to null for all users.

[sfowl]

Got a reply from github Trust & Safety team:

Thanks for reaching out. Our Acceptable Use Policies address your question in the following way:

While using the Service, under no circumstances will you:

violate the privacy of any third party, such as by posting another person's personal information without consent.

Additionally, Section 6 may apply, depending on where you're getting the information:

If you collect any User Personal Information from the Service, you agree that you will only use that User Personal Information for the purpose for which that User has authorized it.

If you're gathering and using public information from GitHub and are receiving consent from each of the data subjects, our Privacy Statement includes some further requirements. The following is especially important:

We expect you to reasonably secure any User Personal Information you have gathered from GitHub, and to respond promptly to complaints, removal requests, and "do not contact" requests from GitHub or GitHub users.

I hope this is helpful. Please let us know if we can answer any other questions.

I think the current proposal be okay then. Users would need to add their own contact info, we can't add any on their behalf.

[mrbobbytables]

One other thing I forgot to mention with Slack, names are not unique in there :| There can be multiple mrbobbytables for examples, thats why we have the mapping of slack name to underlying slack ID in usergroup management. I would avoid including that at all as you could easily ping the wrong person.

[sfowl]

Ah, yeah I see what you mean looking in communication/slack-config. Some other options might be either to ask for the actual slack ID instead (downside: not easily enforceable), or enhance the spec to have keys for both slack name and slack ID (downside: messier, still not really enforceable).

So I think your suggestion to remove the slack field entirely is probably the most sensible. I'll hold back on the change for a day in case there are other ideas/opinions.

[lukehinds]

when this is place I am good to lgtm on this PR.

[sfowl]

Removed the slack field

[sfowl]

Maybe a dumb question, but why's this one the pre-req for bulk changes? Is it just to ensure the docs at contributors/guide/owners.md are right and then the bulk changes are implementing what's documented?

That was pretty much my thinking. It's also a chance to review the planned changes before creating the noise generated by bulk PRs.

I ask mostly because I worry about what might break in this change. Is there anybody out there possibly using SECURITY_CONTACTS as it is an API? The current files aren't highly structured, but one could guess the "schema"? I don't see this as a blocker though. PSC has agreed a reasonable path in kubernetes/security#56 which includes removal of the SECURITY_CONTACTS and you're implementing that. And it looks like you've done some archeology to try to confirm none of our things were using the files.

The existing docs I've seen describe SECURITY_CONTACTS as a data source only for use by the PSC. It's possible that someone else is using it as an API, but if this has the PSC's blessing then I'd agree that it's not a blocker.

In terms of breakages, today I came across the secping tool, which seems to still be used in test-infra. Mostly likely this will need some tweaks to be compatible with this change, but I'll move this back to WIP while I investigate.

[k8s-ci-robot]

@sfowl: The label(s) /label do-not-merge/work-in-progress cannot be applied. These labels are supported: api-review, community/discussion, community/maintenance, community/question, cuj/build-train-deploy, cuj/multi-user, platform/aws, platform/azure, platform/gcp, platform/minikube, platform/other, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash

In response to this:

/label do-not-merge/work-in-progress

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.

[sfowl]

/hold

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: sfowl
To complete the pull request process, please assign parispittman after the PR has been reviewed.
You can assign the PR to them by writing /assign @parispittman in a comment when ready.

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

Needs approval from an approver in each of these files:

• committee-steering/OWNERS
• contributors/guide/OWNERS
• github-management/OWNERS
• sig-service-catalog/OWNERS

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

[sfowl]

Sorry for the long delay on this @tpepper @mrbobbytables. Also CC-ing @tallclair @BenTheElder as they were on the original kubernetes/security issue and might have new feedback from the age it took me to circle back on this.

I've grepped through all repos in the below orgs for usage of SECURITY_CONTACTS:

kubernetes
kubernetes-client
kubernetes-csi
kubernetes-incubator
kubernetes-retired
kubernetes-security
kubernetes-sigs

The only example I found that used SECURITY_CONTACTS like an API was the secping tool, which opens issues in kube repos that are missing a SECURITY_CONTACTS file. I've made a PR already to change this behaviour to work with the new file structure, and another in test-infra to temporarily disable this tool during the transition.

I found no other instances that used SECURITY_CONTACTS like an API.

I've generated patches for the planned bulk updates each repo in the above orgs using this script. Here's an example using kubernetes/community.

My proposal now is to:

1. Ask for this change to be merged
2. Ask for temporary disable of secping in test-infra change to be merged
3. Create bulk PRs to update OWNERS/SECURITY_CONTACTS in all repos in all kube orgs
4. Update and re-enable secping tool

PS - Any suggestions on somewhere I could drop an email to give a heads up to repo owners about this change would be appreciated.

[sfowl]

/unhold

[mrbobbytables]

Putting the hold back on it just to make sure it gets reviews from the right groups before merge 👍
/hold

cc @kubernetes/steering-committee
cc @kubernetes/owners

As an FYI the orgs kubernetes-incubator and kubernetes-retired are archived, they are not receiving any more updates.

[lukehinds]

cc @kubernetes/product-security-committee

[sfowl]

Based on some feedback in kubernetes/committee-security-response#56 from @BenTheElder , I'm gonna re-examine this PR and see how I can re-add slack IDs (i.e. not slack display names) in combination with some validation in kubernetes/test-infra/prow/repoowners and kubernetes/test-infra/prow/plugins/verify_owners.

Will mark as draft again for the meantime.

[sfowl]

@BenTheElder I made a draft test-infra PR with some validation for slack IDs. My updated proposal is now:

1. Get for this change (the spec update) to be merged
2. Get test-infra validation PR merged
3. Get temporary disable of secping in test-infra change to be merged
4. Create bulk PRs to update OWNERS/SECURITY_CONTACTS in all repos in all kube orgs
5. Update and re-enable secping tool

[k8s-triage-robot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[nikhita]

@sfowl 👋 just wanted to check in -- is this still desired/are you still looking for review?

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Close this issue or PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

[k8s-triage-robot]

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

[k8s-ci-robot]

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages issues and PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the issue is closed

You can:

• Reopen this issue or PR with /reopen
• Mark this issue or PR as fresh with /remove-lifecycle rotten
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

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.