Skip to contentSkip to conversation

Make verify-tag a builtin.

PATCH In review Carlos Rica 2 participants 6 messages Jun 15, 2016 Kbuild

lore
Share & tools
lore

Carlos Rica

author
#0 lore raw

PATCH 1/1 Make verify-tag a builtin.

This replaces "git-verify-tag.sh" with "builtin-verify-tag.c".

Signal SIGPIPE is ignored because the program sometimes was stopped because that signal when writing the input for gpg.

Signed-off-by Carlos Rica <[email protected]>

---

Comments are welcomed.

 Makefile                                           |    3 +-
 builtin-verify-tag.c                               |  113 ++++++++++++++++++++
 builtin.h                                          |    1 +
 .../examples/git-verify-tag.sh                     |    0
 git.c                                              |    1 +
 5 files changed, 117 insertions(+), 1 deletions(-)
 create mode 100644 builtin-verify-tag.c
 rename git-verify-tag.sh => contrib/examples/git-verify-tag.sh (100%)
Makefile +2 -1
index 73b487f..c6ed79f 100644
@@ -206,7 +206,7 @@ SCRIPT_SH = \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
 	git-repack.sh git-request-pull.sh git-reset.sh \
 	git-sh-setup.sh \
-	git-tag.sh git-verify-tag.sh \
+	git-tag.sh \
 	git-am.sh \
 	git-merge.sh git-merge-stupid.sh git-merge-octopus.sh \
 	git-merge-resolve.sh git-merge-ours.sh \

@@ -367,6 +367,7 @@ BUILTIN_OBJS = \
 	builtin-update-ref.o \
 	builtin-upload-archive.o \
 	builtin-verify-pack.o \
+	builtin-verify-tag.o \
 	builtin-write-tree.o \
 	builtin-show-ref.o \
 	builtin-pack-refs.o
builtin-verify-tag.c new file +113 -0

113 lines not on this page · raw message

builtin.h +1 -0
index 4cc228d..cb860a0 100644
@@ -76,6 +76,7 @@ extern int cmd_update_index(int argc, const char **argv, const char *prefix);
 extern int cmd_update_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_archive(int argc, const char **argv, const char *prefix);
 extern int cmd_upload_tar(int argc, const char **argv, const char *prefix);
+extern int cmd_verify_tag(int argc, const char **argv, const char *prefix);
 extern int cmd_version(int argc, const char **argv, const char *prefix);
 extern int cmd_whatchanged(int argc, const char **argv, const char *prefix);
 extern int cmd_write_tree(int argc, const char **argv, const char *prefix);
contrib/examples/git-verify-tag.sh renamed from git-verify-tag.sh +0 -0
similarity index 100%
rename from git-verify-tag.sh
rename to contrib/examples/git-verify-tag.sh
git.c +1 -0
index a647f9c..1dfe120 100644
@@ -368,6 +368,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "update-index", cmd_update_index, RUN_SETUP },
 		{ "update-ref", cmd_update_ref, RUN_SETUP },
 		{ "upload-archive", cmd_upload_archive },
+		{ "verify-tag", cmd_verify_tag, RUN_SETUP },
 		{ "version", cmd_version },
 		{ "whatchanged", cmd_whatchanged, RUN_SETUP | USE_PAGER },
 		{ "write-tree", cmd_write_tree, RUN_SETUP },
Signature · 1 line
1.5.0
#1 lore raw

Hi,

Quoted 7 lines · Carlos Rica
builtin-verify-tag.c new file +7 -0
+static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
+{
+	struct child_process gpg;
+	const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
+	char *path, *eol;
+	size_t len;
+	int fd, ret;

builtin-verify-tag.c line 35

Quoted 10 lines
builtin-verify-tag.c new file +10 -0
+
+	path = xstrdup(git_path("VTAG_TMP"));
+	fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+	if (fd < 0)
+		return error("could not create temporary file '%s': %s",
+						path, strerror(errno));
+	if (write_in_full(fd, buf, size) < 0)
+		return error("failed writing temporary file '%s': %s",
+						path, strerror(errno));
+	close(fd);

I just tested something like "gpg --verify - - < <filename>" and it worked...

builtin-verify-tag.c line 42

Quoted 6 lines
builtin-verify-tag.c new file +6 -0
+	/* find the length without signature */
+	len = 0;
+	while (len < size && prefixcmp(buf + len, PGP_SIGNATURE "\n")) {
+		eol = memchr(buf + len, '\n', size - len);
+		len += eol ? eol - (buf + len) + 1 : size - len;
+	}

How about this instead:

	const char *sig = strstr(buf, "\n" PGP_SIGNATURE "\n"));
	if (!sig)
		sig = buf + len;

Hmm?

But of course only if gpg does not grok "git cat-file tag <signed-tag> | gpg --verify - -".

builtin-verify-tag.c line 59

Quoted 1 line
builtin-verify-tag.c new file +1 -0
+	remove(path);

I think we use unlink() consistently, since we are sure it is no directory.

builtin-verify-tag.c line 81

Quoted 5 lines
builtin-verify-tag.c new file +5 -0
+	if (type <= OBJ_NONE)
+		return error("%s: bad object type.", name);
+	if (type != OBJ_TAG)
+		return error("%s: cannot verify a non-tag object of type %s.",
+				name, typename(type));

These two can be unified into one, right?

builtin-verify-tag.c line 102

