ossf / tac

Technical Advisory Council
https://openssf.org
Other
105 stars 46 forks source link

GitHub Enterprise Production Readiness Document Review - Authentication, Authorization, Audit Log Ingestion, Monitoring and Alerting #322

Closed Danajoyluck closed 1 month ago

Danajoyluck commented 1 month ago

Thanks everyone for investing time.....This issue is per Arnaud's request in response to the 4/30 TAC pre-read that CRob shared.

GitHub Enterprise Production Readiness Document needs TAC review and feedback. The document covers Authentication, Authorization, Audit Log Ingestion, Monitoring and Alerting. Authentication and authorization contents are built in conjunction with TAC issue 292 ( GitHub enterprise account structure and shared responsibility model). It is easier to review the account structure and shared responsibility model first before review the documents under this issue.

Production readiness document for this issue is here: https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.whnal1dq5jsm

mlieberman85 commented 1 month ago

I'm in favor of the hybrid model over the federated model. Federated lets a poorly governed org to be compromised more easily by a bad actor. The hybrid model gives LF staff a break glass mechanism.

sevansdell commented 1 month ago

Comments in document: https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.whnal1dq5jsm

SecurityCRob commented 1 month ago

I've also added a series of comments in the doc. It looks like we have a few separate things to officially consider that the TAC will need to provide feedback on the preferred direction on. We can work these asynchronously or we can make it part of the next call (28May).

SecurityCRob commented 1 month ago

me-- fat.fingers--

SecurityCRob commented 1 month ago

Staff is seeking a decision from the TAC on these elements from Dana's doc: 1.) GitHub Enterprise Account Structure - Hybrid or Federated Model? 2.) Fork pull request workflows from outside collaborators - y/n? 3.) require approval for first-time contributors - y/n? 4.) Allow GitHub actions to create and approve pull requests - y/n?

@ossf/tac please add a comment that indicates your choices for these 4 items

SecurityCRob commented 1 month ago

1.) Hybrid 2.) yes 3.) yes 4.) i need more info/debate to understand pros/cons. Seems like we need this for software projects, but want to hear from experts

bobcallaway commented 1 month ago

Here's the collective view after polling several folks that work on Sigstore:

  1. Hybrid: Sigstore wishes to stay as its own organization. We recognize that some projects may not have the backing to operate their own organizations however, so hybrid would be preferred over federated.
  2. See answer below. Yes, we should enforce that first-time contributors cannot run workflows.
  3. Require approval for first-time contributors or first-time contributors who are new. Anything more restrictive, "require approval for all outside collaborators", may be disruptive to maintainers.
  4. We SHOULD allow GitHub Actions to create and approve pull requests. Within Sigstore, we need the ability to create PRs from GitHub Actions for TUF and infrastructure automation. However, we do not need approval, as we currently leverage a bot account to handle PR approvals, and could move to a GitHub app at a later point. Because these options cannot be separated, we would recommend enabling this.
sevansdell commented 1 month ago

1.) Hybrid 2.) What is an outside collaborator? Need more information to understand pros/cons 3.) yes 4.) I need more info/debate to understand pros/cons. Seems like we need this for software projects, but want to hear from experts

steiza commented 1 month ago

1) GitHub Enterprise Account Structure - hybrid

It makes sense to me that mature projects want to have their own GitHub organization

2) Require approval for all outside collaborators to run Actions workflows on pull requests - yes

The potential attack here is if there's an insecure workflow (e.g. one that runs shell commands based on a field from the pull request), anyone could open up a pull request and be able to access anything the workflow has access to (secrets, pushing changes, etc).

Here "collaborators" means "anyone listed on the repository's /settings/access page". So you'd have to explicitly have a role in that repository (including Read), in order for workflows to run automatically on your pull requests.

In practice, anyone frequently making pull requests against a repository, who is trusted, should be given the Read repository role. Otherwise, it's not a huge deal to review the pull request for malicious code and then allow the Actions workflows to run after you've determined the pull request is safe.

3) Require approval for first-time contributors to run Actions workflows on pull requests - no

I believe option 3 is mutually exclusive with option 2 - they are radio buttons in the same config section - and I think we should do option 2.

4) Allow GitHub Actions to create and approve pull requests - yes

We definitely want it to be able to create pull requests, and unfortunately today you can't have one without the other. Lots of projects use GitHub Actions to create pull requests, see for example https://github.com/repository-service-tuf/repository-service-tuf/blob/755774b43efec464c2b98879547830e584ca038b/.github/workflows/update-python-deps.yml#L24 and https://github.com/theupdateframework/tuf-on-ci/blob/9799dece746fe39e402fa61f5eebbc96f19beafc/.github/workflows/update-pinned-deps.yml#L68.

marcelamelara commented 1 month ago

1.) GitHub Enterprise Account Structure: Hybrid

