[RFD/PATCH] push: heed user.signingkey for signed pushes

Discussion:

Michael J Gruber

2014-10-22 14:47:03 UTC

push --signed promises to take user.signingkey as the signing key but
fails to read the config.

Make it do so.

Signed-off-by: Michael J Gruber <***@drmicha.warpmail.net>
---
Interestingly, when I wrote the test I had the impression that user.email
is not heeded either - or do we have GIT_COMMITTER_EMAIL in the environment
of the tests by default?

In any case, that is why the test looks the way it looks and why this is RFD.

1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index ae56f73..a076b19 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -471,6 +471,17 @@ static int option_parse_recurse_submodules(const struct option *opt,
return 0;
}

+static int git_push_config(const char *k, const char *v, void *cb)
+{
+ struct wt_status *s = cb;
+ int status;
+
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
+ return git_default_config(k, v, s);
+}
+
int cmd_push(int argc, const char **argv, const char *prefix)
{
int flags = 0;
@@ -511,7 +522,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
};

packet_trace_identity("push");
- git_config(git_default_config, NULL);
+ git_config(git_push_config, NULL);
argc = parse_options(argc, argv, prefix, options, push_usage, 0);

if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
--
2.1.2.756.gfa53a0a

builtin/push.c | 13 ++++++++++++-
t/lib-gpg/trustdb.gpg | Bin 1360 -> 1360 bytes
t/t5534-push-signed.sh | 43 +++++++++++++++++++++++++++++++++++++++++++
3 files changed, 55 insertions(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index ae56f73..a076b19 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -471,6 +471,17 @@ static int option_parse_recurse_submodules(const struct option *opt,
return 0;
}

+static int git_push_config(const char *k, const char *v, void *cb)
+{
+ struct wt_status *s = cb;
+ int status;
+
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
+ return git_default_config(k, v, s);
+}
+
int cmd_push(int argc, const char **argv, const char *prefix)
{
int flags = 0;
@@ -511,7 +522,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
};

packet_trace_identity("push");
- git_config(git_default_config, NULL);
+ git_config(git_push_config, NULL);
argc = parse_options(argc, argv, prefix, options, push_usage, 0);

if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
diff --git a/t/lib-gpg/trustdb.gpg b/t/lib-gpg/trustdb.gpg
index 4879ae9a84650a93a4d15bd6560c5d1b89eb4c2f..c11b1464b3d13b45966a493e2174fc0e253ddd0c 100644
GIT binary patch
delta 47
ncmcb>b%9HOF})z2nVFH5k%@sJ#C^}~iH71E)x}wb7%%_;=xPS!

delta 51
tcmcb>b%9HSF})z2nVFH5k%@sJ&}Z5*1_lPkiGso#)x}wb*nk{V008$***@JE

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 2786346..c68867d 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -124,4 +124,47 @@ test_expect_success GPG 'signed push sends push certificate' '
test_cmp expect dst/push-cert-status
'

+test_expect_success GPG 'fail without key and heed user.signingkey' '
+ prepare_dst &&
+ mkdir -p dst/.git/hooks &&
+ git -C dst config receive.certnonceseed sekrit &&
+ write_script dst/.git/hooks/post-receive <<-\EOF &&
+ # discard the update list
+ cat >/dev/null
+ # record the push certificate
+ if test -n "${GIT_PUSH_CERT-}"
+ then
+ git cat-file blob $GIT_PUSH_CERT >../push-cert
+ fi &&
+
+ cat >../push-cert-status <<E_O_F
+ SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
+ KEY=${GIT_PUSH_CERT_KEY-nokey}
+ STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
+ NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
+ NONCE=${GIT_PUSH_CERT_NONCE-nononce}
+ E_O_F
+
+ EOF
+
+ git config user.email ***@nowhere.com &&
+ test_must_fail env GIT_COMMITTER_EMAIL=***@nowhere.com git push --signed dst noop ff +noff &&
+ git config user.signingkey ***@example.com &&
+ GIT_COMMITTER_EMAIL=***@nowhere.com git push --signed dst noop ff +noff &&
+
+ (
+ cat <<-\EOF &&
+ SIGNER=C O Mitter <***@example.com>
+ KEY=13B6F51ECDDE430D
+ STATUS=G
+ NONCE_STATUS=OK
+ EOF
+ sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
+ ) >expect &&
+
+ grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
+ grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
+ test_cmp expect dst/push-cert-status
+'
+
test_done

