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

feat: add first set of allstars policies #3

Merged
merged 4 commits into from
Feb 6, 2024

Conversation

sakshi-1505
Copy link
Contributor

@sakshi-1505 sakshi-1505 commented Jan 19, 2024

fixes #2

@sakshi-1505 sakshi-1505 requested a review from a team January 19, 2024 16:03
Copy link
Contributor

@codeboten codeboten 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 opening this @sakshi-1505! really excited to see progress on this front! Just some questions as I'm not super familiar w/ the configuration presented.

binary_artifacts.yaml Show resolved Hide resolved
admin.yaml Show resolved Hide resolved
actions.yaml Show resolved Hide resolved
@codeboten
Copy link
Contributor

Pinging @open-telemetry/go-maintainers as well as this impacts their repos

Copy link

@MrAlias MrAlias left a comment

Choose a reason for hiding this comment

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

What is allstars and why is it being applied to our repos?

It would be preferred if this change request was proposed after some kind of communication. Ideally synchronous. Can you attend next week's SIG meeting to describe what this is and why it is being applied?

@sakshi-1505
Copy link
Contributor Author

What is allstars and why is it being applied to our repos?

It would be preferred if this change request was proposed after some kind of communication. Ideally synchronous. Can you attend next week's SIG meeting to describe what this is and why it is being applied?

Hi Tyler; This is not exclusive to your repos but it is going to be rolled out to complete organisation. For the first pilot me & @jpkrohling had decided to go with otel-go repos. This is to mandatory implement an organisation wide governance policy which is a requirement from SIG-Security. You can read how & what Allstars will do on your repositories here https://github.com/open-telemetry/.allstar/blob/94009c49bc3e8483359187f74af891e477869614/README.md.
You can find more details around the requirement & why we went with Allstar here:

@jpkrohling
Copy link
Member

@MrAlias, you are right, we should have pinged the Go folks before doing this. At this point, the SIG Security is testing this tool to check whether it can help us have visibility into the adherence of all repos in the organization to some (arbitrary) security practices. Once we know how this tool behaves, we'll be able to suggest it more broadly to all SIGs. We are not at that stage yet, but again, we should definitely have had your buy-in before using the Go repos for this trial.

In any case, I'll add a mention to this for the next maintainer's call and #otel-maintainers, so that you are all aware that we are experimenting with this.

@jpkrohling
Copy link
Member

@MrAlias, I created a new thread on the #otel-go repo, so that other maintainers are aware: https://cloud-native.slack.com/archives/C01NPAXACKT/p1706539315137359

Is there any specific concern you'd like us to address?

@MrAlias
Copy link

MrAlias commented Jan 29, 2024

Is there any specific concern you'd like us to address?

What is allstar and what does it do? This PR description links to an issue that says it's going to set it up on our repository. There is no information about it and why this is being done.

I would like the impact this will have on our ability to uphold our responsibilities to be explained. When I joined this project maintainer are the ones responsible for the success and health of their repositories. This PR is being added by a security SIG that does not share that responsibility nor have a charter to do so.

I would also like to understand how this decision was made in such a vacuum. I expect this is not going to be the last of such decisions and it really deserves a retrospective. Having an external SIG impose decisions on existing SIGs with independent authority is not the way to run this project. Both the security SIG and governance committee need to address this.

@pellared
Copy link
Member

pellared commented Jan 29, 2024

From https://github.com/ossf/allstar:

For some security policies, Allstar can also automatically change the project setting that caused the violation, reverting it to the expected state.

I think we should make sure that it is NOT able to do any changes to the project settings. Otherwise, we are increasing the attack surface. This tool must have only readonly permissions (it could be OK to create issues, but no more than that).

The app asks for [...] file contents

This is concerning me a lot 😬

allstar.yaml Outdated
Comment on lines 7 to 8
- "opentelemetry-go"
- "opentelemtry-go-contrib"

Choose a reason for hiding this comment

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

Should we also include opentelemetry-go-build-tools and opentelemetry-proto-go?

@MadVikingGod
Copy link

