Skip to content

Conversation

@bk2204
Copy link
Member

@bk2204 bk2204 commented May 31, 2023

Git recently added a new field to the credential helper, wwwauth[], which may be repeated and includes all of the WWW-Authenticate headers so that the credential helper may choose an appropriate set of credentials and extract any sort of necessary data from the field (such as challenge).

In Git LFS, we also want to do this with the LFS-Authenticate headers as well, since those are often used for the same purpose, so include both these headers in that field when passing them to git credential fill.

Note that git credential fill only honours this value and passes it to the credential helper in Git 2.41 and newer (including the latest HEAD). However, just to be safe, let's add an undocumented and experimental option (credential.*.skipwwwauth) that users can use to control this, which we can remove in a few releases if it turns out it's not needed. Similarly, skip our new tests if we have an older version of Git where this doesn't work, since they'll otherwise fail.

In addition, perform some preparatory work to allow multiple values for a key, which we'll use in the second commit.

Fixes: #5222

@bk2204 bk2204 force-pushed the credential-helper-v2 branch from 0c5d118 to 17c256a Compare May 31, 2023 14:36
@bk2204 bk2204 marked this pull request as ready for review June 1, 2023 13:57
@bk2204 bk2204 requested a review from a team as a code owner June 1, 2023 13:57
bk2204 added 2 commits June 2, 2023 13:25
In the upcoming changes to the credential helper protocol, we'll be able
to specify multiple values for the same key.  In order to make this
work, let's make the credential structure able to handle multiple
values.

Note that if there are multiple values, we'll only use the first one for
most keys.  That simplifies the logic and allows us to gracefully handle
future extensions to the protocol.
Git recently added a new field to the credential helper, `wwwauth[]`,
which may be repeated and includes all of the `WWW-Authenticate` headers
so that the credential helper may choose an appropriate set of
credentials and extract any sort of necessary data from the field (such
as challenge).

In Git LFS, we also want to do this with the `LFS-Authenticate` headers
as well, since those are often used for the same purpose, so include
both these headers in that field when passing them to `git credential
fill`.

Note that `git credential fill` only honours this value and passes it to
the credential helper in Git 2.41 and newer (including the latest
`HEAD`).  However, just to be safe, let's add an undocumented and
experimental option (`credential.*.skipwwwauth`) that users can use to
control this, which we can remove in a few releases if it turns out it's
not needed.  Similarly, skip our new tests if we have an older version
of Git where this doesn't work, since they'll otherwise fail.
@bk2204 bk2204 force-pushed the credential-helper-v2 branch from 17c256a to badde87 Compare June 2, 2023 13:29
@bk2204 bk2204 merged commit 29dc7cc into git-lfs:main Jun 5, 2023
@bk2204 bk2204 deleted the credential-helper-v2 branch June 5, 2023 14:18
chrisd8088 added a commit that referenced this pull request Jan 14, 2025
The Creds structure in our "creds" package was first introduced in
commit e8de7a3, where it was part of
a different package in an early pre-alpha version of the Git LFS client.
A single method was defined for the structure in that commit, the
Buffer() method, which formatted lines of key/value pairs to be sent
to the git-credential(1) command.

In commit 3a271b1 of PR #1839 this
method was converted into the current bufferCreds() function, a change
made to avoid exporting the method out of the "lfsapi" package, where
the structure was defined at that time.

We next moved the Creds structure (which was then a simple map of
strings to strings) and its one function into the current "creds"
package in commit c752dcd of PR #3270.
Later, in commits 5d5f90e of PR #5381
and 6783078 of PR #5803 we revised the
Creds structure so its map contains arrays of strings, and added the
IsMultistage() method, so we again have a single method defined for
this structure.

In subsequent commits in this PR we will refactor the bufferCreds()
function to accept and return additional values.  As a first step, in
order to keep our code as consistent as possible, we convert it back into
a method, as it was originally defined, except with the lowercase name of
buffer() so it is not exported outside of the "creds" package.  This
retains the intent of commit 3a271b1
from PR #1839, but uses a naming pattern that is more idiomatic for the
Go language.

As well, we add a test function named TestCredsBufferFormat() to the
accompanying creds/creds_test.go source file.  This test function simply
verifies the behaviour of the former bufferCreds() function that we have
now renamed to the buffer() method.  To permit comparison of the contents
of the method's output buffer with the lines of key/value pairs expected
by the test function, we also add an assertCredsLinesMatch() helper
function which splits and sorts the lines in the buffer and compares
them with the sorted set of expected output lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Improve credential manager handling

2 participants