--
2.1.2.756.gfa53a0a

Michael J Gruber

2014-10-22 14:57:49 UTC

Permalink

push --signed promises to take user.signingkey as the signing key but
fails to read the config.

Make it do so.

Signed-off-by: Michael J Gruber <***@drmicha.warpmail.net>
---
Okay, I guess this is nicer. We do have the committer info in the env. Sorry.

builtin/push.c | 13 ++++++++++++-
t/lib-gpg/trustdb.gpg | Bin 1360 -> 1360 bytes
t/t5534-push-signed.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 1 deletion(-)

diff --git a/builtin/push.c b/builtin/push.c
index ae56f73..a076b19 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -471,6 +471,17 @@ static int option_parse_recurse_submodules(const struct option *opt,
return 0;
}

+static int git_push_config(const char *k, const char *v, void *cb)
+{
+ struct wt_status *s = cb;
+ int status;
+
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
+ return git_default_config(k, v, s);
+}
+
int cmd_push(int argc, const char **argv, const char *prefix)
{
int flags = 0;
@@ -511,7 +522,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
};

packet_trace_identity("push");
- git_config(git_default_config, NULL);
+ git_config(git_push_config, NULL);
argc = parse_options(argc, argv, prefix, options, push_usage, 0);

if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
diff --git a/t/lib-gpg/trustdb.gpg b/t/lib-gpg/trustdb.gpg
index 4879ae9a84650a93a4d15bd6560c5d1b89eb4c2f..c11b1464b3d13b45966a493e2174fc0e253ddd0c 100644
GIT binary patch
delta 47
ncmcb>b%9HOF})z2nVFH5k%@sJ#C^}~iH71E)x}wb7%%_;=xPS!

delta 51
tcmcb>b%9HSF})z2nVFH5k%@sJ&}Z5*1_lPkiGso#)x}wb*nk{V008$***@JE

diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 2786346..ecb8d44 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -124,4 +124,48 @@ test_expect_success GPG 'signed push sends push certificate' '
test_cmp expect dst/push-cert-status
'

+test_expect_success GPG 'fail without key and heed user.signingkey' '
+ prepare_dst &&
+ mkdir -p dst/.git/hooks &&
+ git -C dst config receive.certnonceseed sekrit &&
+ write_script dst/.git/hooks/post-receive <<-\EOF &&
+ # discard the update list
+ cat >/dev/null
+ # record the push certificate
+ if test -n "${GIT_PUSH_CERT-}"
+ then
+ git cat-file blob $GIT_PUSH_CERT >../push-cert
+ fi &&
+
+ cat >../push-cert-status <<E_O_F
+ SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
+ KEY=${GIT_PUSH_CERT_KEY-nokey}
+ STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
+ NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
+ NONCE=${GIT_PUSH_CERT_NONCE-nononce}
+ E_O_F
+
+ EOF
+
+ unset GIT_COMMITTER_EMAIL &&
+ git config user.email ***@nowhere.com &&
+ test_must_fail git push --signed dst noop ff +noff &&
+ git config user.signingkey ***@example.com &&
+ git push --signed dst noop ff +noff &&
+
+ (
+ cat <<-\EOF &&
+ SIGNER=C O Mitter <***@example.com>
+ KEY=13B6F51ECDDE430D
+ STATUS=G
+ NONCE_STATUS=OK
+ EOF
+ sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
+ ) >expect &&
+
+ grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
+ grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
+ test_cmp expect dst/push-cert-status
+'
+
test_done

--
2.1.2.756.gfa53a0a

Junio C Hamano

2014-10-22 22:05:55 UTC

Permalink

Post by Michael J Gruber
push --signed promises to take user.signingkey as the signing key but
fails to read the config.
Make it do so.
---
Okay, I guess this is nicer. We do have the committer info in the env. Sorry.
builtin/push.c | 13 ++++++++++++-
t/lib-gpg/trustdb.gpg | Bin 1360 -> 1360 bytes
t/t5534-push-signed.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 56 insertions(+), 1 deletion(-)