This was discussed at the SIG today:

  • We would like some more info on how this decision was made. With just surface exploration, we have seen extensive permissions that seem to be a risk. So, how this was evaluated vs other solutions would be helpful in making this decision.
  • We would like some way to be notified of changes to our repositories, including if there is new scope or if this changes from issues to fix.
  • Is there some process that you have for going from creating issues to automatic enforcement? We sometimes have trouble doing tasks that require admin permissions. We don't want to be fighting a bot that removes that.
  • Is it possible to rollback the automatic enforcement in case something goes wrong?
  • Could we start this with a setting as log? That would mean that the Security Sig would need to review it instead of pushing this onto the maintainer immediately.

@jpkrohling
Copy link
Member

We would like some more info on how this decision was made

That's fair, and given it's not the first time this was asked, I decided to follow the paper trail to give a more complete picture here: the Security SIG was established last year with the goal of assisting maintainers in the security-related practices, and one of the things we are working on is a set of recommendations for maintainers, given that not everyone is expected to be knowledgeable in the security domain.

According to our notes for the meeting from August 23, @codeboten brought this to the issue to the maintainers' call earlier that week and it was well received. On that call, he shared the following tracker issue: open-telemetry/sig-security#12 . This is the one pertaining Go: open-telemetry/opentelemetry-go#4459

One week later, while talking about ways to do this on a bigger scale, we were pointed out to Allstar, which we started to investigate (open-telemetry/sig-security#21).

With just surface exploration, we have seen extensive permissions that seem to be a risk

I understand we've given read-only permissions to this app. I'll let @sakshi-1505 confirm if that's all we need, and I'll let someone from the TC double-check. Our TC sponsor is @reyang, with @arminru working closely with us on this.

We would like some way to be notified of changes to our repositories

The SIG Security has no permission nor intention to change anything in anyone's repositories. All changes go through the TC, like this: open-telemetry/community#1831 .

In retrospect, I should have touched base with the Go SIG maintainers before including their repos during the installation of the app in the organization and I should have asked maintainers first regarding who'd be on board for trialing this tool.

Is there some process that you have for going from creating issues to automatic enforcement?

We don't plan on having automatic enforcement. The tool will create an issue if it sees a deviation from our recommendations. That's exactly what the humans of the SIG Security would also do manually. If a specific issue is seen as a false-positive, we can remove the specific repo from that specific check.

Is it possible to rollback the automatic enforcement in case something goes wrong?

No automatic enforcement is planned.

For the moment, we'll remove the Go repositories from the application installed at the organization level, adding only the Java Auto instrumentation, which volunteered for this trial.

@sakshi-1505
Copy link
Contributor Author

I understand we've given read-only permissions to this app. I'll let @sakshi-1505 confirm if that's all we need, and I'll let someone from the TC double-check. Our TC sponsor is @reyang, with @arminru working closely with us on this.

We don't need the intrusive permissions; those are only required when you want Allstar to have autofix for the issues but we are not doing that since we want the issues to get logged & manual intervention on fixes.

Could we start this with a setting as log? That would mean that the Security Sig would need to review it instead of pushing this onto the maintainer immediately.

This setting is a no-op from a user point of view until & unless one self-host their Allstar application. The log in Allstar policy refers to the application log from where one can set custom alerts to get notified.

Signed-off-by: sakshi-1505 <[email protected]>
@sakshi-1505
Copy link
Contributor Author

@jpkrohling @codeboten Can you folks please re-check & merge accordingly?

Copy link
Contributor

@codeboten codeboten left a comment

Choose a reason for hiding this comment

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

Thanks @sakshi-1505! @MrAlias I believe your request for change has been addressed correct?

@MrAlias
Copy link

MrAlias commented Feb 6, 2024

@MrAlias I believe your request for change has been addressed correct?

Yeah. I don't see a way for me to dismiss my review. Please dismiss my review if you're able.

@codeboten codeboten merged commit 879018d into open-telemetry:main Feb 6, 2024
1 check passed
@sakshi-1505 sakshi-1505 deleted the add_allstars branch February 6, 2024 17:25
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.

Add first set of Allstar policies
6 participants