git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
@ 2022-09-13 19:25 Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 1/8] wincred: ignore unknown lines (do not die) Matthew John Cheetham via GitGitGadget
                   ` (9 more replies)
  0 siblings, 10 replies; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham

Hello! I have an RFC to update the existing credential helper design in
order to allow for some new scenarios, and future evolution of auth methods
that Git hosts may wish to provide. I outline the background, summary of
changes and some challenges below. I also attach a series of patches to
illustrate the design proposal.

One missing element from the patches are extensive tests of the new
behaviour. It appears existing tests focus either on the credential helper
protocol/format, or rely on testing basic authentication only via an Apache
webserver. In order to have a full end to end test coverage of these new
features it make be that we need a more comprehensive test bed to mock these
more nuanced authentication methods. I lean on the experts on the list for
advice here.


Background
==========

Git uses a variety of protocols [1]: local, Smart HTTP, Dumb HTTP, SSH, and
Git. Here I focus on the Smart HTTP protocol, and attempt to enhance the
authentication capabilities of this protocol to address limitations (see
below).

The Smart HTTP protocol in Git supports a few different types of HTTP
authentication - Basic and Digest (RFC 2617) [2], and Negotiate (RFC 2478)
[3]. Git uses a extensible model where credential helpers can provide
credentials for protocols [4]. Several helpers support alternatives such as
OAuth authentication (RFC 6749) [5], but this is typically done as an
extension. For example, a helper might use basic auth and set the password
to an OAuth Bearer access token. Git uses standard input and output to
communicate with credential helpers.

After a HTTP 401 response, Git would call a credential helper with the
following over standard input:

protocol=https
host=example.com


And then a credential helper would return over standard output:

protocol=https
host=example.com
username=bob@id.example.com
password=<BEARER-TOKEN>


Git then the following request to the remote, including the standard HTTP
Authorization header (RFC 7235 Section 4.2) [6]:

GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: Basic base64(bob@id.example.com:<BEARER-TOKEN>)


Credential helpers are encouraged (see gitcredentials.txt) to return the
minimum information necessary.


Limitations
===========

Because this credential model was built mostly for password based
authentication systems, it's somewhat limited. In particular:

 1. To generate valid credentials, additional information about the request
    (or indeed the requestee and their device) may be required. For example,
    OAuth is based around scopes. A scope, like "git.read", might be
    required to read data from the remote. However, the remote cannot tell
    the credential helper what scope is required for this request.

 2. This system is not fully extensible. Each time a new type of
    authentication (like OAuth Bearer) is invented, Git needs updates before
    credential helpers can take advantage of it (or leverage a new
    capability in libcurl).


Goals
=====

 * As a user with multiple federated cloud identities:
   
   * Reach out to a remote and have my credential helper automatically
     prompt me for the correct identity.
   * Leverage existing authentication systems built-in to many operating
     systems and devices to boost security and reduce reliance on passwords.

 * As a Git host and/or cloud identity provider:
   
   * Leverage newest identity standards, enhancements, and threat
     mitigations - all without updating Git.
   * Enforce security policies (like requiring two-factor authentication)
     dynamically.
   * Allow integration with third party standard based identity providers in
     enterprises allowing customers to have a single plane of control for
     critical identities with access to source code.


Design Principles
=================

 * Use the existing infrastructure. Git credential helpers are an
   already-working model.
 * Follow widely-adopted time-proven open standards, avoid net new ideas in
   the authentication space.
 * Minimize knowledge of authentication in Git; maintain modularity and
   extensibility.


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[n]=value, where n
    is a zero based index of the values.
    
    For the WWW-Authenticate header values we opt to use the key wwwauth[n].

 3. Teach Git to specify authentication schemes other than Basic in
    subsequent HTTP requests based on credential helper responses.


Handling the WWW-Authenticate header in detail
==============================================

RFC 6750 [7] envisions that OAuth Bearer resource servers would give
responses that include WWW-Authenticate headers, for example:

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"


Specifically, a WWW-Authenticate header consists of a scheme and arbitrary
attributes, depending on the scheme. This pattern enables generic OAuth or
OpenID Connect [8] authorities. Note that it is possible to have several
WWW-Authenticate challenges in a response.

First Git attempts to make a request, unauthenticated, which fails with a
401 response and includes WWW-Authenticate header(s).

Next, Git invokes a credential helper which may prompt the user. If the user
approves, a credential helper can generate a token (or any auth challenge
response) to be used for that request.

For example: with a remote that supports bearer tokens from an OpenID
Connect [8] authority, a credential helper can use OpenID Connect's
Discovery [9] and Dynamic Client Registration [9] to register a client and
make a request with the correct permissions to access the remote. In this
manner, a user can be dynamically sent to the right federated identity
provider for a remote without any up-front configuration or manual
processes.

Following from the principle of keeping authentication knowledge in Git to a
minimum, we modify Git to add all WWW-Authenticate values to the credential
helper call.

Git sends over standard input:

protocol=https
host=example.com
wwwauth[0]=Bearer realm="login.example", scope="git.readwrite"
wwwauth[1]=Basic realm="login.example"


A credential helper that understands the extra wwwauth[n] property can
decide on the "best" or correct authentication scheme, generate credentials
for the request, and interact with the user.

The credential helper would then return over standard output:

protocol=https
host=example.com
path=foo.git
username=bob@identity.example
password=<BEARER-TOKEN>


Note that WWW-Authenticate supports multiple challenges, either in one
header:

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"


or in multiple headers:

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Bearer realm="login.example", scope="git.readwrite"
WWW-Authenticate: Basic realm="login.example"


These have equivalent meaning (RFC 2616 Section 4.2 [11]). To simplify the
implementation, Git will not merge or split up any of these WWW-Authenticate
headers, and instead pass each header line as one credential helper
property. The credential helper is responsible for splitting, merging, and
otherwise parsing these header values.

An alternative option to sending the header fields individually would be to
merge the header values in to one key=value property, for example:

...
wwwauth=Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"



Future flexibility
==================

By allowing the credential helpers decide the best authentication scheme, we
can allow the remote Git server to both offer new schemes (or remove old
ones) that enlightened credential helpers could take immediate advantage of,
and to use credentials that are much more tightly scoped and bound to the
specific request.

For example imagine a new "FooBar" authentication scheme that is surfaced in
the following response:

HTTP/1.1 401 Unauthorized
WWW-Authenticate: FooBar realm="login.example", algs="ES256 PS256"


With support for arbitrary authentication schemes, Git would call credential
helpers with the following over standard input:

protocol=https
host=example.com
wwwauth[0]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123"


And then an enlightened credential helper would return over standard output:

protocol=https
host=example.com
authtype=FooBar
username=bob@id.example.com
password=<FooBar credential>


Git would be expected to attach this authorization header to the next
request:

GET /info/refs?service=git-upload-pack HTTP/1.1
Host: git.example
Git-Protocol: version=2
Authorization: FooBar <FooBar credential>



Should Git not control the set of authentication schemes?
=========================================================

One concern that the reader may have regarding these changes is in allowing
helpers to select the authentication mechanism to use, it may be possible
that a weaker form of authentication is used.

Take for example a Git remote server that responds with the following
authentication schemes:

HTTP/1.1 401 Unauthorized
WWW-Authenticate: Negotiate ...
WWW-Authenticate: Basic ...


Today Git (and libcurl) prefer to Negotiate over Basic authentication [12].
If a helper responded with authtype=basic Git would now be using a "less
secure" mechanism.

The reason we still propose the credential helper decide on the
authentication scheme is that Git is not the best placed entity to decide
what type of authentication should be used for a particular request (see
Design Principle 3).

OAuth Bearer tokens are often bundled in Basic Authorization headers [13],
but given that the tokens are/can be short-lived and have a highly scoped
set of permissions, this solution could be argued as being more secure than
something like NTLM [14]. Similarly, the user may wish to be consulted on
selecting a particular user account, or directly selecting an authentication
mechanism for a request that otherwise they would not be able to use.

Also, as new authentication protocols appear Git does not need to be
modified or updated for the user to take advantage of them; the credential
helpers take on the responsibility of learning and selecting the "best"
option.


Why not SSH?
============

There's nothing wrong with SSH. However, Git's Smart HTTP transport is
widely used, often with OAuth Bearer tokens. Git's Smart HTTP transport
sometimes requires less client setup than SSH transport, and works in
environments when SSH ports may be blocked. As long as Git supports HTTP
transport, it should support common and popular HTTP authentication methods.


References
==========

 * [1] Git on the Server - The Protocols
   https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols

 * [2] HTTP Authentication: Basic and Digest Access Authentication
   https://datatracker.ietf.org/doc/html/rfc2617

 * [3] The Simple and Protected GSS-API Negotiation Mechanism
   https://datatracker.ietf.org/doc/html/rfc2478

 * [4] Git Credentials - Custom Helpers
   https://git-scm.com/docs/gitcredentials#_custom_helpers

 * [5] The OAuth 2.0 Authorization Framework
   https://datatracker.ietf.org/doc/html/rfc6749

 * [6] Hypertext Transfer Protocol (HTTP/1.1): Authentication
   https://datatracker.ietf.org/doc/html/rfc7235

 * [7] The OAuth 2.0 Authorization Framework: Bearer Token Usage
   https://datatracker.ietf.org/doc/html/rfc6750

 * [8] OpenID Connect Core 1.0
   https://openid.net/specs/openid-connect-core-1_0.html

 * [9] OpenID Connect Discovery 1.0
   https://openid.net/specs/openid-connect-discovery-1_0.html

 * [10] OpenID Connect Dynamic Client Registration 1.0
   https://openid.net/specs/openid-connect-registration-1_0.html

 * [11] Hypertext Transfer Protocol (HTTP/1.1)
   https://datatracker.ietf.org/doc/html/rfc2616

 * [12] libcurl http.c pickoneauth Function
   https://github.com/curl/curl/blob/c495dcd02e885fc3f35164b1c3c5f72fa4b60c46/lib/http.c#L381-L416

 * [13] Git Credential Manager GitHub Host Provider (using PAT as password)
   https://github.com/GitCredentialManager/git-credential-manager/blob/f77b766f6875b90251249f2aa1702b921309cf00/src/shared/GitHub/GitHubHostProvider.cs#L157

 * [14] NT LAN Manager (NTLM) Authentication Protocol
   https://docs.microsoft.com/en-us/openspecs/windows_protocols/ms-nlmp/b38c36ed-2804-4868-a9ff-8dd3182128e4

Matthew John Cheetham (8):
  wincred: ignore unknown lines (do not die)
  netrc: ignore unknown lines (do not die)
  osxkeychain: clarify that we ignore unknown lines
  http: read HTTP WWW-Authenticate response headers
  credential: add WWW-Authenticate header to cred requests
  http: store all request headers on active_request_slot
  http: move proactive auth to first slot creation
  http: set specific auth scheme depending on credential

 Documentation/git-credential.txt              |  18 ++
 .../netrc/git-credential-netrc.perl           |   5 +-
 .../osxkeychain/git-credential-osxkeychain.c  |   5 +
 .../wincred/git-credential-wincred.c          |   7 +-
 credential.c                                  |  18 ++
 credential.h                                  |  11 +
 git-curl-compat.h                             |   7 +
 http-push.c                                   | 103 ++++-----
 http-walker.c                                 |   2 +-
 http.c                                        | 199 +++++++++++++-----
 http.h                                        |   4 +-
 remote-curl.c                                 |  36 ++--
 t/lib-httpd/apache.conf                       |  13 ++
 t/t5551-http-fetch-smart.sh                   |  46 ++++
 14 files changed, 335 insertions(+), 139 deletions(-)


base-commit: dd3f6c4cae7e3b15ce984dce8593ff7569650e24
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1352%2Fmjcheetham%2Femu-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1352/mjcheetham/emu-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1352
-- 
gitgitgadget

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/8] wincred: ignore unknown lines (do not die)
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 2/8] netrc: " Matthew John Cheetham via GitGitGadget
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

It is the expectation that credential helpers be liberal in what they
accept and conservative in what they return, to allow for future growth
and evolution of the protocol/interaction.

All of the other helpers (store, cache, osxkeychain, libsecret,
gnome-keyring) except `netrc` currently ignore any credential lines
that are not recognised, whereas the Windows helper (wincred) instead
dies.

Fix the discrepancy and ignore unknown lines in the wincred helper.

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 contrib/credential/wincred/git-credential-wincred.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index 5091048f9c6..ead6e267c78 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -278,8 +278,11 @@ static void read_credential(void)
 			wusername = utf8_to_utf16_dup(v);
 		} else if (!strcmp(buf, "password"))
 			password = utf8_to_utf16_dup(v);
-		else
-			die("unrecognized input");
+		/*
+		 * 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.
+		 */
 	}
 }
 
-- 
@@ -278,8 +278,11 @@ static void read_credential(void)
 			wusername = utf8_to_utf16_dup(v);
 		} else if (!strcmp(buf, "password"))
 			password = utf8_to_utf16_dup(v);
-		else
-			die("unrecognized input");
+		/*
+		 * 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.
+		 */
 	}
 }
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 2/8] netrc: ignore unknown lines (do not die)
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 1/8] wincred: ignore unknown lines (do not die) Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines Matthew John Cheetham via GitGitGadget
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

Contrary to the documentation on credential helpers, as well as the help
text for git-credential-netrc itself, this helper will `die` when
presented with an unknown property/attribute/token.

Correct the behaviour here by skipping and ignoring any tokens that are
unknown. This means all helpers in the tree are consistent and ignore
any unknown credential properties/attributes.

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 contrib/credential/netrc/git-credential-netrc.perl | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/contrib/credential/netrc/git-credential-netrc.perl b/contrib/credential/netrc/git-credential-netrc.perl
index bc57cc65884..9fb998ae090 100755
--- a/contrib/credential/netrc/git-credential-netrc.perl
+++ b/contrib/credential/netrc/git-credential-netrc.perl
@@ -356,7 +356,10 @@ sub read_credential_data_from_stdin {
 		next unless m/^([^=]+)=(.+)/;
 
 		my ($token, $value) = ($1, $2);
-		die "Unknown search token $token" unless exists $q{$token};
+
+		# skip any unknown tokens
+		next unless exists $q{$token};
+
 		$q{$token} = $value;
 		log_debug("We were given search token $token and value $value");
 	}
-- 
@@ -356,7 +356,10 @@ sub read_credential_data_from_stdin {
 		next unless m/^([^=]+)=(.+)/;
 
 		my ($token, $value) = ($1, $2);
-		die "Unknown search token $token" unless exists $q{$token};
+
+		# skip any unknown tokens
+		next unless exists $q{$token};
+
 		$q{$token} = $value;
 		log_debug("We were given search token $token and value $value");
 	}
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 1/8] wincred: ignore unknown lines (do not die) Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 2/8] netrc: " Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-19 16:12   ` Derrick Stolee
  2022-09-13 19:25 ` [PATCH 4/8] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
                   ` (6 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

Like in all the other credential helpers, the osxkeychain helper
ignores unknown credential lines.

Add a comment (a la the other helpers) to make it clear and explicit
that this is the desired behaviour.

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 contrib/credential/osxkeychain/git-credential-osxkeychain.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/contrib/credential/osxkeychain/git-credential-osxkeychain.c b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
index bf77748d602..e29cc28779d 100644
--- a/contrib/credential/osxkeychain/git-credential-osxkeychain.c
+++ b/contrib/credential/osxkeychain/git-credential-osxkeychain.c
@@ -159,6 +159,11 @@ static void read_credential(void)
 			username = xstrdup(v);
 		else if (!strcmp(buf, "password"))
 			password = xstrdup(v);
+		/*
+		 * 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.
+		 */
 	}
 }
 
