threads / patch / 44261

patchcontrib: add credential helper for libsecret

Subject: [PATCH] contrib: add credential helper for libsecret

## tl;dr

13 messages between Oct 9, 2016 and Oct 11, 2016. Diffs are folded; open one to read it.

replies: 12people: 4as markdown or json

Mantas Mikulėnas· Oct 9, 2016, 12:34 UTC · lore

This is based on the existing gnome-keyring helper, but instead of libgnome-keyring (which was specific to GNOME and is deprecated), it uses libsecret which can support other implementations of XDG Secret Service API.

Passes t0303-credential-external.sh.
Signed-off-by: Mantas Mikulėnas <grawity@gmail.com>
---
 contrib/credential/libsecret/Makefile              |  25 ++
 .../libsecret/git-credential-libsecret.c           | 370 +++++++++++++++++++++
 2 files changed, 395 insertions(+)
 create mode 100644 contrib/credential/libsecret/Makefile
 create mode 100644 contrib/credential/libsecret/git-credential-libsecret.c
Show changes to 2 files +395 −0

contrib/credential/libsecret/Makefile, contrib/credential/libsecret/git-credential-libsecret.c

diff --git a/contrib/credential/libsecret/Makefile b/contrib/credential/libsecret/Makefile
new file mode 100644
index 000000000000..3e67552cc5b5
--- /dev/null
+++ b/contrib/credential/libsecret/Makefile
@@ -0,0 +1,25 @@
+MAIN:=git-credential-libsecret
+all:: $(MAIN)
+
+CC = gcc
+RM = rm -f
+CFLAGS = -g -O2 -Wall
+PKG_CONFIG = pkg-config
+
+-include ../../../config.mak.autogen
+-include ../../../config.mak
+
+INCS:=$(shell $(PKG_CONFIG) --cflags libsecret-1 glib-2.0)
+LIBS:=$(shell $(PKG_CONFIG) --libs libsecret-1 glib-2.0)
+
+SRCS:=$(MAIN).c
+OBJS:=$(SRCS:.c=.o)
+
+%.o: %.c
+	$(CC) $(CFLAGS) $(CPPFLAGS) $(INCS) -o $@ -c $<
+
+$(MAIN): $(OBJS)
+	$(CC) -o $@ $(LDFLAGS) $^ $(LIBS)
+
+clean:
+	@$(RM) $(MAIN) $(OBJS)
diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
new file mode 100644
index 000000000000..4c56979d8a08
--- /dev/null
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -0,0 +1,370 @@
+/*
+ * Copyright (C) 2011 John Szakmeister <john@szakmeister.net>
+ *               2012 Philipp A. Hartmann <pah@qo.cx>
+ *               2016 Mantas Mikulėnas <grawity@gmail.com>
+ *
+ *  This program is free software; you can redistribute it and/or modify
+ *  it under the terms of the GNU General Public License as published by
+ *  the Free Software Foundation; either version 2 of the License, or
+ *  (at your option) any later version.
+ *
+ *  This program is distributed in the hope that it will be useful,
+ *  but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *  GNU General Public License for more details.
+ *
+ *  You should have received a copy of the GNU General Public License
+ *  along with this program; if not, write to the Free Software
+ *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
+ */
+
+/*
+ * Credits:
+ * - GNOME Keyring API handling originally written by John Szakmeister
+ * - ported to credential helper API by Philipp A. Hartmann
+ */
+
+#include <stdio.h>
+#include <string.h>
+#include <stdlib.h>
+#include <glib.h>
+#include <libsecret/secret.h>
+
+/*
+ * This credential struct and API is simplified from git's credential.{h,c}
+ */
+struct credential {
+	char *protocol;
+	char *host;
+	unsigned short port;
+	char *path;
+	char *username;
+	char *password;
+};
+
+#define CREDENTIAL_INIT { NULL, NULL, 0, NULL, NULL, NULL }
+
+typedef int (*credential_op_cb)(struct credential *);
+
+struct credential_operation {
+	char *name;
+	credential_op_cb op;
+};
+
+#define CREDENTIAL_OP_END { NULL, NULL }
+
+/* ----------------- Secret Service functions ----------------- */
+
+static char *make_label(struct credential *c)
+{
+	if (c->port)
+		return g_strdup_printf("Git: %s://%s:%hu/%s",
+					c->protocol, c->host, c->port, c->path ? c->path : "");
+	else
+		return g_strdup_printf("Git: %s://%s/%s",
+					c->protocol, c->host, c->path ? c->path : "");
+}
+
+static GHashTable *make_attr_list(struct credential *c)
+{
+	GHashTable *al = g_hash_table_new_full(g_str_hash, g_str_equal, NULL, g_free);
+
+	if (c->username)
+		g_hash_table_insert(al, "user", g_strdup(c->username));
+	if (c->protocol)
+		g_hash_table_insert(al, "protocol", g_strdup(c->protocol));
+	if (c->host)
+		g_hash_table_insert(al, "server", g_strdup(c->host));
+	if (c->port)
+		g_hash_table_insert(al, "port", g_strdup_printf("%hu", c->port));
+	if (c->path)
+		g_hash_table_insert(al, "object", g_strdup(c->path));
+
+	return al;
+}
+
+static int keyring_get(struct credential *c)
+{
+	SecretService *service = NULL;
+	GHashTable *attributes = NULL;
+	GError *error = NULL;
+	GList *items = NULL;
+
+	if (!c->protocol || !(c->host || c->path))
+		return EXIT_FAILURE;
+
+	service = secret_service_get_sync(0, NULL, &error);
+	if (error != NULL) {
+		g_critical("could not connect to Secret Service: %s", error->message);
+		g_error_free(error);
+		return EXIT_FAILURE;
+	}
+
+	attributes = make_attr_list(c);
+	items = secret_service_search_sync(service,
+					   SECRET_SCHEMA_COMPAT_NETWORK,
+					   attributes,
+					   SECRET_SEARCH_LOAD_SECRETS,
+					   NULL,
+					   &error);
+	g_hash_table_unref(attributes);
+	if (error != NULL) {
+		g_critical("lookup failed: %s", error->message);
+		g_error_free(error);
+		return EXIT_FAILURE;
+	}
+
+	if (items != NULL) {
+		SecretItem *item;
+		SecretValue *secret;
+		const char *s;
+
+		item = items->data;
+		secret = secret_item_get_secret(item);
+		attributes = secret_item_get_attributes(item);
+
+		s = g_hash_table_lookup(attributes, "user");
+		if (s) {
+			g_free(c->username);
+			c->username = g_strdup(s);
+		}
+
+		s = secret_value_get_text(secret);
+		if (s) {
+			g_free(c->password);
+			c->password = g_strdup(s);
+		}
+
+		g_hash_table_unref(attributes);
+		secret_value_unref(secret);
+		g_list_free_full(items, g_object_unref);
+	}
+
+	return EXIT_SUCCESS;
+}
+
+
+static int keyring_store(struct credential *c)
+{
+	char *label = NULL;
+	GHashTable *attributes = NULL;
+	GError *error = NULL;
+
+	/*
+	 * Sanity check that what we are storing is actually sensible.
+	 * In particular, we can't make a URL without a protocol field.
+	 * Without either a host or pathname (depending on the scheme),
+	 * we have no primary key. And without a username and password,
+	 * we are not actually storing a credential.
+	 */
+	if (!c->protocol || !(c->host || c->path) ||
+	    !c->username || !c->password)
+		return EXIT_FAILURE;
+
+	label = make_label(c);
+	attributes = make_attr_list(c);
+	secret_password_storev_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+				    attributes,
+				    NULL,
+				    label,
+				    c->password,
+				    NULL,
+				    &error);
+	g_free(label);
+	g_hash_table_unref(attributes);
+
+	if (error != NULL) {
+		g_critical("store failed: %s", error->message);
+		g_error_free(error);
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
+
+static int keyring_erase(struct credential *c)
+{
+	GHashTable *attributes = NULL;
+	GError *error = NULL;
+
+	/*
+	 * Sanity check that we actually have something to match
+	 * against. The input we get is a restrictive pattern,
+	 * so technically a blank credential means "erase everything".
+	 * But it is too easy to accidentally send this, since it is equivalent
+	 * to empty input. So explicitly disallow it, and require that the
+	 * pattern have some actual content to match.
+	 */
+	if (!c->protocol && !c->host && !c->path && !c->username)
+		return EXIT_FAILURE;
+
+	attributes = make_attr_list(c);
+	secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
+				    attributes,
+				    NULL,
+				    &error);
+	g_hash_table_unref(attributes);
+
+	if (error != NULL) {
+		g_critical("erase failed: %s", error->message);
+		g_error_free(error);
+		return EXIT_FAILURE;
+	}
+
+	return EXIT_SUCCESS;
+}
+
+/*
+ * Table with helper operation callbacks, used by generic
+ * credential helper main function.
+ */
+static struct credential_operation const credential_helper_ops[] = {
+	{ "get",   keyring_get },
+	{ "store", keyring_store },
+	{ "erase", keyring_erase },
+	CREDENTIAL_OP_END
+};
+
+/* ------------------ credential functions ------------------ */
+
+static void credential_init(struct credential *c)
+{
+	memset(c, 0, sizeof(*c));
+}
+
+static void credential_clear(struct credential *c)
+{
+	g_free(c->protocol);
+	g_free(c->host);
+	g_free(c->path);
+	g_free(c->username);
+	g_free(c->password);
+
+	credential_init(c);
+}
+
+static int credential_read(struct credential *c)
+{
+	char *buf;
+	size_t line_len;
+	char *key;
+	char *value;
+
+	key = buf = g_malloc(1024);
+
+	while (fgets(buf, 1024, stdin)) {
+		line_len = strlen(buf);
+
+		if (line_len && buf[line_len-1] == '\n')
+			buf[--line_len] = '\0';
+
+		if (!line_len)
+			break;
+
+		value = strchr(buf, '=');
+		if (!value) {
+			g_warning("invalid credential line: %s", key);
+			g_free(buf);
+			return -1;
+		}
+		*value++ = '\0';
+
+		if (!strcmp(key, "protocol")) {
+			g_free(c->protocol);
+			c->protocol = g_strdup(value);
+		} else if (!strcmp(key, "host")) {
+			g_free(c->host);
+			c->host = g_strdup(value);
+			value = strrchr(c->host, ':');
+			if (value) {
+				*value++ = '\0';
+				c->port = atoi(value);
+			}
+		} else if (!strcmp(key, "path")) {
+			g_free(c->path);
+			c->path = g_strdup(value);
+		} else if (!strcmp(key, "username")) {
+			g_free(c->username);
+			c->username = g_strdup(value);
+		} else if (!strcmp(key, "password")) {
+			g_free(c->password);
+			c->password = g_strdup(value);
+			while (*value)
+				*value++ = '\0';
+		}
+		/*
+		 * Ignore other lines; we don't know what they mean, but
+		 * this future-proofs us when later versions of git do
+		 * learn new lines, and the helpers are updated to match.
+		 */
+	}
+
+	g_free(buf);
+
+	return 0;
+}
+
+static void credential_write_item(FILE *fp, const char *key, const char *value)
+{
+	if (!value)
+		return;
+	fprintf(fp, "%s=%s\n", key, value);
+}
+
+static void credential_write(const struct credential *c)
+{
+	/* only write username/password, if set */
+	credential_write_item(stdout, "username", c->username);
+	credential_write_item(stdout, "password", c->password);
+}
+
+static void usage(const char *name)
+{
+	struct credential_operation const *try_op = credential_helper_ops;
+	const char *basename = strrchr(name, '/');
+
+	basename = (basename) ? basename + 1 : name;
+	fprintf(stderr, "usage: %s <", basename);
+	while (try_op->name) {
+		fprintf(stderr, "%s", (try_op++)->name);
+		if (try_op->name)
+			fprintf(stderr, "%s", "|");
+	}
+	fprintf(stderr, "%s", ">\n");
+}
+
+int main(int argc, char *argv[])
+{
+	int ret = EXIT_SUCCESS;
+
+	struct credential_operation const *try_op = credential_helper_ops;
+	struct credential cred = CREDENTIAL_INIT;
+
+	if (!argv[1]) {
+		usage(argv[0]);
+		exit(EXIT_FAILURE);
+	}
+
+	g_set_application_name("Git Credential Helper");
+
+	/* lookup operation callback */
+	while (try_op->name && strcmp(argv[1], try_op->name))
+		try_op++;
+
+	/* unsupported operation given -- ignore silently */
+	if (!try_op->name || !try_op->op)
+		goto out;
+
+	ret = credential_read(&cred);
+	if (ret)
+		goto out;
+
+	/* perform credential operation */
+	ret = (*try_op->op)(&cred);
+
+	credential_write(&cred);
+
+out:
+	credential_clear(&cred);
+	return ret;
+}
-- 
2.10.0
Jeff King· Oct 10, 2016, 18:34 UTC · re: Mantas Mikulėnas · lore

