| Message ID | pull.1352.v7.git.1674252530.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Enhance credential helper protocol to include auth headers | expand |
Matthew John Cheetham via GitGitGadget wrote: > Updates in v6 > ============= > > * Clarify the change to make logging optional in the check_dead_children() > function during libification of daemon.c. > > * Fix missing pointer dereference bugs identified in libification of child > process handling functions for daemon.c. > > * Add doc comments to child process handling function declarations in the > daemon-utils.h header. > > * Align function parameter names with variable names at callsites for > libified daemon functions. > > * Re-split out the test-http-server test helper commits in to smaller > patches: error response handling, request parsing, http-backend > pass-through, simple authentication, arbitrary header support. > > * Call out auth configuration file format for test-http-server test helper > and supported options in commit messages, as well as a test to exercise > and demonstrate these options. > > * Permit auth.token and auth.challenge to appear in any order; create the > struct auth_module just-in-time as options for that scheme are read. This > simplifies the configuration authoring of the test-http-server test > helper. > > * Update tests to use auth.allowAnoymous in the patch that introduces the > new test helper option. > > * Drop the strvec_push_nodup() commit and update the implementation of HTTP > request header line folding to use xstrdup and strvec_pop and _pushf. > > * Use size_t instead of int in credential.c when iterating over the struct > strvec credential members. Also drop the not required const and cast from > the full_key definition and free. > > * Replace in-tree test-credential-helper-reply.sh test cred helper script > with the lib-credential-helper.sh reusable 'lib' test script and shell > functions to configure the helper behaviour. > > * Leverage sed over the while read $line loop in the test credential helper > script. > > > Updates in v7 > ============= > > * Address several whitespace and arg/param list alignment issues. > > * Rethink the test-http-helper worker-mode error and result enum to be more > simple and more informative to the nature of the error. > > * Use uintmax_t to store the Content-Length of a request in the helper > test-http-server. Maintain a bit flag to store if we received such a > header. > > * Return a "400 Bad Request" HTTP response if we fail to parse the request > in the test-http-server. > > * Add test case to cover request message parsing in test-http-server. > > * Use size_t and ALLOC_ARRAY over int and CALLOC_ARRAY respectively in > get_auth_module. > > * Correctly free the split strbufs created in the header parsing loop in > test-http-server. > > * Avoid needless comparison > 0 for unsigned types. > > * Always set optional outputs to NULL if not present in test helper config > value handling. > > * Remove an accidentally commented-out test cleanup line for one test case > in t5556. I've re-read the patches in this version; all of my comments from v5 have been addressed, and the additional updates w.r.t. other reviewer feedback all look good as well. At this point, I think the series is ready for 'next'. Thanks!
Victoria Dye <vdye@github.com> writes: > Matthew John Cheetham via GitGitGadget wrote: >> Updates in v6 >> ============= >> ... > I've re-read the patches in this version; all of my comments from v5 have > been addressed, and the additional updates w.r.t. other reviewer feedback > all look good as well. At this point, I think the series is ready for > 'next'. > > Thanks! Thanks, both. Let's merge it down.
On Tue, Jan 24, 2023 at 10:03:02AM -0800, Junio C Hamano wrote: > Victoria Dye <vdye@github.com> writes: > > > Matthew John Cheetham via GitGitGadget wrote: > >> Updates in v6 > >> ============= > >> ... > > I've re-read the patches in this version; all of my comments from v5 have > > been addressed, and the additional updates w.r.t. other reviewer feedback > > all look good as well. At this point, I think the series is ready for > > 'next'. > > > > Thanks! > > Thanks, both. Let's merge it down. Sorry, I'm a bit late to the party, but I left some comments just now (this topic had been on my review backlog for ages, but I never quite got to it). Many of my comments were small bits that could be fixed on top (tiny leaks, etc). But some of my comments were of the form "no, do it totally differently". It may simply be too late for those ones, but let's see if Matthew finds anything compelling in them. -Peff
Jeff King <peff@peff.net> writes: >> Thanks, both. Let's merge it down. > > Sorry, I'm a bit late to the party, but I left some comments just now > (this topic had been on my review backlog for ages, but I never quite > got to it). > > Many of my comments were small bits that could be fixed on top (tiny > leaks, etc). But some of my comments were of the form "no, do it totally > differently". It may simply be too late for those ones, but let's see if > Matthew finds anything compelling in them. I do not mind reverting the merge to 'next' to have an improved version. Your "do we really want to add a custom server based on questionable codebase whose quality as a test-bed for real world usage is dubious?" is a valid concern.
> Future work > =========== > > In the future we can further expand the protocol to allow credential helpers > decide the best authentication scheme. Today credential helpers are still > only expected to return a username/password pair to Git, meaning the other > authentication schemes that may be offered still need challenge responses > sent via a Basic Authorization header. The changes outlined above still > permit helpers to select and configure an available authentication mode, but > require the remote for example to unpack a bearer token from a basic > challenge. > > More careful consideration is required in the handling of custom > authentication schemes which may not have a username, or may require > arbitrary additional request header values be set. > > 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[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123" > > > And then an enlightened credential helper could return over standard output: > > protocol=https > host=example.com > authtype=FooBar > username=bob@id.example.com > password=<FooBar credential> > header[]=X-FooBar: 12345 > header[]=X-FooBar-Alt: ABCDEF > > > 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> > X-FooBar: 12345 > X-FooBar-Alt: ABCDEF Interesting! Can you tell us more about how you hope to use this at GitHub? Could this be used for OAuth 2.0 Demonstrating Proof-of-Possession at the Application Layer (DPoP)? https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop (some of the fields in your example look familiar). Challenge responses are typically short lived [1]. What happens if a storage helper is configured before a challenge-response helper? We want to maintain composability of helpers. [credential] helper = storage # eg. cache or osxkeychain helper = challenge-response # eg. oauth-dpop Storage may return an expired challenge response stored earlier. This could be avoided by introducing an expiry attribute to the credential protocol. https://lore.kernel.org/git/pull.1443.git.git.1674914650588.gitgitgadget@gmail.com/T/#u A monolithic helper configured alone doesn't have this problem -- it knows which parts of its output to store or discard. Declaration of interest: I maintain a credential-generating OAuth helper composable with any storage helper. https://github.com/hickford/git-credential-oauth [1] https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop#section-8
On 2023-01-28 06:28, M Hickford wrote: >> Future work >> =========== >> >> In the future we can further expand the protocol to allow credential helpers >> decide the best authentication scheme. Today credential helpers are still >> only expected to return a username/password pair to Git, meaning the other >> authentication schemes that may be offered still need challenge responses >> sent via a Basic Authorization header. The changes outlined above still >> permit helpers to select and configure an available authentication mode, but >> require the remote for example to unpack a bearer token from a basic >> challenge. >> >> More careful consideration is required in the handling of custom >> authentication schemes which may not have a username, or may require >> arbitrary additional request header values be set. >> >> 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[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123" >> >> >> And then an enlightened credential helper could return over standard output: >> >> protocol=https >> host=example.com >> authtype=FooBar >> username=bob@id.example.com >> password=<FooBar credential> >> header[]=X-FooBar: 12345 >> header[]=X-FooBar-Alt: ABCDEF >> >> >> 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> >> X-FooBar: 12345 >> X-FooBar-Alt: ABCDEF > > Interesting! Can you tell us more about how you hope to use this at GitHub? Could this be used for OAuth 2.0 Demonstrating Proof-of-Possession at the Application Layer (DPoP)? https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop (some of the fields in your example look familiar). This would be exactly the sort of thing that this would enable. DPoP is one example where the correct auth response requires more than just a username/ password pair in the Authorization header. We should also be returning standard headers like 'Authenticate-Info' on 200 OK responses along side the 'store' calls to helpers; they could contain nonces or other important auth information. My end goal here is to extend the credential helper protocol such that that helpers can see more of the initial request challenge, and then modify the subsequent request directly, or configure and let curl handle it (the latter part not present in this iteration). One thing not called out in this doc is really the need for some capability advertisement between Git and helpers - for example if the curl version in use supports CURLOPT_XOAUTH2_BEARER for bearer tokens. > Challenge responses are typically short lived [1]. What happens if a storage helper is configured before a challenge-response helper? We want to maintain composability of helpers. > > [credential] > helper = storage # eg. cache or osxkeychain > helper = challenge-response # eg. oauth-dpop I think really this sort of thing is where the credential helper protocol isn't designed for credential-generating helpers in mind, but only simple storage-only helpers. There is no affinity between get/erase/store commands meaning one helper may return a credential for another helper to store it. Not sure if this was ever the intention, over just the need to consult a list of helpers for a stored credential. > Storage may return an expired challenge response stored earlier. This could be avoided by introducing an expiry attribute to the credential protocol. https://lore.kernel.org/git/pull.1443.git.git.1674914650588.gitgitgadget@gmail.com/T/#u > > A monolithic helper configured alone doesn't have this problem -- it knows which parts of its output to store or discard. > > Declaration of interest: I maintain a credential-generating OAuth helper composable with any storage helper. https://github.com/hickford/git-credential-oauth > > [1] https://datatracker.ietf.org/doc/html/draft-ietf-oauth-dpop#section-8
On Wed, Feb 01, 2023 at 12:15:17PM -0800, Matthew John Cheetham wrote: > > Challenge responses are typically short lived [1]. What happens if a storage helper is configured before a challenge-response helper? We want to maintain composability of helpers. > > > > [credential] > > helper = storage # eg. cache or osxkeychain > > helper = challenge-response # eg. oauth-dpop > > I think really this sort of thing is where the credential helper protocol > isn't designed for credential-generating helpers in mind, but only simple > storage-only helpers. There is no affinity between get/erase/store commands > meaning one helper may return a credential for another helper to store it. > Not sure if this was ever the intention, over just the need to consult a > list of helpers for a stored credential. I actually had envisioned helpers generating credentials. In fact, in the first iteration of the series, Git did not prompt for passwords at all! It would depend on git-credential-prompt to do so. But I ended up folding that in for simplicity. I could well believe that there is not enough context passed around for helpers to make good decisions, though. It's both a feature and a bug that credentials from one helper get passed to another. It's good if you want to cache a generated credential. It's bad if you don't want credentials from one helper to leak to another, less-secure one. So I'm open to improvements that help define and communicate that context. -Peff
Hi Junio & Peff, On Thu, 26 Jan 2023, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > >> Thanks, both. Let's merge it down. > > > > Sorry, I'm a bit late to the party, but I left some comments just now > > (this topic had been on my review backlog for ages, but I never quite > > got to it). > > > > Many of my comments were small bits that could be fixed on top (tiny > > leaks, etc). But some of my comments were of the form "no, do it totally > > differently". It may simply be too late for those ones, but let's see if > > Matthew finds anything compelling in them. > > I do not mind reverting the merge to 'next' to have an improved > version. Your "do we really want to add a custom server based on > questionable codebase whose quality as a test-bed for real world > usage is dubious?" is a valid concern. Except. Except that this code base would have made for a fine base to potentially implement an HTTPS-based replacement for the aging and insecure git-daemon. That code base (which is hardly as questionable codebase as you make it sound because it has been in use for years in a slightly different form) would have had the opportunity to mature in a relatively safe environment: our test suite. And eventually, once robust enough, it could have been extended to allow for easy and painless yet secure ad-hoc serving of Git repositories, addressing the security concerns around git-daemon. And now that we're throwing out that code we don't have that opportunity, making the goal to deprecate the git-daemon and replace it by something that is as easy to set up but talks HTTPS instead much, much harder to reach. In addition, it causes a loss of test coverage because Apache is not available in all the setups where the "questionable" code would have had no problem being built and validating the credential code. Windows, for example, will now go completely uncovered in CI regarding the new code. Ciao, Johannes
On Thu, Feb 02 2023, Johannes Schindelin wrote: > Hi Junio & Peff, > > On Thu, 26 Jan 2023, Junio C Hamano wrote: > >> Jeff King <peff@peff.net> writes: >> >> >> Thanks, both. Let's merge it down. >> > >> > Sorry, I'm a bit late to the party, but I left some comments just now >> > (this topic had been on my review backlog for ages, but I never quite >> > got to it). >> > >> > Many of my comments were small bits that could be fixed on top (tiny >> > leaks, etc). But some of my comments were of the form "no, do it totally >> > differently". It may simply be too late for those ones, but let's see if >> > Matthew finds anything compelling in them. >> >> I do not mind reverting the merge to 'next' to have an improved >> version. Your "do we really want to add a custom server based on >> questionable codebase whose quality as a test-bed for real world >> usage is dubious?" is a valid concern. > > Except. > > Except that this code base would have made for a fine base to potentially > implement an HTTPS-based replacement for the aging and insecure > git-daemon. > > That code base (which is hardly as questionable codebase as you make it > sound because it has been in use for years in a slightly different form) > would have had the opportunity to mature in a relatively safe environment: > our test suite. And eventually, once robust enough, it could have been > extended to allow for easy and painless yet secure ad-hoc serving of Git > repositories, addressing the security concerns around git-daemon. > > And now that we're throwing out that code we don't have that opportunity, > making the goal to deprecate the git-daemon and replace it by something > that is as easy to set up but talks HTTPS instead much, much harder to > reach. There's many reasons for why you almost never see a git:// URL in the wild anymore. But if "easy and painless" was synonymous with "built with git" or "ships with git" as you seem to be using it, surely it would be more common than doing the same with http or https, which requires an external server? So, easy for whom? Just us and our own test suite? Having read both your reply & Jeff's[1] I don't think you're addressing the thrust of his argument. You can share all those goals without the method of getting there requiring us to start maintaining our own webserver. > In addition, it causes a loss of test coverage because Apache is not > available in all the setups where the "questionable" code would have had > no problem being built and validating the credential code. > > Windows, for example, will now go completely uncovered in CI regarding the > new code. I have not set up Apache on Windows, but binaries seem to be available for it[2]. We don't use those now, but is downloading, setting up & running them in CI really harder than emarking on a project of maintaining our own webserver, especially we've got an eye towards non-test suite use? Even if we think that we'd like to have a webserver built when you "make git" I don't see why we'd go the NIH route of writing our own. Unlike the git:// protocol there's a *lot* of implementations of http(s):// servers. If we think apache is too heavyweight for whatever reason, can't we add one of the many light http servers out there to contrib/ use it it from there? 1. https://lore.kernel.org/git/Y9JA0UCRh7qUqKQI@coredump.intra.peff.net/ 2. https://httpd.apache.org/docs/2.4/platform/windows.html
Hi Ævar, On Thu, 2 Feb 2023, Ævar Arnfjörð Bjarmason wrote: > On Thu, Feb 02 2023, Johannes Schindelin wrote: > > > On Thu, 26 Jan 2023, Junio C Hamano wrote: > > > >> Jeff King <peff@peff.net> writes: > >> > >> >> Thanks, both. Let's merge it down. > >> > > >> > Sorry, I'm a bit late to the party, but I left some comments just now > >> > (this topic had been on my review backlog for ages, but I never quite > >> > got to it). > >> > > >> > Many of my comments were small bits that could be fixed on top (tiny > >> > leaks, etc). But some of my comments were of the form "no, do it totally > >> > differently". It may simply be too late for those ones, but let's see if > >> > Matthew finds anything compelling in them. > >> > >> I do not mind reverting the merge to 'next' to have an improved > >> version. Your "do we really want to add a custom server based on > >> questionable codebase whose quality as a test-bed for real world > >> usage is dubious?" is a valid concern. > > > > Except. > > > > Except that this code base would have made for a fine base to potentially > > implement an HTTPS-based replacement for the aging and insecure > > git-daemon. > > > > That code base (which is hardly as questionable codebase as you make it > > sound because it has been in use for years in a slightly different form) > > would have had the opportunity to mature in a relatively safe environment: > > our test suite. And eventually, once robust enough, it could have been > > extended to allow for easy and painless yet secure ad-hoc serving of Git > > repositories, addressing the security concerns around git-daemon. > > > > And now that we're throwing out that code we don't have that opportunity, > > making the goal to deprecate the git-daemon and replace it by something > > that is as easy to set up but talks HTTPS instead much, much harder to > > reach. > > There's many reasons for why you almost never see a git:// URL in the > wild anymore. I am unwilling to accept that statement without any source to back it up. Thin air is no substitute for reliable evidence. > But if "easy and painless" was synonymous with "built with git" or > "ships with git" as you seem to be using it, surely it would be more > common than doing the same with http or https, which requires an > external server? Oh whoa... "requires an external server"? My entire point was to suggest a way forward for an _internal_ server that speaks https:// instead of git://. So I am not suggesting what you seem to have understood me to suggest. Ciao, Johannes
On Thu, Feb 02, 2023 at 11:14:33AM +0100, Johannes Schindelin wrote: > > I do not mind reverting the merge to 'next' to have an improved > > version. Your "do we really want to add a custom server based on > > questionable codebase whose quality as a test-bed for real world > > usage is dubious?" is a valid concern. > > Except. > > Except that this code base would have made for a fine base to potentially > implement an HTTPS-based replacement for the aging and insecure > git-daemon. I'm skeptical that it is a good idea for Git to implement a custom http server from scratch. There are a lot of extended features people would want in a production-ready server. TLS, HTTP/2, and so on. The code under discussion is pretty stripped-down. A network service is also a pretty big attack surface for buffer overflows, etc. It feels to me like the resources required to make it good enough for normal users to run would be substantial. And we'd be better off trying to integrate with an existing project that provides a web server (whether it's a lightweight server that supports us as a CGI, or a library that does most of the heavy lifting). > That code base (which is hardly as questionable codebase as you make it > sound because it has been in use for years in a slightly different form) Perhaps I'm being too hard on git-daemon. But my operational experience with it is that it has several flaws, mostly around the child-management code. We rewrote that code totally to make it usable at GitHub. As a concrete example, the parent daemon process will do a linear walk over all children, calling waitpid() on each one. This makes handling N children quadratic, and the daemon grinds to a halt when there are many clients. > In addition, it causes a loss of test coverage because Apache is not > available in all the setups where the "questionable" code would have had > no problem being built and validating the credential code. > > Windows, for example, will now go completely uncovered in CI regarding the > new code. I'm sympathetic there, though it's a problem for all of the existing http code, too. Is there some server option that would be easier to run everywhere, but that doesn't involve us writing a server from scratch? Certainly I have no particular love for apache or its configuration language. -Peff
On Thu, Feb 02 2023, Johannes Schindelin wrote: > Hi Ævar, > > On Thu, 2 Feb 2023, Ævar Arnfjörð Bjarmason wrote: > >> On Thu, Feb 02 2023, Johannes Schindelin wrote: >> >> > On Thu, 26 Jan 2023, Junio C Hamano wrote: >> > >> >> Jeff King <peff@peff.net> writes: >> >> >> >> >> Thanks, both. Let's merge it down. >> >> > >> >> > Sorry, I'm a bit late to the party, but I left some comments just now >> >> > (this topic had been on my review backlog for ages, but I never quite >> >> > got to it). >> >> > >> >> > Many of my comments were small bits that could be fixed on top (tiny >> >> > leaks, etc). But some of my comments were of the form "no, do it totally >> >> > differently". It may simply be too late for those ones, but let's see if >> >> > Matthew finds anything compelling in them. >> >> >> >> I do not mind reverting the merge to 'next' to have an improved >> >> version. Your "do we really want to add a custom server based on >> >> questionable codebase whose quality as a test-bed for real world >> >> usage is dubious?" is a valid concern. >> > >> > Except. >> > >> > Except that this code base would have made for a fine base to potentially >> > implement an HTTPS-based replacement for the aging and insecure >> > git-daemon. >> > >> > That code base (which is hardly as questionable codebase as you make it >> > sound because it has been in use for years in a slightly different form) >> > would have had the opportunity to mature in a relatively safe environment: >> > our test suite. And eventually, once robust enough, it could have been >> > extended to allow for easy and painless yet secure ad-hoc serving of Git >> > repositories, addressing the security concerns around git-daemon. >> > >> > And now that we're throwing out that code we don't have that opportunity, >> > making the goal to deprecate the git-daemon and replace it by something >> > that is as easy to set up but talks HTTPS instead much, much harder to >> > reach. >> >> There's many reasons for why you almost never see a git:// URL in the >> wild anymore. > > I am unwilling to accept that statement without any source to back it up. > Thin air is no substitute for reliable evidence. Most people exposing git over the Internet use the ssh or http transport, and our own "git" protocol is relatively obscure. If you need data I think major hosting sites not offering it, or deprecating it, is a pretty strong signal, e.g. Microsoft with: https://github.blog/2021-09-01-improving-git-protocol-security-github/#no-more-unauthenticated-git But if you'll grant me that it's 50/50 git/other protocols (I think it's a *lot* more lopsided), then clearly combining git with 3rd party server components isn't the limiting factor on deploying it. Which is the point I was going for. >> But if "easy and painless" was synonymous with "built with git" or >> "ships with git" as you seem to be using it, surely it would be more >> common than doing the same with http or https, which requires an >> external server? > > Oh whoa... "requires an external server"? > > My entire point was to suggest a way forward for an _internal_ server that > speaks https:// instead of git://. I understand that. > So I am not suggesting what you seem to have understood me to suggest. I wasn't suggesting that, and you seem to have not read my reply to the end, which should have addressed that. Briefly, we'd like to be guaranteed to have regcomp() and regexec(), but did the Git project write its own regex engine? No, we imported (with some minor tweaks) one from glibc/gawk (whatever current issues have cropped up with it lately...). So can't we do the same for a httpd? If it really comes to "we must have it in-tree"? It seems to me that there's a continuum here, which is at the very least: 1) We require an external package (e.g. ssh, or apache/httpd) 2) We require an external package *or* built-in (e.g. our SHA-1 implementations) 3) We use an external package as-is (sha1dc) 4) We adapt an external codebase, and perma-fork it (git-imap-send, although that example also kind of sucks) 5) We write it "in-house" from scratch. It seems to me from reading the upthread that we're jumping straight from #1 to #5, and it's not clear to me why that is. Not even that, we currently have CI tests running Apache on *nix boxes, but you're suggesting a loss of coverage on Windows Is it really harder to just install (or even ship our own package of) Apache for Windows than it is to embark on PID file handling, logging, timeout management and the long tail of "80% is easy, the rest is really hard" of writing our own production-class httpd (as the suggestion is to have it eventually mature beyond the test suite)? Maybe, all I'm saying (in trying to mediate the discussion between you and Jeff) is that it's not obvious to me why that is...
Hi Ævar, On Mon, 6 Feb 2023, Ævar Arnfjörð Bjarmason wrote: > we currently have CI tests running Apache on *nix boxes, but you're > suggesting a loss of coverage on Windows > > Is it really harder to just install (or even ship our own package of) > Apache for Windows than it is to embark on PID file handling, logging, > timeout management and the long tail of "80% is easy, the rest is really > hard" of writing our own production-class httpd (as the suggestion is to > have it eventually mature beyond the test suite)? Yes, it _is_ that much harder, and it would result in yet more painful increases of the build times which have really gotten out of hand in the past year. Ciao, Johannes
Hi Jeff, On Fri, 3 Feb 2023, Jeff King wrote: > On Thu, Feb 02, 2023 at 11:14:33AM +0100, Johannes Schindelin wrote: > > > > I do not mind reverting the merge to 'next' to have an improved > > > version. Your "do we really want to add a custom server based on > > > questionable codebase whose quality as a test-bed for real world > > > usage is dubious?" is a valid concern. > > > > Except. > > > > Except that this code base would have made for a fine base to potentially > > implement an HTTPS-based replacement for the aging and insecure > > git-daemon. > > I'm skeptical that it is a good idea for Git to implement a custom http > server from scratch. To be clear: I never suggested to implement a generic HTTP server. All I wanted was to have a replacement for `git daemon` that speaks https:// instead of git://. It does not have to speak to every browser out there, it only needs to respond well when speaking to Git clients. That is a much, much smaller surface than "production-ready server, HTTP/2 and so on". And while the proposed test helper was not quite complete in that way, and while it may have had much of the `git daemon` code that you would love to lose, it would have offered an incremental way forward. I am afraid that this way forward is now blocked, and we're further away from dropping that `git daemon` code you wanted to drop than ever. Ciao, Johannes
On Mon, Mar 27, 2023 at 11:10:40AM +0200, Johannes Schindelin wrote: > > I'm skeptical that it is a good idea for Git to implement a custom http > > server from scratch. > > To be clear: I never suggested to implement a generic HTTP server. > > All I wanted was to have a replacement for `git daemon` that speaks > https:// instead of git://. It does not have to speak to every browser out > there, it only needs to respond well when speaking to Git clients. That is > a much, much smaller surface than "production-ready server, HTTP/2 and so > on". I guess I don't see the point of having this in our test suite, though. We do want to test things like HTTP/2, SSL, and so on in our test suite. So either we have a split in our tests (some use apache, some don't, which presumably means many tests are still not run on Windows), or this custom HTTP server eventually grows to do all of those other things. I can see the utility outside the tests of a quick "let me stand up an HTTP server to access Git" tool. But even there, I'd be considered with feature creep as regular users ignore any warnings about its lack of encryption/robustness, and so on. And it feels like something that could utilize work already done by others in making a web server. Yes, that's a new dependency for the tool, but there are a lot of options out there. Surely one of them is worth building on? > And while the proposed test helper was not quite complete in that way, and > while it may have had much of the `git daemon` code that you would love to > lose, it would have offered an incremental way forward. > > I am afraid that this way forward is now blocked, and we're further away > from dropping that `git daemon` code you wanted to drop than ever. I don't see how pushing the same code into an http server helps. If we could have incrementally improved it there, we could incrementally improve it in git-daemon, too. -Peff
Following from my original RFC submission [0], this submission is considered ready for full review. This patch series is now based on top of current master (9c32cfb49c60fa8173b9666db02efe3b45a8522f) that includes my now separately submitted patches [1] to fix up the other credential helpers' behaviour. In this patch series I 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. Testing these new additions, I introduce a new test helper test-http-server that acts as a frontend to git-http-backend; a mini HTTP server sharing code with git-daemon, with simple authentication configurable by a config file. Background ========== Git uses a variety of protocols [2]: 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) [3], and Negotiate (RFC 2478) [4]. Git uses a extensible model where credential helpers can provide credentials for protocols [5]. Several helpers support alternatives such as OAuth authentication (RFC 6749) [6], 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) [7]: 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. * Allow credential helpers to differentiate between different authorities or authentication/authorization challenge types, even from the same DNS hostname (and without needing to use credential.useHttpPath). * 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: * 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[]=value, where the order of the key=value pairs in the stream specifies the order. For the WWW-Authenticate header values we opt to use the key wwwauth[]. Handling the WWW-Authenticate header in detail ============================================== RFC 6750 [8] 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 [9] 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 [9] authority, a credential helper can use OpenID Connect's Discovery [10] and Dynamic Client Registration [11] 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[]=Bearer realm="login.example", scope="git.readwrite" wwwauth[]=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 [12]). 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 work =========== In the future we can further expand the protocol to allow credential helpers decide the best authentication scheme. Today credential helpers are still only expected to return a username/password pair to Git, meaning the other authentication schemes that may be offered still need challenge responses sent via a Basic Authorization header. The changes outlined above still permit helpers to select and configure an available authentication mode, but require the remote for example to unpack a bearer token from a basic challenge. More careful consideration is required in the handling of custom authentication schemes which may not have a username, or may require arbitrary additional request header values be set. 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[]=FooBar realm="login.example", algs="ES256 PS256", nonce="abc123" And then an enlightened credential helper could return over standard output: protocol=https host=example.com authtype=FooBar username=bob@id.example.com password=<FooBar credential> header[]=X-FooBar: 12345 header[]=X-FooBar-Alt: ABCDEF 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> X-FooBar: 12345 X-FooBar-Alt: ABCDEF 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 ========== * [0] [PATCH 0/8] [RFC] Enhance credential helper protocol to include auth headers https://lore.kernel.org/git/pull.1352.git.1663097156.gitgitgadget@gmail.com/ * [1] [PATCH 0/3] Correct credential helper discrepancies handling input https://lore.kernel.org/git/pull.1363.git.1663865974.gitgitgadget@gmail.com/ * [2] Git on the Server - The Protocols https://git-scm.com/book/en/v2/Git-on-the-Server-The-Protocols * [3] HTTP Authentication: Basic and Digest Access Authentication https://datatracker.ietf.org/doc/html/rfc2617 * [4] The Simple and Protected GSS-API Negotiation Mechanism https://datatracker.ietf.org/doc/html/rfc2478 * [5] Git Credentials - Custom Helpers https://git-scm.com/docs/gitcredentials#_custom_helpers * [6] The OAuth 2.0 Authorization Framework https://datatracker.ietf.org/doc/html/rfc6749 * [7] Hypertext Transfer Protocol (HTTP/1.1): Authentication https://datatracker.ietf.org/doc/html/rfc7235 * [8] The OAuth 2.0 Authorization Framework: Bearer Token Usage https://datatracker.ietf.org/doc/html/rfc6750 * [9] OpenID Connect Core 1.0 https://openid.net/specs/openid-connect-core-1_0.html * [10] OpenID Connect Discovery 1.0 https://openid.net/specs/openid-connect-discovery-1_0.html * [11] OpenID Connect Dynamic Client Registration 1.0 https://openid.net/specs/openid-connect-registration-1_0.html * [12] Hypertext Transfer Protocol (HTTP/1.1) https://datatracker.ietf.org/doc/html/rfc2616 Updates from RFC ================ * Submitted first three patches as separate submission: https://lore.kernel.org/git/pull.1363.git.1663865974.gitgitgadget@gmail.com/ * Various style fixes and updates to- and addition of comments. * Drop the explicit integer index in new 'array' style credential helper attrbiutes ("key[n]=value" becomes just "key[]=value"). * Added test helper; a mini HTTP server, and several tests. Updates in v3 ============= * Split final patch that added the test-http-server in to several, easier to review patches. * Updated wording in git-credential.txt to clarify which side of the credential helper protocol is sending/receiving the new wwwauth and authtype attributes. Updates in v4 ============= * Drop authentication scheme selection authtype attribute patches to greatly simplify the series; auth scheme selection is punted to a future series. This series still allows credential helpers to generate credentials and intelligently select correct identities for a given auth challenge. Updates in v5 ============= * Libify parts of daemon.c and share implementation with test-http-server. * Clarify test-http-server Git request regex pattern and auth logic comments. * Use STD*_FILENO in place of 'magic' file descriptor numbers. * Use strbuf_* functions in continuation header parsing. * Use configuration file to configure auth for test-http-server rather than command-line arguments. Add ability to specify arbitrary extra headers that is useful for testing 'malformed' server responses. * Use st_mult over unchecked multiplication in http.c curl callback functions. * Fix some documentation line break issues. * Reorder some commits to bring in the tests and test-http-server helper first and, then the WWW-Authentication changes, alongside tests to cover. * Expose previously static strvec_push_nodup function. * Merge the two timeout args for test-http-server (--timeout and --init-timeout) that were a hang-over from the original daemon.c but are no longer required here. * Be more careful around continuation headers where they may be empty strings. Add more tests to cover these header types. * Include standard trace2 tracing calls at start of test-http-server helper. Updates in v6 ============= * Clarify the change to make logging optional in the check_dead_children() function during libification of daemon.c. * Fix missing pointer dereference bugs identified in libification of child process handling functions for daemon.c. * Add doc comments to child process handling function declarations in the daemon-utils.h header. * Align function parameter names with variable names at callsites for libified daemon functions. * Re-split out the test-http-server test helper commits in to smaller patches: error response handling, request parsing, http-backend pass-through, simple authentication, arbitrary header support. * Call out auth configuration file format for test-http-server test helper and supported options in commit messages, as well as a test to exercise and demonstrate these options. * Permit auth.token and auth.challenge to appear in any order; create the struct auth_module just-in-time as options for that scheme are read. This simplifies the configuration authoring of the test-http-server test helper. * Update tests to use auth.allowAnoymous in the patch that introduces the new test helper option. * Drop the strvec_push_nodup() commit and update the implementation of HTTP request header line folding to use xstrdup and strvec_pop and _pushf. * Use size_t instead of int in credential.c when iterating over the struct strvec credential members. Also drop the not required const and cast from the full_key definition and free. * Replace in-tree test-credential-helper-reply.sh test cred helper script with the lib-credential-helper.sh reusable 'lib' test script and shell functions to configure the helper behaviour. * Leverage sed over the while read $line loop in the test credential helper script. Updates in v7 ============= * Address several whitespace and arg/param list alignment issues. * Rethink the test-http-helper worker-mode error and result enum to be more simple and more informative to the nature of the error. * Use uintmax_t to store the Content-Length of a request in the helper test-http-server. Maintain a bit flag to store if we received such a header. * Return a "400 Bad Request" HTTP response if we fail to parse the request in the test-http-server. * Add test case to cover request message parsing in test-http-server. * Use size_t and ALLOC_ARRAY over int and CALLOC_ARRAY respectively in get_auth_module. * Correctly free the split strbufs created in the header parsing loop in test-http-server. * Avoid needless comparison > 0 for unsigned types. * Always set optional outputs to NULL if not present in test helper config value handling. * Remove an accidentally commented-out test cleanup line for one test case in t5556. Matthew John Cheetham (12): daemon: libify socket setup and option functions daemon: libify child process handling functions daemon: rename some esoteric/laboured terminology test-http-server: add stub HTTP server test helper test-http-server: add HTTP error response function test-http-server: add HTTP request parsing test-http-server: pass Git requests to http-backend test-http-server: add simple authentication test-http-server: add sending of arbitrary headers http: replace unsafe size_t multiplication with st_mult http: read HTTP WWW-Authenticate response headers credential: add WWW-Authenticate header to cred requests Documentation/git-credential.txt | 19 +- Makefile | 2 + contrib/buildsystems/CMakeLists.txt | 11 +- credential.c | 12 + credential.h | 15 + daemon-utils.c | 286 +++++++++ daemon-utils.h | 55 ++ daemon.c | 306 +-------- http.c | 98 ++- t/helper/.gitignore | 1 + t/helper/test-http-server.c | 943 ++++++++++++++++++++++++++++ t/lib-credential-helper.sh | 27 + t/t5556-http-auth.sh | 463 ++++++++++++++ 13 files changed, 1936 insertions(+), 302 deletions(-) create mode 100644 daemon-utils.c create mode 100644 daemon-utils.h create mode 100644 t/helper/test-http-server.c create mode 100644 t/lib-credential-helper.sh create mode 100755 t/t5556-http-auth.sh base-commit: c48035d29b4e524aed3a32f0403676f0d9128863 Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1352%2Fmjcheetham%2Femu-v7 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1352/mjcheetham/emu-v7 Pull-Request: https://github.com/gitgitgadget/git/pull/1352 Range-diff vs v6: 1: 74b0de14185 = 1: 74b0de14185 daemon: libify socket setup and option functions 2: b6ba344a671 = 2: b6ba344a671 daemon: libify child process handling functions 3: 9967401c972 = 3: 9967401c972 daemon: rename some esoteric/laboured terminology 4: d6e5e8825e8 ! 4: 17c890ee108 test-http-server: add stub HTTP server test helper @@ t/helper/test-http-server.c (new) + +/* + * The code in this section is used by "worker" instances to service -+ * a single connection from a client. The worker talks to the client -+ * on 0 and 1. ++ * a single connection from a client. The worker talks to the client ++ * on stdin and stdout. + */ + +enum worker_result { @@ t/helper/test-http-server.c (new) + * Operation successful. + * Caller *might* keep the socket open and allow keep-alive. + */ -+ WR_OK = 0, ++ WR_OK = 0, + + /* -+ * Various errors while processing the request and/or the response. ++ * Fatal error that is not recoverable. + * Close the socket and clean up. + * Exit child-process with non-zero status. + */ -+ WR_IO_ERROR = 1<<0, -+ -+ /* -+ * Close the socket and clean up. Does not imply an error. -+ */ -+ WR_HANGUP = 1<<1, ++ WR_FATAL_ERROR = 1, +}; + +static enum worker_result worker(void) @@ t/helper/test-http-server.c (new) + while (1) { + if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) { + logerror("unable to write response"); -+ wr = WR_IO_ERROR; ++ wr = WR_FATAL_ERROR; + } + + if (wr != WR_OK) @@ t/helper/test-http-server.c (new) + close(STDIN_FILENO); + close(STDOUT_FILENO); + -+ return !!(wr & WR_IO_ERROR); ++ /* Only WR_OK should result in a non-zero exit code */ ++ return wr != WR_OK; +} + +static int max_connections = 32; 5: 79805f042b9 ! 5: 6e70e304cfe test-http-server: add HTTP error response function @@ Commit message ## t/helper/test-http-server.c ## @@ t/helper/test-http-server.c: enum worker_result { - WR_HANGUP = 1<<1, + * Exit child-process with non-zero status. + */ + WR_FATAL_ERROR = 1, ++ ++ /* ++ * Close the socket and clean up. Does not imply an error. ++ */ ++ WR_HANGUP = 2, }; -+static enum worker_result send_http_error( -+ int fd, -+ int http_code, const char *http_code_name, -+ int retry_after_seconds, struct string_list *response_headers, -+ enum worker_result wr_in) ++static enum worker_result send_http_error(int fd, int http_code, ++ const char *http_code_name, ++ int retry_after_seconds, ++ struct string_list *response_headers, ++ enum worker_result wr_in) +{ + struct strbuf response_header = STRBUF_INIT; + struct strbuf response_content = STRBUF_INIT; + struct string_list_item *h; + enum worker_result wr; + -+ strbuf_addf(&response_content, "Error: %d %s\r\n", -+ http_code, http_code_name); ++ strbuf_addf(&response_content, "Error: %d %s\r\n", http_code, ++ http_code_name); ++ + if (retry_after_seconds > 0) + strbuf_addf(&response_content, "Retry-After: %d\r\n", + retry_after_seconds); + -+ strbuf_addf (&response_header, "HTTP/1.1 %d %s\r\n", http_code, http_code_name); ++ strbuf_addf(&response_header, "HTTP/1.1 %d %s\r\n", http_code, ++ http_code_name); + strbuf_addstr(&response_header, "Cache-Control: private\r\n"); -+ strbuf_addstr(&response_header, "Content-Type: text/plain\r\n"); -+ strbuf_addf (&response_header, "Content-Length: %d\r\n", (int)response_content.len); ++ strbuf_addstr(&response_header, "Content-Type: text/plain\r\n"); ++ strbuf_addf(&response_header, "Content-Length: %"PRIuMAX"\r\n", ++ (uintmax_t)response_content.len); ++ + if (retry_after_seconds > 0) -+ strbuf_addf(&response_header, "Retry-After: %d\r\n", retry_after_seconds); -+ strbuf_addf( &response_header, "Server: test-http-server/%s\r\n", git_version_string); -+ strbuf_addf( &response_header, "Date: %s\r\n", show_date(time(NULL), 0, DATE_MODE(RFC2822))); ++ strbuf_addf(&response_header, "Retry-After: %d\r\n", ++ retry_after_seconds); ++ ++ strbuf_addf(&response_header, "Server: test-http-server/%s\r\n", ++ git_version_string); ++ strbuf_addf(&response_header, "Date: %s\r\n", show_date(time(NULL), 0, ++ DATE_MODE(RFC2822))); ++ + if (response_headers) + for_each_string_list_item(h, response_headers) + strbuf_addf(&response_header, "%s\r\n", h->string); @@ t/helper/test-http-server.c: enum worker_result { + + if (write_in_full(fd, response_header.buf, response_header.len) < 0) { + logerror("unable to write response header"); -+ wr = WR_IO_ERROR; ++ wr = WR_FATAL_ERROR; + goto done; + } + + if (write_in_full(fd, response_content.buf, response_content.len) < 0) { + logerror("unable to write response content body"); -+ wr = WR_IO_ERROR; ++ wr = WR_FATAL_ERROR; + goto done; + } + @@ t/helper/test-http-server.c: static enum worker_result worker(void) while (1) { - if (write_in_full(STDOUT_FILENO, response, strlen(response)) < 0) { - logerror("unable to write response"); -- wr = WR_IO_ERROR; +- wr = WR_FATAL_ERROR; - } + wr = send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, -+ NULL, WR_OK | WR_HANGUP); ++ NULL, WR_HANGUP); if (wr != WR_OK) break; +@@ t/helper/test-http-server.c: static enum worker_result worker(void) + close(STDIN_FILENO); + close(STDOUT_FILENO); + +- /* Only WR_OK should result in a non-zero exit code */ +- return wr != WR_OK; ++ /* Only WR_OK and WR_HANGUP should result in a non-zero exit code */ ++ return wr != WR_OK && wr != WR_HANGUP; + } + + static int max_connections = 32; 6: 252098db219 ! 6: 43f1cdcbb82 test-http-server: add HTTP request parsing @@ Commit message test-http-server: add HTTP request parsing Add ability to parse HTTP requests to the test-http-server test helper. + Introduce `struct req` to store request information including: + + * HTTP method & version + * Request path and query parameters + * Headers + * Content type and length (from `Content-Type` and `-Length` headers) + + Failure to parse the request results in a 400 Bad Request response to + the client. Note that we're not trying to support all possible requests + here, but just enough to exercise all code under test. Signed-off-by: Matthew John Cheetham <mjcheetham@outlook.com> ## t/helper/test-http-server.c ## @@ t/helper/test-http-server.c: enum worker_result { - WR_HANGUP = 1<<1, - }; - + * Close the socket and clean up. Does not imply an error. + */ + WR_HANGUP = 2, ++ ++ /* ++ * Unexpected request message or error in request parsing. ++ * Respond with an 400 error. Close the socket and cleanup. ++ * Exit child-process with a non-zero status. ++ */ ++ WR_CLIENT_ERROR = 3, ++}; ++ +/* + * Fields from a parsed HTTP request. + */ @@ t/helper/test-http-server.c: enum worker_result { + + struct string_list header_list; + const char *content_type; -+ ssize_t content_length; -+}; -+ ++ uintmax_t content_length; ++ unsigned has_content_length:1; + }; + +#define REQ__INIT { \ + .start_line = STRBUF_INIT, \ + .uri_path = STRBUF_INIT, \ + .query_args = STRBUF_INIT, \ + .header_list = STRING_LIST_INIT_NODUP, \ + .content_type = NULL, \ -+ .content_length = -1 \ -+ } ++ .content_length = 0, \ ++ .has_content_length = 0, \ ++} + +static void req__release(struct req *req) +{ @@ t/helper/test-http-server.c: enum worker_result { + string_list_clear(&req->header_list, 0); +} + - static enum worker_result send_http_error( - int fd, - int http_code, const char *http_code_name, + static enum worker_result send_http_error(int fd, int http_code, + const char *http_code_name, + int retry_after_seconds, @@ t/helper/test-http-server.c: done: return wr; } @@ t/helper/test-http-server.c: done: + * + */ + if (strbuf_getwholeline_fd(&req->start_line, fd, '\n') == EOF) { -+ result = WR_OK | WR_HANGUP; ++ result = WR_HANGUP; + goto done; + } + @@ t/helper/test-http-server.c: done: + if (nr_start_line_fields != 3) { + logerror("could not parse request start-line '%s'", + req->start_line.buf); -+ result = WR_IO_ERROR; ++ result = WR_CLIENT_ERROR; + goto done; + } + @@ t/helper/test-http-server.c: done: + if (strcmp(req->http_version, "HTTP/1.1")) { + logerror("unsupported version '%s' (expecting HTTP/1.1)", + req->http_version); -+ result = WR_IO_ERROR; ++ result = WR_CLIENT_ERROR; + goto done; + } + @@ t/helper/test-http-server.c: done: + string_list_append(&req->header_list, hp); + + /* also store common request headers as struct req members */ -+ if (skip_prefix(hp, "Content-Type: ", &hv)) { ++ if (skip_iprefix(hp, "Content-Type: ", &hv)) { + req->content_type = hv; -+ } else if (skip_prefix(hp, "Content-Length: ", &hv)) { -+ req->content_length = strtol(hv, &hp, 10); ++ } else if (skip_iprefix(hp, "Content-Length: ", &hv)) { ++ /* ++ * Content-Length is always non-negative, but has no ++ * upper bound according to RFC 7230 (§3.3.2). ++ */ ++ intmax_t len = 0; ++ if (sscanf(hv, "%"PRIdMAX, &len) != 1 || len < 0 || ++ len == INTMAX_MAX) { ++ logerror("invalid content-length: '%s'", hv); ++ result = WR_CLIENT_ERROR; ++ goto done; ++ } ++ ++ req->content_length = (uintmax_t)len; ++ req->has_content_length = 1; + } + } + @@ t/helper/test-http-server.c: done: + trace2_printf("%s: hmth: %s", TR2_CAT, req->method); + trace2_printf("%s: path: %s", TR2_CAT, req->uri_path.buf); + trace2_printf("%s: qury: %s", TR2_CAT, req->query_args.buf); -+ if (req->content_length >= 0) -+ trace2_printf("%s: clen: %d", TR2_CAT, req->content_length); ++ if (req->has_content_length) ++ trace2_printf("%s: clen: %"PRIuMAX, TR2_CAT, ++ req->content_length); + if (req->content_type) + trace2_printf("%s: ctyp: %s", TR2_CAT, req->content_type); + for_each_string_list_item(item, &req->header_list) @@ t/helper/test-http-server.c: done: +static enum worker_result dispatch(struct req *req) +{ + return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL, -+ WR_OK | WR_HANGUP); ++ WR_HANGUP); +} + static enum worker_result worker(void) @@ t/helper/test-http-server.c: static enum worker_result worker(void) while (1) { - wr = send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, -- NULL, WR_OK | WR_HANGUP); +- NULL, WR_HANGUP); + req__release(&req); + + alarm(timeout); + wr = req__read(&req, 0); + alarm(0); + ++ if (wr == WR_CLIENT_ERROR) ++ wr = send_http_error(STDOUT_FILENO, 400, "Bad Request", ++ -1, NULL, wr); ++ + if (wr != WR_OK) + break; @@ t/helper/test-http-server.c: static enum worker_result worker(void) if (wr != WR_OK) break; } + + ## t/t5556-http-auth.sh (new) ## +@@ ++#!/bin/sh ++ ++test_description='test http auth header and credential helper interop' ++ ++TEST_NO_CREATE_REPO=1 ++. ./test-lib.sh ++ ++# Setup a repository ++# ++REPO_DIR="$TRASH_DIRECTORY"/repo ++ ++SERVER_LOG="$TRASH_DIRECTORY"/OUT.server.log ++ ++PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH ++ ++test_expect_success 'setup repos' ' ++ test_create_repo "$REPO_DIR" && ++ git -C "$REPO_DIR" branch -M main ++' ++ ++run_http_server_worker() { ++ ( ++ cd "$REPO_DIR" ++ test-http-server --worker "$@" 2>"$SERVER_LOG" | tr -d "\r" ++ ) ++} ++ ++per_test_cleanup () { ++ rm -f OUT.* && ++ rm -f IN.* && ++} ++ ++test_expect_success 'http auth server request parsing' ' ++ test_when_finished "per_test_cleanup" && ++ ++ cat >auth.config <<-EOF && ++ [auth] ++ allowAnonymous = true ++ EOF ++ ++ echo "HTTP/1.1 400 Bad Request" >OUT.http400 && ++ echo "HTTP/1.1 200 OK" >OUT.http200 && ++ ++ cat >IN.http.valid <<-EOF && ++ GET /info/refs HTTP/1.1 ++ Content-Length: 0 ++ EOF ++ ++ cat >IN.http.badfirstline <<-EOF && ++ /info/refs GET HTTP ++ EOF ++ ++ cat >IN.http.badhttpver <<-EOF && ++ GET /info/refs HTTP/999.9 ++ EOF ++ ++ cat >IN.http.ltzlen <<-EOF && ++ GET /info/refs HTTP/1.1 ++ Content-Length: -1 ++ EOF ++ ++ cat >IN.http.badlen <<-EOF && ++ GET /info/refs HTTP/1.1 ++ Content-Length: not-a-number ++ EOF ++ ++ cat >IN.http.overlen <<-EOF && ++ GET /info/refs HTTP/1.1 ++ Content-Length: 9223372036854775807 ++ EOF ++ ++ run_http_server_worker \ ++ --auth-config="$TRASH_DIRECTORY/auth.config" <IN.http.valid \ ++ | head -n1 >OUT.actual && ++ test_cmp OUT.http200 OUT.actual && ++ ++ run_http_server_worker <IN.http.badfirstline | head -n1 >OUT.actual && ++ test_cmp OUT.http400 OUT.actual && ++ ++ run_http_server_worker <IN.http.ltzlen | head -n1 >OUT.actual && ++ test_cmp OUT.http400 OUT.actual && ++ ++ run_http_server_worker <IN.http.badlen | head -n1 >OUT.actual && ++ test_cmp OUT.http400 OUT.actual && ++ ++ run_http_server_worker <IN.http.overlen | head -n1 >OUT.actual && ++ test_cmp OUT.http400 OUT.actual ++' ++ ++test_done 7: ab06ac9b965 ! 7: ca9c2787248 test-http-server: pass Git requests to http-backend @@ t/helper/test-http-server.c: done: + return error(_("could not send '%s'"), ok); + + strvec_pushf(&cp.env, "REQUEST_METHOD=%s", req->method); -+ strvec_pushf(&cp.env, "PATH_TRANSLATED=%s", -+ req->uri_path.buf); ++ strvec_pushf(&cp.env, "PATH_TRANSLATED=%s", req->uri_path.buf); + strvec_push(&cp.env, "SERVER_PROTOCOL=HTTP/1.1"); + if (req->query_args.len) -+ strvec_pushf(&cp.env, "QUERY_STRING=%s", -+ req->query_args.buf); ++ strvec_pushf(&cp.env, "QUERY_STRING=%s", req->query_args.buf); + if (req->content_type) -+ strvec_pushf(&cp.env, "CONTENT_TYPE=%s", -+ req->content_type); -+ if (req->content_length >= 0) -+ strvec_pushf(&cp.env, "CONTENT_LENGTH=%" PRIdMAX, -+ (intmax_t)req->content_length); ++ strvec_pushf(&cp.env, "CONTENT_TYPE=%s", req->content_type); ++ if (req->has_content_length) ++ strvec_pushf(&cp.env, "CONTENT_LENGTH=%" PRIuMAX, ++ (uintmax_t)req->content_length); + cp.git_cmd = 1; + strvec_push(&cp.args, "http-backend"); + res = run_command(&cp); @@ t/helper/test-http-server.c: done: + return do__git(req); + return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL, - WR_OK | WR_HANGUP); + WR_HANGUP); } - ## t/t5556-http-auth.sh (new) ## -@@ -+#!/bin/sh -+ -+test_description='test http auth header and credential helper interop' -+ -+TEST_NO_CREATE_REPO=1 -+. ./test-lib.sh -+ + ## t/t5556-http-auth.sh ## +@@ t/t5556-http-auth.sh: test_description='test http auth header and credential helper interop' + TEST_NO_CREATE_REPO=1 + . ./test-lib.sh + +test_set_port GIT_TEST_HTTP_PROTOCOL_PORT + -+# Setup a repository -+# -+REPO_DIR="$TRASH_DIRECTORY"/repo -+ + # Setup a repository + # + REPO_DIR="$TRASH_DIRECTORY"/repo + +# Setup some lookback URLs where test-http-server will be listening. +# We will spawn it directly inside the repo directory, so we avoid +# any need to configure directory mappings etc - we only serve this @@ t/t5556-http-auth.sh (new) +# killing it by PID). +# +PID_FILE="$TRASH_DIRECTORY"/pid-file.pid -+SERVER_LOG="$TRASH_DIRECTORY"/OUT.server.log -+ -+PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH -+ -+test_expect_success 'setup repos' ' -+ test_create_repo "$REPO_DIR" && -+ git -C "$REPO_DIR" branch -M main -+' -+ + SERVER_LOG="$TRASH_DIRECTORY"/OUT.server.log + + PATH="$GIT_BUILD_DIR/t/helper/:$PATH" && export PATH +@@ t/t5556-http-auth.sh: run_http_server_worker() { + ) + } + +stop_http_server () { + if ! test -f "$PID_FILE" + then @@ t/t5556-http-auth.sh (new) + return 1 +} + -+per_test_cleanup () { + per_test_cleanup () { + stop_http_server && -+ rm -f OUT.* -+} + rm -f OUT.* && + rm -f IN.* && + } +@@ t/t5556-http-auth.sh: test_expect_success 'http auth server request parsing' ' + test_cmp OUT.http400 OUT.actual + ' + + +test_expect_success 'http auth anonymous no challenge' ' + test_when_finished "per_test_cleanup" && @@ t/t5556-http-auth.sh (new) + git ls-remote $ORIGIN_URL +' + -+test_done + test_done 8: a1ff55dd6e2 ! 8: b8d3e81b553 test-http-server: add simple authentication @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + strvec_pushf(&cp.env, "REMOTE_USER=%s", user); + strvec_pushf(&cp.env, "REQUEST_METHOD=%s", req->method); - strvec_pushf(&cp.env, "PATH_TRANSLATED=%s", - req->uri_path.buf); + strvec_pushf(&cp.env, "PATH_TRANSLATED=%s", req->uri_path.buf); + strvec_push(&cp.env, "SERVER_PROTOCOL=HTTP/1.1"); @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) return !!res; } @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + +static struct auth_module *get_auth_module(const char *scheme, int create) +{ -+ int i; + struct auth_module *mod; -+ for (i = 0; i < auth_modules_nr; i++) { ++ for (size_t i = 0; i < auth_modules_nr; i++) { + mod = auth_modules[i]; + if (!strcasecmp(mod->scheme, scheme)) + return mod; @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + struct auth_module *mod = xmalloc(sizeof(struct auth_module)); + mod->scheme = xstrdup(scheme); + mod->challenge_params = NULL; -+ CALLOC_ARRAY(mod->tokens, 1); ++ ALLOC_ARRAY(mod->tokens, 1); + string_list_init_dup(mod->tokens); + + ALLOC_GROW(auth_modules, auth_modules_nr + 1, auth_modules_alloc); @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + for_each_string_list_item(hdr, &req->header_list) { + if (skip_iprefix(hdr->string, "Authorization: ", &v)) { + split = strbuf_split_str(v, ' ', 2); -+ if (!split[0] || !split[1]) continue; -+ -+ /* trim trailing space ' ' */ -+ strbuf_setlen(split[0], split[0]->len - 1); -+ -+ mod = get_auth_module(split[0]->buf, 0); -+ if (mod) { -+ result = AUTH_DENY; -+ -+ for_each_string_list_item(token, mod->tokens) { -+ if (!strcmp(split[1]->buf, token->string)) { -+ result = AUTH_ALLOW; -+ break; ++ if (split[0] && split[1]) { ++ /* trim trailing space ' ' */ ++ strbuf_rtrim(split[0]); ++ ++ mod = get_auth_module(split[0]->buf, 0); ++ if (mod) { ++ result = AUTH_DENY; ++ ++ for_each_string_list_item(token, mod->tokens) { ++ if (!strcmp(split[1]->buf, token->string)) { ++ result = AUTH_ALLOW; ++ break; ++ } + } -+ } + -+ goto done; ++ strbuf_list_free(split); ++ goto done; ++ } + } ++ ++ strbuf_list_free(split); + } + } + @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + &hdrs, *wr); + } + -+ strbuf_list_free(split); + string_list_clear(&hdrs, 0); + + return result == AUTH_ALLOW || @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + return -1; + + /* trim trailing ':' */ -+ if (p[0]->len > 0 && p[0]->buf[p[0]->len - 1] == ':') ++ if (p[0]->len && p[0]->buf[p[0]->len - 1] == ':') + strbuf_setlen(p[0], p[0]->len - 1); + + *scheme = strbuf_detach(p[0], NULL); -+ -+ if (p[1]) -+ *val = strbuf_detach(p[1], NULL); ++ *val = p[1] ? strbuf_detach(p[1], NULL) : NULL; + + strbuf_list_free(p); + return 0; @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + char *scheme = NULL; + char *token = NULL; + char *challenge = NULL; -+ struct auth_module *mod = NULL; ++ struct auth_module *mod; + + if (!strcmp(name, "auth.challenge")) { + if (split_auth_param(val, &scheme, &challenge)) { @@ t/helper/test-http-server.c: static enum worker_result do__git(struct req *req) + return do__git(req, user); return send_http_error(STDOUT_FILENO, 501, "Not Implemented", -1, NULL, - WR_OK | WR_HANGUP); + WR_HANGUP); @@ t/helper/test-http-server.c: int cmd_main(int argc, const char **argv) pid_file = v; continue; @@ t/helper/test-http-server.c: int cmd_main(int argc, const char **argv) ## t/t5556-http-auth.sh ## @@ t/t5556-http-auth.sh: per_test_cleanup () { - rm -f OUT.* + stop_http_server && + rm -f OUT.* && + rm -f IN.* && ++ rm -f auth.config } + test_expect_success 'http auth server request parsing' ' +@@ t/t5556-http-auth.sh: test_expect_success 'http auth server request parsing' ' + test_cmp OUT.http400 OUT.actual + ' + +test_expect_success CURL 'http auth server auth config' ' -+ #test_when_finished "per_test_cleanup" && ++ test_when_finished "per_test_cleanup" && + + cat >auth.config <<-EOF && + [auth] @@ t/t5556-http-auth.sh: per_test_cleanup () { + + test_cmp OUT.expected OUT.actual +' -+ + test_expect_success 'http auth anonymous no challenge' ' test_when_finished "per_test_cleanup" && 9: 76125cdf239 = 9: 2f97c94f679 test-http-server: add sending of arbitrary headers 10: cc9a220ed1f = 10: 4b1635b3f69 http: replace unsafe size_t multiplication with st_mult 11: bc1ac8d3eb3 = 11: 5f5e46038cf http: read HTTP WWW-Authenticate response headers 12: 7c8229f0b11 ! 12: 09164f77d56 credential: add WWW-Authenticate header to cred requests @@ t/t5556-http-auth.sh: test_expect_success 'setup repos' ' +setup_credential_helper + - stop_http_server () { - if ! test -f "$PID_FILE" - then -@@ t/t5556-http-auth.sh: start_http_server () { - - per_test_cleanup () { + run_http_server_worker() { + ( + cd "$REPO_DIR" +@@ t/t5556-http-auth.sh: per_test_cleanup () { stop_http_server && -- rm -f OUT.* -+ rm -f OUT.* && + rm -f OUT.* && + rm -f IN.* && + rm -f *.cred && -+ rm -f auth.config + rm -f auth.config } - test_expect_success CURL 'http auth server auth config' ' @@ t/t5556-http-auth.sh: test_expect_success 'http auth anonymous no challenge' ' git ls-remote $ORIGIN_URL '