| Message ID | 20211009163338.2175170-1-rlb@defaultvalue.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] signature-format.txt: add space to fix gpgsig continuation line | expand |
On Sat, Oct 09, 2021 at 11:33:38AM -0500, Rob Browning wrote: > Add a space to the blank line after the version header in the example > gpgsig commit header. Thanks, this is a good catch. The space is important for the header continuation. > diff --git a/Documentation/technical/signature-format.txt b/Documentation/technical/signature-format.txt > index 2c9406a56a..6acc0b1247 100644 > --- a/Documentation/technical/signature-format.txt > +++ b/Documentation/technical/signature-format.txt > @@ -78,7 +78,7 @@ author A U Thor <author@example.com> 1465981137 +0000 > committer C O Mitter <committer@example.com> 1465981137 +0000 > gpgsig -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1 > - > + > iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ > HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 > DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA The patch is quite subtle to read, of course. :) But more importantly, it is subtle for somebody reading the documentation to notice. Perhaps it's worth calling it out explicitly? E.g., squashing in something like: diff --git a/Documentation/technical/signature-format.txt b/Documentation/technical/signature-format.txt index 6acc0b1247..f418431050 100644 --- a/Documentation/technical/signature-format.txt +++ b/Documentation/technical/signature-format.txt @@ -68,7 +68,9 @@ signed tag message body - created by: `git commit -S` - payload: commit object - embedding: header entry `gpgsig` - (content is preceded by a space) + (content is preceded by a space; note that this includes the + "empty" line between the GnuPG header and signature, which + consists of a single space). - example: commit with subject `signed commit` ---- > While investigating this (while parsing commit objects) I also > noticed a commit in a repo that had a blank continuation line (" \n") > after the "END PGP SIGNATURE" line. > > Presuming that's valid, I suppose we could consider detailing any > additional specifications, i.e. what kind of trailing content a > parser should expect/ignore, etc. It is valid. It looks like GitHub's signed-merges may do this. From our perspective, we are not really defining what is valid content or not. We take everything in the gpgsig header (including multiple lines connected by space-continuation), and feed it to gpg. So if gpg is happy with it, we are happy with it. Thinking with a security hat on, it's possible this could lead to confusion (say, if I include multiple signatures but some tools check one and some the other). But I'd be hesitant to start adding restrictions without even a plausible attack scenario. -Peff
Jeff King <peff@peff.net> writes: > On Sat, Oct 09, 2021 at 11:33:38AM -0500, Rob Browning wrote: > >> Add a space to the blank line after the version header in the example >> gpgsig commit header. > > Thanks, this is a good catch. The space is important for the header > continuation. > >> diff --git a/Documentation/technical/signature-format.txt b/Documentation/technical/signature-format.txt >> index 2c9406a56a..6acc0b1247 100644 >> --- a/Documentation/technical/signature-format.txt >> +++ b/Documentation/technical/signature-format.txt >> @@ -78,7 +78,7 @@ author A U Thor <author@example.com> 1465981137 +0000 >> committer C O Mitter <committer@example.com> 1465981137 +0000 >> gpgsig -----BEGIN PGP SIGNATURE----- >> Version: GnuPG v1 >> - >> + >> iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ >> HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 >> DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA > > The patch is quite subtle to read, of course. :) But more importantly, > it is subtle for somebody reading the documentation to notice. Perhaps > it's worth calling it out explicitly? E.g., squashing in something like: > ... > - (content is preceded by a space) > + (content is preceded by a space; note that this includes the > + "empty" line between the GnuPG header and signature, which > + consists of a single space). > - example: commit with subject `signed commit` To those who are reading on paper or on terminal, the difference will not be even seen. It only can be _found_ if you are in an editor or a pager and explicitly look for a trailing whitespace (or told your tool to highlight such for you). I wonder if we can have some typesetting convention for this part of the documentation. Perhaps something like In the following example, the end of line that ends with a whitespace letter is highlighted with a "$" sign; if you are trying to recreate these example by hand, do not cut and paste them---they are there primarily to highlight extra whitespace at the end of some lines. before a displayed material like this: committer C O Mitter <committer@example.com> 1465981137 +0000 gpgsig -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 $ iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA perhaps? I am reasonably sure that running "git am" on Rob's patch will by default end up as a no-op patch, because auto-correcting whitespace breakage is the default setting I use, and a change like the one suggested above would help avoid such a problem, too. Thanks.
On Mon, Oct 11, 2021 at 01:04:48PM -0700, Junio C Hamano wrote: > > The patch is quite subtle to read, of course. :) But more importantly, > > it is subtle for somebody reading the documentation to notice. Perhaps > > it's worth calling it out explicitly? E.g., squashing in something like: > > ... > > - (content is preceded by a space) > > + (content is preceded by a space; note that this includes the > > + "empty" line between the GnuPG header and signature, which > > + consists of a single space). > > - example: commit with subject `signed commit` > > To those who are reading on paper or on terminal, the difference > will not be even seen. It only can be _found_ if you are in an > editor or a pager and explicitly look for a trailing whitespace (or > told your tool to highlight such for you). Yeah, I was hoping that calling it out explicitly would help to serve that purpose, even if they can't see it. But it's still pretty subtle. I did consider annotating it more directly in the example, but I was worried it would end up being syntactically confusing, because we don't have a well-known convention for marking "end of line". But the example you showed: > I wonder if we can have some typesetting convention for this part of > the documentation. Perhaps something like > > In the following example, the end of line that ends with a > whitespace letter is highlighted with a "$" sign; if you are > trying to recreate these example by hand, do not cut and paste > them---they are there primarily to highlight extra whitespace at > the end of some lines. > > before a displayed material like this: > > committer C O Mitter <committer@example.com> 1465981137 +0000 > gpgsig -----BEGIN PGP SIGNATURE----- > Version: GnuPG v1 > $ > iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ > HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 > DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA > > perhaps? ...coupled with the explanation you gave is not too bad. I had thought to maybe do something like: gpgsig -----BEGIN PGP SIGNATURE----- _Version: GnuPG v1 _ _iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ _HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 _DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA but that is pretty ugly. I like yours much better. -Peff
Jeff King <peff@peff.net> writes: >> In the following example, the end of line that ends with a >> whitespace letter is highlighted with a "$" sign; if you are >> trying to recreate these example by hand, do not cut and paste >> them---they are there primarily to highlight extra whitespace at >> the end of some lines. >> >> before a displayed material like this: >> >> committer C O Mitter <committer@example.com> 1465981137 +0000 >> gpgsig -----BEGIN PGP SIGNATURE----- >> Version: GnuPG v1 >> $ >> iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ >> HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 >> DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA >> >> perhaps? > > ...coupled with the explanation you gave is not too bad. I had thought > to maybe do something like: > > gpgsig -----BEGIN PGP SIGNATURE----- > _Version: GnuPG v1 > _ > _iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ > _HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 > _DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA > > but that is pretty ugly. I like yours much better. Yes, I did consider a replacement for SP and rejected for the same reason as you did. We could use the same "we only show the 'here is a SP' sign when the presence of the SP cannot be seen" convention to make it less distracting, but I think '$' taken from "cat -e" probably is easier to see and understand for those who are the target audience of anything in Documentation/technical/ hierarchy. Thanks.
diff --git a/Documentation/technical/signature-format.txt b/Documentation/technical/signature-format.txt index 2c9406a56a..6acc0b1247 100644 --- a/Documentation/technical/signature-format.txt +++ b/Documentation/technical/signature-format.txt @@ -78,7 +78,7 @@ author A U Thor <author@example.com> 1465981137 +0000 committer C O Mitter <committer@example.com> 1465981137 +0000 gpgsig -----BEGIN PGP SIGNATURE----- Version: GnuPG v1 - + iQEcBAABAgAGBQJXYRjRAAoJEGEJLoW3InGJ3IwIAIY4SA6GxY3BjL60YyvsJPh/ HRCJwH+w7wt3Yc/9/bW2F+gF72kdHOOs2jfv+OZhq0q4OAN6fvVSczISY/82LpS7 DVdMQj2/YcHDT4xrDNBnXnviDO9G7am/9OE77kEbXrp7QPxvhjkicHNwy2rEflAA
Add a space to the blank line after the version header in the example gpgsig commit header. Signed-off-by: Rob Browning <rlb@defaultvalue.org> --- While investigating this (while parsing commit objects) I also noticed a commit in a repo that had a blank continuation line (" \n") after the "END PGP SIGNATURE" line. Presuming that's valid, I suppose we could consider detailing any additional specifications, i.e. what kind of trailing content a parser should expect/ignore, etc. Documentation/technical/signature-format.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)