[PATCH 6/7] Support signing pushes iff the server supports it

2015-08-13 Thread Dave Borowitz
Add a new flag --signed-if-possible to push and send-pack that sends a
push certificate if and only if the server advertised a push cert
nonce. If not, at least warn the user that their push may not be as
secure as they thought.

Signed-off-by: Dave Borowitz 
---
 Documentation/git-push.txt  |  9 +++--
 Documentation/git-send-pack.txt |  9 +++--
 builtin/push.c  |  4 +++-
 builtin/send-pack.c |  6 +-
 remote-curl.c   | 14 ++
 send-pack.c | 18 +++---
 send-pack.h |  8 +++-
 transport-helper.c  | 34 +-
 transport.c |  8 +++-
 transport.h |  5 +++--
 10 files changed, 81 insertions(+), 34 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index f8b8b8b..fcfdf73 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git push' [--all | --mirror | --tags] [--follow-tags] [--atomic] [-n | 
--dry-run] [--receive-pack=]
   [--repo=] [-f | --force] [--prune] [-v | --verbose]
-  [-u | --set-upstream] [--signed]
+  [-u | --set-upstream] [--signed] [--signed-if-possible]
   [--force-with-lease[=[:]]]
   [--no-verify] [ [...]]
 
@@ -139,7 +139,12 @@ already exists on the remote side.
logged.  See linkgit:git-receive-pack[1] for the details
on the receiving end.  If the `gpg` executable is not available,
or if the server does not support signed pushes, the push will
-   fail.
+   fail. Takes precedence over --signed-if-possible.
+
+--signed-if-possible::
+   Like --signed, but only if the server supports signed pushes. If
+   the server supports signed pushes but the `gpg` is not available,
+   the push will fail.
 
 --[no-]atomic::
Use an atomic transaction on the remote side if available.
diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index dde13b0..5789208 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 
 [verse]
 'git send-pack' [--all] [--dry-run] [--force] 
[--receive-pack=]
-   [--verbose] [--thin] [--atomic] [--signed]
+   [--verbose] [--thin] [--atomic] [--signed] 
[--signed-if-possible]
[:] [...]
 
 DESCRIPTION
@@ -75,7 +75,12 @@ be in a separate packet, and the list must end with a flush 
packet.
logged.  See linkgit:git-receive-pack[1] for the details
on the receiving end.  If the `gpg` executable is not available,
or if the server does not support signed pushes, the push will
-   fail.
+   fail. Takes precedence over --signed-if-possible.
+
+--signed-if-possible::
+   Like --signed, but only if the server supports signed pushes. If
+   the server supports signed pushes but the `gpg` is not available,
+   the push will fail.
 
 ::
A remote host to house the repository.  When this
diff --git a/builtin/push.c b/builtin/push.c
index 57c138b..95a67c5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -526,7 +526,9 @@ int cmd_push(int argc, const char **argv, const char 
*prefix)
OPT_BIT(0, "no-verify", &flags, N_("bypass pre-push hook"), 
TRANSPORT_PUSH_NO_HOOK),
OPT_BIT(0, "follow-tags", &flags, N_("push missing but relevant 
tags"),
TRANSPORT_PUSH_FOLLOW_TAGS),
-   OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), 
TRANSPORT_PUSH_CERT),
+   OPT_BIT(0, "signed", &flags, N_("GPG sign the push"), 
TRANSPORT_PUSH_CERT_ALWAYS),
+   OPT_BIT(0, "signed-if-possible", &flags, N_("GPG sign the push, 
if supported by the server"),
+   TRANSPORT_PUSH_CERT_IF_POSSIBLE),
OPT_BIT(0, "atomic", &flags, N_("request atomic transaction on 
remote side"), TRANSPORT_PUSH_ATOMIC),
OPT_END()
};
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 23b2962..8eebbf4 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -158,7 +158,11 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
continue;
}
if (!strcmp(arg, "--signed")) {
-   args.push_cert = 1;
+   args.push_cert = SEND_PACK_PUSH_CERT_ALWAYS;
+   continue;
+   }
+   if (!strcmp(arg, "--signed-if-possible")) {
+   args.push_cert = 
SEND_PACK_PUSH_CERT_IF_POSSIBLE;
continue;
}
if (!strcmp(arg, "--progress")) {
diff --git a/remote-curl.c b/remote-curl.c
index af7b678..f049de8 100644
--- a/remote-curl.c
+++ b/r

Re: [PATCH 6/7] Support signing pushes iff the server supports it

2015-08-14 Thread Junio C Hamano
Dave Borowitz  writes:

> diff --git a/send-pack.c b/send-pack.c
> index 2a64fec..6ae9f45 100644
> --- a/send-pack.c
> +++ b/send-pack.c
> @@ -370,7 +370,7 @@ int send_pack(struct send_pack_args *args,
>   args->use_thin_pack = 0;
>   if (server_supports("atomic"))
>   atomic_supported = 1;
> - if (args->push_cert) {
> + if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
>   int len;
>  
>   push_cert_nonce = server_feature_value("push-cert", &len);
> @@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,
>   reject_invalid_nonce(push_cert_nonce, len);
>   push_cert_nonce = xmemdupz(push_cert_nonce, len);
>   }
> + if (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {
> + int len;
> +
> + push_cert_nonce = server_feature_value("push-cert", &len);
> + if (push_cert_nonce) {
> + reject_invalid_nonce(push_cert_nonce, len);
> + push_cert_nonce = xmemdupz(push_cert_nonce, len);
> + } else
> + warning(_("not sending a push certificate since the"
> +   " receiving end does not support --signed"
> +   " push"));
> + }

I wonder if the bodies of these two if statements can be a bit
better organized to avoid duplication (I suspect you have tried
and you may already know that the above is the most readable
version, but I haven't tried to do so myself, so...).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 6/7] Support signing pushes iff the server supports it

2015-08-19 Thread Dave Borowitz
On Fri, Aug 14, 2015 at 7:22 PM, Junio C Hamano  wrote:
> Dave Borowitz  writes:
>
>> diff --git a/send-pack.c b/send-pack.c
>> index 2a64fec..6ae9f45 100644
>> --- a/send-pack.c
>> +++ b/send-pack.c
>> @@ -370,7 +370,7 @@ int send_pack(struct send_pack_args *args,
>>   args->use_thin_pack = 0;
>>   if (server_supports("atomic"))
>>   atomic_supported = 1;
>> - if (args->push_cert) {
>> + if (args->push_cert == SEND_PACK_PUSH_CERT_ALWAYS) {
>>   int len;
>>
>>   push_cert_nonce = server_feature_value("push-cert", &len);
>> @@ -379,6 +379,18 @@ int send_pack(struct send_pack_args *args,
>>   reject_invalid_nonce(push_cert_nonce, len);
>>   push_cert_nonce = xmemdupz(push_cert_nonce, len);
>>   }
>> + if (args->push_cert == SEND_PACK_PUSH_CERT_IF_POSSIBLE) {
>> + int len;
>> +
>> + push_cert_nonce = server_feature_value("push-cert", &len);
>> + if (push_cert_nonce) {
>> + reject_invalid_nonce(push_cert_nonce, len);
>> + push_cert_nonce = xmemdupz(push_cert_nonce, len);
>> + } else
>> + warning(_("not sending a push certificate since the"
>> +   " receiving end does not support --signed"
>> +   " push"));
>> + }
>
> I wonder if the bodies of these two if statements can be a bit
> better organized to avoid duplication (I suspect you have tried
> and you may already know that the above is the most readable
> version, but I haven't tried to do so myself, so...).

Found a slightly less repetitious way.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html