search

lightningnetwork / lnd

Lightning Network Daemon ⚡️
MIT License
7.71k stars 2.09k forks source link

Policy about merging to master #5884

Open naveensrinivasan opened 3 years ago

naveensrinivasan commented 3 years ago

The lnd repository doesn't have a policy that enforces reviews are required for merging to master for an administrator 'administrator PRs are exempt from reviews on branch 'master' example https://github.com/lightningnetwork/lnd/pull/5709

The Branch Protection status can be checked via https://github.com/ossf/scorecard scorecard --repo=github.com/lightningnetwork/lnd --checks=Branch-Protection --show-details --format json | jq

This is an issue in OSS, especially with things that can be compromised like stolen keys, malware, different other attack vectors.

Some recent supply chain attacks where the owner was compromised https://github.com/faisalman/ua-parser-js/issues/536

Can this be fixed to avoid such a situation?

Roasbeef commented 3 years ago

Most of the time we use the admin merge simply to address some issues were still working out re flakes in our integration tests. In the example of that PR, it had already received extensive review online+offline, but the original proposer wasn't able to continue with it, so my job there was just rebasing the PR to fix merge conflicts, etc.

I think the proper way to address this is by removing my signature of the main manifest and relying mainly on the set of threshold signers. The only issues here is that we're very distributed as a team, so it can slow down some release cycles as we need to wait for the relevant window for people to be able to sign.

I think also maybe this would be better as a discussion since it's more of an opened ended question/plan?

Roasbeef commented 3 years ago

Also in the end, admins can just turn off w/e policy merge, then turn things back on, so it isn't a foolproof solution.

naveensrinivasan commented 3 years ago

Also in the end, admins can just turn off w/e policy merge, then turn things back on, so it isn't a foolproof solution.

Yes, it is possible. The admins hold keys to the kingdom and they can do anything they want. It is a matter of policy that they don't do it.

Also, tools like scorecard flag repositories that have PR merge without any reviewers and direct push to master without PR's.

To turn off/on the policy, it is required to use the GitHub password additional to be logged in. If the setting isn't turned on, the SSH key could be compromised (by malware/loss) can be used to push to master. The goal is to avoid such things.

Thoughts?

Roasbeef commented 3 years ago

To turn off/on the policy, it is required to use the GitHub password additional to be logged in. If the setting isn't turned on, the SSH key could be compromised (by malware/loss) can be used to push to master. The goal is to avoid such things.

Makes sense, but as I said earlier, turning it off and having the repo continue its forward moentum isn't possible until we either beef up our CI infrastructure or conclude flake hunting szn.

naveensrinivasan commented 3 years ago

To turn off/on the policy, it is required to use the GitHub password additional to be logged in. If the setting isn't turned on, the SSH key could be compromised (by malware/loss) can be used to push to master. The goal is to avoid such things.

Makes sense, but as I said earlier, turning it off and having the repo continue its forward moentum isn't possible until we either beef up our CI infrastructure or conclude flake hunting szn.

OK, So we could keep this issue open and revisit it later.