-- 
@@ -159,6 +159,11 @@ static void read_credential(void)
 			username = xstrdup(v);
 		else if (!strcmp(buf, "password"))
 			password = xstrdup(v);
+		/*
+		 * 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.
+		 */
 	}
 }
 
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 4/8] http: read HTTP WWW-Authenticate response headers
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
                   ` (2 preceding siblings ...)
  2022-09-13 19:25 ` [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-19 16:21   ` Derrick Stolee
  2022-09-13 19:25 ` [PATCH 5/8] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
                   ` (5 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

Read and store the HTTP WWW-Authenticate response headers made for
a particular request.

This will allow us to pass important authentication challenge
information to credential helpers or others that would otherwise have
been lost.

According to RFC2616 Section 4.2 [1], header field names are not
case-sensitive meaning when collecting multiple values for the same
field name, we can just use the case of the first observed instance of
each field name and no normalisation is required.

libcurl only provides us with the ability to read all headers recieved
for a particular request, including any intermediate redirect requests
or proxies. The lines returned by libcurl include HTTP status lines
delinating any intermediate requests such as "HTTP/1.1 200". We use
these lines to reset the strvec of WWW-Authenticate header values as
we encounter them in order to only capture the final response headers.

The collection of all header values matching the WWW-Authenticate
header is complicated by the fact that it is legal for header fields to
be continued over multiple lines, but libcurl only gives us one line at
a time.

In the future [2] we may be able to leverage functions to read headers
from libcurl itself, but as of today we must do this ourselves.

[1] https://datatracker.ietf.org/doc/html/rfc2616#section-4.2
[2] https://daniel.haxx.se/blog/2022/03/22/a-headers-api-for-libcurl/

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 credential.c |  1 +
 credential.h | 10 +++++++
 http.c       | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 88 insertions(+)

diff --git a/credential.c b/credential.c
index f6389a50684..897b4679333 100644
--- a/credential.c
+++ b/credential.c
@@ -22,6 +22,7 @@ void credential_clear(struct credential *c)
 	free(c->username);
 	free(c->password);
 	string_list_clear(&c->helpers, 0);
+	strvec_clear(&c->wwwauth_headers);
 
 	credential_init(c);
 }
diff --git a/credential.h b/credential.h
index f430e77fea4..6a9d4e3de07 100644
--- a/credential.h
+++ b/credential.h
@@ -2,6 +2,7 @@
 #define CREDENTIAL_H
 
 #include "string-list.h"
+#include "strvec.h"
 
 /**
  * The credentials API provides an abstracted way of gathering username and
@@ -115,6 +116,14 @@ struct credential {
 	 */
 	struct string_list helpers;
 
+	/**
+	 * A `strvec` of WWW-Authenticate header values. Each string
+	 * is the value of a WWW-Authenticate header in an HTTP response,
+	 * in the order they were received in the response.
+	 */
+	struct strvec wwwauth_headers;
+	unsigned header_is_last_match:1;
+
 	unsigned approved:1,
 		 configured:1,
 		 quit:1,
@@ -130,6 +139,7 @@ struct credential {
 
 #define CREDENTIAL_INIT { \
 	.helpers = STRING_LIST_INIT_DUP, \
+	.wwwauth_headers = STRVEC_INIT, \
 }
 
 /* Initialize a credential structure, setting all fields to empty. */
diff --git a/http.c b/http.c
index 5d0502f51fd..091321af98e 100644
--- a/http.c
+++ b/http.c
@@ -183,6 +183,81 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 	return nmemb;
 }
 
+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
+{
+	size_t size = eltsize * nmemb;
+	struct strvec *values = &http_auth.wwwauth_headers;
+	struct strbuf buf = STRBUF_INIT;
+	const char *val;
+	const char *z = NULL;
+
+	/*
+	 * Header lines may not come NULL-terminated from libcurl so we must
+	 * limit all scans to the maximum length of the header line, or leverage
+	 * strbufs for all operations.
+	 *
+	 * In addition, it is possible that header values can be split over
+	 * multiple lines as per RFC 2616 (even though this has since been
+	 * deprecated in RFC 7230). A continuation header field value is
+	 * identified as starting with a space or horizontal tab.
+	 *
+	 * The formal definition of a header field as given in RFC 2616 is:
+	 *
+	 *   message-header = field-name ":" [ field-value ]
+	 *   field-name     = token
+	 *   field-value    = *( field-content | LWS )
+	 *   field-content  = <the OCTETs making up the field-value
+	 *                    and consisting of either *TEXT or combinations
+	 *                    of token, separators, and quoted-string>
+	 */
+
+	strbuf_add(&buf, ptr, size);
+
+	/* Strip the CRLF that should be present at the end of each field */
+	strbuf_trim_trailing_newline(&buf);
+
+	/* Start of a new WWW-Authenticate header */
+	if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
+		while (isspace(*val)) val++;
+
+		strvec_push(values, val);
+		http_auth.header_is_last_match = 1;
+		goto exit;
+	}
+
+	/*
+	 * This line could be a continuation of the previously matched header
+	 * field. If this is the case then we should append this value to the
+	 * end of the previously consumed value.
+	 */
+	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
+		const char **v = values->v + values->nr - 1;
+		char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
+
+		free((void*)*v);
+		*v = append;
+
+		goto exit;
+	}
+
+	/* This is the start of a new header we don't care about */
+	http_auth.header_is_last_match = 0;
+
+	/*
+	 * If this is a HTTP status line and not a header field, this signals
+	 * a different HTTP response. libcurl writes all the output of all
+	 * response headers of all responses, including redirects.
+	 * We only care about the last HTTP request response's headers so clear
+	 * the existing array.
+	 */
+	if (skip_iprefix(buf.buf, "http/", &z))
+		strvec_clear(values);
+
+exit:
+	strbuf_release(&buf);
+	return size;
+}
+
 size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
 {
 	return nmemb;
@@ -1829,6 +1904,8 @@ static int http_request(const char *url,
 					 fwrite_buffer);
 	}
 
+	curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
+
 	accept_language = http_get_accept_language_header();
 
 	if (accept_language)
-- 
@@ -183,6 +183,81 @@ size_t fwrite_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
 	return nmemb;
 }
 
+static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
+{
+	size_t size = eltsize * nmemb;
+	struct strvec *values = &http_auth.wwwauth_headers;
+	struct strbuf buf = STRBUF_INIT;
+	const char *val;
+	const char *z = NULL;
+
+	/*
+	 * Header lines may not come NULL-terminated from libcurl so we must
+	 * limit all scans to the maximum length of the header line, or leverage
+	 * strbufs for all operations.
+	 *
+	 * In addition, it is possible that header values can be split over
+	 * multiple lines as per RFC 2616 (even though this has since been
+	 * deprecated in RFC 7230). A continuation header field value is
+	 * identified as starting with a space or horizontal tab.
+	 *
+	 * The formal definition of a header field as given in RFC 2616 is:
+	 *
+	 *   message-header = field-name ":" [ field-value ]
+	 *   field-name     = token
+	 *   field-value    = *( field-content | LWS )
+	 *   field-content  = <the OCTETs making up the field-value
+	 *                    and consisting of either *TEXT or combinations
+	 *                    of token, separators, and quoted-string>
+	 */
+
+	strbuf_add(&buf, ptr, size);
+
+	/* Strip the CRLF that should be present at the end of each field */
+	strbuf_trim_trailing_newline(&buf);
+
+	/* Start of a new WWW-Authenticate header */
+	if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
+		while (isspace(*val)) val++;
+
+		strvec_push(values, val);
+		http_auth.header_is_last_match = 1;
+		goto exit;
+	}
+
+	/*
+	 * This line could be a continuation of the previously matched header
+	 * field. If this is the case then we should append this value to the
+	 * end of the previously consumed value.
+	 */
+	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
+		const char **v = values->v + values->nr - 1;
+		char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
+
+		free((void*)*v);
+		*v = append;
+
+		goto exit;
+	}
+
+	/* This is the start of a new header we don't care about */
+	http_auth.header_is_last_match = 0;
+
+	/*
+	 * If this is a HTTP status line and not a header field, this signals
+	 * a different HTTP response. libcurl writes all the output of all
+	 * response headers of all responses, including redirects.
+	 * We only care about the last HTTP request response's headers so clear
+	 * the existing array.
+	 */
+	if (skip_iprefix(buf.buf, "http/", &z))
+		strvec_clear(values);
+
+exit:
+	strbuf_release(&buf);
+	return size;
+}
+
 size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
 {
 	return nmemb;
@@ -1829,6 +1904,8 @@ static int http_request(const char *url,
 					 fwrite_buffer);
 	}
 
+	curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
+
 	accept_language = http_get_accept_language_header();
 
 	if (accept_language)
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 5/8] credential: add WWW-Authenticate header to cred requests
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
                   ` (3 preceding siblings ...)
  2022-09-13 19:25 ` [PATCH 4/8] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-19 16:33   ` Derrick Stolee
  2022-09-13 19:25 ` [PATCH 6/8] http: store all request headers on active_request_slot Matthew John Cheetham via GitGitGadget
                   ` (4 subsequent siblings)
  9 siblings, 1 reply; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

Add the value of the WWW-Authenticate response header to credential
requests. Credential helpers that understand and support HTTP
authentication and authorization can use this standard header (RFC 2616
Section 14.47 [1]) to generate valid credentials.

WWW-Authenticate headers can contain information pertaining to the
authority, authentication mechanism, or extra parameters/scopes that are
required.

The current I/O format for credential helpers only allows for unique
names for properties/attributes, so in order to transmit multiple header
values (with a specific order) we introduce a new convention whereby a
C-style array syntax is used in the property name to denote multiple
ordered values for the same property.

In this case we send multiple `wwwauth[n]` properties where `n` is a
zero-indexed number, reflecting the order the WWW-Authenticate headers
appeared in the HTTP response.

[1] https://datatracker.ietf.org/doc/html/rfc2616#section-14.47

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 Documentation/git-credential.txt |  9 +++++++
 credential.c                     | 12 +++++++++
 t/lib-httpd/apache.conf          | 13 +++++++++
 t/t5551-http-fetch-smart.sh      | 46 ++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index f18673017f5..7d4a788c63d 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -151,6 +151,15 @@ Git understands the following attributes:
 	were read (e.g., `url=https://example.com` would behave as if
 	`protocol=https` and `host=example.com` had been provided). This
 	can help callers avoid parsing URLs themselves.
+
+`wwwauth[n]`::
+
+	When an HTTP response is received that includes one or more
+	'WWW-Authenticate' authentication headers, these can be passed to Git
+	(and subsequent credential helpers) with these attributes.
+	Each 'WWW-Authenticate' header value should be passed as a separate
+	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
+	appear in the HTTP response.
 +
 Note that specifying a protocol is mandatory and if the URL
 doesn't specify a hostname (e.g., "cert:///path/to/file") the
diff --git a/credential.c b/credential.c
index 897b4679333..4ad40323fc7 100644
--- a/credential.c
+++ b/credential.c
@@ -263,6 +263,17 @@ static void credential_write_item(FILE *fp, const char *key, const char *value,
 	fprintf(fp, "%s=%s\n", key, value);
 }
 
+static void credential_write_strvec(FILE *fp, const char *key,
+				    const struct strvec *vec)
+{
+	int i = 0;
+	for (; i < vec->nr; i++) {
+		const char *full_key = xstrfmt("%s[%d]", key, i);
+		credential_write_item(fp, full_key, vec->v[i], 0);
+		free((void*)full_key);
+	}
+}
+
 void credential_write(const struct credential *c, FILE *fp)
 {
 	credential_write_item(fp, "protocol", c->protocol, 1);
@@ -270,6 +281,7 @@ void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path, 0);
 	credential_write_item(fp, "username", c->username, 0);
 	credential_write_item(fp, "password", c->password, 0);
+	credential_write_strvec(fp, "wwwauth", &c->wwwauth_headers);
 }
 
 static int run_credential_helper(struct credential *c,
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 497b9b9d927..fe118d76f98 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -235,6 +235,19 @@ SSLEngine On
 	Require valid-user
 </LocationMatch>
 
+# Advertise two additional auth methods above "Basic".
+# Neither of them actually work but serve test cases showing these
+# additional auth headers are consumed correctly.
+<Location /auth-wwwauth/>
+	AuthType Basic
+	AuthName "git-auth"
+	AuthUserFile passwd
+	Require valid-user
+	SetEnvIf Authorization "^\S+" authz
+	Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
+	Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
+</Location>
+
 RewriteCond %{QUERY_STRING} service=git-receive-pack [OR]
 RewriteCond %{REQUEST_URI} /git-receive-pack$
 RewriteRule ^/half-auth-complete/ - [E=AUTHREQUIRED:yes]
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 6a38294a476..c99d8e253df 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -564,6 +564,52 @@ test_expect_success 'http auth forgets bogus credentials' '
 	expect_askpass both user@host
 '
 
+test_expect_success 'http auth sends www-auth headers to credential helper' '
+	write_script git-credential-tee <<-\EOF &&
+		cmd=$1
+		teefile=credential-$cmd
+		if [ -f "$teefile" ]; then
+			rm $teefile
+		fi
+		(
+			while read line;
+			do
+				if [ -z "$line" ]; then
+					exit 0
+				fi
+				echo "$line" >> $teefile
+				echo $line
+			done
+		) | git credential-store $cmd
+	EOF
+
+	cat >expected-get <<-EOF &&
+	protocol=http
+	host=127.0.0.1:5551
+	wwwauth[0]=Bearer authority=https://login.example.com
+	wwwauth[1]=FooAuth foo=bar baz=1
+	wwwauth[2]=Basic realm="git-auth"
+	EOF
+
+	cat >expected-store <<-EOF &&
+	protocol=http
+	host=127.0.0.1:5551
+	username=user@host
+	password=pass@host
+	EOF
+
+	rm -f .git-credentials &&
+	test_config credential.helper tee &&
+	set_askpass user@host pass@host &&
+	(
+		PATH="$PWD:$PATH" &&
+		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
+	) &&
+	expect_askpass both user@host &&
+	test_cmp expected-get credential-get &&
+	test_cmp expected-store credential-store
+'
+
 test_expect_success 'client falls back from v2 to v0 to match server' '
 	GIT_TRACE_PACKET=$PWD/trace \
 	GIT_TEST_PROTOCOL_VERSION=2 \
-- 
@@ -564,6 +564,52 @@ test_expect_success 'http auth forgets bogus credentials' '
 	expect_askpass both user@host
 '
 
+test_expect_success 'http auth sends www-auth headers to credential helper' '
+	write_script git-credential-tee <<-\EOF &&
+		cmd=$1
+		teefile=credential-$cmd
+		if [ -f "$teefile" ]; then
+			rm $teefile
+		fi
+		(
+			while read line;
+			do
+				if [ -z "$line" ]; then
+					exit 0
+				fi
+				echo "$line" >> $teefile
+				echo $line
+			done
+		) | git credential-store $cmd
+	EOF
+
+	cat >expected-get <<-EOF &&
+	protocol=http
+	host=127.0.0.1:5551
+	wwwauth[0]=Bearer authority=https://login.example.com
+	wwwauth[1]=FooAuth foo=bar baz=1
+	wwwauth[2]=Basic realm="git-auth"
+	EOF
+
+	cat >expected-store <<-EOF &&
+	protocol=http
+	host=127.0.0.1:5551
+	username=user@host
+	password=pass@host
+	EOF
+
+	rm -f .git-credentials &&
+	test_config credential.helper tee &&
+	set_askpass user@host pass@host &&
+	(
+		PATH="$PWD:$PATH" &&
+		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
+	) &&
+	expect_askpass both user@host &&
+	test_cmp expected-get credential-get &&
+	test_cmp expected-store credential-store
+'
+
 test_expect_success 'client falls back from v2 to v0 to match server' '
 	GIT_TRACE_PACKET=$PWD/trace \
 	GIT_TEST_PROTOCOL_VERSION=2 \
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 6/8] http: store all request headers on active_request_slot
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
                   ` (4 preceding siblings ...)
  2022-09-13 19:25 ` [PATCH 5/8] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 7/8] http: move proactive auth to first slot creation Matthew John Cheetham via GitGitGadget
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