Re: [PATCH] contrib: add credential helper for libsecret

On Sun, Oct 09, 2016 at 03:34:17PM +0300, Mantas Mikulėnas wrote:
> This is based on the existing gnome-keyring helper, but instead of
> libgnome-keyring (which was specific to GNOME and is deprecated), it
> uses libsecret which can support other implementations of XDG Secret
> Service API.

Thanks for working on this; I'm happy to see credential helpers for as many storage APIs as possible.

> Passes t0303-credential-external.sh.

Thank you for running that, as my first question would have been about its results. :)

I don't know much about the Secret Service API, nor do I run a desktop environment which provides the server side of it. But the code looks straightforward and reasonable, it passes t0303, and presumably it is working for you. So this seems worth merging to me.

-Peff
Dennis Kaarsemaker· Oct 10, 2016, 20:20 UTC · re: Mantas Mikulėnas · lore

Re: [PATCH] contrib: add credential helper for libsecret

On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
Show 6 quoted lines
> This is based on the existing gnome-keyring helper, but instead of
> libgnome-keyring (which was specific to GNOME and is deprecated), it
> uses libsecret which can support other implementations of XDG Secret
> Service API.
> 
> Passes t0303-credential-external.sh.
When setting credential.helper to this helper, I get the following output:

$ git clone https://private-repo-url-removed private Cloning into 'private'... /home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied

Looks suboptimal. Am I holding it wrong?
D.
Jeff King· Oct 10, 2016, 20:46 UTC · re: Dennis Kaarsemaker · lore

Re: [PATCH] contrib: add credential helper for libsecret

On Mon, Oct 10, 2016 at 10:20:50PM +0200, Dennis Kaarsemaker wrote:
Show 15 quoted lines
> On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
> > This is based on the existing gnome-keyring helper, but instead of
> > libgnome-keyring (which was specific to GNOME and is deprecated), it
> > uses libsecret which can support other implementations of XDG Secret
> > Service API.
> > 
> > Passes t0303-credential-external.sh.
> 
> When setting credential.helper to this helper, I get the following output:
> 
> $ git clone https://private-repo-url-removed private
> Cloning into 'private'...
> /home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied
> 
> Looks suboptimal. Am I holding it wrong?

That looks like a directory name in your error message. How did you set up credential.helper? I'd expect normal usage to be something like this:

  # do this once, or cp the binary into your $PATH
  PATH=$PATH:/home/dennis/code/git/contrib/credential/libsecret
  git config --global credential.helper libsecret
But if you don't want to put it in your PATH, then I think:
  git config --global credential.helper \
    '!/home/dennis/code/git/contrib/credential/git-credential-libsecret'
