On Fri, Jan 20, 2023 at 10:08:50PM +0000, Matthew John Cheetham via GitGitGadget wrote: > From: Matthew John Cheetham <mjcheetham@xxxxxxxxxxx> > > 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. I'm definitely on board with sending these to the helpers. It does feel a bit weird that we don't parse them at all, and just foist that on the helpers. If I understand the RFC correctly, you can have multiple challenges per header, but also multiple headers. So: WWW-Authenticate: Basic realm="foo", OtherAuth realm="bar" WWW-Authenticate: YetAnotherScheme some-token could be normalized as: www-auth-challenge=Basic realm="foo" www-auth-challenge=OtherAuth realm="bar" www-auth-challenge=YetAnotherScheme some-token which saves each helper from having to do the same work. Likewise, we can do a _little_ more parsing to get: www-auth-basic=realm="foo" www-auth-otherauth=realm="bar" www-auth-yetanotherscheme=some-token I don't think we can go beyond there, though, without understanding the syntax of individual schemes. Which is a shame, as one of the goals of the credential format was to let the helpers do as little as possible (so they can't get it wrong!). But helpers are stuck doing things like handling backslashed double-quotes, soaking up extra whitespace, etc. I'm not really sure what we expect to see in the real world. I guess for your purposes, you are working on an already-big helper that is happy to just get the raw values and process them according to the rfc. I'm just wondering if there are use cases where somebody might want to do something with this header, but in a quick shell script kind of way. For example, my credential config is still: [credential "https://github.com";] username = peff helper = "!f() { test $1 = get && echo password=$(pass ...); }; f" That's an extreme example, but I'm wondering if there's _anything_ useful somebody would want to do in a similar quick-and-dirty kind of way. For example, deciding which cred to use based on basic realm, like: realm=foo while read line; do case "$line" in www-auth-basic=) value=${line#*=} # oops, we're just assuming it's realm= here, and we're # not handling quotes at all. I think it could technically be # realm=foo or realm="foo" realm=${value#realm=} ;; esac done echo password=$(pass "pats-by-realm/$realm") which could be made a lot easier if we did more parsing (e.g., www-auth-basic-realm or something). I dunno. Maybe that is just opening up a can of worms, as we're stuffing structured data into a linearized key-value list. The nice thing about your proposal is that Git does not even have to know anything about these schemes; it's all the problem of the helper. My biggest fear is just that we'll want to shift that later, and we'll be stuck with this microformat forever. > 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. I don't know if this is strictly necessary. The semantics of duplicate keys are not really defined anywhere, and just because the implementations of current readers happen to replace duplicates for the current set of keys doesn't mean everything has to. So you could just define "wwwauth" to behave differently. But I don't mind having a syntactic marker to indicate this new type. If you're at all convinced by what I said above, then we also might be able to get away with having unique keys anyway. > Documentation/git-credential.txt | 19 ++- > credential.c | 11 ++ > t/lib-credential-helper.sh | 27 ++++ > t/t5556-http-auth.sh | 242 +++++++++++++++++++++++++++++++ > 4 files changed, 298 insertions(+), 1 deletion(-) > create mode 100644 t/lib-credential-helper.sh The patch itself looks pretty reasonable to me. One small thing I noticed: > + git -c "credential.helper=!\"$CREDENTIAL_HELPER\"" ls-remote $ORIGIN_URL && As you undoubtedly figured out, the helper path is fed to the shell, so spaces in the trash directory are a problem. You've solved it here by adding a layer of double quotes, which handles spaces. But you'd run into problems if the absolute path that somebody is using for the test suite has a backslash or a double quote in it. I don't know how careful we want to be here (or how careful we already are[1]), but one simple-ish solution is: export CREDENTIAL_HELPER git -c "credential.helper=!\"\$CREDENTIAL_HELPER\"" ... I.e., letting the inner shell expand the variable itself. Another option is to put the helper into $TRASH_DIRECTORY/bin and add that to the $PATH. I also wondered if it was worth having setup_credential_helper() just stick it in $TRASH_DIRECTORY/.gitconfig so that individual tests don't have to keep doing that ugly "-c" invocation. Or if you really want to have each test enable it, perhaps have set_credential_reply() turn it on via test_config (which will auto-remove it at the end of the test). -Peff [1] Curious, I tried cloning git into this directory: mkdir '/tmp/foo/"horrible \"path\"' and we do indeed already fail. The first breakage I saw was recent, but going further back, it looks like bin-wrappers don't correctly handle this case anyway. So maybe that's evidence that nobody would do something so ridiculous in practice.