Once a list of headers has been set on the curl handle, it is not
possible to recover that `struct curl_slist` instance to add or modify
headers.

In future commits we will want to modify the set of request headers in
response to an authentication challenge/401 response from the server,
with information provided by a credential helper.

There are a number of different places where curl is used for an HTTP
request, and they do not have a common handling of request headers.
However, given that they all do call the `start_active_slot()` function,
either directly or indirectly via `run_slot()` or `run_one_slot()`, we
use this as the point to set the `CURLOPT_HTTPHEADER` option just
before the request is made.

We collect all request headers in a `struct curl_slist` on the
`struct active_request_slot` that is obtained from a call to
`get_active_slot(int)`. This function now takes a single argument to
define if the initial set of headers on the slot should include the
"Pragma: no-cache" header, along with all extra headers specified via
`http.extraHeader` config values.

The active request slot obtained from `get_active_slot(int)` will always
contain a fresh set of default headers and any headers set in previous
usages of this slot will be freed.

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 http-push.c   | 103 ++++++++++++++++++++++----------------------------
 http-walker.c |   2 +-
 http.c        |  82 ++++++++++++++++++----------------------
 http.h        |   4 +-
 remote-curl.c |  36 +++++++++---------
 5 files changed, 101 insertions(+), 126 deletions(-)

diff --git a/http-push.c b/http-push.c
index 5f4340a36e6..2b40959b376 100644
--- a/http-push.c
+++ b/http-push.c
@@ -211,29 +211,29 @@ static void curl_setup_http(CURL *curl, const char *url,
 	curl_easy_setopt(curl, CURLOPT_UPLOAD, 1);
 }
 
-static struct curl_slist *get_dav_token_headers(struct remote_lock *lock, enum dav_header_flag options)
+static struct curl_slist *append_dav_token_headers(struct curl_slist *headers,
+	struct remote_lock *lock, enum dav_header_flag options)
 {
 	struct strbuf buf = STRBUF_INIT;
-	struct curl_slist *dav_headers = http_copy_default_headers();
 
 	if (options & DAV_HEADER_IF) {
 		strbuf_addf(&buf, "If: (<%s>)", lock->token);
-		dav_headers = curl_slist_append(dav_headers, buf.buf);
+		headers = curl_slist_append(headers, buf.buf);
 		strbuf_reset(&buf);
 	}
 	if (options & DAV_HEADER_LOCK) {
 		strbuf_addf(&buf, "Lock-Token: <%s>", lock->token);
-		dav_headers = curl_slist_append(dav_headers, buf.buf);
+		headers = curl_slist_append(headers, buf.buf);
 		strbuf_reset(&buf);
 	}
 	if (options & DAV_HEADER_TIMEOUT) {
 		strbuf_addf(&buf, "Timeout: Second-%ld", lock->timeout);
-		dav_headers = curl_slist_append(dav_headers, buf.buf);
+		headers = curl_slist_append(headers, buf.buf);
 		strbuf_reset(&buf);
 	}
 	strbuf_release(&buf);
 
-	return dav_headers;
+	return headers;
 }
 
 static void finish_request(struct transfer_request *request);
@@ -281,7 +281,7 @@ static void start_mkcol(struct transfer_request *request)
 
 	request->url = get_remote_object_url(repo->url, hex, 1);
 
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->callback_func = process_response;
 	slot->callback_data = request;
 	curl_setup_http_get(slot->curl, request->url, DAV_MKCOL);
@@ -399,7 +399,7 @@ static void start_put(struct transfer_request *request)
 	strbuf_add(&buf, request->lock->tmpfile_suffix, the_hash_algo->hexsz + 1);
 	request->url = strbuf_detach(&buf, NULL);
 
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->callback_func = process_response;
 	slot->callback_data = request;
 	curl_setup_http(slot->curl, request->url, DAV_PUT,
@@ -417,15 +417,13 @@ static void start_put(struct transfer_request *request)
 static void start_move(struct transfer_request *request)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *dav_headers = http_copy_default_headers();
 
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->callback_func = process_response;
 	slot->callback_data = request;
 	curl_setup_http_get(slot->curl, request->url, DAV_MOVE);
-	dav_headers = curl_slist_append(dav_headers, request->dest);
-	dav_headers = curl_slist_append(dav_headers, "Overwrite: T");
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
+	slot->headers = curl_slist_append(slot->headers, request->dest);
+	slot->headers = curl_slist_append(slot->headers, "Overwrite: T");
 
 	if (start_active_slot(slot)) {
 		request->slot = slot;
@@ -440,17 +438,16 @@ static int refresh_lock(struct remote_lock *lock)
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *dav_headers;
 	int rc = 0;
 
 	lock->refreshing = 1;
 
-	dav_headers = get_dav_token_headers(lock, DAV_HEADER_IF | DAV_HEADER_TIMEOUT);
-
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->results = &results;
+	slot->headers = append_dav_token_headers(slot->headers, lock,
+		DAV_HEADER_IF | DAV_HEADER_TIMEOUT);
+
 	curl_setup_http_get(slot->curl, lock->url, DAV_LOCK);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
@@ -464,7 +461,6 @@ static int refresh_lock(struct remote_lock *lock)
 	}
 
 	lock->refreshing = 0;
-	curl_slist_free_all(dav_headers);
 
 	return rc;
 }
@@ -838,7 +834,6 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	char *ep;
 	char timeout_header[25];
 	struct remote_lock *lock = NULL;
-	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	char *escaped;
 
@@ -849,7 +844,7 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	while (ep) {
 		char saved_character = ep[1];
 		ep[1] = '\0';
-		slot = get_active_slot();
+		slot = get_active_slot(0);
 		slot->results = &results;
 		curl_setup_http_get(slot->curl, url, DAV_MKCOL);
 		if (start_active_slot(slot)) {
@@ -875,14 +870,15 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 	strbuf_addf(&out_buffer.buf, LOCK_REQUEST, escaped);
 	free(escaped);
 
+	slot = get_active_slot(0);
+	slot->results = &results;
+
 	xsnprintf(timeout_header, sizeof(timeout_header), "Timeout: Second-%ld", timeout);
-	dav_headers = curl_slist_append(dav_headers, timeout_header);
-	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");
+	slot->headers = curl_slist_append(slot->headers, timeout_header);
+	slot->headers = curl_slist_append(slot->headers,
+		"Content-Type: text/xml");
 
-	slot = get_active_slot();
-	slot->results = &results;
 	curl_setup_http(slot->curl, url, DAV_LOCK, &out_buffer, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);
 
 	CALLOC_ARRAY(lock, 1);
@@ -921,7 +917,6 @@ static struct remote_lock *lock_remote(const char *path, long timeout)
 		fprintf(stderr, "Unable to start LOCK request\n");
 	}
 
-	curl_slist_free_all(dav_headers);
 	strbuf_release(&out_buffer.buf);
 	strbuf_release(&in_buffer);
 
@@ -945,15 +940,14 @@ static int unlock_remote(struct remote_lock *lock)
 	struct active_request_slot *slot;
 	struct slot_results results;
 	struct remote_lock *prev = repo->locks;
-	struct curl_slist *dav_headers;
 	int rc = 0;
 
-	dav_headers = get_dav_token_headers(lock, DAV_HEADER_LOCK);
-
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->results = &results;
+	slot->headers = append_dav_token_headers(slot->headers, lock,
+		DAV_HEADER_LOCK);
+
 	curl_setup_http_get(slot->curl, lock->url, DAV_UNLOCK);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
@@ -966,8 +960,6 @@ static int unlock_remote(struct remote_lock *lock)
 		fprintf(stderr, "Unable to start UNLOCK request\n");
 	}
 
