diff mbox series

[3/3] fast-export, fast-import: implement signed-commits

Message ID 20210419225441.3139048-4-lukeshu@lukeshu.com (mailing list archive)
State Superseded
Headers show
Series fast-export, fast-import: implement signed-commits | expand

Commit Message

Luke Shumaker April 19, 2021, 10:54 p.m. UTC
From: Luke Shumaker <lukeshu@datawire.io>

fast-export has an existing --signed-tags= flag that controls how to
handle tag signatures.  However, there is no equivalent for commit
signatures; it just silently strips the signature out of the commit
(analogously to --signed-tags=strip).

While signatures are generally problematic for fast-export/fast-import
(because hashes are likely to change), if they're going to support tag
signatures, there's no reason to not also support commit signatures.

So, implement signed-commits.

On the fast-export side, try to be as much like signed-tags as possible,
in both implementation and in user-interface; with the exception that
the default should be `--signed-commits=strip` (compared to the default
`--signed-tags=abort`), in order to continue defaulting to the
historical behavior.  Only bother implementing "gpgsig", not
"gpgsig-sha256"; the existing signed-tag support doesn't implement
"gpgsig-sha256" either.

On the fast-import side, I'm not entirely sure that I got the ordering
correct between "gpgsig" and "encoding" when generating the commit
object.

Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
---
 Documentation/git-fast-export.txt | 12 +++++
 Documentation/git-fast-import.txt |  7 +++
 builtin/fast-export.c             | 86 +++++++++++++++++++++++++------
 builtin/fast-import.c             | 15 ++++++
 t/t9350-fast-export.sh            | 70 +++++++++++++++++++++++++
 5 files changed, 174 insertions(+), 16 deletions(-)

Comments

brian m. carlson April 20, 2021, 1:41 a.m. UTC | #1
On 2021-04-19 at 22:54:41, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@datawire.io>
> 
> fast-export has an existing --signed-tags= flag that controls how to
> handle tag signatures.  However, there is no equivalent for commit
> signatures; it just silently strips the signature out of the commit
> (analogously to --signed-tags=strip).
> 
> While signatures are generally problematic for fast-export/fast-import
> (because hashes are likely to change), if they're going to support tag
> signatures, there's no reason to not also support commit signatures.
> 
> So, implement signed-commits.
> 
> On the fast-export side, try to be as much like signed-tags as possible,
> in both implementation and in user-interface; with the exception that
> the default should be `--signed-commits=strip` (compared to the default
> `--signed-tags=abort`), in order to continue defaulting to the
> historical behavior.  Only bother implementing "gpgsig", not
> "gpgsig-sha256"; the existing signed-tag support doesn't implement
> "gpgsig-sha256" either.

I would appreciate it if we did in fact implement it.  I would like to
use this functionality to round-trip objects between SHA-1 and SHA-256,
and it would be nice if both worked.

The situation with tags is different: the signature using the current
algorithm is always trailing, and the signature for the other algorithm
is in the header.  That wasn't how we intended it to be, but that's how
it ended up being.

As a result, tag output can support SHA-256 data, but with your
proposal, SHA-256 commits wouldn't work at all.  Considering SHA-1 is
wildly insecure and therefore signing SHA-1 commits adds very little
security, whereas SHA-256 is presently considered strong, I'd argue that
only supporting SHA-1 isn't the right move here.

Provided we do that and the test suite passes under both algorithms, I'm
strongly in favor of this feature.  In fact, I had been thinking about
implementing this feature myself just the other day, so I'm delighted
you decided to do it.

> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 458af0a2d6..3d0c5dbf7d 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -437,6 +437,7 @@ change to the project.
>  	original-oid?
>  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
>  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> +	('gpgsig' LF data)?

