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