-	curl_slist_free_all(dav_headers);
-
 	if (repo->locks == lock) {
 		repo->locks = lock->next;
 	} else {
@@ -1121,7 +1113,6 @@ static void remote_ls(const char *path, int flags,
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	struct remote_ls_ctx ls;
 
@@ -1134,14 +1125,14 @@ static void remote_ls(const char *path, int flags,
 
 	strbuf_addstr(&out_buffer.buf, PROPFIND_ALL_REQUEST);
 
-	dav_headers = curl_slist_append(dav_headers, "Depth: 1");
-	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");
-
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->results = &results;
+	slot->headers = curl_slist_append(slot->headers, "Depth: 1");
+	slot->headers = curl_slist_append(slot->headers,
+		"Content-Type: text/xml");
+
 	curl_setup_http(slot->curl, url, DAV_PROPFIND,
 			&out_buffer, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);
 
 	if (start_active_slot(slot)) {
@@ -1177,7 +1168,6 @@ static void remote_ls(const char *path, int flags,
 	free(url);
 	strbuf_release(&out_buffer.buf);
 	strbuf_release(&in_buffer);
-	curl_slist_free_all(dav_headers);
 }
 
 static void get_remote_object_list(unsigned char parent)
@@ -1199,7 +1189,6 @@ static int locking_available(void)
 	struct slot_results results;
 	struct strbuf in_buffer = STRBUF_INIT;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers = http_copy_default_headers();
 	struct xml_ctx ctx;
 	int lock_flags = 0;
 	char *escaped;
@@ -1208,14 +1197,14 @@ static int locking_available(void)
 	strbuf_addf(&out_buffer.buf, PROPFIND_SUPPORTEDLOCK_REQUEST, escaped);
 	free(escaped);
 
-	dav_headers = curl_slist_append(dav_headers, "Depth: 0");
-	dav_headers = curl_slist_append(dav_headers, "Content-Type: text/xml");
-
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->results = &results;
+	slot->headers = curl_slist_append(slot->headers, "Depth: 0");
+	slot->headers = curl_slist_append(slot->headers,
+		"Content-Type: text/xml");
+
 	curl_setup_http(slot->curl, repo->url, DAV_PROPFIND,
 			&out_buffer, fwrite_buffer);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &in_buffer);
 
 	if (start_active_slot(slot)) {
@@ -1257,7 +1246,6 @@ static int locking_available(void)
 
 	strbuf_release(&out_buffer.buf);
 	strbuf_release(&in_buffer);
-	curl_slist_free_all(dav_headers);
 
 	return lock_flags;
 }
@@ -1374,17 +1362,16 @@ static int update_remote(const struct object_id *oid, struct remote_lock *lock)
 	struct active_request_slot *slot;
 	struct slot_results results;
 	struct buffer out_buffer = { STRBUF_INIT, 0 };
-	struct curl_slist *dav_headers;
-
-	dav_headers = get_dav_token_headers(lock, DAV_HEADER_IF);
 
 	strbuf_addf(&out_buffer.buf, "%s\n", oid_to_hex(oid));
 
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->results = &results;
+	slot->headers = append_dav_token_headers(slot->headers, lock,
+		DAV_HEADER_IF);
+
 	curl_setup_http(slot->curl, lock->url, DAV_PUT,
 			&out_buffer, fwrite_null);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
 
 	if (start_active_slot(slot)) {
 		run_active_slot(slot);
@@ -1486,18 +1473,18 @@ static void update_remote_info_refs(struct remote_lock *lock)
 	struct buffer buffer = { STRBUF_INIT, 0 };
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *dav_headers;
 
 	remote_ls("refs/", (PROCESS_FILES | RECURSIVE),
 		  add_remote_info_ref, &buffer.buf);
 	if (!aborted) {
-		dav_headers = get_dav_token_headers(lock, DAV_HEADER_IF);
 
-		slot = get_active_slot();
+		slot = get_active_slot(0);
 		slot->results = &results;
+		slot->headers = append_dav_token_headers(slot->headers, lock,
+			DAV_HEADER_IF);
+
 		curl_setup_http(slot->curl, lock->url, DAV_PUT,
 				&buffer, fwrite_null);
-		curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, dav_headers);
 
 		if (start_active_slot(slot)) {
 			run_active_slot(slot);
@@ -1652,7 +1639,7 @@ static int delete_remote_branch(const char *pattern, int force)
 	if (dry_run)
 		return 0;
 	url = xstrfmt("%s%s", repo->url, remote_ref->name);
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->results = &results;
 	curl_setup_http_get(slot->curl, url, DAV_DELETE);
 	if (start_active_slot(slot)) {
diff --git a/http-walker.c b/http-walker.c
index b8f0f98ae14..8747de2fcdb 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -373,7 +373,7 @@ static void fetch_alternates(struct walker *walker, const char *base)
 	 * Use a callback to process the result, since another request
 	 * may fail and need to have alternates loaded before continuing
 	 */
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 	slot->callback_func = process_alternates_response;
 	alt_req.walker = walker;
 	slot->callback_data = &alt_req;
diff --git a/http.c b/http.c
index 091321af98e..42616f746b1 100644
--- a/http.c
+++ b/http.c
@@ -124,8 +124,6 @@ static unsigned long empty_auth_useless =
 	| CURLAUTH_DIGEST_IE
 	| CURLAUTH_DIGEST;
 
-static struct curl_slist *pragma_header;
-static struct curl_slist *no_pragma_header;
 static struct string_list extra_http_headers = STRING_LIST_INIT_DUP;
 
 static struct curl_slist *host_resolutions;
@@ -1132,11 +1130,6 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
 	if (remote)
 		var_override(&http_proxy_authmethod, remote->http_proxy_authmethod);
 
-	pragma_header = curl_slist_append(http_copy_default_headers(),
-		"Pragma: no-cache");
-	no_pragma_header = curl_slist_append(http_copy_default_headers(),
-		"Pragma:");
-
 	{
 		char *http_max_requests = getenv("GIT_HTTP_MAX_REQUESTS");
 		if (http_max_requests)
@@ -1198,6 +1191,8 @@ void http_cleanup(void)
 
 	while (slot != NULL) {
 		struct active_request_slot *next = slot->next;
+		if (slot->headers)
+			curl_slist_free_all(slot->headers);
 		if (slot->curl) {
 			xmulti_remove_handle(slot);
 			curl_easy_cleanup(slot->curl);
@@ -1214,12 +1209,6 @@ void http_cleanup(void)
 
 	string_list_clear(&extra_http_headers, 0);
 
-	curl_slist_free_all(pragma_header);
-	pragma_header = NULL;
-
-	curl_slist_free_all(no_pragma_header);
-	no_pragma_header = NULL;
-
 	curl_slist_free_all(host_resolutions);
 	host_resolutions = NULL;
 
@@ -1254,7 +1243,18 @@ void http_cleanup(void)
 	FREE_AND_NULL(cached_accept_language);
 }
 
-struct active_request_slot *get_active_slot(void)
+static struct curl_slist *http_copy_default_headers(void)
+{
+	struct curl_slist *headers = NULL;
+	const struct string_list_item *item;
+
+	for_each_string_list_item(item, &extra_http_headers)
+		headers = curl_slist_append(headers, item->string);
+
+	return headers;
+}
+
+struct active_request_slot *get_active_slot(int no_pragma_header)
 {
 	struct active_request_slot *slot = active_queue_head;
 	struct active_request_slot *newslot;
@@ -1276,6 +1276,7 @@ struct active_request_slot *get_active_slot(void)
 		newslot->curl = NULL;
 		newslot->in_use = 0;
 		newslot->next = NULL;
+		newslot->headers = NULL;
 
 		slot = active_queue_head;
 		if (!slot) {
@@ -1293,6 +1294,15 @@ struct active_request_slot *get_active_slot(void)
 		curl_session_count++;
 	}
 
+	if (slot->headers)
+		curl_slist_free_all(slot->headers);
+
+	slot->headers = http_copy_default_headers();
+
+	if (!no_pragma_header)
+		slot->headers = curl_slist_append(slot->headers,
+			"Pragma: no-cache");
+
 	active_requests++;
 	slot->in_use = 1;
 	slot->results = NULL;
@@ -1302,7 +1312,6 @@ struct active_request_slot *get_active_slot(void)
 	curl_easy_setopt(slot->curl, CURLOPT_COOKIEFILE, curl_cookie_file);
 	if (curl_save_cookies)
 		curl_easy_setopt(slot->curl, CURLOPT_COOKIEJAR, curl_cookie_file);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, pragma_header);
 	curl_easy_setopt(slot->curl, CURLOPT_RESOLVE, host_resolutions);
 	curl_easy_setopt(slot->curl, CURLOPT_ERRORBUFFER, curl_errorstr);
 	curl_easy_setopt(slot->curl, CURLOPT_CUSTOMREQUEST, NULL);
@@ -1334,9 +1343,12 @@ struct active_request_slot *get_active_slot(void)
 
 int start_active_slot(struct active_request_slot *slot)
 {
-	CURLMcode curlm_result = curl_multi_add_handle(curlm, slot->curl);
+	CURLMcode curlm_result;
 	int num_transfers;
 
+	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, slot->headers);
+	curlm_result = curl_multi_add_handle(curlm, slot->curl);
+
 	if (curlm_result != CURLM_OK &&
 	    curlm_result != CURLM_CALL_MULTI_PERFORM) {
 		warning("curl_multi_add_handle failed: %s",
@@ -1651,17 +1663,6 @@ int run_one_slot(struct active_request_slot *slot,
 	return handle_curl_result(results);
 }
 
-struct curl_slist *http_copy_default_headers(void)
-{
-	struct curl_slist *headers = NULL;
-	const struct string_list_item *item;
-
-	for_each_string_list_item(item, &extra_http_headers)
-		headers = curl_slist_append(headers, item->string);
-
-	return headers;
-}
-
 static CURLcode curlinfo_strbuf(CURL *curl, CURLINFO info, struct strbuf *buf)
 {
 	char *ptr;
@@ -1879,12 +1880,11 @@ static int http_request(const char *url,
 {
 	struct active_request_slot *slot;
 	struct slot_results results;
-	struct curl_slist *headers = http_copy_default_headers();
-	struct strbuf buf = STRBUF_INIT;
+	int no_cache = options && options->no_cache;
 	const char *accept_language;
 	int ret;
 
-	slot = get_active_slot();
+	slot = get_active_slot(!no_cache);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPGET, 1);
 
 	if (!result) {
@@ -1909,27 +1909,23 @@ static int http_request(const char *url,
 	accept_language = http_get_accept_language_header();
 
 	if (accept_language)
-		headers = curl_slist_append(headers, accept_language);
+		slot->headers = curl_slist_append(slot->headers,
+			accept_language);
 
-	strbuf_addstr(&buf, "Pragma:");
-	if (options && options->no_cache)
-		strbuf_addstr(&buf, " no-cache");
 	if (options && options->initial_request &&
 	    http_follow_config == HTTP_FOLLOW_INITIAL)
 		curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
 
-	headers = curl_slist_append(headers, buf.buf);
-
 	/* Add additional headers here */
 	if (options && options->extra_headers) {
 		const struct string_list_item *item;
 		for_each_string_list_item(item, options->extra_headers) {
-			headers = curl_slist_append(headers, item->string);
+			slot->headers = curl_slist_append(slot->headers,
+				item->string);
 		}
 	}
 
 	curl_easy_setopt(slot->curl, CURLOPT_URL, url);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, "");
 	curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
 
@@ -1947,9 +1943,6 @@ static int http_request(const char *url,
 		curlinfo_strbuf(slot->curl, CURLINFO_EFFECTIVE_URL,
 				options->effective_url);
 
-	curl_slist_free_all(headers);
-	strbuf_release(&buf);
-
 	return ret;
 }
 
@@ -2310,12 +2303,10 @@ struct http_pack_request *new_direct_http_pack_request(
 		goto abort;
 	}
 
-	preq->slot = get_active_slot();
+	preq->slot = get_active_slot(1);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_WRITEDATA, preq->packfile);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite);
 	curl_easy_setopt(preq->slot->curl, CURLOPT_URL, preq->url);
-	curl_easy_setopt(preq->slot->curl, CURLOPT_HTTPHEADER,
-		no_pragma_header);
 
 	/*
 	 * If there is data present from a previous transfer attempt,
@@ -2480,14 +2471,13 @@ struct http_object_request *new_http_object_request(const char *base_url,
 		}
 	}
 
-	freq->slot = get_active_slot();
+	freq->slot = get_active_slot(1);
 
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEDATA, freq);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_FAILONERROR, 0);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_WRITEFUNCTION, fwrite_sha1_file);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_ERRORBUFFER, freq->errorstr);
 	curl_easy_setopt(freq->slot->curl, CURLOPT_URL, freq->url);
-	curl_easy_setopt(freq->slot->curl, CURLOPT_HTTPHEADER, no_pragma_header);
 
 	/*
 	 * If we have successfully processed data from a previous fetch
diff --git a/http.h b/http.h
index 3c94c479100..a304cc408b2 100644
--- a/http.h
+++ b/http.h
@@ -22,6 +22,7 @@ struct slot_results {
 struct active_request_slot {
 	CURL *curl;
 	int in_use;
+	struct curl_slist *headers;
 	CURLcode curl_result;
 	long http_code;
 	int *finished;
@@ -43,7 +44,7 @@ size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf);
 curlioerr ioctl_buffer(CURL *handle, int cmd, void *clientp);
 
 /* Slot lifecycle functions */
-struct active_request_slot *get_active_slot(void);
+struct active_request_slot *get_active_slot(int no_pragma_header);
 int start_active_slot(struct active_request_slot *slot);
 void run_active_slot(struct active_request_slot *slot);
 void finish_all_active_slots(void);
@@ -64,7 +65,6 @@ void step_active_slots(void);
 void http_init(struct remote *remote, const char *url,
 	       int proactive_auth);
 void http_cleanup(void);
-struct curl_slist *http_copy_default_headers(void);
 
 extern long int git_curl_ipresolve;
 extern int active_requests;
diff --git a/remote-curl.c b/remote-curl.c
index 72dfb8fb86a..edbd4504beb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -847,14 +847,13 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 
-	headers = curl_slist_append(headers, rpc->hdr_content_type);
-	headers = curl_slist_append(headers, rpc->hdr_accept);
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_content_type);
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_accept);
 
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
@@ -862,13 +861,11 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, "0000");
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &buf);
 
 	err = run_slot(slot, results);
 
-	curl_slist_free_all(headers);
 	strbuf_release(&buf);
 	return err;
 }
@@ -888,7 +885,6 @@ static curl_off_t xcurl_off_t(size_t len)
 static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_received)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = http_copy_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
@@ -930,21 +926,23 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 			needs_100_continue = 1;
 	}
 
-	headers = curl_slist_append(headers, rpc->hdr_content_type);
-	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, needs_100_continue ?
+retry:
+	slot = get_active_slot(0);
+
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_content_type);
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_accept);
+	slot->headers = curl_slist_append(slot->headers, needs_100_continue ?
 		"Expect: 100-continue" : "Expect:");
 
 	/* Add Accept-Language header */
 	if (rpc->hdr_accept_language)
-		headers = curl_slist_append(headers, rpc->hdr_accept_language);
+		slot->headers = curl_slist_append(slot->headers,
+			rpc->hdr_accept_language);
 
 	/* Add the extra Git-Protocol header */
 	if (rpc->protocol_header)
-		headers = curl_slist_append(headers, rpc->protocol_header);
-
-retry:
-	slot = get_active_slot();
+		slot->headers = curl_slist_append(slot->headers,
+			rpc->protocol_header);
 
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
@@ -955,7 +953,8 @@ retry:
 		/* The request body is large and the size cannot be predicted.
 		 * We must use chunked encoding to send it.
 		 */
-		headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+		slot->headers = curl_slist_append(slot->headers,
+			"Transfer-Encoding: chunked");
 		rpc->initial_buffer = 1;
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
 		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
@@ -1002,7 +1001,8 @@ retry:
 
 		gzip_size = stream.total_out;
 
-		headers = curl_slist_append(headers, "Content-Encoding: gzip");
+		slot->headers = curl_slist_append(slot->headers,
+			"Content-Encoding: gzip");
 		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);
 		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(gzip_size));
 
@@ -1025,7 +1025,6 @@ retry:
 		}
 	}
 
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	rpc_in_data.rpc = rpc;
 	rpc_in_data.slot = slot;
@@ -1055,7 +1054,6 @@ retry:
 	if (stateless_connect)
 		packet_response_end(rpc->in);
 
-	curl_slist_free_all(headers);
 	free(gzip_body);
 	return err;
 }
-- 
@@ -847,14 +847,13 @@ static int run_slot(struct active_request_slot *slot,
 static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = http_copy_default_headers();
 	struct strbuf buf = STRBUF_INIT;
 	int err;
 
-	slot = get_active_slot();
+	slot = get_active_slot(0);
 
-	headers = curl_slist_append(headers, rpc->hdr_content_type);
-	headers = curl_slist_append(headers, rpc->hdr_accept);
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_content_type);
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_accept);
 
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
@@ -862,13 +861,11 @@ static int probe_rpc(struct rpc_state *rpc, struct slot_results *results)
 	curl_easy_setopt(slot->curl, CURLOPT_ENCODING, NULL);
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, "0000");
 	curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE, 4);
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, fwrite_buffer);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEDATA, &buf);
 
 	err = run_slot(slot, results);
 
-	curl_slist_free_all(headers);
 	strbuf_release(&buf);
 	return err;
 }
@@ -888,7 +885,6 @@ static curl_off_t xcurl_off_t(size_t len)
 static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_received)
 {
 	struct active_request_slot *slot;
-	struct curl_slist *headers = http_copy_default_headers();
 	int use_gzip = rpc->gzip_request;
 	char *gzip_body = NULL;
 	size_t gzip_size = 0;
@@ -930,21 +926,23 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece
 			needs_100_continue = 1;
 	}
 