would work.
-Peff
Dennis Kaarsemaker· Oct 11, 2016, 15:01 UTC · re: Jeff King · lore

Re: [PATCH] contrib: add credential helper for libsecret

On Mon, 2016-10-10 at 16:46 -0400, Jeff King wrote:
Show 20 quoted lines
> On Mon, Oct 10, 2016 at 10:20:50PM +0200, Dennis Kaarsemaker wrote:
> 
> > On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
> > > This is based on the existing gnome-keyring helper, but instead of
> > > libgnome-keyring (which was specific to GNOME and is deprecated), it
> > > uses libsecret which can support other implementations of XDG Secret
> > > Service API.
> > > 
> > > Passes t0303-credential-external.sh.
> > 
> > When setting credential.helper to this helper, I get the following output:
> > 
> > $ git clone https://private-repo-url-removed private
> > Cloning into 'private'...
> > /home/dennis/code/git/contrib/credential/libsecret/ get: 1: /home/dennis/code/git/contrib/credential/libsecret/ get: /home/dennis/code/git/contrib/credential/libsecret/: Permission denied
> > 
> > Looks suboptimal. Am I holding it wrong?
> 
> That looks like a directory name in your error message. How did you set
> up credential.helper?
Doh.
I had done git config --global credential.helper ~/code/git/contrib/credential/libsecret/

