4 ms·

Recommendations missing from the article: - Enable mandatory 2fa within your Github organization (if you don't use an organization, you probably should) - Dis

by cj 3y ago

Recommendations missing from the article: - Enable mandatory 2fa within your Github organization (if you don't use an organization, you probably should) - Disable the ability to fork repos in your organization - Configure and enable mandatory SAML authentication. In combination with mandatory 2fa, this makes phishing and even key leakage less likely (specific keys need to be double authorized for SAML, so that random private key you have on a random CI/CD platform from a few years ago can't access repos in SAML organizations even if it's leaked) - Disable the ability to make repos public at the organization level These are some quick setting changes that should address at least 3 of the 4 main issues in the article. Additional suggestion: Enable branch protection on master. Require at least 1 peer review to merge anything in it. Enforce branch restrictions to include repo admins so restrictions can't be bypassed. This should stop obvious mistakes like accidentally committing .git or random credentials. Edit: if the author sees this, feel free to add these things to the article!

flerchin 3y ago
What does "Disable the ability to fork repos in your organization" do? Isn't that entirely defeated by users changing the origin?
- lelandfe 3y ago
  Yes, it's just lip service. The moment you've cloned a repo to your computer, you have "forked" it. It's simply the nature of decentralized version control.
- cj 3y ago
  It prevents it from happening in the GitHub UI. Of course it’s still possible to download the code and upload it to a separate repo (but then it’s not a fork).
  - hedora 3y ago
    Of course bypassing the UI means that normal development flows (like force rebasing, or futzing with the CI setup) now are the same set of commands as malicious exfiltration of source code. It also breaks GitHub's normal protection against accidentally creating public forks of private repos (and the feature where it auto-deletes your private forks when you change jobs). There are probably other ways in which disabling private forks undermines your organization's security, but those are pretty obvious.
    - cj 3y ago
      (Genuine question) what is a use case where an employee needs to fork your company’s repo rather than open a PR? The only one I can think of is what you mentioned (testing CI workflows that match a branch name) but at least at our company that can usually be done in a PR. I don’t see any obvious reason why an employee would ever need to fork a company’s private repo to their personal GitHub account.
      - owl57 3y ago
        > I don’t see any obvious reason why an employee would ever need to fork a company’s private repo to their personal GitHub account. I don't see it as well, but Github docs seem to suggest that forking a private repo inside the same org is allowed/forbidden by the same setting?
      - edgyquant 3y ago
        For large projects this can happen. Think the Linux forks that AMD etc maintain and then regularly do a series of PRs to integrate them into some staging branch which then makes its way to the kernel if approved. It’s a typical workflow for inter-org codebase.
        halostatue 3y ago
        Those are public repositories. I do not see the benefit of forking private repositories and have disabled it in the company org that I manage.
      - flerchin 3y ago
        Every single clone is a fork. I feel like you're missing the fundamental concept of git as a DVCS.
        howinteresting 3y ago
        They clearly mean fork in the GitHub UI, not fork as in running git clone locally.
3np 3y ago
Your other points are useful but I find the below questionable and counterproductive: > - Disable the ability to fork repos in your organization If someone can read it, they can trivially fork it (clone locally, then republish as new repo). The only thing you're preventing with this advice is the free discoverability and tracking of forks which you get with forks created with the GitHub "fork" button. The forks are still there but now you have a harder time finding them.
- bluGill 3y ago
  But now it becomes a harder mistake to make. Creating a new repository is easy, but not so easy you would do it by accident. Importantly when this is discovered you can fire the person who did it, and they can't say it's was an accident, while the fork button to the wrong place is potentially an accident.
  - masklinn 3y ago
    I’d say it becomes much easier: if you fork a private repository, you need to go through multiple layers of warning to move it to public. If you want a collaboration repository with forking disabled, public is the default.
  - chrisfosterelli 3y ago
    You can't accidentally share a private fork any easier than you could a new repo. If you try to make a private fork public in GitHub there is just a message that "For security reasons, you cannot change the visibility of a fork." As far as I know the article's example of 'a developer forks a private repo and makes it public' is not possible in github.
- blackoil 3y ago
  The idea is to protect against accidental sharing. A rogue employee can simply take dump and share on 4Chan no matter what tools you use.
  - Y_Y 3y ago
hedora 3y ago
> Enable branch protection on master. Require at least 1 peer review to merge anything in it. Enforce branch restrictions to include repo admins so restrictions can't be bypassed. This should stop obvious mistakes like accidentally committing .git or random credentials. Every team I've seen do this had their productivity drop by over a half when it was implemented. YMMV, but my normal heuristic is to see if I'm making 2x market value (due to stock vesting or whatever). If not, I dust off my LinkedIn profile and start catching up with old colleagues once this gets turned on. The mindset that leads to enabling that policy implies a few bad things: - The team has probably seen growing pains, but did not switch to feature branches for each team, which means it needs to have a release manager, but doesn't know what those people do. - The product has inadequate testing, and there is no QA organization, and the mismanagement is creating revenue headwinds. - Management doesn't trust prior hiring decisions, and has decided to treat the dev team like children (treating them like a cost center comes next). - The company could be chasing revenue via regulatory compliance. There is a backwater of companies that adopt some viral compliance standard that mandates all suppliers also comply with the boneheaded compliance rules. This last reason is the least ominous of the possible reasons, and usually comes with checkbox style implementation of master protection (e.g., admins can override, and over half the team members are admins.)
- cj 3y ago
  Edit: Longer response below, but if requiring peer reviews triggers you to start looking for a new job, what does your dev workflow look like? Do you do peer reviews at all or only sometimes? Admittedly, branch protection requiring peer review into master was something we started for SOC 2 compliance. But, it’s actually great if implemented well. Some suggestions: - Limit the use of “master” branch to code currently deployed to production (don’t use master to stage code that hasn’t been deployed yet). This branch should have the strictest restrictions (no admin override) because merging anything into this branch only happens immediately before a deploy. This also allows your team to assume that any pushes to master should/could trigger an automated deploy workflow - Have a release branch where all PRs are merged/squashed into. Use this branch as the base branch for all PRs. At our company this is simply the “staging” branch. This branch can have looser restrictions (allow overriding restrictions). The release branch is anything that’s going to be deployed in the next release. Merged into the release branch can trigger automatic deploys to staging for QA team to do any final functional testing before code gets to production - Or for a smaller company, don’t have any restrictions on the release branch. You’ll still be in compliance with all frameworks because, at a minimum, no code gets to production without a peer review (because the entire release branch has to be peer approved before it can be merged to master) - Still allow hotfix PRs into master if you need to deploy something urgently without deploying the release branch. But hotfixes ideally should be rare (if they happen all the time it means you’re deploying a lot of buggy code to production and probably need to do better testing/QA) There’s always a balance between security/quality control and productivity. At the absolute minimum, you really should enable branch restrictions even if the only restriction is requiring all merges come from a PR. This will block developers accidentally force pushing their local master and potentially overwriting master branch completely (this has happened at our company prior to enabling restrictions, and it required another developer force pushing their, more up to date, local master to resurrect the correct state) The goal should be to block actions that are obviously bad in all cases (e.g. pushing a commit directly to master without a PR). Whatever your branch restrictions are, they should align with whatever your company’s internal code review processes are. Then, the branch restrictions are simply acting as a fall back in case developers make a mistake (e.g. avoids merging/pushing to master by accident) For a small startup, all this advice is irrelevant because you probably care a lot more about how fast you can pump out changes and care much less about bugs getting into prod. And that’s fine. These suggestions are mostly relevant for mission critical code bases where the restrictions align with your QA process. The branch restrictions should be enabled after documenting a QA process. Branch restrictions should not be arbitrarily enabled if the reasons for the restrictions don’t align with your existing internal processes/workflows.
sebazzz 3y ago
You can also just use Github Enterprise. You're on Github.com but in a completely separate tenant and any collaboration outside of your tenant is impossible: you're recognised but it says you're not allowed.

by cj 3y ago

flerchin 3y ago

What does "Disable the ability to fork repos in your organization" do? Isn't that entirely defeated by users changing the origin?

lelandfe 3y ago
Yes, it's just lip service. The moment you've cloned a repo to your computer, you have "forked" it. It's simply the nature of decentralized version control.
cj 3y ago
It prevents it from happening in the GitHub UI. Of course it’s still possible to download the code and upload it to a separate repo (but then it’s not a fork).
- hedora 3y ago
  Of course bypassing the UI means that normal development flows (like force rebasing, or futzing with the CI setup) now are the same set of commands as malicious exfiltration of source code. It also breaks GitHub's normal protection against accidentally creating public forks of private repos (and the feature where it auto-deletes your private forks when you change jobs). There are probably other ways in which disabling private forks undermines your organization's security, but those are pretty obvious.
  - cj 3y ago
    (Genuine question) what is a use case where an employee needs to fork your company’s repo rather than open a PR? The only one I can think of is what you mentioned (testing CI workflows that match a branch name) but at least at our company that can usually be done in a PR. I don’t see any obvious reason why an employee would ever need to fork a company’s private repo to their personal GitHub account.
    - owl57 3y ago
      > I don’t see any obvious reason why an employee would ever need to fork a company’s private repo to their personal GitHub account. I don't see it as well, but Github docs seem to suggest that forking a private repo inside the same org is allowed/forbidden by the same setting?
    - edgyquant 3y ago
      For large projects this can happen. Think the Linux forks that AMD etc maintain and then regularly do a series of PRs to integrate them into some staging branch which then makes its way to the kernel if approved. It’s a typical workflow for inter-org codebase.
      - halostatue 3y ago
        Those are public repositories. I do not see the benefit of forking private repositories and have disabled it in the company org that I manage.
    - flerchin 3y ago
      Every single clone is a fork. I feel like you're missing the fundamental concept of git as a DVCS.
      - howinteresting 3y ago
        They clearly mean fork in the GitHub UI, not fork as in running git clone locally.

3np 3y ago

Your other points are useful but I find the below questionable and counterproductive: > - Disable the ability to fork repos in your organization If someone can read it, they can trivially fork it (clone locally, then republish as new repo). The only thing you're preventing with this advice is the free discoverability and tracking of forks which you get with forks created with the GitHub "fork" button. The forks are still there but now you have a harder time finding them.

bluGill 3y ago
But now it becomes a harder mistake to make. Creating a new repository is easy, but not so easy you would do it by accident. Importantly when this is discovered you can fire the person who did it, and they can't say it's was an accident, while the fork button to the wrong place is potentially an accident.
- masklinn 3y ago
  I’d say it becomes much easier: if you fork a private repository, you need to go through multiple layers of warning to move it to public. If you want a collaboration repository with forking disabled, public is the default.
- chrisfosterelli 3y ago
  You can't accidentally share a private fork any easier than you could a new repo. If you try to make a private fork public in GitHub there is just a message that "For security reasons, you cannot change the visibility of a fork." As far as I know the article's example of 'a developer forks a private repo and makes it public' is not possible in github.
blackoil 3y ago
The idea is to protect against accidental sharing. A rogue employee can simply take dump and share on 4Chan no matter what tools you use.
- Y_Y 3y ago

hedora 3y ago

> Enable branch protection on master. Require at least 1 peer review to merge anything in it. Enforce branch restrictions to include repo admins so restrictions can't be bypassed. This should stop obvious mistakes like accidentally committing .git or random credentials. Every team I've seen do this had their productivity drop by over a half when it was implemented. YMMV, but my normal heuristic is to see if I'm making 2x market value (due to stock vesting or whatever). If not, I dust off my LinkedIn profile and start catching up with old colleagues once this gets turned on. The mindset that leads to enabling that policy implies a few bad things: - The team has probably seen growing pains, but did not switch to feature branches for each team, which means it needs to have a release manager, but doesn't know what those people do. - The product has inadequate testing, and there is no QA organization, and the mismanagement is creating revenue headwinds. - Management doesn't trust prior hiring decisions, and has decided to treat the dev team like children (treating them like a cost center comes next). - The company could be chasing revenue via regulatory compliance. There is a backwater of companies that adopt some viral compliance standard that mandates all suppliers also comply with the boneheaded compliance rules. This last reason is the least ominous of the possible reasons, and usually comes with checkbox style implementation of master protection (e.g., admins can override, and over half the team members are admins.)

cj 3y ago
Edit: Longer response below, but if requiring peer reviews triggers you to start looking for a new job, what does your dev workflow look like? Do you do peer reviews at all or only sometimes? Admittedly, branch protection requiring peer review into master was something we started for SOC 2 compliance. But, it’s actually great if implemented well. Some suggestions: - Limit the use of “master” branch to code currently deployed to production (don’t use master to stage code that hasn’t been deployed yet). This branch should have the strictest restrictions (no admin override) because merging anything into this branch only happens immediately before a deploy. This also allows your team to assume that any pushes to master should/could trigger an automated deploy workflow - Have a release branch where all PRs are merged/squashed into. Use this branch as the base branch for all PRs. At our company this is simply the “staging” branch. This branch can have looser restrictions (allow overriding restrictions). The release branch is anything that’s going to be deployed in the next release. Merged into the release branch can trigger automatic deploys to staging for QA team to do any final functional testing before code gets to production - Or for a smaller company, don’t have any restrictions on the release branch. You’ll still be in compliance with all frameworks because, at a minimum, no code gets to production without a peer review (because the entire release branch has to be peer approved before it can be merged to master) - Still allow hotfix PRs into master if you need to deploy something urgently without deploying the release branch. But hotfixes ideally should be rare (if they happen all the time it means you’re deploying a lot of buggy code to production and probably need to do better testing/QA) There’s always a balance between security/quality control and productivity. At the absolute minimum, you really should enable branch restrictions even if the only restriction is requiring all merges come from a PR. This will block developers accidentally force pushing their local master and potentially overwriting master branch completely (this has happened at our company prior to enabling restrictions, and it required another developer force pushing their, more up to date, local master to resurrect the correct state) The goal should be to block actions that are obviously bad in all cases (e.g. pushing a commit directly to master without a PR). Whatever your branch restrictions are, they should align with whatever your company’s internal code review processes are. Then, the branch restrictions are simply acting as a fall back in case developers make a mistake (e.g. avoids merging/pushing to master by accident) For a small startup, all this advice is irrelevant because you probably care a lot more about how fast you can pump out changes and care much less about bugs getting into prod. And that’s fine. These suggestions are mostly relevant for mission critical code bases where the restrictions align with your QA process. The branch restrictions should be enabled after documenting a QA process. Branch restrictions should not be arbitrarily enabled if the reasons for the restrictions don’t align with your existing internal processes/workflows.

sebazzz 3y ago

You can also just use Github Enterprise. You're on Github.com but in a completely separate tenant and any collaboration outside of your tenant is impossible: you're recognised but it says you're not allowed.