Hmph, I simply forgot about that configuration, I guess.

What is this change to trustdb about, though? The log message does
not say anything about it.

Post by Michael J Gruber
diff --git a/builtin/push.c b/builtin/push.c
index ae56f73..a076b19 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -471,6 +471,17 @@ static int option_parse_recurse_submodules(const struct option *opt,
return 0;
}
+static int git_push_config(const char *k, const char *v, void *cb)
+{
+ struct wt_status *s = cb;
+ int status;
+
+ status = git_gpg_config(k, v, NULL);
+ if (status)
+ return status;
+ return git_default_config(k, v, s);
+}
+
int cmd_push(int argc, const char **argv, const char *prefix)
{
int flags = 0;
@@ -511,7 +522,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
};
packet_trace_identity("push");
- git_config(git_default_config, NULL);
+ git_config(git_push_config, NULL);
argc = parse_options(argc, argv, prefix, options, push_usage, 0);
if (deleterefs && (tags || (flags & (TRANSPORT_PUSH_ALL | TRANSPORT_PUSH_MIRROR))))
diff --git a/t/lib-gpg/trustdb.gpg b/t/lib-gpg/trustdb.gpg
index 4879ae9a84650a93a4d15bd6560c5d1b89eb4c2f..c11b1464b3d13b45966a493e2174fc0e253ddd0c 100644
GIT binary patch
delta 47
delta 51
diff --git a/t/t5534-push-signed.sh b/t/t5534-push-signed.sh
index 2786346..ecb8d44 100755
--- a/t/t5534-push-signed.sh
+++ b/t/t5534-push-signed.sh
@@ -124,4 +124,48 @@ test_expect_success GPG 'signed push sends push certificate' '
test_cmp expect dst/push-cert-status
'
+test_expect_success GPG 'fail without key and heed user.signingkey' '
+ prepare_dst &&
+ mkdir -p dst/.git/hooks &&
+ git -C dst config receive.certnonceseed sekrit &&
+ write_script dst/.git/hooks/post-receive <<-\EOF &&
+ # discard the update list
+ cat >/dev/null
+ # record the push certificate
+ if test -n "${GIT_PUSH_CERT-}"
+ then
+ git cat-file blob $GIT_PUSH_CERT >../push-cert
+ fi &&
+
+ cat >../push-cert-status <<E_O_F
+ SIGNER=${GIT_PUSH_CERT_SIGNER-nobody}
+ KEY=${GIT_PUSH_CERT_KEY-nokey}
+ STATUS=${GIT_PUSH_CERT_STATUS-nostatus}
+ NONCE_STATUS=${GIT_PUSH_CERT_NONCE_STATUS-nononcestatus}
+ NONCE=${GIT_PUSH_CERT_NONCE-nononce}
+ E_O_F
+
+ EOF
+
+ unset GIT_COMMITTER_EMAIL &&
+ test_must_fail git push --signed dst noop ff +noff &&
+ git push --signed dst noop ff +noff &&
+
+ (
+ cat <<-\EOF &&
+ KEY=13B6F51ECDDE430D
+ STATUS=G
+ NONCE_STATUS=OK
+ EOF
+ sed -n -e "s/^nonce /NONCE=/p" -e "/^$/q" dst/push-cert
+ ) >expect &&
+
+ grep "$(git rev-parse noop ff) refs/heads/ff" dst/push-cert &&
+ grep "$(git rev-parse noop noff) refs/heads/noff" dst/push-cert &&
+ test_cmp expect dst/push-cert-status
+'
+
test_done

Junio C Hamano

2014-10-22 23:47:28 UTC

Permalink

Post by Junio C Hamano

Hmph, I simply forgot about that configuration, I guess.
What is this change to trustdb about, though? The log message does
not say anything about it.

This is a related tangent, but I just tried this:

$ git clone ... git.git
$ cd git.git
$ chmod a-w t/lib-gpg/* t/lib-gpg
$ make test

which makes GPG related tests to fail, as running GPG with the GNUPGHOME
set there involves writing into the files in the directory (or
removing and recreating).

Perhaps GPG tests should create their own copy in the playpen (aka
"trash directory") and use that as GNUPGHOME so that we do not have
to write into the single shared directory? I wonder if automated
parallel tests can intermittently fail because of this...