After doing it properly (actual path to the binary), it works just fine. The error message above is slightly suboptimal, but there's no solution for pebkac.

D.
Dennis Kaarsemaker· Oct 11, 2016, 15:01 UTC · re: Mantas Mikulėnas · lore

Re: [PATCH] contrib: add credential helper for libsecret

On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
Show 5 quoted lines
> +		s = g_hash_table_lookup(attributes, "user");
> +		if (s) {
> +			g_free(c->username);
> +			c->username = g_strdup(s);
> +		}

This always overwrites c->username, the original gnome-keyring version only does that when the username isn't set. Other than that it looks good to me.

Reviewed-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
Tested-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
D.
Junio C Hamano· Oct 11, 2016, 19:36 UTC · re: Dennis Kaarsemaker · lore

Re: [PATCH] contrib: add credential helper for libsecret

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
Show 14 quoted lines
> On Sun, 2016-10-09 at 15:34 +0300, Mantas Mikulėnas wrote:
>
>> +		s = g_hash_table_lookup(attributes, "user");
>> +		if (s) {
>> +			g_free(c->username);
>> +			c->username = g_strdup(s);
>> +		}
>
> This always overwrites c->username, the original gnome-keyring version
> only does that when the username isn't set. Other than that it looks
> good to me.
>
> Reviewed-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
> Tested-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>

Thanks for a review. I'll wait until one of (1) a squashable patch to address the "we do not want unconditional overwrite" issue, (2) a reroll from Mantas to do the same, or (3) a counter-argument from somebody to explain why unconditional overwrite is a good idea here (but not in the original) appears.

Mantas Mikulėnas· Oct 11, 2016, 19:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] contrib: add credential helper for libsecret

On 2016-10-11 22:36, Junio C Hamano wrote:
Show 5 quoted lines
> Thanks for a review.  I'll wait until one of (1) a squashable patch
> to address the "we do not want unconditional overwrite" issue, (2) a
> reroll from Mantas to do the same, or (3) a counter-argument from
> somebody to explain why unconditional overwrite is a good idea here
> (but not in the original) appears.

I overlooked that. I can write a patch, but it shouldn't make any difference in practice – if c->username *was* set, then it would also get added to the search attribute list, therefore the search couldn't possibly return any results with a different username anyway.

-- 
Mantas Mikulėnas <grawity@gmail.com>
Mantas Mikulėnas· Oct 11, 2016, 19:59 UTC · re: Mantas Mikulėnas · lore

Re: [PATCH] contrib: add credential helper for libsecret

On 2016-10-11 22:48, Mantas Mikulėnas wrote:
Show 11 quoted lines
> On 2016-10-11 22:36, Junio C Hamano wrote:
>> Thanks for a review.  I'll wait until one of (1) a squashable patch
>> to address the "we do not want unconditional overwrite" issue, (2) a
>> reroll from Mantas to do the same, or (3) a counter-argument from
>> somebody to explain why unconditional overwrite is a good idea here
>> (but not in the original) appears.
> 
> I overlooked that. I can write a patch, but it shouldn't make any
> difference in practice – if c->username *was* set, then it would also
> get added to the search attribute list, therefore the search couldn't
> possibly return any results with a different username anyway.