Quoted 10 lines
builtin-verify-tag.c new file +10 -0
+int cmd_verify_tag(int argc, const char **argv, const char *prefix)
+{
+	int i, verbose = 0, had_error = 0;
+
+	git_config(git_default_config);
+
+	if (argc == 1)
+		usage(builtin_verify_tag_usage);
+
+	i = 1;

You can initialise i above.

builtin-verify-tag.c line 110

Quoted 8 lines
builtin-verify-tag.c new file +8 -0
+	if (!strcmp(argv[i], "-v") || !strcmp(argv[i], "--verbose")) {
+		verbose = 1;
+		i++;
+	}
+
+	signal(SIGPIPE, SIG_IGN);
+	while (i < argc)
+		if (verify_tag(argv[i++], verbose))

Heh. I thought that you only made this a separate function to help building it into builtin-tag...

That was quick!

Ciao,
Dscho

author
#2 lore raw

2007/7/23, Johannes Schindelin <[email protected]>:

Quoted 13 lines · Carlos Rica
Quoted 12 lines
builtin-verify-tag.c new file +12 -0
+static int run_gpg_verify(const char *buf, unsigned long size, int verbose)
+{
+     struct child_process gpg;
+     const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
+     char *path, *eol;
+     size_t len;
+     int fd, ret;
+
+     path = xstrdup(git_path("VTAG_TMP"));
+     fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+     if (fd < 0)
+             return error("could not create temporary file '%s': %s",
Quoted 8 lines

builtin-verify-tag.c line 35

Quoted 5 lines
builtin-verify-tag.c new file +5 -0
+                                             path, strerror(errno));
+     if (write_in_full(fd, buf, size) < 0)
+             return error("failed writing temporary file '%s': %s",
+                                             path, strerror(errno));
+     close(fd);

I just tested something like "gpg --verify - - < <filename>" and it worked...

I couldn't verify tags using "gpg --verify - - <file" as you pointed in your response.

In my tests, it only allowed me to run:
"gpg --verify file.asc" or
"gpg --verify <file.asc" or
"gpg --verify - <file.asc"
when file.asc was generated using the command "gpg --clearsign file", in which it inserts a header:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
<empty-line>

I even tested to insert the header myself after creating a signed file as git does:
cp file otherfile
gpg -bsa otherfile
...
cat otherfile.asc >>otherfile
( echo -e "-----BEGIN PGP SIGNED MESSAGE-----\nHash: SHA1\n" ; cat

otherfile ) \
    | gpg --verify

Note that sending them to "diff file.asc -" also shows different signatures, but anyway it also says "gpg: BAD signature from ...", the same message as in:
gpg --verify - - <otherfile

and different from the "gpg: Good signature from ..." when I run:
gpg --verify file.asc

#3 lore raw

Hi,

Quoted 24 lines · Carlos Rica

2007/7/23, Johannes Schindelin <[email protected]>:

Quoted 14 lines · Carlos Rica
Quoted 1 line

+static int run_gpg_verify(const char *buf, unsigned long size, int

verbose)

Quoted 11 lines
builtin-verify-tag.c new file +11 -0
+{
+     struct child_process gpg;
+     const char *args_gpg[] = {"gpg", "--verify", "FILE", "-", NULL};
+     char *path, *eol;
+     size_t len;
+     int fd, ret;
+
+     path = xstrdup(git_path("VTAG_TMP"));
+     fd = open(path, O_CREAT | O_TRUNC | O_WRONLY, 0600);
+     if (fd < 0)
+             return error("could not create temporary file '%s': %s",
Quoted 8 lines

builtin-verify-tag.c line 35

Quoted 5 lines
builtin-verify-tag.c new file +5 -0
+                                             path, strerror(errno));
+     if (write_in_full(fd, buf, size) < 0)
+             return error("failed writing temporary file '%s': %s",
+                                             path, strerror(errno));
+     close(fd);

I just tested something like "gpg --verify - - < <filename>" and it worked...

Quoted 2 lines

I couldn't verify tags using "gpg --verify - - <file" as you pointed in your response.

As discussed on IRC, my test did not use a detached signature, while tags _do_ use a detached signature.

Oh, well. I retract my suggestion. But I slightly disagree that having to write a temporary file is good for security, as the gpg man page suggests:

--verify [[sigfile] [signed-files]]
[...] To read the signed stuff from stdin, use - as the second
filename. For security reasons a detached signature cannot read the signed material from stdin without denoting it in the above way.

So we have to live, or die, with the temporary file.

Just one request: please use lock_file, to prevent multiple instances from getting confused... Even if that cannot be 100% secure: if you write to the file, and only then start gpg, there is still the odd chance for an attacker. Even if you created the file with 0600 (very wise, BTW).

Ciao,
Dscho

author
#4 lore raw

2007/7/23, Johannes Schindelin <[email protected]>:

Quoted 4 lines

Just one request: please use lock_file, to prevent multiple instances from getting confused... Even if that cannot be 100% secure: if you write to the file, and only then start gpg, there is still the odd chance for an attacker. Even if you created the file with 0600 (very wise, BTW).

Why not using also a random name for the temporary file? Something like "VTAG_TMP30946".

#5 lore raw

Hi,

Quoted 8 lines · Carlos Rica

2007/7/23, Johannes Schindelin <[email protected]>:

Quoted 4 lines

Just one request: please use lock_file, to prevent multiple instances from getting confused... Even if that cannot be 100% secure: if you write to the file, and only then start gpg, there is still the odd chance for an attacker. Even if you created the file with 0600 (very wise, BTW).

Why not using also a random name for the temporary file? Something like "VTAG_TMP30946".

Yes, makes sense. One call to mkstemp() away.

Ciao,
Dscho