-	headers = curl_slist_append(headers, rpc->hdr_content_type);
-	headers = curl_slist_append(headers, rpc->hdr_accept);
-	headers = curl_slist_append(headers, needs_100_continue ?
+retry:
+	slot = get_active_slot(0);
+
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_content_type);
+	slot->headers = curl_slist_append(slot->headers, rpc->hdr_accept);
+	slot->headers = curl_slist_append(slot->headers, needs_100_continue ?
 		"Expect: 100-continue" : "Expect:");
 
 	/* Add Accept-Language header */
 	if (rpc->hdr_accept_language)
-		headers = curl_slist_append(headers, rpc->hdr_accept_language);
+		slot->headers = curl_slist_append(slot->headers,
+			rpc->hdr_accept_language);
 
 	/* Add the extra Git-Protocol header */
 	if (rpc->protocol_header)
-		headers = curl_slist_append(headers, rpc->protocol_header);
-
-retry:
-	slot = get_active_slot();
+		slot->headers = curl_slist_append(slot->headers,
+			rpc->protocol_header);
 
 	curl_easy_setopt(slot->curl, CURLOPT_NOBODY, 0);
 	curl_easy_setopt(slot->curl, CURLOPT_POST, 1);
@@ -955,7 +953,8 @@ retry:
 		/* The request body is large and the size cannot be predicted.
 		 * We must use chunked encoding to send it.
 		 */
-		headers = curl_slist_append(headers, "Transfer-Encoding: chunked");
+		slot->headers = curl_slist_append(slot->headers,
+			"Transfer-Encoding: chunked");
 		rpc->initial_buffer = 1;
 		curl_easy_setopt(slot->curl, CURLOPT_READFUNCTION, rpc_out);
 		curl_easy_setopt(slot->curl, CURLOPT_INFILE, rpc);
@@ -1002,7 +1001,8 @@ retry:
 
 		gzip_size = stream.total_out;
 
-		headers = curl_slist_append(headers, "Content-Encoding: gzip");
+		slot->headers = curl_slist_append(slot->headers,
+			"Content-Encoding: gzip");
 		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDS, gzip_body);
 		curl_easy_setopt(slot->curl, CURLOPT_POSTFIELDSIZE_LARGE, xcurl_off_t(gzip_size));
 
@@ -1025,7 +1025,6 @@ retry:
 		}
 	}
 
-	curl_easy_setopt(slot->curl, CURLOPT_HTTPHEADER, headers);
 	curl_easy_setopt(slot->curl, CURLOPT_WRITEFUNCTION, rpc_in);
 	rpc_in_data.rpc = rpc;
 	rpc_in_data.slot = slot;
@@ -1055,7 +1054,6 @@ retry:
 	if (stateless_connect)
 		packet_response_end(rpc->in);
 
