Skip to contentSkip to conversation

contrib: add credential helper for libsecret

PATCH In review Mantas Mikulėnas 4 participants 13 messages Oct 9, 2016 – Oct 11, 2016

lore
Share & tools
lore

Mantas Mikulėnas

author
#0 lore raw

PATCH 1/1 contrib: add credential helper for libsecret

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 <[email protected]>
---
 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
contrib/credential/libsecret/Makefile new file +25 -0
new file mode 100644
index 000000000000..3e67552cc5b5
@@ -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)
contrib/credential/libsecret/git-credential-libsecret.c new file +370 -0

370 lines not on this page · raw message

Signature · 1 line
2.10.0
#1 lore raw
Quoted 4 lines · Mantas Mikulėnas

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.

Quoted 1 line

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

#2 lore raw
In reply to Mantas Mikulėnas
Quoted 6 lines · Mantas Mikulėnas

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.

#3 lore raw
Quoted 8 lines · Dennis Kaarsemaker
Quoted 6 lines · Mantas Mikulėnas

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.

Quoted 7 lines

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

#4 lore raw
Quoted 12 lines · Jeff King
Quoted 10 lines · Dennis Kaarsemaker
Quoted 6 lines · Mantas Mikulėnas

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:

Quoted 8 lines
Quoted 5 lines
$ 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.

Reviewed-by Tested-by on v1 1/1
#5 lore raw
In reply to Mantas Mikulėnas

contrib/credential/libsecret/git-credential-libsecret.c line 130

Quoted 5 lines · Mantas Mikulėnas
contrib/credential/libsecret/git-credential-libsecret.c new file +5 -0
+		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 <[email protected]>
Tested-by Dennis Kaarsemaker <[email protected]>

D.

#6 lore raw
Quoted 12 lines · Dennis Kaarsemaker

contrib/credential/libsecret/git-credential-libsecret.c line 130

Quoted 5 lines · Mantas Mikulėnas
contrib/credential/libsecret/git-credential-libsecret.c new file +5 -0
+		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.

Quoted 2 lines
Reviewed-by Dennis Kaarsemaker <[email protected]>
Tested-by Dennis Kaarsemaker <[email protected]>

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.

author
#7 lore raw
Quoted 5 lines · Junio C Hamano

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.

Signature · 1 line
Mantas Mikulėnas <[email protected]>
author
#8 lore raw
Quoted 11 lines · Mantas Mikulėnas
Quoted 5 lines · Junio C Hamano

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.

Signature · 1 line
Mantas Mikulėnas <[email protected]>
#9 lore raw
In reply to Mantas Mikulėnas
Quoted 8 lines · Mantas Mikulėnas
Quoted 5 lines · Junio C Hamano

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.

Quoted 4 lines

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.

#10 lore raw
Quoted 9 lines · Dennis Kaarsemaker
Quoted 8 lines · Mantas Mikulėnas
Quoted 5 lines · Junio C Hamano

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.

Quoted 6 lines
Quoted 4 lines

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?

#11 lore raw
Quoted 11 lines · Junio C Hamano
Quoted 9 lines · Dennis Kaarsemaker
Quoted 8 lines · Mantas Mikulėnas
Quoted 5 lines · Junio C Hamano

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.

Quoted 8 lines
Quoted 6 lines
Quoted 4 lines

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.

#12 lore raw
In reply to Junio C Hamano
Quoted 18 lines · Junio C Hamano
Quoted 16 lines · Dennis Kaarsemaker
Quoted 13 lines · Mantas Mikulėnas
Quoted 5 lines · Junio C Hamano

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.

Quoted 4 lines
Quoted 1 line

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.