On a second thought, it doesn't actually make sense _not_ to override the username. Let's say the search query *somehow* returned a different account than requested. With the original (gnome-keyring helper's) behavior, the final output would have the old account's username, but the new account's password – which has very little chance of working.

-- 
Mantas Mikulėnas <grawity@gmail.com>
Dennis Kaarsemaker· Oct 11, 2016, 20:09 UTC · re: Mantas Mikulėnas · lore

Re: [PATCH] contrib: add credential helper for libsecret

On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:
Show 12 quoted lines
> On 2016-10-11 22:36, Junio C Hamano wrote:
> > Thanks for a review.  I'll wait until one of (1) a squashable patch
> > to address the "we do not want unconditional overwrite" issue, (2) a
> > reroll from Mantas to do the same, or (3) a counter-argument from
> > somebody to explain why unconditional overwrite is a good idea here
> > (but not in the original) appears.
> 
> 
> I overlooked that. I can write a patch, but it shouldn't make any
> difference in practice – if c->username *was* set, then it would also
> get added to the search attribute list, therefore the search couldn't
> possibly return any results with a different username anyway.
Makes sense, so a (3) it is.
D.
Junio C Hamano· Oct 11, 2016, 20:13 UTC · re: Dennis Kaarsemaker · lore

Re: [PATCH] contrib: add credential helper for libsecret

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
Show 15 quoted lines
> On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:
>> On 2016-10-11 22:36, Junio C Hamano wrote:
>> > Thanks for a review.  I'll wait until one of (1) a squashable patch
>> > to address the "we do not want unconditional overwrite" issue, (2) a
>> > reroll from Mantas to do the same, or (3) a counter-argument from
>> > somebody to explain why unconditional overwrite is a good idea here
>> > (but not in the original) appears.
>> 
>> 
>> I overlooked that. I can write a patch, but it shouldn't make any
>> difference in practice – if c->username *was* set, then it would also
>> get added to the search attribute list, therefore the search couldn't
>> possibly return any results with a different username anyway.
>
> Makes sense, so a (3) it is.
So... does it mean the gnome-keyring one needs a bugfix?
Junio C Hamano· Oct 11, 2016, 20:19 UTC · re: Junio C Hamano · lore

Re: [PATCH] contrib: add credential helper for libsecret

Junio C Hamano <gitster@pobox.com> writes:
Show 19 quoted lines
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
>
>> On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:
>>> On 2016-10-11 22:36, Junio C Hamano wrote:
>>> > Thanks for a review.  I'll wait until one of (1) a squashable patch
>>> > to address the "we do not want unconditional overwrite" issue, (2) a
>>> > reroll from Mantas to do the same, or (3) a counter-argument from
>>> > somebody to explain why unconditional overwrite is a good idea here
>>> > (but not in the original) appears.
>>> 
>>> 
>>> I overlooked that. I can write a patch, but it shouldn't make any
>>> difference in practice – if c->username *was* set, then it would also
>>> get added to the search attribute list, therefore the search couldn't
>>> possibly return any results with a different username anyway.
>>
>> Makes sense, so a (3) it is.
>
> So... does it mean the gnome-keyring one needs a bugfix?

Just so there is no misunderstanding, updating (or not) gnome-keyring code is an unrelated issue.

I'll queue the patch under discussion in this thread, and if an update to gnome-keyring appears, that will be queued separately.

Thanks again, both of you.
Dennis Kaarsemaker· Oct 11, 2016, 20:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] contrib: add credential helper for libsecret

On Tue, 2016-10-11 at 13:13 -0700, Junio C Hamano wrote:
Show 22 quoted lines
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
> > On Tue, 2016-10-11 at 22:48 +0300, Mantas Mikulėnas wrote:
> > > On 2016-10-11 22:36, Junio C Hamano wrote:
> > > > Thanks for a review.  I'll wait until one of (1) a squashable patch
> > > > to address the "we do not want unconditional overwrite" issue, (2) a
> > > > reroll from Mantas to do the same, or (3) a counter-argument from
> > > > somebody to explain why unconditional overwrite is a good idea here
> > > > (but not in the original) appears.
> > > 
> > > 
> > > 
> > > I overlooked that. I can write a patch, but it shouldn't make any
> > > difference in practice – if c->username *was* set, then it would also
> > > get added to the search attribute list, therefore the search couldn't
> > > possibly return any results with a different username anyway.
> > 
> > 
> > Makes sense, so a (3) it is.
> 
> 
> So... does it mean the gnome-keyring one needs a bugfix?
I'd say both behaviours are correct.

When you do a search without a username, both helpers fill in the username returned by the actual credential storage. When you do a search with a username, the gnome-keyring helper won't overwrite the value passed in and the libsecret helper overwrites it with the same value, as the search can only return matches that have the same value.

D.

← back to recent threads