Could we emit this as "gpgsig sha1 data" and "gpgsig sha256 data"?  That
would allow us to consider the future case where the hash algorithm
changes again without requiring a change of format.
Taylor Blau April 20, 2021, 1:45 a.m. UTC | #2
On Mon, Apr 19, 2021 at 04:54:41PM -0600, Luke Shumaker wrote:
> From: Luke Shumaker <lukeshu@datawire.io>
>
> fast-export has an existing --signed-tags= flag that controls how to
> handle tag signatures.  However, there is no equivalent for commit
> signatures; it just silently strips the signature out of the commit
> (analogously to --signed-tags=strip).
>
> While signatures are generally problematic for fast-export/fast-import
> (because hashes are likely to change), if they're going to support tag
> signatures, there's no reason to not also support commit signatures.
>
> So, implement signed-commits.
>
> On the fast-export side, try to be as much like signed-tags as possible,
> in both implementation and in user-interface; with the exception that
> the default should be `--signed-commits=strip` (compared to the default
> `--signed-tags=abort`), in order to continue defaulting to the
> historical behavior.  Only bother implementing "gpgsig", not
> "gpgsig-sha256"; the existing signed-tag support doesn't implement
> "gpgsig-sha256" either.
>
> On the fast-import side, I'm not entirely sure that I got the ordering
> correct between "gpgsig" and "encoding" when generating the commit
> object.
>
> Signed-off-by: Luke Shumaker <lukeshu@datawire.io>
> ---
>  Documentation/git-fast-export.txt | 12 +++++
>  Documentation/git-fast-import.txt |  7 +++
>  builtin/fast-export.c             | 86 +++++++++++++++++++++++++------
>  builtin/fast-import.c             | 15 ++++++
>  t/t9350-fast-export.sh            | 70 +++++++++++++++++++++++++
>  5 files changed, 174 insertions(+), 16 deletions(-)
>
> diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> index d4a2bfe037..6fdb678b54 100644
> --- a/Documentation/git-fast-export.txt
> +++ b/Documentation/git-fast-export.txt
> @@ -39,6 +39,18 @@ warning will be displayed, with 'verbatim', they will be silently
>  exported and with 'warn-verbatim', they will be exported, but you will
>  see a warning.
>
> +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> +	Specify how to handle signed commits.  Since any transformation
> +	after the export can change the commit (which can also happen
> +	when excluding revisions) the signatures will not match.
> ++
> +When asking to 'abort', this program will die when encountering a
> +signed commit.  With 'strip' (which is the default), the commits will
> +silently be made unsigned, with 'warn-strip' they will be made
> +unsigned but a warning will be displayed, with 'verbatim', they will
> +be silently exported and with 'warn-verbatim', they will be exported,
> +but you will see a warning.
> +

OK, this all seems normal to me. But it may be worth shortening it to
say "behaves exactly as --signed-tags, but for commits", or something.

>  --tag-of-filtered-object=(abort|drop|rewrite)::
>  	Specify how to handle tags whose tagged object is filtered out.
>  	Since revisions and files to export can be limited by path,
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index 458af0a2d6..3d0c5dbf7d 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -437,6 +437,7 @@ change to the project.
>  	original-oid?
>  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
>  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> +	('gpgsig' LF data)?

Is this missing a LF after data?

