Message ID | pull.1352.v3.git.1667426969.gitgitgadget@gmail.com (mailing list archive) |
---|---|
Headers | show |
Series | Enhance credential helper protocol to include auth headers | expand |
On Wed, 2 Nov 2022 at 22:09, Matthew John Cheetham via GitGitGadget <gitgitgadget@gmail.com> wrote: > > `authtype`:: > > Indicates the type of authentication scheme that should be used by Git. > Credential helpers may reply to a request from Git with this attribute, > such that subsequent authenticated requests include the correct > `Authorization` header. > If this attribute is not present, the default value is "Basic". > Known values include "Basic", "Digest", and "Bearer". > If an unknown value is provided, this is taken as the authentication > scheme for the `Authorization` header, and the `password` field is > used as the raw unencoded authorization parameters of the same header. Do you have an example using authtype=Digest? Would the helper populate the password field with the user's verbatim password or the Digest challenge response? Put another way, is the Digest challenge-response logic in Git (libcurl) or the helper? https://www.rfc-editor.org/rfc/rfc7616#section-3.4
On 11/2/22 6:09 PM, Matthew John Cheetham via GitGitGadget wrote: > Following from my original RFC submission [0], this submission is considered > ready for full review. This patch series is now based on top of current > master (9c32cfb49c60fa8173b9666db02efe3b45a8522f) that includes my now > separately submitted patches [1] to fix up the other credential helpers' > behaviour. > Updates in v3 > ============= > > * Split final patch that added the test-http-server in to several, easier > to review patches. > > * Updated wording in git-credential.txt to clarify which side of the > credential helper protocol is sending/receiving the new wwwauth and > authtype attributes. You also updated some commit messages based on v2 feedback. Thanks! The commit splitting you did in this version is greatly appreciated. I found this version to be in good shape. It's a solid foundation to build upon (if any future work is necessary). Thanks, -Stolee
Hi Matthew! We covered this series in Review Club. As usual, participants will send their own feedback on this thread, but you may also find the meeting notes handy: https://docs.google.com/document/d/14L8BAumGTpsXpjDY8VzZ4rRtpAjuGrFSRqn3stCuS_w/edit?pli=1# "Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com> writes: > Background > ========== > > [...] > > Limitations > =========== > > [...] > > Goals > ===== > > [...] > > Design Principles > ================= > > [...] Thanks for the well-written cover letter! I suspect that not many folks are familiar with the history and workings of credential helpers, the current state of auth and how credential helper limitations create challenges for auth. I've learned a lot reading this, and it makes the motivations of this series clear :) > Proposed Changes > ================ > > 1. Teach Git to read HTTP response headers, specifically the standard > WWW-Authenticate (RFC 7235 Section 4.1) headers. > > 2. Teach Git to include extra information about HTTP responses that require > authentication when calling credential helpers. Specifically the > WWW-Authenticate header information. > > Because the extra information forms an ordered list, and the existing > credential helper I/O format only provides for simple key=value pairs, > we introduce a new convention for transmitting an ordered list of > values. Key names that are suffixed with a C-style array syntax should > have values considered to form an order list, i.e. key[]=value, where > the order of the key=value pairs in the stream specifies the order. > > For the WWW-Authenticate header values we opt to use the key wwwauth[]. > > 3. Teach Git to specify authentication schemes other than Basic in > subsequent HTTP requests based on credential helper responses. > From a reading of this section + the subject line, it's not immediately obvious that 3. also requires extending the credential helper protocol to include the "authtype" field. IMO it's significant enough to warrant an explicit call-out.
"Matthew John Cheetham via GitGitGadget" <gitgitgadget@gmail.com> writes: > Testing these new additions, I introduce a new test helper test-http-server > that acts as a frontend to git-http-backend; a mini HTTP server based > heavily on git-daemon, with simple authentication configurable by command > line args. I did not try to figure out the reason but the topic with its tests seem to break in 'seen' the linux-cmake-ctest CI job. https://github.com/git/git/actions/runs/3562942886/jobs/5985179202 but the same test does not break under usual "make test". Can people who are interested in the cmake-ctest stuff take a look? It is tempting to eject the ab/cmake-nix-and-ci topic that is already in 'next', under the theory that what that topic does to the tests "works" for some tests but not for others, and this topic is an unfortunate collateral damage whose tests weren't something the other topic did not support well. If the cmake-ctest stuff is in such a shape, then it may have been a bit premature to merge it down. Thanks.
On 2022-11-09 15:06, Glen Choo wrote: >> Proposed Changes >> ================ >> >> 1. Teach Git to read HTTP response headers, specifically the standard >> WWW-Authenticate (RFC 7235 Section 4.1) headers. >> >> 2. Teach Git to include extra information about HTTP responses that require >> authentication when calling credential helpers. Specifically the >> WWW-Authenticate header information. >> >> Because the extra information forms an ordered list, and the existing >> credential helper I/O format only provides for simple key=value pairs, >> we introduce a new convention for transmitting an ordered list of >> values. Key names that are suffixed with a C-style array syntax should >> have values considered to form an order list, i.e. key[]=value, where >> the order of the key=value pairs in the stream specifies the order. >> >> For the WWW-Authenticate header values we opt to use the key wwwauth[]. >> >> 3. Teach Git to specify authentication schemes other than Basic in >> subsequent HTTP requests based on credential helper responses. >> > > From a reading of this section + the subject line, it's not immediately > obvious that 3. also requires extending the credential helper protocol > to include the "authtype" field. IMO it's significant enough to warrant > an explicit call-out. After some consideration I've decided to split out #3 here to a future patch series. Parts 1 and 2 surround Git to credential helper contextual information which is still useful in it's own right. Part 3 should really be expanded here to better cover and explain the reverse helper-to-Git direction, whereby helpers can modify Git's response headers to the remote. With 1+2 most of the benefits of having an enlightened helper understand the auth challenge, and intelligently select identities is still possible. Remotes just need to continue to extract tokens from the basic Authorization header as they do today until then. Thanks, Matthew
On 2022-11-03 12:00, M Hickford wrote: > On Wed, 2 Nov 2022 at 22:09, Matthew John Cheetham via GitGitGadget > <gitgitgadget@gmail.com> wrote: >> >> `authtype`:: >> >> Indicates the type of authentication scheme that should be used by Git. >> Credential helpers may reply to a request from Git with this attribute, >> such that subsequent authenticated requests include the correct >> `Authorization` header. >> If this attribute is not present, the default value is "Basic". >> Known values include "Basic", "Digest", and "Bearer". >> If an unknown value is provided, this is taken as the authentication >> scheme for the `Authorization` header, and the `password` field is >> used as the raw unencoded authorization parameters of the same header. > > Do you have an example using authtype=Digest? Would the helper > populate the password field with the user's verbatim password or the > Digest challenge response? Put another way, is the Digest > challenge-response logic in Git (libcurl) or the helper? > > https://www.rfc-editor.org/rfc/rfc7616#section-3.4 Digest should be handled by libcurl, but you've spotted that I missed configuring libcurl here to select digest over basic for a returned username and password. You may have noticed I've dropped these `authtype`/response config patches from the latest iteration (v4) as I intend to expand this part in a separate future series. I'll be sure to specifically test and handle digest here! Thanks for spotting :) Thanks, Matthew