2.) Fork pull request workflows from outside collaborators: Yes, and we also add a requirement for TIs that maintainers of projects be assigned a GH role to the repo in question.

3.) Require approval for first-time contributors: As Zach mentioned, option 2 and 3 are mutually exclusive settings.

Which setting to pick, I actually think it depends on the scope and threat model of the project. For projects that are larger in scope and/or have a stricter threat model, I think it makes sense to make it a requirement to declare the official maintainers by assigning them a role on the repo. Then the "require approval for all outside collaborators" option can work. For projects that are more limited in scope or have a more relaxed threat model, only approving "first-time contributors" could be appropriate. I do worry about xz-style attacks where a project with a small maintainer group allows contributions by untrusted entities so I'm generally in favor of the stricter settings, but I do also worry about burdening maintainers with extra requirements that may not be needed in some cases.

4.) Allow GitHub actions to create and approve pull requests: yes on create, unsure on approve, but based on Zach's comment, it sounds like it's all or nothing right now.

torgo commented 1 month ago

Regarding the permissions for "outside collaborators" - what implications will this have for people coming to our repos to find out about or make use of our tooling or who wish to open issues? I'm slightly concerned that if we lock things down too tightly we might discourage potential collaborators?

steiza commented 1 month ago

what implications will this have for people coming to our repos to find out about or make use of our tooling or who wish to open issues?

Yes, to clarify this setting is regarding if GitHub Actions workflows should automatically run on pull requests from forks (like unit testing, a linter, or a spell-checker) or if they should require approval.

This does not affect the ability of anyone to fork an OpenSSF repo, or to open up a pull request. Just if Actions workflows run automatically on that pull request or not.

Danajoyluck commented 1 month ago

Staff is seeking a decision from the TAC on these elements from Dana's doc: 1.) GitHub Enterprise Account Structure - Hybrid or Federated Model? 2.) Fork pull request workflows from outside collaborators - y/n? 3.) require approval for first-time contributors - y/n? 4.) Allow GitHub actions to create and approve pull requests - y/n?

@ossf/tac please add a comment that indicates your choices for these 4 items

Thanks all for investing time in this..... @steiza @marcelamelara item 2 & 3 in the list from @SecurityCRob is one item, "Fork pull request workflows from outside collaborators" is the heading of the section that has 3 settings to choose from, "require approval for first-time contributors " is the current setting. Since we don't have self hosted runner, this setting should work.

marcelamelara commented 1 month ago

"Fork pull request workflows from outside collaborators" is the heading of the section that has 3 settings to choose from, "require approval for first-time contributors " is the current setting. Since we don't have self hosted runner, this setting should work.

@Danajoyluck I read the article linked in the main doc. While I generally agree that the issue can be largely mitigated by running GH-hosted runners only, this seems potentially difficult to enforce. Is the intent to require GH-hosted runners only, or are we currently assuming that projects won't use them? As I said in my previous comment, the "require approval for first-time contributors" will probably work in many cases, but I also think that these settings should also take project scope/threat model into consideration.

Danajoyluck commented 1 month ago

"Fork pull request workflows from outside collaborators" is the heading of the section that has 3 settings to choose from, "require approval for first-time contributors " is the current setting. Since we don't have self hosted runner, this setting should work.

@Danajoyluck I read the article linked in the main doc. While I generally agree that the issue can be largely mitigated by running GH-hosted runners only, this seems potentially difficult to enforce. Is the intent to require GH-hosted runners only, or are we currently assuming that projects won't use them? As I said in my previous comment, the "require approval for first-time contributors" will probably work in many cases, but I also think that these settings should also take project scope/threat model into consideration.

@marcelamelara thanks a lot for the comments.......All the OpenSSF projects are under the OpenSSF enterprise account, there is no plan to support self hosted runner in the foreseeable future. yes, It's safe to assume that projects won't use them.

sevansdell commented 1 month ago

@omkhar and @Danajoyluck :

  1. Is this the document you are looking to implement as is: https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl. If so, i will re-read through it and see if for each section and subsection there is a recommendation that will be implemented.

2.. Do you need anything further by the TAC from https://github.com/ossf/tac/issues/322#issuecomment-2117639284, or were the aggregate asynch comments good?

  1. Did you get everything you needed from https://github.com/ossf/tac/issues/292?

cc: @lehors , @bobcallaway , @SecurityCRob , @torgo @camaleon2016 , @marcelamelara, @mlieberman85

mlieberman85 commented 1 month ago
  1. Hybrid
  2. Yes
  3. Yes
  4. Eventually. I think whatever does this should itself have some level of sign off. So for now I would say we do it on a case by case basis.
Danajoyluck commented 1 month ago

@omkhar and @Danajoyluck :

  1. Is this the document you are looking to implement as is: https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl. If so, i will re-read through it and see if for each section and subsection there is a recommendation that will be implemented.

