GitHub Community
Githubs web-flow OpenPGP signatures and key are in incorrect format #27607
-
|
I noticed that the OpenPGP/GPG/GnuPG signatures that Github makes on commits and tags and the associated key are in an incorrect format. This is currently being discussed in the IETF OpenPGP WG: https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/209 Can anyone tell us if the OpenPGP implementation that Github is using is used anywhere else? Is the source available anywhere? Some information copied here for context:
Below is the dump of the signature. Also see the analysis of the key in https://gitlab.com/openpgp-wg/rfc4880bis/-/merge_requests/209#note_1044594616 . |
Beta Was this translation helpful? Give feedback.
Replies: 5 comments 12 replies
-
|
This issue affects not only signatures made over commits, but the self-signatures on the key. It is also not the only issue - importing this key into go-crypto fails because the digest prefix is set incorrectly on the self-sigs (sequoia and gnupg do not check this but other implementations do). It is reasonable to assume that digest prefixes are set incorrectly on all other sigs. |
Beta Was this translation helpful? Give feedback.
-
|
In the OpenPGP Working Group meeting at IETF 114 today, this consumed a bit of time and discussion. This divergence from standards is likely to cause problems for the OpenPGP community (i'm a co-chair of the OpenPGP WG). It would be great if Github could:
--- web-flow.gpg 2022-07-29 13:43:15.566983574 -0400
+++ github-webflow-fixed-certificate.pgp.txt 2022-07-29 13:19:37.711800616 -0400
@@ -7,11 +7,11 @@
buXCDscJcLJRKZVc62gOZXXtPfoHqvUPp3nuLA4YjH9bphbrMWMf810Wxz9JTd3v
yWgGqNY0zbBqeZoGv+TuExlRHT8ASGFS9SVDABEBAAHNNUdpdEh1YiAod2ViLWZs
b3cgY29tbWl0IHNpZ25pbmcpIDxub3JlcGx5QGdpdGh1Yi5jb20+wsBiBBMBCAAW
-BQJZlGhBCRBK7hj4Ov3rIwIbAwIZAQAAmQEIACATWFmi2oxlBh3wAsySNCNV4IPf
+BQJZlGhBCRBK7hj4Ov3rIwIbAwIZAQAAmQEH/iATWFmi2oxlBh3wAsySNCNV4IPf
DDMeh6j80WT7cgoX7V7xqJOxrfrqPEthQ3hgHIm7b5MPQlUr2q+UPL22t/I+ESF6
9b0QWLFSMJbMSk+BXkvSjH9q8jAO0986/pShPV5DU2sMxnx4LfLfHNhTzjXKokws
+8ptJ8uhMNIDXfXuzkZHIxoXk3rNcjDN5c5X+sK8UBRH092BIJWCOfaQt7v7wig5
4Ra28pM9GbHKXVNxmdLpCFyzvyMuCmINYYADsC848QQFFwnd4EQnupo6QvhEVx1O
j7wDwvuH5dCrLuLwtwXaQh0onG4583p0LGms2Mf5F+Ick6o/4peOlBoZz48=
-=HXDP
+=HsBQ
-----END PGP PUBLIC KEY BLOCK-----
If those two steps can be done, that would solve everything going forward! Subsequently, you might want to retrospectively fix signatures that have already been issued. I think that can be done without breaking anything, but i'm not entirely sure -- I'd be happy to talk with anyone on your team who is looking into that to try to help figure out the details. |
Beta Was this translation helpful? Give feedback.
-
|
if the signature is on a terminal element, like a tag or the tip commit of a branch, it can be fixed. if the commit is part of a history, then i agree it can't be safely fixed. there's nothing wrong replacing the OpenPGP certificate. implementations that don't like the malformed self-signature will ignore it, while accepting the corrected one. implementations that are fine with both will just see a duplicate self-signature, which isn't a problem. |
Beta Was this translation helpful? Give feedback.
-
While history could be technically changed, it should not be changed even for the tip or tags, as people could have already fetched these, so it would lead to a lot of work. A history rewrite looks the same to git clients independent of depth, even if it is only the tip or tags.
With sq patched with https://gitlab.com/sequoia-pgp/sequoia/-/merge_requests/1341 it doesn't update the key, which is similar enough to how the discussion went on what the spec should require: |
Beta Was this translation helpful? Give feedback.
-
|
i should be clear that the fix above for the github certificate is just for the MPI framing issue -- @andrewgdotcom suggests that there is also a problem with the leading two octets of digest in the signatures, but i haven't investigated or tried to fix that . |
Beta Was this translation helpful? Give feedback.
-
|
@dkg would you have any resources on how to fix the two octets of the digest in the signature? We were wondering if there was an easy way to just change the the |
Beta Was this translation helpful? Give feedback.
-
|
@harrycguo, i'm not sure about the right tooling to make it easy to update the digest prefix octets, sorry. if |
Beta Was this translation helpful? Give feedback.
-
|
It would be great to get a response from someone at github about this! |
Beta Was this translation helpful? Give feedback.
-
|
I filed a ticket linking to this discussion from a paid org for this and on Aug 2, 2022, 8:15 AM UTC i received the following response: "Thanks for the report. I've asked the team for their input on this one. I can't promise an ETA for when we'll get an update, but we'll write back to you if there's any news to share." |
Beta Was this translation helpful? Give feedback.
-
|
Received an update in the ticket:
I answered: Thank you! x/crypto/openpgp is marked deprecated ( golang/go#44226 ) but you can use https://github.com/ProtonMail/go-crypto as a replacement, which AFAIK does not have this issue. Reply:
Notes when I quickly looked at this, I found this commit: sylabs/golang-x-crypto@374053e#diff-47e53358306da9dcb5ca7dd110d31067d11f231fc3baed4f51e4026e26b521bfL506 Context for author: https://latacora.micro.blog/2019/07/16/the-pgp-problem.html golang/go#44226 https://nitter.net/FiloSottile/status/1555677349621932032#m |
Beta Was this translation helpful? Give feedback.
-
|
that commit is distressing -- it's explicitly generating non-standard MPIs on the wire format in knowing contravention of the cited RFC, while acknowledging that these variations are likely to cause problems with reserialization ☹ Either Filippo and his reviewers weren't thinking about the impact on other consumers of these objects or they were aware and thought that those impacts were OK. @JanZerebecki do you have any word about whether the github team has switched to a supported openpgp module? |
Beta Was this translation helpful? Give feedback.
-
|
FWIW (and I'm reading that commit with no particular context as it was four years ago), I think that commit didn't introduce the spec divergence, just documented it, and it's saying that removing the variation would (also) cause problems since it would introduce a situation where a value gets parsed and reserialized into a different encoding (unless we start rejecting our own signatures produced by previous versions, which has its own issues). This kind of lose-lose-lose problems are part of why we deprecated the package and encouraged moving to a different one. |
Beta Was this translation helpful? Give feedback.
-
|
What prevents this from both retaining the same serialization for existing MPIs and creating new MPIs in the spec compliant way? Yes there is a downside of carrying that state and more complexity, but I currently can't see why that would be an actual problem. That seems to me as not loosing from the point of view of OpenPGP users. I have not yet fully written it down nor checked it for correctness, but I think safely dealing with the existence of keys and fingerprints with that differing representation is also possible without outright rejecting them. Though from my reading that was not a reason for why you talked about losing. One could always compute a mapping to both versions, the rounded up MPI representation and the spec compliant one, so revocations can be looked up in either representation. (Maybe there are more variants in addition to exactly rounded up to a byte in the wild, but few enough of them for this to work.) |
Beta Was this translation helpful? Give feedback.
-
|
Hi there! I'm looking into this issue at the moment, though depending on our discussion I can't concretely promise that I'll be able to resolve the issue. I'll also add the caveat that I'm not an OpenPGP expert, I'm learning as I go. I grabbed the signature from containers/podman@b0d7a3a, a commit from earlier today. Our commit signing key was rotated in January, and there have been several other security updates to our internal services in the past few months that may have fixed this issue. Is this still a problem? If it is, how would you recommend I test it (since |
Beta Was this translation helpful? Give feedback.
-
|
Hi :) I just verified that the issue is still there. There is a chance that the signature is correctly encoded, so you need to look at a few of them. I have a test repository here, and created four commits today (simply by editing the file using the web gui). By chance, the first commit was good, the next three were bad: $ git clone https://github.com/teythoon/test-signed-commit
$ cd test-signed-commit
$ git log --no-show-signature --pretty=oneline | while read commit message ; do echo $commit $message ; git cat-file -p $commit | sed s/gpgsig// | sq dearmor | sq packet dump ; done
aa56919b50398a652b4b6e6b9b3a6cbcf90703dc Update README.md
Unknown or Unsupported Packet, new CTB, 540 bytes
Tag: Signature Packet
Error: Malformed MPI: leading bit is not set: expected bit 8 to be set in 11011 (1b)
3ab981dcf42579a4a584d78761e4b7085ec01e71 Update README.md
Unknown or Unsupported Packet, new CTB, 540 bytes
Tag: Signature Packet
Error: Malformed MPI: leading bit is not set: expected bit 8 to be set in 1110100 (74)
7a090c071612fba30cc09a5ba6f6e458a66d9074 Update README.md
Unknown or Unsupported Packet, new CTB, 540 bytes
Tag: Signature Packet
Error: Malformed MPI: leading bit is not set: expected bit 8 to be set in 10010 (12)
45157dbf8ebd8bd8e88d759d80871d903b21e441 Update README.md
Signature Packet, new CTB, 540 bytes
Version: 4
Type: Binary
Pk algo: RSA
Hash algo: SHA256
Hashed area:
Signature creation time: 2024-02-14 16:23:31 UTC
Issuer: B5690EEEBB952194
Digest prefix: AC72
Level: 0 (signature over data)
6ca1db88ee8fff0af121441d7433b9eff4fbf515 Initial commit
Unknown or Unsupported Packet, new CTB, 284 bytes
Tag: Signature Packet
Error: Malformed MPI: leading bit is not set: expected bit 8 to be set in 11010 (1a)
|
Beta Was this translation helpful? Give feedback.
Received an update in the ticket: