diff mbox series

[v2] credential/libsecret: erase matching creds only

Message ID pull.1527.v2.git.git.1690387585634.gitgitgadget@gmail.com (mailing list archive)
State Superseded
Headers show
Series [v2] credential/libsecret: erase matching creds only | expand

Commit Message

M Hickford July 26, 2023, 4:06 p.m. UTC 
From: M Hickford <mirth.hickford@gmail.com>

The credential erase request typically includes protocol, host, username
and password.

credential-libsecret erases a stored credential if it matches protocol,
host and username, regardless of password.

This is confusing in the case the stored password differs from that
in the request. This case can occur when multiple credential helpers are
configured.

Only erase credential if stored password matches request (or request
omits password).

This fixes test "helper ... does not erase a password distinct from
input" introduced in aeb21ce22e (credential: avoid erasing distinct
password, 2023-06-13)

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    credential/libsecret: erase more carefully
    
    Patch v2 has a more detailed commit message following
    https://lore.kernel.org/git/xmqqpmax5c4v.fsf@gitster.g/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1527%2Fhickford%2Ffix-libsecret-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1527/hickford/fix-libsecret-v2
Pull-Request: https://github.com/git/git/pull/1527

Range-diff vs v1:

 1:  dcc429c1afc ! 1:  b5d5c3159ad credential/libsecret: erase matching creds only
     @@ Metadata
       ## Commit message ##
          credential/libsecret: erase matching creds only
      
     -    Fix test "helper ... does not erase a password distinct from input"
     -    introduced in aeb21ce22e (credential: avoid erasing distinct password,
     -    2023-06-13)
     +    The credential erase request typically includes protocol, host, username
     +    and password.
     +
     +    credential-libsecret erases a stored credential if it matches protocol,
     +    host and username, regardless of password.
     +
     +    This is confusing in the case the stored password differs from that
     +    in the request. This case can occur when multiple credential helpers are
     +    configured.
     +
     +    Only erase credential if stored password matches request (or request
     +    omits password).
     +
     +    This fixes test "helper ... does not erase a password distinct from
     +    input" introduced in aeb21ce22e (credential: avoid erasing distinct
     +    password, 2023-06-13)
      
          Signed-off-by: M Hickford <mirth.hickford@gmail.com>
      


 .../libsecret/git-credential-libsecret.c        | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)


base-commit: a80be152923a46f04a06bade7bcc72870e46ca09

Comments

Junio C Hamano July 26, 2023, 5:15 p.m. UTC | #1 
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> The credential erase request typically includes protocol, host, username
> and password.
>
> credential-libsecret erases a stored credential if it matches protocol,
> host and username, regardless of password.
>
> This is confusing in the case the stored password differs from that
> in the request. This case can occur when multiple credential helpers are
> configured.
>
> Only erase credential if stored password matches request (or request
> omits password).

This is much better.

> This fixes test "helper ... does not erase a password distinct from
> input" introduced in aeb21ce22e (credential: avoid erasing distinct
> password, 2023-06-13)

This was still confusing for a patch that does not touch anything in
t/, but after re-reading aeb21ce22e and the above a few times, I
think I get it.  Adding the following

	, when t0303 is run with GIT_TEST_CREDENTIAL_HELPER set to
	"libsecret".

at the end may help, but perhaps it is too obvious for folks who are
ready to actually review this change---presumably they are familiar
with how t0303 is to be used and read that without being told from
the context?

Thanks.
M Hickford July 26, 2023, 7:44 p.m. UTC | #2 
On Wed, 26 Jul 2023 at 18:15, Junio C Hamano <gitster@pobox.com> wrote:
> This was still confusing for a patch that does not touch anything in
> t/, but after re-reading aeb21ce22e and the above a few times, I
> think I get it.  Adding the following
>
>         , when t0303 is run with GIT_TEST_CREDENTIAL_HELPER set to
>         "libsecret".
>
> at the end may help, but perhaps it is too obvious for folks who are
> ready to actually review this change---presumably they are familiar
> with how t0303 is to be used and read that without being told from
> the context?

Good idea, the clearer the better. I'll add this in patch v3.
diff mbox series

Patch

diff --git a/contrib/credential/libsecret/git-credential-libsecret.c b/contrib/credential/libsecret/git-credential-libsecret.c
index ef681f29d5b..9110714601e 100644
--- a/contrib/credential/libsecret/git-credential-libsecret.c
+++ b/contrib/credential/libsecret/git-credential-libsecret.c
@@ -52,6 +52,8 @@  struct credential_operation {
 
 #define CREDENTIAL_OP_END { NULL, NULL }
 
+static void credential_clear(struct credential *c);
+
 /* ----------------- Secret Service functions ----------------- */
 
 static char *make_label(struct credential *c)
@@ -185,6 +187,7 @@  static int keyring_erase(struct credential *c)
 {
 	GHashTable *attributes = NULL;
 	GError *error = NULL;
+	struct credential existing = CREDENTIAL_INIT;
 
 	/*
 	 * Sanity check that we actually have something to match
@@ -197,6 +200,20 @@  static int keyring_erase(struct credential *c)
 	if (!c->protocol && !c->host && !c->path && !c->username)
 		return EXIT_FAILURE;
 
+	if (c->password) {
+		existing.host = g_strdup(c->host);
+		existing.path = g_strdup(c->path);
+		existing.port = c->port;
+		existing.protocol = g_strdup(c->protocol);
+		existing.username = g_strdup(c->username);
+		keyring_get(&existing);
+		if (existing.password && strcmp(c->password, existing.password)) {
+			credential_clear(&existing);
+			return EXIT_SUCCESS;
+		}
+		credential_clear(&existing);
+	}
+
 	attributes = make_attr_list(c);
 	secret_password_clearv_sync(SECRET_SCHEMA_COMPAT_NETWORK,
 				    attributes,