-	curl_slist_free_all(headers);
 	free(gzip_body);
 	return err;
 }
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 7/8] http: move proactive auth to first slot creation
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
                   ` (5 preceding siblings ...)
  2022-09-13 19:25 ` [PATCH 6/8] http: store all request headers on active_request_slot Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-13 19:25 ` [PATCH 8/8] http: set specific auth scheme depending on credential Matthew John Cheetham via GitGitGadget
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

Rather than proactively seek credentials to authenticate a request at
`http_init()` time, do it when the first `active_request_slot` is
created.

Because credential helpers may modify the headers used for a request we
can only auth when a slot is created (when we can first start to gather
request headers).

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 http.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/http.c b/http.c
index 42616f746b1..8e107ff19b8 100644
--- a/http.c
+++ b/http.c
@@ -514,18 +514,18 @@ static int curl_empty_auth_enabled(void)
 	return 0;
 }
 
-static void init_curl_http_auth(CURL *result)
+static void init_curl_http_auth(struct active_request_slot *slot)
 {
 	if (!http_auth.username || !*http_auth.username) {
 		if (curl_empty_auth_enabled())
-			curl_easy_setopt(result, CURLOPT_USERPWD, ":");
+			curl_easy_setopt(slot->curl, CURLOPT_USERPWD, ":");
 		return;
 	}
 
 	credential_fill(&http_auth);
 
-	curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username);
-	curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password);
+	curl_easy_setopt(slot->curl, CURLOPT_USERNAME, http_auth.username);
+	curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, http_auth.password);
 }
 
 /* *var must be free-able */
@@ -900,9 +900,6 @@ static CURL *get_curl_handle(void)
 #endif
 	}
 
-	if (http_proactive_auth)
-		init_curl_http_auth(result);
-
 	if (getenv("GIT_SSL_VERSION"))
 		ssl_version = getenv("GIT_SSL_VERSION");
 	if (ssl_version && *ssl_version) {
@@ -1259,6 +1256,7 @@ struct active_request_slot *get_active_slot(int no_pragma_header)
 	struct active_request_slot *slot = active_queue_head;
 	struct active_request_slot *newslot;
 
+	int proactive_auth = 0;
 	int num_transfers;
 
 	/* Wait for a slot to open up if the queue is full */
@@ -1281,6 +1279,9 @@ struct active_request_slot *get_active_slot(int no_pragma_header)
 		slot = active_queue_head;
 		if (!slot) {
 			active_queue_head = newslot;
+
+			/* Auth first slot if asked for proactive auth */
+			proactive_auth = http_proactive_auth;
 		} else {
 			while (slot->next != NULL)
 				slot = slot->next;
@@ -1335,8 +1336,9 @@ struct active_request_slot *get_active_slot(int no_pragma_header)
 
 	curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
-	if (http_auth.password || curl_empty_auth_enabled())
-		init_curl_http_auth(slot->curl);
+
+	if (http_auth.password || curl_empty_auth_enabled() || proactive_auth)
+		init_curl_http_auth(slot);
 
 	return slot;
 }
-- 
@@ -514,18 +514,18 @@ static int curl_empty_auth_enabled(void)
 	return 0;
 }
 
-static void init_curl_http_auth(CURL *result)
+static void init_curl_http_auth(struct active_request_slot *slot)
 {
 	if (!http_auth.username || !*http_auth.username) {
 		if (curl_empty_auth_enabled())
-			curl_easy_setopt(result, CURLOPT_USERPWD, ":");
+			curl_easy_setopt(slot->curl, CURLOPT_USERPWD, ":");
 		return;
 	}
 
 	credential_fill(&http_auth);
 
-	curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username);
-	curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password);
+	curl_easy_setopt(slot->curl, CURLOPT_USERNAME, http_auth.username);
+	curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, http_auth.password);
 }
 
 /* *var must be free-able */
@@ -900,9 +900,6 @@ static CURL *get_curl_handle(void)
 #endif
 	}
 
-	if (http_proactive_auth)
-		init_curl_http_auth(result);
-
 	if (getenv("GIT_SSL_VERSION"))
 		ssl_version = getenv("GIT_SSL_VERSION");
 	if (ssl_version && *ssl_version) {
@@ -1259,6 +1256,7 @@ struct active_request_slot *get_active_slot(int no_pragma_header)
 	struct active_request_slot *slot = active_queue_head;
 	struct active_request_slot *newslot;
 
+	int proactive_auth = 0;
 	int num_transfers;
 
 	/* Wait for a slot to open up if the queue is full */
@@ -1281,6 +1279,9 @@ struct active_request_slot *get_active_slot(int no_pragma_header)
 		slot = active_queue_head;
 		if (!slot) {
 			active_queue_head = newslot;
+
+			/* Auth first slot if asked for proactive auth */
+			proactive_auth = http_proactive_auth;
 		} else {
 			while (slot->next != NULL)
 				slot = slot->next;
@@ -1335,8 +1336,9 @@ struct active_request_slot *get_active_slot(int no_pragma_header)
 
 	curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
 	curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, http_auth_methods);
-	if (http_auth.password || curl_empty_auth_enabled())
-		init_curl_http_auth(slot->curl);
+
+	if (http_auth.password || curl_empty_auth_enabled() || proactive_auth)
+		init_curl_http_auth(slot);
 
 	return slot;
 }
-- 
gitgitgadget


^ permalink raw reply related	[flat|nested] 21+ messages in thread

* [PATCH 8/8] http: set specific auth scheme depending on credential
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
                   ` (6 preceding siblings ...)
  2022-09-13 19:25 ` [PATCH 7/8] http: move proactive auth to first slot creation Matthew John Cheetham via GitGitGadget
@ 2022-09-13 19:25 ` Matthew John Cheetham via GitGitGadget
  2022-09-19 16:42   ` Derrick Stolee
  2022-09-19 16:08 ` [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Derrick Stolee
  2022-09-19 23:36 ` Lessley Dennington
  9 siblings, 1 reply; 21+ messages in thread
From: Matthew John Cheetham via GitGitGadget @ 2022-09-13 19:25 UTC (permalink / raw)
  To: git; +Cc: Matthew John Cheetham, Matthew John Cheetham

From: Matthew John Cheetham <mjcheetham@outlook.com>

Introduce a new credential field `authtype` that can be used by
credential helpers to indicate the type of the credential or
authentication mechanism to use for a request.

Modify http.c to now specify the correct authentication scheme or
credential type when authenticating the curl handle. If the new
`authtype` field in the credential structure is `NULL` or "Basic" then
use the existing username/password options. If the field is "Bearer"
then use the OAuth bearer token curl option. Otherwise, the `authtype`
field is the authentication scheme and the `password` field is the
raw, unencoded value.

Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com>
---
 Documentation/git-credential.txt |  9 +++++++++
 credential.c                     |  5 +++++
 credential.h                     |  1 +
 git-curl-compat.h                |  7 +++++++
 http.c                           | 24 +++++++++++++++++++++---
 5 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index 7d4a788c63d..3b6ef6f4906 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -152,6 +152,15 @@ Git understands the following attributes:
 	`protocol=https` and `host=example.com` had been provided). This
 	can help callers avoid parsing URLs themselves.
 
+`authtype`::
+
+	Indicates the type of authentication scheme used. If this is not
+	present the default 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.
+
 `wwwauth[n]`::
 
 	When an HTTP response is received that includes one or more
diff --git a/credential.c b/credential.c
index 4ad40323fc7..9d4a0f3fd51 100644
--- a/credential.c
+++ b/credential.c
@@ -21,6 +21,7 @@ void credential_clear(struct credential *c)
 	free(c->path);
 	free(c->username);
 	free(c->password);
+	free(c->authtype);
 	string_list_clear(&c->helpers, 0);
 	strvec_clear(&c->wwwauth_headers);
 
@@ -235,6 +236,9 @@ int credential_read(struct credential *c, FILE *fp)
 		} else if (!strcmp(key, "path")) {
 			free(c->path);
 			c->path = xstrdup(value);
+		} else if (!strcmp(key, "authtype")) {
+			free(c->authtype);
+			c->authtype = xstrdup(value);
 		} else if (!strcmp(key, "url")) {
 			credential_from_url(c, value);
 		} else if (!strcmp(key, "quit")) {
@@ -281,6 +285,7 @@ void credential_write(const struct credential *c, FILE *fp)
 	credential_write_item(fp, "path", c->path, 0);
 	credential_write_item(fp, "username", c->username, 0);
 	credential_write_item(fp, "password", c->password, 0);
+	credential_write_item(fp, "authtype", c->authtype, 0);
 	credential_write_strvec(fp, "wwwauth", &c->wwwauth_headers);
 }
 
diff --git a/credential.h b/credential.h
index 6a9d4e3de07..a6572aacf1d 100644
--- a/credential.h
+++ b/credential.h
@@ -135,6 +135,7 @@ struct credential {
 	char *protocol;
 	char *host;
 	char *path;
+	char *authtype;
 };
 
 #define CREDENTIAL_INIT { \
diff --git a/git-curl-compat.h b/git-curl-compat.h
index 56a83b6bbd8..74732500a9f 100644
--- a/git-curl-compat.h
+++ b/git-curl-compat.h
@@ -126,4 +126,11 @@
 #define GIT_CURL_HAVE_CURLSSLSET_NO_BACKENDS
 #endif
 
+/**
+ * CURLAUTH_BEARER was added in 7.61.0, released in July 2018.
+ */
+#if LIBCURL_VERSION_NUM >= 0x073D00
+#define GIT_CURL_HAVE_CURLAUTH_BEARER
+#endif
+
 #endif
diff --git a/http.c b/http.c
index 8e107ff19b8..d8913b2c641 100644
--- a/http.c
+++ b/http.c
@@ -516,7 +516,8 @@ static int curl_empty_auth_enabled(void)
 
 static void init_curl_http_auth(struct active_request_slot *slot)
 {
-	if (!http_auth.username || !*http_auth.username) {
+	if (!http_auth.authtype &&
+		(!http_auth.username || !*http_auth.username)) {
 		if (curl_empty_auth_enabled())
 			curl_easy_setopt(slot->curl, CURLOPT_USERPWD, ":");
 		return;
@@ -524,8 +525,25 @@ static void init_curl_http_auth(struct active_request_slot *slot)
 
 	credential_fill(&http_auth);
 
-	curl_easy_setopt(slot->curl, CURLOPT_USERNAME, http_auth.username);
-	curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, http_auth.password);
+	if (!http_auth.authtype || !strcasecmp(http_auth.authtype, "basic")
+				|| !strcasecmp(http_auth.authtype, "digest")) {
+		curl_easy_setopt(slot->curl, CURLOPT_USERNAME,
+			http_auth.username);
+		curl_easy_setopt(slot->curl, CURLOPT_PASSWORD,
+			http_auth.password);
+#ifdef GIT_CURL_HAVE_CURLAUTH_BEARER
+	} else if (!strcasecmp(http_auth.authtype, "bearer")) {
+		curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, CURLAUTH_BEARER);
+		curl_easy_setopt(slot->curl, CURLOPT_XOAUTH2_BEARER,
+			http_auth.password);
+#endif
+	} else {
+		struct strbuf auth = STRBUF_INIT;
+		strbuf_addf(&auth, "Authorization: %s %s",
+			http_auth.authtype, http_auth.password);
+		slot->headers = curl_slist_append(slot->headers, auth.buf);
+		strbuf_release(&auth);
+	}
 }
 
 /* *var must be free-able */
-- 
@@ -516,7 +516,8 @@ static int curl_empty_auth_enabled(void)
 
 static void init_curl_http_auth(struct active_request_slot *slot)
 {
-	if (!http_auth.username || !*http_auth.username) {
+	if (!http_auth.authtype &&
+		(!http_auth.username || !*http_auth.username)) {
 		if (curl_empty_auth_enabled())
 			curl_easy_setopt(slot->curl, CURLOPT_USERPWD, ":");
 		return;
@@ -524,8 +525,25 @@ static void init_curl_http_auth(struct active_request_slot *slot)
 
 	credential_fill(&http_auth);
 
-	curl_easy_setopt(slot->curl, CURLOPT_USERNAME, http_auth.username);
-	curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, http_auth.password);
+	if (!http_auth.authtype || !strcasecmp(http_auth.authtype, "basic")
+				|| !strcasecmp(http_auth.authtype, "digest")) {
+		curl_easy_setopt(slot->curl, CURLOPT_USERNAME,
+			http_auth.username);
+		curl_easy_setopt(slot->curl, CURLOPT_PASSWORD,
+			http_auth.password);
+#ifdef GIT_CURL_HAVE_CURLAUTH_BEARER
+	} else if (!strcasecmp(http_auth.authtype, "bearer")) {
+		curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, CURLAUTH_BEARER);
+		curl_easy_setopt(slot->curl, CURLOPT_XOAUTH2_BEARER,
+			http_auth.password);
+#endif
+	} else {
+		struct strbuf auth = STRBUF_INIT;
+		strbuf_addf(&auth, "Authorization: %s %s",
+			http_auth.authtype, http_auth.password);
+		slot->headers = curl_slist_append(slot->headers, auth.buf);
+		strbuf_release(&auth);
+	}
 }
 
 /* *var must be free-able */
-- 
gitgitgadget

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
                   ` (7 preceding siblings ...)
  2022-09-13 19:25 ` [PATCH 8/8] http: set specific auth scheme depending on credential Matthew John Cheetham via GitGitGadget
@ 2022-09-19 16:08 ` Derrick Stolee
  2022-09-19 16:44   ` Derrick Stolee
  2022-09-21 22:19   ` Matthew John Cheetham
  2022-09-19 23:36 ` Lessley Dennington
  9 siblings, 2 replies; 21+ messages in thread
From: Derrick Stolee @ 2022-09-19 16:08 UTC (permalink / raw)
  To: Matthew John Cheetham via GitGitGadget, git; +Cc: Matthew John Cheetham

On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> Hello! I have an RFC to update the existing credential helper design in
> order to allow for some new scenarios, and future evolution of auth methods
> that Git hosts may wish to provide. I outline the background, summary of
> changes and some challenges below. I also attach a series of patches to
> illustrate the design proposal.

It's unfortunate that we didn't get to talk about this during the
contributor summit, but it is super-technical and worth looking closely
at all the details. 

> One missing element from the patches are extensive tests of the new
> behaviour. It appears existing tests focus either on the credential helper
> protocol/format, or rely on testing basic authentication only via an Apache
> webserver. In order to have a full end to end test coverage of these new
> features it make be that we need a more comprehensive test bed to mock these
> more nuanced authentication methods. I lean on the experts on the list for
> advice here.

The microsoft/git fork has a feature (the GVFS Protocol) that requires a
custom HTTP server as a test helper. We might need a similar test helper
to return these WWW-Authenticate headers and check the full request list
from Git matches the spec. Doing that while also executing the proper Git
commands to serve the HTTP bodies is hopefully not too large. It might be
nice to adapt such a helper to replace the need for a full Apache install
in our test suite, but that's an independent concern from this RFC.

> Limitations
> ===========
> 
> Because this credential model was built mostly for password based
> authentication systems, it's somewhat limited. In particular:
> 
>  1. To generate valid credentials, additional information about the request
>     (or indeed the requestee and their device) may be required. For example,
>     OAuth is based around scopes. A scope, like "git.read", might be
>     required to read data from the remote. However, the remote cannot tell
>     the credential helper what scope is required for this request.
> 
>  2. This system is not fully extensible. Each time a new type of
>     authentication (like OAuth Bearer) is invented, Git needs updates before
>     credential helpers can take advantage of it (or leverage a new
>     capability in libcurl).
> 
> 
> Goals
> =====
> 
>  * As a user with multiple federated cloud identities:

I'm not sure if you mentioned it anywhere else, but this is specifically
for cases where a user might have multiple identities _on the same host
by DNS name_. The credential.useHttpPath config option might seem like it
could help here, but the credential helper might pick the wrong identity
that is the most-recent login. Either this workflow will require the user
to re-login with every new URL or the fetches/clones will fail when the
guess is wrong and the user would need to learn how to log into that other
identity.

Please correct me if I'm wrong about any of this, but the details of your
goals make it clear that the workflow will be greatly improved:

>    * Reach out to a remote and have my credential helper automatically
>      prompt me for the correct identity.
>    * Leverage existing authentication systems built-in to many operating
>      systems and devices to boost security and reduce reliance on passwords.
> 
>  * As a Git host and/or cloud identity provider:
>    
>    * Leverage newest identity standards, enhancements, and threat
>      mitigations - all without updating Git.
>    * Enforce security policies (like requiring two-factor authentication)
>      dynamically.
>    * Allow integration with third party standard based identity providers in
>      enterprises allowing customers to have a single plane of control for
>      critical identities with access to source code.

I had a question with this part of your proposal:

>     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[n]=value, where n
>     is a zero based index of the values.
>     
>     For the WWW-Authenticate header values we opt to use the key wwwauth[n].
...
> Git sends over standard input:
> 
> protocol=https
> host=example.com
> wwwauth[0]=Bearer realm="login.example", scope="git.readwrite"
> wwwauth[1]=Basic realm="login.example"

The important part here is that we provide a way to specify a multi-valued
key as opposed to a "last one wins" key, right?

Using empty braces (wwwauth[]) would suffice to indicate this, right? That
allows us to not care about the values inside the braces. The biggest
issues I see with a value in the braces are:

1. What if it isn't an integer?
2. What if we are missing a value?
3. What if they come out of order?

Without a value inside, then the order in which they appear provides
implicit indices in their multi-valued list.

Other than that, I support this idea and will start looking at the code
now.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines
  2022-09-13 19:25 ` [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines Matthew John Cheetham via GitGitGadget
@ 2022-09-19 16:12   ` Derrick Stolee
  2022-09-21 22:48     ` Matthew John Cheetham
  0 siblings, 1 reply; 21+ messages in thread
From: Derrick Stolee @ 2022-09-19 16:12 UTC (permalink / raw)
  To: Matthew John Cheetham via GitGitGadget, git
  Cc: Matthew John Cheetham, Matthew John Cheetham

On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
> 
> Like in all the other credential helpers, the osxkeychain helper
> ignores unknown credential lines.
> 
> Add a comment (a la the other helpers) to make it clear and explicit
> that this is the desired behaviour.

I recommend that these first three patches be submitted for full
review and merging, since they seem important independent of this
RFC.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/8] http: read HTTP WWW-Authenticate response headers
  2022-09-13 19:25 ` [PATCH 4/8] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
@ 2022-09-19 16:21   ` Derrick Stolee
  2022-09-21 22:24     ` Matthew John Cheetham
  0 siblings, 1 reply; 21+ messages in thread
From: Derrick Stolee @ 2022-09-19 16:21 UTC (permalink / raw)
  To: Matthew John Cheetham via GitGitGadget, git
  Cc: Matthew John Cheetham, Matthew John Cheetham

On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:

> +	/**
> +	 * A `strvec` of WWW-Authenticate header values. Each string
> +	 * is the value of a WWW-Authenticate header in an HTTP response,
> +	 * in the order they were received in the response.
> +	 */
> +	struct strvec wwwauth_headers;

I like this careful documentation.

> +	unsigned header_is_last_match:1;

But then this member is unclear how it is attached. It could use its
own "for internal use" comment if we don't want to describe it in full
detail here.

> +static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
> +{
> +	size_t size = eltsize * nmemb;
> +	struct strvec *values = &http_auth.wwwauth_headers;
> +	struct strbuf buf = STRBUF_INIT;
> +	const char *val;
> +	const char *z = NULL;
> +
> +	/*
> +	 * Header lines may not come NULL-terminated from libcurl so we must
> +	 * limit all scans to the maximum length of the header line, or leverage
> +	 * strbufs for all operations.
> +	 *
> +	 * In addition, it is possible that header values can be split over
> +	 * multiple lines as per RFC 2616 (even though this has since been
> +	 * deprecated in RFC 7230). A continuation header field value is
> +	 * identified as starting with a space or horizontal tab.
> +	 *
> +	 * The formal definition of a header field as given in RFC 2616 is:
> +	 *
> +	 *   message-header = field-name ":" [ field-value ]
> +	 *   field-name     = token
> +	 *   field-value    = *( field-content | LWS )
> +	 *   field-content  = <the OCTETs making up the field-value
> +	 *                    and consisting of either *TEXT or combinations
> +	 *                    of token, separators, and quoted-string>
> +	 */
> +
> +	strbuf_add(&buf, ptr, size);
> +
> +	/* Strip the CRLF that should be present at the end of each field */

Is it really a CRLF? Or just an LF?

> +	strbuf_trim_trailing_newline(&buf);

Thankfully, this will trim an LF _or_ CR/LF pair, so either way would be fine.

> +	/* Start of a new WWW-Authenticate header */
> +	if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
> +		while (isspace(*val)) val++;

Break the "val++;" to its own line:

		while (isspace(*val))
			val++;

While we are here, do we need to be careful about the end of the string at
this point? Is it possible that the server will send all spaces up until the
maximum header size (as mentioned in the message)?

> +
> +		strvec_push(values, val);
> +		http_auth.header_is_last_match = 1;
> +		goto exit;
> +	}
> +
> +	/*
> +	 * This line could be a continuation of the previously matched header
> +	 * field. If this is the case then we should append this value to the
> +	 * end of the previously consumed value.
> +	 */
> +	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
> +		const char **v = values->v + values->nr - 1;

I suppose we expect leading spaces as critical to this header, right?

> +		char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);

We might have better luck using a strbuf, initializing it with the expected
size and using strbuf_add() to append the strings. Maybe I'm just prematurely
optimizing, though.

> +
> +		free((void*)*v);
> +		*v = append;
> +
> +		goto exit;
> +	}
> +
> +	/* This is the start of a new header we don't care about */
> +	http_auth.header_is_last_match = 0;
> +
> +	/*
> +	 * If this is a HTTP status line and not a header field, this signals
> +	 * a different HTTP response. libcurl writes all the output of all
> +	 * response headers of all responses, including redirects.
> +	 * We only care about the last HTTP request response's headers so clear
> +	 * the existing array.
> +	 */
> +	if (skip_iprefix(buf.buf, "http/", &z))
> +		strvec_clear(values);
> +
> +exit:
> +	strbuf_release(&buf);
> +	return size;
> +}
> +
>  size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
>  {
>  	return nmemb;
> @@ -1829,6 +1904,8 @@ static int http_request(const char *url,
>  					 fwrite_buffer);
>  	}
>  
> +	curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);

Nice integration point!

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/8] credential: add WWW-Authenticate header to cred requests
  2022-09-13 19:25 ` [PATCH 5/8] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
@ 2022-09-19 16:33   ` Derrick Stolee
  2022-09-21 22:20     ` Matthew John Cheetham
  0 siblings, 1 reply; 21+ messages in thread
From: Derrick Stolee @ 2022-09-19 16:33 UTC (permalink / raw)
  To: Matthew John Cheetham via GitGitGadget, git
  Cc: Matthew John Cheetham, Matthew John Cheetham

On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>

> In this case we send multiple `wwwauth[n]` properties where `n` is a
> zero-indexed number, reflecting the order the WWW-Authenticate headers
> appeared in the HTTP response.
> @@ -151,6 +151,15 @@ Git understands the following attributes:
>  	were read (e.g., `url=https://example.com` would behave as if
>  	`protocol=https` and `host=example.com` had been provided). This
>  	can help callers avoid parsing URLs themselves.
> +
> +`wwwauth[n]`::
> +
> +	When an HTTP response is received that includes one or more
> +	'WWW-Authenticate' authentication headers, these can be passed to Git
> +	(and subsequent credential helpers) with these attributes.
> +	Each 'WWW-Authenticate' header value should be passed as a separate
> +	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
> +	appear in the HTTP response.
>  +
>  Note that specifying a protocol is mandatory and if the URL
>  doesn't specify a hostname (e.g., "cert:///path/to/file") the

This "+" means that this paragraph should be connected to the previous
one, so it seems that you've inserted your new value in the middle of
the `url` key. You'll want to move yours to be after those two connected
paragraphs. Your diff hunk should look like this:

--- >8 ---

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index f18673017f..127ae29be3 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -160,6 +160,15 @@ empty string.
 Components which are missing from the URL (e.g., there is no
 username in the example above) will be left unset.
 
+`wwwauth[n]`::
+
+	When an HTTP response is received that includes one or more
+	'WWW-Authenticate' authentication headers, these can be passed to Git
+	(and subsequent credential helpers) with these attributes.
+	Each 'WWW-Authenticate' header value should be passed as a separate
+	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
+	appear in the HTTP response.
+
 GIT
 ---
 Part of the linkgit:git[1] suite


--- >8 ---

> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
> index 497b9b9d927..fe118d76f98 100644
> --- a/t/lib-httpd/apache.conf
> +++ b/t/lib-httpd/apache.conf
> @@ -235,6 +235,19 @@ SSLEngine On
>  	Require valid-user
>  </LocationMatch>
>  
> +# Advertise two additional auth methods above "Basic".
> +# Neither of them actually work but serve test cases showing these
> +# additional auth headers are consumed correctly.
> +<Location /auth-wwwauth/>
> +	AuthType Basic
> +	AuthName "git-auth"
> +	AuthUserFile passwd
> +	Require valid-user
> +	SetEnvIf Authorization "^\S+" authz
> +	Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
> +	Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
> +</Location>
> +

This is cool that you've figured out how to make our Apache tests
add these headers! Maybe we won't need that extra test helper like
I thought (unless we want to confirm the second request sends the
right information).

> +test_expect_success 'http auth sends www-auth headers to credential helper' '
> +	write_script git-credential-tee <<-\EOF &&
> +		cmd=$1
> +		teefile=credential-$cmd
> +		if [ -f "$teefile" ]; then

I think we prefer using "test" over the braces (and linebreak
before then) like this:

		if test -n "$teefile"
		then

> +			rm $teefile
> +		fi

Alternatively, you could always run "rm -f $teefile" for
simplicity.

> +		(
> +			while read line;
> +			do
> +				if [ -z "$line" ]; then
> +					exit 0
> +				fi
> +				echo "$line" >> $teefile
> +				echo $line
> +			done
> +		) | git credential-store $cmd

Since I'm not sure, I'll ask the question: do we need the sub-shell
here, or could we pipe directly off of the "done"? Like this:

		while read line;
		do
			if [ -z "$line" ]; then
				exit 0
			fi
			echo "$line" >> $teefile
			echo $line
		done | git credential-store $cmd

> +	EOF


> +	cat >expected-get <<-EOF &&
> +	protocol=http
> +	host=127.0.0.1:5551
> +	wwwauth[0]=Bearer authority=https://login.example.com
> +	wwwauth[1]=FooAuth foo=bar baz=1
> +	wwwauth[2]=Basic realm="git-auth"
> +	EOF
> +
> +	cat >expected-store <<-EOF &&
> +	protocol=http
> +	host=127.0.0.1:5551
> +	username=user@host
> +	password=pass@host
> +	EOF
> +
> +	rm -f .git-credentials &&
> +	test_config credential.helper tee &&
> +	set_askpass user@host pass@host &&
> +	(
> +		PATH="$PWD:$PATH" &&
> +		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
> +	) &&
> +	expect_askpass both user@host &&
> +	test_cmp expected-get credential-get &&
> +	test_cmp expected-store credential-store

Elegant check for both calls.

Thanks,
-Stolee

^ permalink raw reply related	[flat|nested] 21+ messages in thread

* Re: [PATCH 8/8] http: set specific auth scheme depending on credential
  2022-09-13 19:25 ` [PATCH 8/8] http: set specific auth scheme depending on credential Matthew John Cheetham via GitGitGadget
@ 2022-09-19 16:42   ` Derrick Stolee
  0 siblings, 0 replies; 21+ messages in thread
From: Derrick Stolee @ 2022-09-19 16:42 UTC (permalink / raw)
  To: Matthew John Cheetham via GitGitGadget, git
  Cc: Matthew John Cheetham, Matthew John Cheetham

On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> From: Matthew John Cheetham <mjcheetham@outlook.com>
> 
> Introduce a new credential field `authtype` that can be used by
> credential helpers to indicate the type of the credential or
> authentication mechanism to use for a request.
> 
> Modify http.c to now specify the correct authentication scheme or
> credential type when authenticating the curl handle. If the new
> `authtype` field in the credential structure is `NULL` or "Basic" then
> use the existing username/password options. If the field is "Bearer"
> then use the OAuth bearer token curl option. Otherwise, the `authtype`
> field is the authentication scheme and the `password` field is the
> raw, unencoded value.


> @@ -524,8 +525,25 @@ static void init_curl_http_auth(struct active_request_slot *slot)
>  
>  	credential_fill(&http_auth);
>  
> -	curl_easy_setopt(slot->curl, CURLOPT_USERNAME, http_auth.username);
> -	curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, http_auth.password);
> +	if (!http_auth.authtype || !strcasecmp(http_auth.authtype, "basic")
> +				|| !strcasecmp(http_auth.authtype, "digest")) {
> +		curl_easy_setopt(slot->curl, CURLOPT_USERNAME,
> +			http_auth.username);
> +		curl_easy_setopt(slot->curl, CURLOPT_PASSWORD,
> +			http_auth.password);
> +#ifdef GIT_CURL_HAVE_CURLAUTH_BEARER
> +	} else if (!strcasecmp(http_auth.authtype, "bearer")) {
> +		curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, CURLAUTH_BEARER);
> +		curl_easy_setopt(slot->curl, CURLOPT_XOAUTH2_BEARER,
> +			http_auth.password);
> +#endif
> +	} else {
> +		struct strbuf auth = STRBUF_INIT;
> +		strbuf_addf(&auth, "Authorization: %s %s",
> +			http_auth.authtype, http_auth.password);
> +		slot->headers = curl_slist_append(slot->headers, auth.buf);
> +		strbuf_release(&auth);
> +	}
>  }