2.. Do you need anything further by the TAC from #322 (comment), or were the aggregate asynch comments good?

cc: @lehors , @bobcallaway , @SecurityCRob , @torgo @camaleon2016 , @marcelamelara, @mlieberman85

@sevansdell About Question 1, does this paragraph in the document answer your question? If not, we should get on zoom so I understand your question better and provide better response

image

About question 2, could you please clarify what you meant?

sevansdell commented 1 month ago

@omkhar and @Danajoyluck :

  1. Is this the document you are looking to implement as is: https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl. If so, i will re-read through it and see if for each section and subsection there is a recommendation that will be implemented.

2.. Do you need anything further by the TAC from #322 (comment), or were the aggregate asynch comments good? cc: @lehors , @bobcallaway , @SecurityCRob , @torgo @camaleon2016 , @marcelamelara, @mlieberman85

@sevansdell About Question 1, does this paragraph in the document answer your question? If not, we should get on zoom so I understand your question better and provide better response

image

About question 2, could you please clarify what you meant?

@Danajoyluck
q1 . For each section in https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl what is the outcome that will be implemented? q2. TAC is commenting on this question from crob on your behalf. Did you get what you needed in the issue comments by TAC? Staff is seeking a decision from the TAC on these elements from Dana's doc: 1.) GitHub Enterprise Account Structure - Hybrid or Federated Model? 2.) Fork pull request workflows from outside collaborators - y/n? 3.) require approval for first-time contributors - y/n? 4.) Allow GitHub actions to create and approve pull requests - y/n?

@ossf/tac please add a comment that indicates your choices for these 4 items

Happy to meet synchronously if needed - i don't think I can provide more context asynch here.

Danajoyluck commented 1 month ago

@omkhar and @Danajoyluck :

  1. Is this the document you are looking to implement as is: https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl. If so, i will re-read through it and see if for each section and subsection there is a recommendation that will be implemented.

2.. Do you need anything further by the TAC from #322 (comment), or were the aggregate asynch comments good? cc: @lehors , @bobcallaway , @SecurityCRob , @torgo @camaleon2016 , @marcelamelara, @mlieberman85

@sevansdell About Question 1, does this paragraph in the document answer your question? If not, we should get on zoom so I understand your question better and provide better response image About question 2, could you please clarify what you meant?

@Danajoyluck q1 . For each section in https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl what is the outcome that will be implemented? q2. TAC is commenting on this question from crob on your behalf. Did you get what you needed in the issue comments by TAC? Staff is seeking a decision from the TAC on these elements from Dana's doc: 1.) GitHub Enterprise Account Structure - Hybrid or Federated Model? 2.) Fork pull request workflows from outside collaborators - y/n? 3.) require approval for first-time contributors - y/n? 4.) Allow GitHub actions to create and approve pull requests - y/n?

@ossf/tac please add a comment that indicates your choices for these 4 items

Happy to meet synchronously if needed - i don't think I can provide more context asynch here.

@sevansdell I put a meeting on your calendar to discuss these

sevansdell commented 1 month ago

@omkhar and @Danajoyluck :

  1. Is this the document you are looking to implement as is: https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl. If so, i will re-read through it and see if for each section and subsection there is a recommendation that will be implemented.

2.. Do you need anything further by the TAC from #322 (comment), or were the aggregate asynch comments good? cc: @lehors , @bobcallaway , @SecurityCRob , @torgo @camaleon2016 , @marcelamelara, @mlieberman85

@sevansdell About Question 1, does this paragraph in the document answer your question? If not, we should get on zoom so I understand your question better and provide better response image About question 2, could you please clarify what you meant?

@Danajoyluck q1 . For each section in https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit#heading=h.4fuv1otu8gtl what is the outcome that will be implemented? q2. TAC is commenting on this question from crob on your behalf. Did you get what you needed in the issue comments by TAC? Staff is seeking a decision from the TAC on these elements from Dana's doc: 1.) GitHub Enterprise Account Structure - Hybrid or Federated Model? 2.) Fork pull request workflows from outside collaborators - y/n? 3.) require approval for first-time contributors - y/n? 4.) Allow GitHub actions to create and approve pull requests - y/n? @ossf/tac please add a comment that indicates your choices for these 4 items Happy to meet synchronously if needed - i don't think I can provide more context asynch here.

@sevansdell I put a meeting on your calendar to discuss these

Sounds good! I'll do my best to help - I am but one of the TAC members and can only speak for myself. Might be good to also set up time with other individual TAC members too?

sevansdell commented 1 month ago

This has been documented in https://docs.google.com/document/d/1E5RAj0EvOQp-bF8B3gf09Bp0NiZEcqMtZ5Sa__QXbDQ/edit. and is closing out. Thanks for all your comments and input! :)