> +static const char *find_signature(const char *begin, const char *end)
> +{
> +	const char *needle = "\ngpgsig ";
> +	char *bod, *eod, *eol;
> +
> +	bod = memmem(begin, end ? end - begin : strlen(begin),
> +		     needle, strlen(needle));
> +	if (!bod)
> +		return NULL;
> +	bod += strlen(needle);
> +	eod = strchrnul(bod, '\n');
> +	while (eod[0] == '\n' && eod[1] == ' ') {
> +		eod = strchrnul(eod+1, '\n');
> +	}
> +	*eod = '\0';
> +
> +	while ((eol = strstr(bod, "\n ")))
> +		memmove(eol+1, eol+2, strlen(eol+1));

Hmm. I'm not quite sure I follow these last two lines. Perhaps a comment
would help? The rest of this patch looks reasonable to me.

Thanks,
Taylor
Luke Shumaker April 20, 2021, 3:51 p.m. UTC | #3
On Mon, 19 Apr 2021 16:54:41 -0600,
Luke Shumaker wrote:
> On the fast-import side, I'm not entirely sure that I got the ordering
> correct between "gpgsig" and "encoding" when generating the commit
> object.

Whoops, I forgot to take that out of the commit message; I did check
the ordering before submitting the patch.
Luke Shumaker April 20, 2021, 4:23 p.m. UTC | #4
On Mon, 19 Apr 2021 19:45:46 -0600,
Taylor Blau wrote:
> > diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
> > index d4a2bfe037..6fdb678b54 100644
> > --- a/Documentation/git-fast-export.txt
> > +++ b/Documentation/git-fast-export.txt
> > @@ -39,6 +39,18 @@ warning will be displayed, with 'verbatim', they will be silently
> >  exported and with 'warn-verbatim', they will be exported, but you will
> >  see a warning.
> >
> > +--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
> > +	Specify how to handle signed commits.  Since any transformation
> > +	after the export can change the commit (which can also happen
> > +	when excluding revisions) the signatures will not match.
> > ++
> > +When asking to 'abort', this program will die when encountering a
> > +signed commit.  With 'strip' (which is the default), the commits will
> > +silently be made unsigned, with 'warn-strip' they will be made
> > +unsigned but a warning will be displayed, with 'verbatim', they will
> > +be silently exported and with 'warn-verbatim', they will be exported,
> > +but you will see a warning.
> > +
> 
> OK, this all seems normal to me. But it may be worth shortening it to
> say "behaves exactly as --signed-tags, but for commits", or something.

Good suggestion, it would also make it more obvious that the default
is different, since I'd have to call it out explictly.

> >  --tag-of-filtered-object=(abort|drop|rewrite)::
> >  	Specify how to handle tags whose tagged object is filtered out.
> >  	Since revisions and files to export can be limited by path,
> > diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> > index 458af0a2d6..3d0c5dbf7d 100644
> > --- a/Documentation/git-fast-import.txt
> > +++ b/Documentation/git-fast-import.txt
> > @@ -437,6 +437,7 @@ change to the project.
> >  	original-oid?
> >  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
> >  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> > +	('gpgsig' LF data)?
> 
> Is this missing a LF after data?

No, the definition of `data` has a byte-count prefix, so it doesn't
need an `LF` to act as a terminator (and it also already includes an
optional trailing `LF?` just in case you want to include one).

In fact, my implementation in fast-export does not include the LF,
which is why the test greps for "encoding ISO-8859-1" instead of
"^encoding ISO-8859-1".

I'll add a comment saying that it's intentional.

> > +static const char *find_signature(const char *begin, const char *end)
> > +{
> > +	const char *needle = "\ngpgsig ";
> > +	char *bod, *eod, *eol;
> > +
> > +	bod = memmem(begin, end ? end - begin : strlen(begin),
> > +		     needle, strlen(needle));
> > +	if (!bod)
> > +		return NULL;
> > +	bod += strlen(needle);
> > +	eod = strchrnul(bod, '\n');
> > +	while (eod[0] == '\n' && eod[1] == ' ') {
> > +		eod = strchrnul(eod+1, '\n');
> > +	}
> > +	*eod = '\0';
> > +
> > +	while ((eol = strstr(bod, "\n ")))
> > +		memmove(eol+1, eol+2, strlen(eol+1));
> 
> Hmm. I'm not quite sure I follow these last two lines. Perhaps a comment
> would help? The rest of this patch looks reasonable to me.

In the commit object, multi-line header values are stored by prefixing
continuation lines begin with a space.  So within the commit object,
it looks like

    "gpgsig -----BEGIN PGP SIGNATURE-----\n"
    " Version: GnuPG v1.4.5 (GNU/Linux)\n"
    " \n"
    " base64_pem_here\n"
    " -----END PGP SIGNATURE-----\n"

However, we want the raw value; we want to return

    "-----BEGIN PGP SIGNATURE-----\n"
    "Version: GnuPG v1.4.5 (GNU/Linux)\n"
    "\n"
    "base64_pem_here\n"
    "-----END PGP SIGNATURE-----\n"

without all the extra spaces.  That's what those two lines are doing,
stripping out the extra spaces.

I'll add some comments.
Luke Shumaker April 20, 2021, 5:15 p.m. UTC | #5
On Mon, 19 Apr 2021 19:41:55 -0600,
brian m. carlson wrote:
> 
> [1  <text/plain; utf-8 (quoted-printable)>]
> On 2021-04-19 at 22:54:41, Luke Shumaker wrote:
> > From: Luke Shumaker <lukeshu@datawire.io>
> > 
> > fast-export has an existing --signed-tags= flag that controls how to
> > handle tag signatures.  However, there is no equivalent for commit
> > signatures; it just silently strips the signature out of the commit
> > (analogously to --signed-tags=strip).
> > 
> > While signatures are generally problematic for fast-export/fast-import
> > (because hashes are likely to change), if they're going to support tag
> > signatures, there's no reason to not also support commit signatures.
> > 
> > So, implement signed-commits.
> > 
> > On the fast-export side, try to be as much like signed-tags as possible,
> > in both implementation and in user-interface; with the exception that
> > the default should be `--signed-commits=strip` (compared to the default
> > `--signed-tags=abort`), in order to continue defaulting to the
> > historical behavior.  Only bother implementing "gpgsig", not
> > "gpgsig-sha256"; the existing signed-tag support doesn't implement
> > "gpgsig-sha256" either.
> 
> I would appreciate it if we did in fact implement it.  I would like to
> use this functionality to round-trip objects between SHA-1 and SHA-256,
> and it would be nice if both worked.
> 
> The situation with tags is different: the signature using the current
> algorithm is always trailing, and the signature for the other algorithm
> is in the header.  That wasn't how we intended it to be, but that's how
> it ended up being.
> 
> As a result, tag output can support SHA-256 data,

I don't believe that's true?  With SHA-1-signed tags, the signature
gets included in the fast-import stream as part of the tag message
(the `data` line in the BNF).  Since SHA-256-signed tags have their
signature as a header (rather than just appending it to the message),
we'd have to add a 'gpgsig' sub-command to the 'tag' top-level-command
(like I've done to the 'commit' top-level-command).

>                                                   but with your
> proposal, SHA-256 commits wouldn't work at all.  Considering SHA-1 is
> wildly insecure and therefore signing SHA-1 commits adds very little
> security, whereas SHA-256 is presently considered strong, I'd argue that
> only supporting SHA-1 isn't the right move here.

The main reason I didn't implement SHA-256 support (well, besides that
the repo I'm working on turned out to not have any SHA-256-signed
commits in it) is that I had questions about SHA-256 that I didn't
know/couldn't find the answers to.

However, looking again, I see a few of the answers in
t7510-signed-commit.sh, so I'll have a go at it.  If I get stuck, I'll
go ahead and implement the below "gpgsig sha1" suggestion, and leave
the sha256 implementation to someone else.

> Provided we do that and the test suite passes under both algorithms, I'm
> strongly in favor of this feature.  In fact, I had been thinking about
> implementing this feature myself just the other day, so I'm delighted
> you decided to do it.

That's one of the big reasons I didn't implement both--I wasn't sure
how to test sha256 (within the test harness, `git commit -S` gives a
sha1 signature).

I see that t7510-signed-commit.sh 'verify-commit verifies multiply
signed commits' tests sha256 by hard-coding a raw commit object in the
test itself, and feeding that to `git hash-object`.  I'd prefer to
figure out how to get `git commit` itself to generate a sha256
signature rather than a sha1 signature, so that I can _know_ that I'm
getting the ordering of headers the same as `git commit`.  But I don't
think that needs to be a blocker; if the test doesn't do the same
ordering as `git commit`, I guess that can just be a bugfix later?

> > diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> > index 458af0a2d6..3d0c5dbf7d 100644
> > --- a/Documentation/git-fast-import.txt
> > +++ b/Documentation/git-fast-import.txt
> > @@ -437,6 +437,7 @@ change to the project.
> >  	original-oid?
> >  	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
> >  	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
> > +	('gpgsig' LF data)?
> 
> Could we emit this as "gpgsig sha1 data" and "gpgsig sha256 data"?  That
> would allow us to consider the future case where the hash algorithm
> changes again without requiring a change of format.

I like that idea.  I'll implement it.

FWIW, I thought about instead adding a fast-import command to insert
arbitrary headers in to the commit object, rather than having to add a
new command for every new header we want to be able to round-trip.
But it's like, if we're exposing that much of the low-levels of a
commit object, why are we keeping up the facade fast-import stream at
all, instead of streaming raw Git objects around?
brian m. carlson April 20, 2021, 11:07 p.m. UTC | #6
On 2021-04-20 at 17:15:25, Luke Shumaker wrote:
> On Mon, 19 Apr 2021 19:41:55 -0600,
> brian m. carlson wrote:
> > I would appreciate it if we did in fact implement it.  I would like to
> > use this functionality to round-trip objects between SHA-1 and SHA-256,
> > and it would be nice if both worked.
> > 
> > The situation with tags is different: the signature using the current
> > algorithm is always trailing, and the signature for the other algorithm
> > is in the header.  That wasn't how we intended it to be, but that's how
> > it ended up being.
> > 
> > As a result, tag output can support SHA-256 data,
> 
> I don't believe that's true?  With SHA-1-signed tags, the signature
> gets included in the fast-import stream as part of the tag message
> (the `data` line in the BNF).  Since SHA-256-signed tags have their
> signature as a header (rather than just appending it to the message),
> we'd have to add a 'gpgsig' sub-command to the 'tag' top-level-command
> (like I've done to the 'commit' top-level-command).

If you're using a repository that's SHA-1, then the tag signature that's
part of the message is a signature over the SHA-1 contents of the
object, and the gpgsig-sha256 header is a signature over the SHA-256
contents of the object.  If you're using a repository that's SHA-256,
it's reversed: the signature at the end of the message covers the
SHA-256 contents of the object and the gpgsig header covers the SHA-1
contents.

It isn't currently possible to create objects with both signatures in
place, but that will be possible in the future.

> >                                                   but with your
> > proposal, SHA-256 commits wouldn't work at all.  Considering SHA-1 is
> > wildly insecure and therefore signing SHA-1 commits adds very little
> > security, whereas SHA-256 is presently considered strong, I'd argue that
> > only supporting SHA-1 isn't the right move here.
> 
> The main reason I didn't implement SHA-256 support (well, besides that
> the repo I'm working on turned out to not have any SHA-256-signed
> commits in it) is that I had questions about SHA-256 that I didn't
> know/couldn't find the answers to.

Currently, repositories using SHA-256 currently don't interoperate with
SHA-1 repositories.  However, if you want to create a test repo, you can
do so with "git init --object-format=sha256" in an empty directory.

If you want to run the testsuite in SHA-256 mode, set
GIT_TEST_DEFAULT_HASH=sha256, and all the repositories created will use
SHA-256.

That should be sufficient to get this series such that it will work with
simple SHA-256 repos.  If you have more questions about this work or how
to get things working, I'm happy to answer them.

> However, looking again, I see a few of the answers in
> t7510-signed-commit.sh, so I'll have a go at it.  If I get stuck, I'll
> go ahead and implement the below "gpgsig sha1" suggestion, and leave
> the sha256 implementation to someone else.

Not implementing this means the CI will fail when the testsuite is run
in SHA-256 mode, so your patch probably won't be accepted.

> > Provided we do that and the test suite passes under both algorithms, I'm
> > strongly in favor of this feature.  In fact, I had been thinking about
> > implementing this feature myself just the other day, so I'm delighted
> > you decided to do it.
> 
> That's one of the big reasons I didn't implement both--I wasn't sure
> how to test sha256 (within the test harness, `git commit -S` gives a
> sha1 signature).
> 
> I see that t7510-signed-commit.sh 'verify-commit verifies multiply
> signed commits' tests sha256 by hard-coding a raw commit object in the
> test itself, and feeding that to `git hash-object`.  I'd prefer to
> figure out how to get `git commit` itself to generate a sha256
> signature rather than a sha1 signature, so that I can _know_ that I'm
> getting the ordering of headers the same as `git commit`.  But I don't
> think that needs to be a blocker; if the test doesn't do the same
> ordering as `git commit`, I guess that can just be a bugfix later?

Yes, dual-signed objects have to manually created right now; there's no
tooling to create them because that code hasn't landed yet.  It's in my
tree and very broken.  But you can create SHA-256 repositories as I
mentioned above and test those, and the testsuite does run in that mode,
so it should be easy enough to check at least single-signed commits for
now, even if you don't implement dual-signed ones.  I think it's fine if
that comes later, and I can pick that up as part of a future series.
Luke Shumaker April 21, 2021, 10:03 p.m. UTC | #7
On Tue, 20 Apr 2021 17:07:09 -0600,
brian m. carlson wrote:
> On 2021-04-20 at 17:15:25, Luke Shumaker wrote:
> > I don't believe that's true?  With SHA-1-signed tags, the signature
> > gets included in the fast-import stream as part of the tag message
> > (the `data` line in the BNF).  Since SHA-256-signed tags have their
> > signature as a header (rather than just appending it to the message),
> > we'd have to add a 'gpgsig' sub-command to the 'tag' top-level-command
> > (like I've done to the 'commit' top-level-command).
> 
> If you're using a repository that's SHA-1, then the tag signature that's
> part of the message is a signature over the SHA-1 contents of the
> object, and the gpgsig-sha256 header is a signature over the SHA-256
> contents of the object.  If you're using a repository that's SHA-256,
> it's reversed: the signature at the end of the message covers the
> SHA-256 contents of the object and the gpgsig header covers the SHA-1
> contents.

Good to know!  It seems I've been mislead by
Documentation/technical/hash-function-transition.txt

> Not implementing this means the CI will fail when the testsuite is run
> in SHA-256 mode, so your patch probably won't be accepted.

Gotcha.  I guess I will be implementing it then.  I'll let you know if
I have any further questions, the information you've given already has
been very helpful!
diff mbox series

Patch

diff --git a/Documentation/git-fast-export.txt b/Documentation/git-fast-export.txt
index d4a2bfe037..6fdb678b54 100644
--- a/Documentation/git-fast-export.txt
+++ b/Documentation/git-fast-export.txt
@@ -39,6 +39,18 @@  warning will be displayed, with 'verbatim', they will be silently
 exported and with 'warn-verbatim', they will be exported, but you will
 see a warning.
 
+--signed-commits=(verbatim|warn-verbatim|warn-strip|strip|abort)::
+	Specify how to handle signed commits.  Since any transformation
+	after the export can change the commit (which can also happen
+	when excluding revisions) the signatures will not match.
++
+When asking to 'abort', this program will die when encountering a
+signed commit.  With 'strip' (which is the default), the commits will
+silently be made unsigned, with 'warn-strip' they will be made
+unsigned but a warning will be displayed, with 'verbatim', they will
+be silently exported and with 'warn-verbatim', they will be exported,
+but you will see a warning.
+
 --tag-of-filtered-object=(abort|drop|rewrite)::
 	Specify how to handle tags whose tagged object is filtered out.
 	Since revisions and files to export can be limited by path,
diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 458af0a2d6..3d0c5dbf7d 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -437,6 +437,7 @@  change to the project.
 	original-oid?
 	('author' (SP <name>)? SP LT <email> GT SP <when> LF)?
 	'committer' (SP <name>)? SP LT <email> GT SP <when> LF
+	('gpgsig' LF data)?
 	('encoding' SP <encoding> LF)?
 	data
 	('from' SP <commit-ish> LF)?
@@ -505,6 +506,12 @@  that was selected by the --date-format=<fmt> command-line option.
 See ``Date Formats'' above for the set of supported formats, and
 their syntax.
 
+`gpgsig`
+^^^^^^^^
+
+The optional `gpgsig` command is used to include a PGP/GPG signature
+that signs the commit data.
+
 `encoding`
 ^^^^^^^^^^
 The optional `encoding` command indicates the encoding of the commit
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index d121dd2ee6..d48adbc9b9 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -30,8 +30,11 @@  static const char *fast_export_usage[] = {
 	NULL
 };
 
+enum sign_mode { SIGN_ABORT, SIGN_VERBATIM, SIGN_STRIP, SIGN_VERBATIM_WARN, SIGN_STRIP_WARN };
+
 static int progress;
-static enum { SIGNED_TAG_ABORT, VERBATIM, WARN, WARN_STRIP, STRIP } signed_tag_mode = SIGNED_TAG_ABORT;
+static enum sign_mode signed_tag_mode = SIGN_ABORT;
+static enum sign_mode signed_commit_mode = SIGN_STRIP;
 static enum { TAG_FILTERING_ABORT, DROP, REWRITE } tag_of_filtered_mode = TAG_FILTERING_ABORT;
 static enum { REENCODE_ABORT, REENCODE_YES, REENCODE_NO } reencode_mode = REENCODE_ABORT;
 static int fake_missing_tagger;
@@ -48,21 +51,24 @@  static int anonymize;
 static struct hashmap anonymized_seeds;
 static struct revision_sources revision_sources;
 
-static int parse_opt_signed_tag_mode(const struct option *opt,
+static int parse_opt_sign_mode(const struct option *opt,
 				     const char *arg, int unset)
 {
-	if (unset || !strcmp(arg, "abort"))
-		signed_tag_mode = SIGNED_TAG_ABORT;
+	enum sign_mode *valptr = opt->value;
+	if (unset)
+		return 0;
+	else if (!strcmp(arg, "abort"))
+		*valptr = SIGN_ABORT;
 	else if (!strcmp(arg, "verbatim") || !strcmp(arg, "ignore"))
-		signed_tag_mode = VERBATIM;
+		*valptr = SIGN_VERBATIM;
 	else if (!strcmp(arg, "warn-verbatim") || !strcmp(arg, "warn"))
-		signed_tag_mode = WARN;
+		*valptr = SIGN_VERBATIM_WARN;
 	else if (!strcmp(arg, "warn-strip"))
-		signed_tag_mode = WARN_STRIP;
+		*valptr = SIGN_STRIP_WARN;
 	else if (!strcmp(arg, "strip"))
-		signed_tag_mode = STRIP;
+		*valptr = SIGN_STRIP;
 	else
-		return error("Unknown signed-tags mode: %s", arg);
+		return error("Unknown %s mode: %s", opt->long_name, arg);
 	return 0;
 }
 
@@ -499,6 +505,28 @@  static void show_filemodify(struct diff_queue_struct *q,
 	}
 }
 
+static const char *find_signature(const char *begin, const char *end)
+{
+	const char *needle = "\ngpgsig ";
+	char *bod, *eod, *eol;
+
+	bod = memmem(begin, end ? end - begin : strlen(begin),
+		     needle, strlen(needle));
+	if (!bod)
+		return NULL;
+	bod += strlen(needle);
+	eod = strchrnul(bod, '\n');
+	while (eod[0] == '\n' && eod[1] == ' ') {
+		eod = strchrnul(eod+1, '\n');
+	}
+	*eod = '\0';
+
+	while ((eol = strstr(bod, "\n ")))
+		memmove(eol+1, eol+2, strlen(eol+1));
+
+	return bod;
+}
+
 static const char *find_encoding(const char *begin, const char *end)
 {
 	const char *needle = "\nencoding ";
@@ -621,7 +649,7 @@  static void handle_commit(struct commit *commit, struct rev_info *rev,
 	int saved_output_format = rev->diffopt.output_format;
 	const char *commit_buffer;
 	const char *author, *author_end, *committer, *committer_end;
-	const char *encoding, *message;
+	const char *encoding, *signature, *message;
 	char *reencoded = NULL;
 	struct commit_list *p;
 	const char *refname;
@@ -644,6 +672,7 @@  static void handle_commit(struct commit *commit, struct rev_info *rev,
 	committer++;
 	committer_end = strchrnul(committer, '\n');
 	message = strstr(committer_end, "\n\n");
+	signature = find_signature(committer_end, message);
 	encoding = find_encoding(committer_end, message);
 	if (message)
 		message += 2;
@@ -703,6 +732,28 @@  static void handle_commit(struct commit *commit, struct rev_info *rev,
 	printf("%.*s\n%.*s\n",
 	       (int)(author_end - author), author,
 	       (int)(committer_end - committer), committer);
+	if (signature)
+		switch(signed_commit_mode) {
+		case SIGN_ABORT:
+			die("encountered signed commit %s; use "
+			    "--signed-commits=<mode> to handle it",
+			    oid_to_hex(&commit->object.oid));
+		case SIGN_VERBATIM_WARN:
+			warning("exporting signed commit %s",
+				oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_VERBATIM:
+			printf("gpgsig\ndata %u\n%s",
+			       (unsigned)strlen(signature),
+			       signature);
+			break;
+		case SIGN_STRIP_WARN:
+			warning("stripping signature from commit %s",
+			       oid_to_hex(&commit->object.oid));
+			/* fallthru */
+		case SIGN_STRIP:
+			break;
+		}
 	if (!reencoded && encoding)
 		printf("encoding %s\n", encoding);
 	printf("data %u\n%s",
@@ -830,21 +881,21 @@  static void handle_tag(const char *name, struct tag *tag)
 					       "\n-----BEGIN PGP SIGNATURE-----\n");
 		if (signature)
 			switch(signed_tag_mode) {
-			case SIGNED_TAG_ABORT:
+			case SIGN_ABORT:
 				die("encountered signed tag %s; use "
 				    "--signed-tags=<mode> to handle it",
 				    oid_to_hex(&tag->object.oid));
-			case WARN:
+			case SIGN_VERBATIM_WARN:
 				warning("exporting signed tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case VERBATIM:
+			case SIGN_VERBATIM:
 				break;
-			case WARN_STRIP:
+			case SIGN_STRIP_WARN:
 				warning("stripping signature from tag %s",
 					oid_to_hex(&tag->object.oid));
 				/* fallthru */
-			case STRIP:
+			case SIGN_STRIP:
 				message_size = signature + 1 - message;
 				break;
 			}
@@ -1197,7 +1248,10 @@  int cmd_fast_export(int argc, const char **argv, const char *prefix)
 			    N_("show progress after <n> objects")),
 		OPT_CALLBACK(0, "signed-tags", &signed_tag_mode, N_("mode"),
 			     N_("select handling of signed tags"),
-			     parse_opt_signed_tag_mode),
+			     parse_opt_sign_mode),
+		OPT_CALLBACK(0, "signed-commits", &signed_commit_mode, N_("mode"),
+			     N_("select handling of signed commits"),
+			     parse_opt_sign_mode),
 		OPT_CALLBACK(0, "tag-of-filtered-object", &tag_of_filtered_mode, N_("mode"),
 			     N_("select handling of tags that tag filtered objects"),
 			     parse_opt_tag_of_filtered_mode),
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 3afa81cf9a..74d08e09fd 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -2669,7 +2669,9 @@  static struct hash_list *parse_merge(unsigned int *count)
 
 static void parse_new_commit(const char *arg)
 {
+	static struct strbuf sig = STRBUF_INIT;
 	static struct strbuf msg = STRBUF_INIT;
+	struct string_list siglines = STRING_LIST_INIT_NODUP;
 	struct branch *b;
 	char *author = NULL;
 	char *committer = NULL;
@@ -2696,6 +2698,12 @@  static void parse_new_commit(const char *arg)
 	}
 	if (!committer)
 		die("Expected committer but didn't get one");
+	if (!strcmp(command_buf.buf, "gpgsig")) {
+		read_next_command();
+		parse_data(&sig, 0, NULL);
+		read_next_command();
+	} else
+		strbuf_setlen(&sig, 0);
 	if (skip_prefix(command_buf.buf, "encoding ", &v)) {
 		encoding = xstrdup(v);
 		read_next_command();
@@ -2769,8 +2777,15 @@  static void parse_new_commit(const char *arg)
 		strbuf_addf(&new_data,
 			"encoding %s\n",
 			encoding);
+	if (sig.len) {
+		strbuf_addstr(&new_data, "gpgsig ");
+		string_list_split_in_place(&siglines, sig.buf, '\n', -1);
+		strbuf_add_separated_string_list(&new_data, "\n ", &siglines);
+		strbuf_addch(&new_data, '\n');
+	}
 	strbuf_addch(&new_data, '\n');
 	strbuf_addbuf(&new_data, &msg);
+	string_list_clear(&siglines, 1);
 	free(author);
 	free(committer);
 	free(encoding);
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index db0e58b1e8..49a2827be2 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -8,6 +8,7 @@  GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
+. "$TEST_DIRECTORY/lib-gpg.sh"
 
 test_expect_success 'setup' '
 
@@ -278,9 +279,78 @@  test_expect_success 'signed-tags=warn-strip' '
 	test -s err
 '
 
+test_expect_success GPG 'set up signed commit' '
+
+	# Generate a commit with both "gpgsig" and "encoding" set, so
+	# that we can test that fast-import gets the ordering correct
+	# between the two.
+	test_config i18n.commitEncoding ISO-8859-1 &&
+	git checkout -f -b commit-signing main &&
+	echo Sign your name > file-sign &&
+	git add file-sign &&
+	git commit -S -m "signed commit" &&
+	COMMIT_SIGNING=$(git rev-parse --verify commit-signing)
+
+'
+
+test_expect_success GPG 'signed-commits=abort' '
+
+	test_must_fail git fast-export --signed-commits=abort commit-signing
+
+'
+
+test_expect_success GPG 'signed-commits=verbatim' '
+
+	git fast-export --signed-commits=verbatim --reencode=no commit-signing >output &&
+	grep ^gpgsig output &&
+	grep "encoding ISO-8859-1" output &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=warn-verbatim' '
+
+	git fast-export --signed-commits=warn-verbatim --reencode=no commit-signing >output 2>err &&
+	grep ^gpgsig output &&
+	grep "encoding ISO-8859-1" output &&
+	test -s err &&
+	(cd new &&
+	 git fast-import &&
+	 test $COMMIT_SIGNING = $(git rev-parse --verify refs/heads/commit-signing)) <output
+
+'
+
+test_expect_success GPG 'signed-commits=strip' '
+
+	git fast-export --signed-commits=strip --reencode=no commit-signing >output &&
+	! grep ^gpgsig output &&
+	grep "^encoding ISO-8859-1" output &&
+	sed "s/commit-signing/commit-strip-signing/" output |
+		(cd new &&
+		 git fast-import &&
+		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))
+
+'
+
+test_expect_success GPG 'signed-commits=warn-strip' '
+
+	git fast-export --signed-commits=warn-strip --reencode=no commit-signing >output 2>err &&
+	! grep ^gpgsig output &&
+	grep "^encoding ISO-8859-1" output &&
+	test -s err &&
+	sed "s/commit-signing/commit-strip-signing/" output |
+		(cd new &&
+		 git fast-import &&
+		 test $COMMIT_SIGNING != $(git rev-parse --verify refs/heads/commit-strip-signing))
+
+'
+
 test_expect_success 'setup submodule' '
 
 	git checkout -f main &&
+	{ git update-ref -d refs/heads/commit-signing || true; } &&
 	mkdir sub &&
 	(
 		cd sub &&