4 ms·

I fail to so what security you gain by trusting Github's key.

the_mitsuhiko 1y ago
You get an attestation that the person that merged corresponds to a particular github identity. More importantly you know that at the time the person merged the commit they had a 2FA token that was valid. The only way the commit could have been forged is that at the time it took place, the user account itself was compromised.
- captn3m0 1y ago
  GitHub uses fairly long-lived sessions. "sudo mode"[0] on GitHub, where it asks for a verification of the 2FA is only for sensitive actions, which PR merges are not. So a cookie-stealing attack can easily merge PRs for quite a while. And 2FA isn't a requirement for a PR merge afaik, Except via org-wide enforcement? So the guarantee is lower - the commit was merged with a valid session token. [0]: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/sudo-mode https://docs.github.com/en/authentication/keeping-your-accou...
  - the_mitsuhiko 1y ago
    If you enforce an organization to have a 2FA sign-in then yes, it's enforced that the session was created with a second factor. In Sentry's case you also need to go through SSO once every 24 hours. There is no way for you to get a valid session token without going through that which can be used to create a signed merge commit.
- leni536 1y ago
  Don't you get all those guarantees by just pulling from github? I guess it's nice that you get an attestation that you can verify offline, but it feels like a marginal benefit.
  - the_mitsuhiko 1y ago
    > Don't you get all those guarantees by just pulling from github? Without that information I do not know that a particular commit came from a particular person. Anyone can impersonate anyone else.
    - leni536 1y ago
      So Github attests that the merge was done on Github's side, but what does "commit came from a particular person" mean? Who opened the PR? Who is the author of the commits in the PR (can be impersonated)? Who are the "committers" of said commits? Who pushed the merge button? Github doesn't put the info of who pushed the merge button into the merge commit message that it signs. I wonder what it actually attests by putting authors and coauthors into the merge commit. edit: The Co-authored-by fields can be trivially forged, and then Github signs it. The only question is who it acknowledges as the author. It seems to be the PR opener, from what I could gather.
      - the_mitsuhiko 1y ago
        It doesn’t have to because you can discover that information from the UI.
        leni536 1y ago
        Signing with the github key doesn't add anything if you can verify based on UI/API anyway.
        the_mitsuhiko 1y ago
        I'm not really sure I follow. Can you explain the attack vector with squash signed commits today?
        leni536 1y ago
        I'm not trying to find an attack vector, I'm trying to find a threat model where relying on non-signed commits on master is insecure, but relying on commits signed by the github key is secure. If you are looking at and trusting github UI/API anyway as part of your verification, then you might as well just look at the green "verified" badge without actually verifying the signature locally. At which point actually signing by the github key is just useless ceremony.

You get an attestation that the person that merged corresponds to a particular github identity. More importantly you know that at the time the person merged the commit they had a 2FA token that was valid. The only way the commit could have been forged is that at the time it took place, the user account itself was compromised.

captn3m0 1y ago
GitHub uses fairly long-lived sessions. "sudo mode"[0] on GitHub, where it asks for a verification of the 2FA is only for sensitive actions, which PR merges are not. So a cookie-stealing attack can easily merge PRs for quite a while. And 2FA isn't a requirement for a PR merge afaik, Except via org-wide enforcement? So the guarantee is lower - the commit was merged with a valid session token. [0]: https://docs.github.com/en/authentication/keeping-your-account-and-data-secure/sudo-mode https://docs.github.com/en/authentication/keeping-your-accou...
- the_mitsuhiko 1y ago
  If you enforce an organization to have a 2FA sign-in then yes, it's enforced that the session was created with a second factor. In Sentry's case you also need to go through SSO once every 24 hours. There is no way for you to get a valid session token without going through that which can be used to create a signed merge commit.
leni536 1y ago
Don't you get all those guarantees by just pulling from github? I guess it's nice that you get an attestation that you can verify offline, but it feels like a marginal benefit.
- the_mitsuhiko 1y ago
  > Don't you get all those guarantees by just pulling from github? Without that information I do not know that a particular commit came from a particular person. Anyone can impersonate anyone else.
  - leni536 1y ago
    So Github attests that the merge was done on Github's side, but what does "commit came from a particular person" mean? Who opened the PR? Who is the author of the commits in the PR (can be impersonated)? Who are the "committers" of said commits? Who pushed the merge button? Github doesn't put the info of who pushed the merge button into the merge commit message that it signs. I wonder what it actually attests by putting authors and coauthors into the merge commit. edit: The Co-authored-by fields can be trivially forged, and then Github signs it. The only question is who it acknowledges as the author. It seems to be the PR opener, from what I could gather.
    - the_mitsuhiko 1y ago
      It doesn’t have to because you can discover that information from the UI.
      - leni536 1y ago
        Signing with the github key doesn't add anything if you can verify based on UI/API anyway.
        the_mitsuhiko 1y ago
        I'm not really sure I follow. Can you explain the attack vector with squash signed commits today?
        leni536 1y ago
        I'm not trying to find an attack vector, I'm trying to find a threat model where relying on non-signed commits on master is insecure, but relying on commits signed by the github key is secure. If you are looking at and trusting github UI/API anyway as part of your verification, then you might as well just look at the green "verified" badge without actually verifying the signature locally. At which point actually signing by the github key is just useless ceremony.