It would be good to have a test here, and the only way I can think
to add it would be to modify one of the test credential helpers to
indicate that OAuth is being used.

The test would somehow need to be careful about the curl version,
though, and I'm not sure if we have prior work for writing prereqs
based on the linked curl version.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
  2022-09-19 16:08 ` [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Derrick Stolee
@ 2022-09-19 16:44   ` Derrick Stolee
  2022-09-21 22:19   ` Matthew John Cheetham
  1 sibling, 0 replies; 21+ messages in thread
From: Derrick Stolee @ 2022-09-19 16:44 UTC (permalink / raw)
  To: Matthew John Cheetham via GitGitGadget, git; +Cc: Matthew John Cheetham

On 9/19/2022 12:08 PM, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:

>> protocol=https
>> host=example.com
>> wwwauth[0]=Bearer realm="login.example", scope="git.readwrite"
>> wwwauth[1]=Basic realm="login.example"
> 
> The important part here is that we provide a way to specify a multi-valued
> key as opposed to a "last one wins" key, right?
> 
> Using empty braces (wwwauth[]) would suffice to indicate this, right? That
> allows us to not care about the values inside the braces. The biggest
> issues I see with a value in the braces are:
> 
> 1. What if it isn't an integer?
> 2. What if we are missing a value?
> 3. What if they come out of order?
> 
> Without a value inside, then the order in which they appear provides
> implicit indices in their multi-valued list.

After looking at the code, it would not be difficult at all to make this
change in-place for these patches. But I won't push too hard if there is
some reason to keep the index values.
 
> Other than that, I support this idea and will start looking at the code
> now.

I took a look and provided feedback as I could. Patches 6 and 7 eluded
me only because I'm so unfamiliar with the http.c code and don't have
time to learn it today.

I mentioned that patches 1-3 could easily be picked up as a topic while
the rest of the series is considered carefully.

I tried to add some mentions of testing, but you've already tested more
than I expected, by adding the headers to the Apache output.

Thanks,
-Stolee


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
  2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
                   ` (8 preceding siblings ...)
  2022-09-19 16:08 ` [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Derrick Stolee
@ 2022-09-19 23:36 ` Lessley Dennington
  9 siblings, 0 replies; 21+ messages in thread
From: Lessley Dennington @ 2022-09-19 23:36 UTC (permalink / raw)
  To: Matthew John Cheetham via GitGitGadget, git; +Cc: Matthew John Cheetham

This is a really exciting idea! Based on your patches, it seems to be a
great opportunity to add extensibility and flexibility to the credential
helper model without huge disruptions to the codebase. Well done!

On 9/13/22 12:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>   3. Teach Git to specify authentication schemes other than Basic in
>      subsequent HTTP requests based on credential helper responses.
> 
This!! Yes!!
> 
> ...
> wwwauth=Bearer realm="login.example", scope="git.readwrite", Basic realm="login.example"
> 
I think sending the fields individually (as you describe in this doc and
implement in your patches) is the right call. In my opinion, it's more
legible, consistent with the remote response, and aligns with your goal of
minimizing authentication-related actions in Git.

Best,

Lessley

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers
  2022-09-19 16:08 ` [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Derrick Stolee
  2022-09-19 16:44   ` Derrick Stolee
@ 2022-09-21 22:19   ` Matthew John Cheetham
  1 sibling, 0 replies; 21+ messages in thread
From: Matthew John Cheetham @ 2022-09-21 22:19 UTC (permalink / raw)
  To: Derrick Stolee, Matthew John Cheetham via GitGitGadget, git

On 2022-09-19 09:08, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>> Hello! I have an RFC to update the existing credential helper design in
>> order to allow for some new scenarios, and future evolution of auth methods
>> that Git hosts may wish to provide. I outline the background, summary of
>> changes and some challenges below. I also attach a series of patches to
>> illustrate the design proposal.
> 
> It's unfortunate that we didn't get to talk about this during the
> contributor summit, but it is super-technical and worth looking closely
> at all the details. 
> 
>> One missing element from the patches are extensive tests of the new
>> behaviour. It appears existing tests focus either on the credential helper
>> protocol/format, or rely on testing basic authentication only via an Apache
>> webserver. In order to have a full end to end test coverage of these new
>> features it make be that we need a more comprehensive test bed to mock these
>> more nuanced authentication methods. I lean on the experts on the list for
>> advice here.
> 
> The microsoft/git fork has a feature (the GVFS Protocol) that requires a
> custom HTTP server as a test helper. We might need a similar test helper
> to return these WWW-Authenticate headers and check the full request list
> from Git matches the spec. Doing that while also executing the proper Git
> commands to serve the HTTP bodies is hopefully not too large. It might be
> nice to adapt such a helper to replace the need for a full Apache install
> in our test suite, but that's an independent concern from this RFC.

That's a good reference and possible solution to the testing question, and
definitely something I can look at adding. I just wanted another pair of
eyes and thoughts on any other options that I may have been missing in the
existing testing repertoire, before embarking on writing such a test helper.

>> Limitations
>> ===========
>>
>> Because this credential model was built mostly for password based
>> authentication systems, it's somewhat limited. In particular:
>>
>>  1. To generate valid credentials, additional information about the request
>>     (or indeed the requestee and their device) may be required. For example,
>>     OAuth is based around scopes. A scope, like "git.read", might be
>>     required to read data from the remote. However, the remote cannot tell
>>     the credential helper what scope is required for this request.
>>
>>  2. This system is not fully extensible. Each time a new type of
>>     authentication (like OAuth Bearer) is invented, Git needs updates before
>>     credential helpers can take advantage of it (or leverage a new
>>     capability in libcurl).
>>
>>
>> Goals
>> =====
>>
>>  * As a user with multiple federated cloud identities:
> 
> I'm not sure if you mentioned it anywhere else, but this is specifically
> for cases where a user might have multiple identities _on the same host
> by DNS name_. The credential.useHttpPath config option might seem like it
> could help here, but the credential helper might pick the wrong identity
> that is the most-recent login. Either this workflow will require the user
> to re-login with every new URL or the fetches/clones will fail when the
> guess is wrong and the user would need to learn how to log into that other
> identity.
> 
> Please correct me if I'm wrong about any of this, but the details of your
> goals make it clear that the workflow will be greatly improved:

Such a scenario where multiple identities may be available for the same DNS
hostname would indeed be improved (with an appropriately enlightened
credential helper of course). As you mentioned, credential.useHttpPath can
also be used to workaround such a situation, but that just creates another
problem in that users need to provide the same set of credentials for each
repository with a full remote URL path that use the same identity.

By providing information about the auth challenge (including parameters
like authority or realm if present) would allow credential helpers select
or filter known identities and credentials automatically, avoiding user
input.

>>    * Reach out to a remote and have my credential helper automatically
>>      prompt me for the correct identity.
>>    * Leverage existing authentication systems built-in to many operating
>>      systems and devices to boost security and reduce reliance on passwords.
>>
>>  * As a Git host and/or cloud identity provider:
>>    
>>    * Leverage newest identity standards, enhancements, and threat
>>      mitigations - all without updating Git.
>>    * Enforce security policies (like requiring two-factor authentication)
>>      dynamically.
>>    * Allow integration with third party standard based identity providers in
>>      enterprises allowing customers to have a single plane of control for
>>      critical identities with access to source code.
> 
> I had a question with this part of your proposal:
> 
>>     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[n]=value, where n
>>     is a zero based index of the values.
>>     
>>     For the WWW-Authenticate header values we opt to use the key wwwauth[n].
> ...
>> Git sends over standard input:
>>
>> protocol=https
>> host=example.com
>> wwwauth[0]=Bearer realm="login.example", scope="git.readwrite"
>> wwwauth[1]=Basic realm="login.example"
> 
> The important part here is that we provide a way to specify a multi-valued
> key as opposed to a "last one wins" key, right?
> 
> Using empty braces (wwwauth[]) would suffice to indicate this, right? That
> allows us to not care about the values inside the braces. The biggest
> issues I see with a value in the braces are:
> 
> 1. What if it isn't an integer?
> 2. What if we are missing a value?
> 3. What if they come out of order?
> 
> Without a value inside, then the order in which they appear provides
> implicit indices in their multi-valued list.
> 
> Other than that, I support this idea and will start looking at the code
> now.

There are two important things this extension to the I/O format provides:
1) multi-valued keys, and 2) ordering to the multiple values.

You are correct that dropping the integer index still means we still meet
requirement 1, and implicitly meet requirement 2. In this proposal I was
just being explicit in the ordering - it's not something I'm overly
attached to however, and may indeed make parsing or identifiying these
multi-valued keys easier on the credential helper side of things.

> Thanks,
> -Stolee

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 5/8] credential: add WWW-Authenticate header to cred requests
  2022-09-19 16:33   ` Derrick Stolee
@ 2022-09-21 22:20     ` Matthew John Cheetham
  0 siblings, 0 replies; 21+ messages in thread
From: Matthew John Cheetham @ 2022-09-21 22:20 UTC (permalink / raw)
  To: Derrick Stolee, Matthew John Cheetham via GitGitGadget, git

On 2022-09-19 09:33, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
> 
>> In this case we send multiple `wwwauth[n]` properties where `n` is a
>> zero-indexed number, reflecting the order the WWW-Authenticate headers
>> appeared in the HTTP response.
>> @@ -151,6 +151,15 @@ Git understands the following attributes:
>>  	were read (e.g., `url=https://example.com` would behave as if
>>  	`protocol=https` and `host=example.com` had been provided). This
>>  	can help callers avoid parsing URLs themselves.
>> +
>> +`wwwauth[n]`::
>> +
>> +	When an HTTP response is received that includes one or more
>> +	'WWW-Authenticate' authentication headers, these can be passed to Git
>> +	(and subsequent credential helpers) with these attributes.
>> +	Each 'WWW-Authenticate' header value should be passed as a separate
>> +	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
>> +	appear in the HTTP response.
>>  +
>>  Note that specifying a protocol is mandatory and if the URL
>>  doesn't specify a hostname (e.g., "cert:///path/to/file") the
> 
> This "+" means that this paragraph should be connected to the previous
> one, so it seems that you've inserted your new value in the middle of
> the `url` key. You'll want to move yours to be after those two connected
> paragraphs. Your diff hunk should look like this:
> 
> --- >8 ---
> 
> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
> index f18673017f..127ae29be3 100644
> --- a/Documentation/git-credential.txt
> +++ b/Documentation/git-credential.txt
> @@ -160,6 +160,15 @@ empty string.
>  Components which are missing from the URL (e.g., there is no
>  username in the example above) will be left unset.
>  
> +`wwwauth[n]`::
> +
> +	When an HTTP response is received that includes one or more
> +	'WWW-Authenticate' authentication headers, these can be passed to Git
> +	(and subsequent credential helpers) with these attributes.
> +	Each 'WWW-Authenticate' header value should be passed as a separate
> +	attribute 'wwwauth[n]' where 'n' is the zero-indexed order the headers
> +	appear in the HTTP response.
> +
>  GIT
>  ---
>  Part of the linkgit:git[1] suite
> 
> 
> --- >8 ---

Thanks for catching!

>> diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
>> index 497b9b9d927..fe118d76f98 100644
>> --- a/t/lib-httpd/apache.conf
>> +++ b/t/lib-httpd/apache.conf
>> @@ -235,6 +235,19 @@ SSLEngine On
>>  	Require valid-user
>>  </LocationMatch>
>>  
>> +# Advertise two additional auth methods above "Basic".
>> +# Neither of them actually work but serve test cases showing these
>> +# additional auth headers are consumed correctly.
>> +<Location /auth-wwwauth/>
>> +	AuthType Basic
>> +	AuthName "git-auth"
>> +	AuthUserFile passwd
>> +	Require valid-user
>> +	SetEnvIf Authorization "^\S+" authz
>> +	Header always add WWW-Authenticate "Bearer authority=https://login.example.com" env=!authz
>> +	Header always add WWW-Authenticate "FooAuth foo=bar baz=1" env=!authz
>> +</Location>
>> +
> 
> This is cool that you've figured out how to make our Apache tests
> add these headers! Maybe we won't need that extra test helper like
> I thought (unless we want to confirm the second request sends the
> right information).

This will exercise the new header parsing and passing the info to the helper
but will indeed not test the response. I feel like a test helper would be
beneficial still.. what I've done here doesn't feel 100% clean or complete.

>> +test_expect_success 'http auth sends www-auth headers to credential helper' '
>> +	write_script git-credential-tee <<-\EOF &&
>> +		cmd=$1
>> +		teefile=credential-$cmd
>> +		if [ -f "$teefile" ]; then
> 
> I think we prefer using "test" over the braces (and linebreak
> before then) like this:
> 
> 		if test -n "$teefile"
> 		then
> 
>> +			rm $teefile
>> +		fi
> 
> Alternatively, you could always run "rm -f $teefile" for
> simplicity.
I like simple :-)

>> +		(
>> +			while read line;
>> +			do
>> +				if [ -z "$line" ]; then
>> +					exit 0
>> +				fi
>> +				echo "$line" >> $teefile
>> +				echo $line
>> +			done
>> +		) | git credential-store $cmd
> 
> Since I'm not sure, I'll ask the question: do we need the sub-shell
> here, or could we pipe directly off of the "done"? Like this:
> 
> 		while read line;
> 		do
> 			if [ -z "$line" ]; then
> 				exit 0
> 			fi
> 			echo "$line" >> $teefile
> 			echo $line
> 		done | git credential-store $cmd

That we can.. I will update in next iteration.

>> +	EOF
> 
> 
>> +	cat >expected-get <<-EOF &&
>> +	protocol=http
>> +	host=127.0.0.1:5551
>> +	wwwauth[0]=Bearer authority=https://login.example.com
>> +	wwwauth[1]=FooAuth foo=bar baz=1
>> +	wwwauth[2]=Basic realm="git-auth"
>> +	EOF
>> +
>> +	cat >expected-store <<-EOF &&
>> +	protocol=http
>> +	host=127.0.0.1:5551
>> +	username=user@host
>> +	password=pass@host
>> +	EOF
>> +
>> +	rm -f .git-credentials &&
>> +	test_config credential.helper tee &&
>> +	set_askpass user@host pass@host &&
>> +	(
>> +		PATH="$PWD:$PATH" &&
>> +		git ls-remote "$HTTPD_URL/auth-wwwauth/smart/repo.git"
>> +	) &&
>> +	expect_askpass both user@host &&
>> +	test_cmp expected-get credential-get &&
>> +	test_cmp expected-store credential-store
> 
> Elegant check for both calls.
> 
> Thanks,
> -Stolee

Thanks,
Matthew

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/8] http: read HTTP WWW-Authenticate response headers
  2022-09-19 16:21   ` Derrick Stolee
@ 2022-09-21 22:24     ` Matthew John Cheetham
  2022-09-26 14:13       ` Derrick Stolee
  0 siblings, 1 reply; 21+ messages in thread
From: Matthew John Cheetham @ 2022-09-21 22:24 UTC (permalink / raw)
  To: Derrick Stolee, Matthew John Cheetham via GitGitGadget, git

On 2022-09-19 09:21, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
> 
>> +	/**
>> +	 * A `strvec` of WWW-Authenticate header values. Each string
>> +	 * is the value of a WWW-Authenticate header in an HTTP response,
>> +	 * in the order they were received in the response.
>> +	 */
>> +	struct strvec wwwauth_headers;
> 
> I like this careful documentation.
> 
>> +	unsigned header_is_last_match:1;
> 
> But then this member is unclear how it is attached. It could use its
> own "for internal use" comment if we don't want to describe it in full
> detail here.

A fair point. I will update in a future iteration.

>> +static size_t fwrite_wwwauth(char *ptr, size_t eltsize, size_t nmemb, void *p)
>> +{
>> +	size_t size = eltsize * nmemb;
>> +	struct strvec *values = &http_auth.wwwauth_headers;
>> +	struct strbuf buf = STRBUF_INIT;
>> +	const char *val;
>> +	const char *z = NULL;
>> +
>> +	/*
>> +	 * Header lines may not come NULL-terminated from libcurl so we must
>> +	 * limit all scans to the maximum length of the header line, or leverage
>> +	 * strbufs for all operations.
>> +	 *
>> +	 * In addition, it is possible that header values can be split over
>> +	 * multiple lines as per RFC 2616 (even though this has since been
>> +	 * deprecated in RFC 7230). A continuation header field value is
>> +	 * identified as starting with a space or horizontal tab.
>> +	 *
>> +	 * The formal definition of a header field as given in RFC 2616 is:
>> +	 *
>> +	 *   message-header = field-name ":" [ field-value ]
>> +	 *   field-name     = token
>> +	 *   field-value    = *( field-content | LWS )
>> +	 *   field-content  = <the OCTETs making up the field-value
>> +	 *                    and consisting of either *TEXT or combinations
>> +	 *                    of token, separators, and quoted-string>
>> +	 */
>> +
>> +	strbuf_add(&buf, ptr, size);
>> +
>> +	/* Strip the CRLF that should be present at the end of each field */
> 
> Is it really a CRLF? Or just an LF?

It is indeed an CRLF, agnostic of platform. HTTP defines CRLF as the
end-of-line marker for all entities other than the body.

See RFC 2616 section 2.2: https://www.rfc-editor.org/rfc/rfc2616#section-2.2

>> +	strbuf_trim_trailing_newline(&buf);
> 
> Thankfully, this will trim an LF _or_ CR/LF pair, so either way would be fine.
> 
>> +	/* Start of a new WWW-Authenticate header */
>> +	if (skip_iprefix(buf.buf, "www-authenticate:", &val)) {
>> +		while (isspace(*val)) val++;
> 
> Break the "val++;" to its own line:
> 
> 		while (isspace(*val))
> 			val++;

Sure! Sorry I missed this one.

> While we are here, do we need to be careful about the end of the string at
> this point? Is it possible that the server will send all spaces up until the
> maximum header size (as mentioned in the message)?
> 
>> +
>> +		strvec_push(values, val);
>> +		http_auth.header_is_last_match = 1;
>> +		goto exit;
>> +	}
>> +
>> +	/*
>> +	 * This line could be a continuation of the previously matched header
>> +	 * field. If this is the case then we should append this value to the
>> +	 * end of the previously consumed value.
>> +	 */
>> +	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
>> +		const char **v = values->v + values->nr - 1;
> 
> I suppose we expect leading spaces as critical to this header, right?

Leading (and trailing) spaces are not part of the header value.

From RFC 2616 section 2.2 regarding header field values:

"All linear white space, including folding, has the same semantics as SP.
A recipient MAY replace any linear white space with a single SP before
interpreting the field value or forwarding the message downstream."

>> +		char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
> 
> We might have better luck using a strbuf, initializing it with the expected
> size and using strbuf_add() to append the strings. Maybe I'm just prematurely
> optimizing, though.

This code path is used to re-join/fold a header value continuation, which is
pretty rare in the wild (if at all with modern web servers).

>> +
>> +		free((void*)*v);
>> +		*v = append;
>> +
>> +		goto exit;
>> +	}
>> +
>> +	/* This is the start of a new header we don't care about */
>> +	http_auth.header_is_last_match = 0;
>> +
>> +	/*
>> +	 * If this is a HTTP status line and not a header field, this signals
>> +	 * a different HTTP response. libcurl writes all the output of all
>> +	 * response headers of all responses, including redirects.
>> +	 * We only care about the last HTTP request response's headers so clear
>> +	 * the existing array.
>> +	 */
>> +	if (skip_iprefix(buf.buf, "http/", &z))
>> +		strvec_clear(values);
>> +
>> +exit:
>> +	strbuf_release(&buf);
>> +	return size;
>> +}
>> +
>>  size_t fwrite_null(char *ptr, size_t eltsize, size_t nmemb, void *strbuf)
>>  {
>>  	return nmemb;
>> @@ -1829,6 +1904,8 @@ static int http_request(const char *url,
>>  					 fwrite_buffer);
>>  	}
>>  
>> +	curl_easy_setopt(slot->curl, CURLOPT_HEADERFUNCTION, fwrite_wwwauth);
> 
> Nice integration point!
> 
> Thanks,
> -Stolee

Thanks,
Matthew

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines
  2022-09-19 16:12   ` Derrick Stolee
@ 2022-09-21 22:48     ` Matthew John Cheetham
  0 siblings, 0 replies; 21+ messages in thread
From: Matthew John Cheetham @ 2022-09-21 22:48 UTC (permalink / raw)
  To: Derrick Stolee, Matthew John Cheetham via GitGitGadget, git

On 2022-09-19 09:12, Derrick Stolee wrote:
> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:
>> From: Matthew John Cheetham <mjcheetham@outlook.com>
>>
>> Like in all the other credential helpers, the osxkeychain helper
>> ignores unknown credential lines.
>>
>> Add a comment (a la the other helpers) to make it clear and explicit
>> that this is the desired behaviour.
> 
> I recommend that these first three patches be submitted for full
> review and merging, since they seem important independent of this
> RFC.
> 
> Thanks,
> -Stolee

That's a fair point. I will submit these independently.

Thanks,
Matthew

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 4/8] http: read HTTP WWW-Authenticate response headers
  2022-09-21 22:24     ` Matthew John Cheetham
@ 2022-09-26 14:13       ` Derrick Stolee
  0 siblings, 0 replies; 21+ messages in thread
From: Derrick Stolee @ 2022-09-26 14:13 UTC (permalink / raw)
  To: Matthew John Cheetham, Matthew John Cheetham via GitGitGadget, git

On 9/21/2022 6:24 PM, Matthew John Cheetham wrote:
> On 2022-09-19 09:21, Derrick Stolee wrote:
>> On 9/13/2022 3:25 PM, Matthew John Cheetham via GitGitGadget wrote:

>>> +
>>> +		strvec_push(values, val);
>>> +		http_auth.header_is_last_match = 1;
>>> +		goto exit;
>>> +	}
>>> +
>>> +	/*
>>> +	 * This line could be a continuation of the previously matched header
>>> +	 * field. If this is the case then we should append this value to the
>>> +	 * end of the previously consumed value.
>>> +	 */
>>> +	if (http_auth.header_is_last_match && isspace(*buf.buf)) {
>>> +		const char **v = values->v + values->nr - 1;
>>
>> I suppose we expect leading spaces as critical to this header, right?
> 
> Leading (and trailing) spaces are not part of the header value.
> 
> From RFC 2616 section 2.2 regarding header field values:
> 
> "All linear white space, including folding, has the same semantics as SP.
> A recipient MAY replace any linear white space with a single SP before
> interpreting the field value or forwarding the message downstream."
> 
>>> +		char *append = xstrfmt("%s%.*s", *v, (int)(size - 1), ptr + 1);
>>
>> We might have better luck using a strbuf, initializing it with the expected
>> size and using strbuf_add() to append the strings. Maybe I'm just prematurely
>> optimizing, though.
> 
> This code path is used to re-join/fold a header value continuation, which is
> pretty rare in the wild (if at all with modern web servers).

I think the point is that I noticed that you removed the leading whitespace
in a header's first line, but additional whitespace after this first space
will be included in the concatenated content of the header value.

As long as that is the intention, then I'm happy here.

Thanks,
-Stolee

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2022-09-26 15:31 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-13 19:25 [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 1/8] wincred: ignore unknown lines (do not die) Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 2/8] netrc: " Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 3/8] osxkeychain: clarify that we ignore unknown lines Matthew John Cheetham via GitGitGadget
2022-09-19 16:12   ` Derrick Stolee
2022-09-21 22:48     ` Matthew John Cheetham
2022-09-13 19:25 ` [PATCH 4/8] http: read HTTP WWW-Authenticate response headers Matthew John Cheetham via GitGitGadget
2022-09-19 16:21   ` Derrick Stolee
2022-09-21 22:24     ` Matthew John Cheetham
2022-09-26 14:13       ` Derrick Stolee
2022-09-13 19:25 ` [PATCH 5/8] credential: add WWW-Authenticate header to cred requests Matthew John Cheetham via GitGitGadget
2022-09-19 16:33   ` Derrick Stolee
2022-09-21 22:20     ` Matthew John Cheetham
2022-09-13 19:25 ` [PATCH 6/8] http: store all request headers on active_request_slot Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 7/8] http: move proactive auth to first slot creation Matthew John Cheetham via GitGitGadget
2022-09-13 19:25 ` [PATCH 8/8] http: set specific auth scheme depending on credential Matthew John Cheetham via GitGitGadget
2022-09-19 16:42   ` Derrick Stolee
2022-09-19 16:08 ` [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers Derrick Stolee
2022-09-19 16:44   ` Derrick Stolee
2022-09-21 22:19   ` Matthew John Cheetham
2022-09-19 23:36 ` Lessley Dennington

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).