| Message ID | 20240324011301.1553072-6-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for arbitrary schemes in credentials | expand |
On Sun, Mar 24, 2024 at 01:12:53AM +0000, brian m. carlson wrote: > We support the new credential and authtype fields, but we lack a way to > indicate to a credential helper that we'd like them to be used. Without > some sort of indication, the credential helper doesn't know if it should > try to provide us a username and password, or a pre-encoded credential. > For example, the helper might prefer a more restricted Bearer token if > pre-encoded credentials are possible, but might have to fall back to > more general username and password if not. > > Let's provide a simple way to indicate whether Git (or, for that matter, > the helper) is capable of understanding the authtype and credential > fields. We send this capability when we generate a request, and the > other side may reply to indicate to us that it does, too. > > For now, don't enable sending capabilities for the HTTP code. In a > future commit, we'll introduce appropriate handling for that code, > which requires more in-depth work. > > The logic for determining whether a capability is supported may seem > complex, but it is not. At each stage, we emit the capability to the > following stage if all preceding stages have declared it. Thus, if the > caller to git credential fill didn't declare it, then we won't send it > to the helper, and if fill's caller did send but the helper doesn't > understand it, then we won't send it on in the response. If we're an > internal user, then we know about all capabilities and will request > them. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > builtin/credential-cache--daemon.c | 2 +- > builtin/credential-store.c | 2 +- > builtin/credential.c | 6 +- > credential.c | 58 ++++++++++++++-- > credential.h | 30 +++++++- > http.c | 10 +-- > imap-send.c | 2 +- > remote-curl.c | 4 +- > t/t0300-credentials.sh | 107 ++++++++++++++++++++++++++++- > 9 files changed, 197 insertions(+), 24 deletions(-) > > diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c > index 3a6a750a8e..ccbcf99ac1 100644 > --- a/builtin/credential-cache--daemon.c > +++ b/builtin/credential-cache--daemon.c > @@ -115,7 +115,7 @@ static int read_request(FILE *fh, struct credential *c, > return error("client sent bogus timeout line: %s", item.buf); > *timeout = atoi(p); > > - if (credential_read(c, fh) < 0) > + if (credential_read(c, fh, CREDENTIAL_OP_HELPER) < 0) > return -1; > return 0; > } > diff --git a/builtin/credential-store.c b/builtin/credential-store.c > index 4a492411bb..494c809332 100644 > --- a/builtin/credential-store.c > +++ b/builtin/credential-store.c > @@ -205,7 +205,7 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix) > if (!fns.nr) > die("unable to set up default path; use --file"); > > - if (credential_read(&c, stdin) < 0) > + if (credential_read(&c, stdin, CREDENTIAL_OP_HELPER) < 0) > die("unable to read credential"); > > if (!strcmp(op, "get")) > diff --git a/builtin/credential.c b/builtin/credential.c > index 7010752987..5123dabcf1 100644 > --- a/builtin/credential.c > +++ b/builtin/credential.c > @@ -17,12 +17,12 @@ int cmd_credential(int argc, const char **argv, const char *prefix UNUSED) > usage(usage_msg); > op = argv[1]; > > - if (credential_read(&c, stdin) < 0) > + if (credential_read(&c, stdin, CREDENTIAL_OP_INITIAL) < 0) > die("unable to read credential from stdin"); > > if (!strcmp(op, "fill")) { > - credential_fill(&c); > - credential_write(&c, stdout); > + credential_fill(&c, 0); > + credential_write(&c, stdout, CREDENTIAL_OP_RESPONSE); > } else if (!strcmp(op, "approve")) { > credential_approve(&c); > } else if (!strcmp(op, "reject")) { > diff --git a/credential.c b/credential.c > index c521822e5a..f2a26b8672 100644 > --- a/credential.c > +++ b/credential.c > @@ -34,6 +34,11 @@ void credential_clear(struct credential *c) > credential_init(c); > } > > +static void credential_set_all_capabilities(struct credential *c) > +{ > + c->capa_authtype.request_initial = 1; > +} > + > int credential_match(const struct credential *want, > const struct credential *have, int match_password) > { > @@ -210,7 +215,39 @@ static void credential_getpass(struct credential *c) > PROMPT_ASKPASS); > } > > -int credential_read(struct credential *c, FILE *fp) > +static void credential_set_capability(struct credential_capability *capa, int op_type) > +{ > + switch (op_type) { > + case CREDENTIAL_OP_INITIAL: > + capa->request_initial = 1; > + break; > + case CREDENTIAL_OP_HELPER: > + capa->request_helper = 1; > + break; > + case CREDENTIAL_OP_RESPONSE: > + capa->response = 1; > + break; > + } > +} > + > +static int credential_has_capability(const struct credential_capability *capa, int op_type) > +{ > + /* > + * We're checking here if each previous step indicated that we had the > + * capability. If it did, then we want to pass it along; conversely, if > + * it did not, we don't want to report that to our caller. > + */ > + switch (op_type) { > + case CREDENTIAL_OP_HELPER: > + return capa->request_initial; > + case CREDENTIAL_OP_RESPONSE: > + return capa->request_initial && capa->request_helper; > + default: > + return 0; > + } > +} I think I'm missing the bigger picture here, so please bear with me. What you provide here is simply an `op_type` that indicates the phase we are currently in and thus allows us to check whether all of the preceding phases had the capability set. But to me it seems like a phase and the actual capability should be different things. So why is it that the capability seems to be a mere boolean value instead of something like a bitfield indicating whether a specific capability is supported or not? Or is all of this infra really only to support a single capability, namely the credential capability? I'm mostly coming from the angle of how capabilities work with remote helpers. When asked, the helper will announce a set of capabilities that it supports, e.g. "capabilities stateless-connect". So from thereon the client of the helper knows that it can assume "stateless-connect" to be understood by the helper. I would have expected capabilities to work similarly for the credential helper, where it announces "I know to handle pre-encoded credentials". But given that I have basically no clue at all for how the credential helper works there may very well be good reasons why things work so differently here. > +int credential_read(struct credential *c, FILE *fp, int op_type) > { > struct strbuf line = STRBUF_INIT; > > @@ -249,6 +286,8 @@ int credential_read(struct credential *c, FILE *fp) > c->path = xstrdup(value); > } else if (!strcmp(key, "wwwauth[]")) { > strvec_push(&c->wwwauth_headers, value); > + } else if (!strcmp(key, "capability[]") && !strcmp(value, "authtype")) { > + credential_set_capability(&c->capa_authtype, op_type); > } else if (!strcmp(key, "password_expiry_utc")) { > errno = 0; > c->password_expiry_utc = parse_timestamp(value, NULL, 10); > @@ -288,14 +327,18 @@ static void credential_write_item(FILE *fp, const char *key, const char *value, > fprintf(fp, "%s=%s\n", key, value); > } > > -void credential_write(const struct credential *c, FILE *fp) > +void credential_write(const struct credential *c, FILE *fp, int op_type) > { > + if (credential_has_capability(&c->capa_authtype, op_type)) { > + credential_write_item(fp, "capability[]", "authtype", 0); > + credential_write_item(fp, "authtype", c->authtype, 0); > + credential_write_item(fp, "credential", c->credential, 0); > + } > credential_write_item(fp, "protocol", c->protocol, 1); > credential_write_item(fp, "host", c->host, 1); > 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, "credential", c->credential, 0); > credential_write_item(fp, "oauth_refresh_token", c->oauth_refresh_token, 0); > if (c->password_expiry_utc != TIME_MAX) { > char *s = xstrfmt("%"PRItime, c->password_expiry_utc); > @@ -304,7 +347,6 @@ void credential_write(const struct credential *c, FILE *fp) > } > for (size_t i = 0; i < c->wwwauth_headers.nr; i++) > credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); > - credential_write_item(fp, "authtype", c->authtype, 0); > } > > static int run_credential_helper(struct credential *c, > @@ -327,14 +369,14 @@ static int run_credential_helper(struct credential *c, > > fp = xfdopen(helper.in, "w"); > sigchain_push(SIGPIPE, SIG_IGN); > - credential_write(c, fp); > + credential_write(c, fp, want_output ? CREDENTIAL_OP_HELPER : CREDENTIAL_OP_RESPONSE); > fclose(fp); > sigchain_pop(SIGPIPE); > > if (want_output) { > int r; > fp = xfdopen(helper.out, "r"); > - r = credential_read(c, fp); > + r = credential_read(c, fp, CREDENTIAL_OP_HELPER); > fclose(fp); > if (r < 0) { > finish_command(&helper); > @@ -367,7 +409,7 @@ static int credential_do(struct credential *c, const char *helper, > return r; > } > > -void credential_fill(struct credential *c) > +void credential_fill(struct credential *c, int all_capabilities) > { > int i; > > @@ -375,6 +417,8 @@ void credential_fill(struct credential *c) > return; > > credential_apply_config(c); > + if (all_capabilities) > + credential_set_all_capabilities(c); > > for (i = 0; i < c->helpers.nr; i++) { > credential_do(c, c->helpers.items[i].string, "get"); > diff --git a/credential.h b/credential.h > index 9db892cf4d..2051d04c5a 100644 > --- a/credential.h > +++ b/credential.h > @@ -93,6 +93,25 @@ > * ----------------------------------------------------------------------- > */ > > +/* > + * These values define the kind of operation we're performing and the > + * capabilities at each stage. The first is either an external request (via git > + * credential fill) or an internal request (e.g., via the HTTP) code. The > + * second is the call to the credential helper, and the third is the response > + * we're providing. > + * > + * At each stage, we will emit the capability only if the previous stage > + * supported it. > + */ > +#define CREDENTIAL_OP_INITIAL 1 > +#define CREDENTIAL_OP_HELPER 2 > +#define CREDENTIAL_OP_RESPONSE 3 Is there any specific reason why you're using defines instead of an enum here? I think the latter would be more self-explanatory when you see that functions take `enum credential_op` as input instead of an `int`. Patrick > +struct credential_capability { > + unsigned request_initial:1, > + request_helper:1, > + response:1; > +}; > > /** > * This struct represents a single username/password combination > @@ -136,6 +155,8 @@ struct credential { > use_http_path:1, > username_from_proto:1; > > + struct credential_capability capa_authtype; > + > char *username; > char *password; > char *credential; > @@ -174,8 +195,11 @@ void credential_clear(struct credential *); > * returns, the username and password fields of the credential are > * guaranteed to be non-NULL. If an error occurs, the function will > * die(). > + * > + * If all_capabilities is set, this is an internal user that is prepared > + * to deal with all known capabilities, and we should advertise that fact. > */ > -void credential_fill(struct credential *); > +void credential_fill(struct credential *, int all_capabilities); > > /** > * Inform the credential subsystem that the provided credentials > @@ -198,8 +222,8 @@ void credential_approve(struct credential *); > */ > void credential_reject(struct credential *); > > -int credential_read(struct credential *, FILE *); > -void credential_write(const struct credential *, FILE *); > +int credential_read(struct credential *, FILE *, int); > +void credential_write(const struct credential *, FILE *, int); > > /* > * Parse a url into a credential struct, replacing any existing contents. > diff --git a/http.c b/http.c > index 1c2200da77..4f5df6fb14 100644 > --- a/http.c > +++ b/http.c > @@ -569,7 +569,7 @@ static void init_curl_http_auth(CURL *result) > return; > } > > - credential_fill(&http_auth); > + credential_fill(&http_auth, 0); > > curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username); > curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password); > @@ -596,7 +596,7 @@ static void init_curl_proxy_auth(CURL *result) > { > if (proxy_auth.username) { > if (!proxy_auth.password) > - credential_fill(&proxy_auth); > + credential_fill(&proxy_auth, 0); > set_proxyauth_name_password(result); > } > > @@ -630,7 +630,7 @@ static int has_cert_password(void) > cert_auth.host = xstrdup(""); > cert_auth.username = xstrdup(""); > cert_auth.path = xstrdup(ssl_cert); > - credential_fill(&cert_auth); > + credential_fill(&cert_auth, 0); > } > return 1; > } > @@ -645,7 +645,7 @@ static int has_proxy_cert_password(void) > proxy_cert_auth.host = xstrdup(""); > proxy_cert_auth.username = xstrdup(""); > proxy_cert_auth.path = xstrdup(http_proxy_ssl_cert); > - credential_fill(&proxy_cert_auth); > + credential_fill(&proxy_cert_auth, 0); > } > return 1; > } > @@ -2191,7 +2191,7 @@ static int http_request_reauth(const char *url, > BUG("Unknown http_request target"); > } > > - credential_fill(&http_auth); > + credential_fill(&http_auth, 0); > > return http_request(url, result, target, options); > } > diff --git a/imap-send.c b/imap-send.c > index f2e1947e63..8c89e866b6 100644 > --- a/imap-send.c > +++ b/imap-send.c > @@ -944,7 +944,7 @@ static void server_fill_credential(struct imap_server_conf *srvc, struct credent > cred->username = xstrdup_or_null(srvc->user); > cred->password = xstrdup_or_null(srvc->pass); > > - credential_fill(cred); > + credential_fill(cred, 1); > > if (!srvc->user) > srvc->user = xstrdup(cred->username); > diff --git a/remote-curl.c b/remote-curl.c > index e37eaa17b7..f96bda2431 100644 > --- a/remote-curl.c > +++ b/remote-curl.c > @@ -926,7 +926,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece > do { > err = probe_rpc(rpc, &results); > if (err == HTTP_REAUTH) > - credential_fill(&http_auth); > + credential_fill(&http_auth, 0); > } while (err == HTTP_REAUTH); > if (err != HTTP_OK) > return -1; > @@ -1044,7 +1044,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece > rpc->any_written = 0; > err = run_slot(slot, NULL); > if (err == HTTP_REAUTH && !large_request) { > - credential_fill(&http_auth); > + credential_fill(&http_auth, 0); > curl_slist_free_all(headers); > goto retry; > } > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh > index 400f6bdbca..8477108b28 100755 > --- a/t/t0300-credentials.sh > +++ b/t/t0300-credentials.sh > @@ -12,7 +12,13 @@ test_expect_success 'setup helper scripts' ' > IFS== > while read key value; do > echo >&2 "$whoami: $key=$value" > - eval "$key=$value" > + if test -z "${key%%*\[\]}" > + then > + key=${key%%\[\]} > + eval "$key=\"\$$key $value\"" > + else > + eval "$key=$value" > + fi > done > IFS=$OIFS > EOF > @@ -35,6 +41,16 @@ test_expect_success 'setup helper scripts' ' > test -z "$pass" || echo password=$pass > EOF > > + write_script git-credential-verbatim-cred <<-\EOF && > + authtype=$1; shift > + credential=$1; shift > + . ./dump > + echo capability[]=authtype > + test -z "${capability##*authtype*}" || return > + test -z "$authtype" || echo authtype=$authtype > + test -z "$credential" || echo credential=$credential > + EOF > + > write_script git-credential-verbatim-with-expiry <<-\EOF && > user=$1; shift > pass=$1; shift > @@ -64,6 +80,26 @@ test_expect_success 'credential_fill invokes helper' ' > EOF > ' > > +test_expect_success 'credential_fill invokes helper with credential' ' > + check fill "verbatim-cred Bearer token" <<-\EOF > + capability[]=authtype > + protocol=http > + host=example.com > + -- > + capability[]=authtype > + authtype=Bearer > + credential=token > + protocol=http > + host=example.com > + -- > + verbatim-cred: get > + verbatim-cred: capability[]=authtype > + verbatim-cred: protocol=http > + verbatim-cred: host=example.com > + EOF > +' > + > + > test_expect_success 'credential_fill invokes multiple helpers' ' > check fill useless "verbatim foo bar" <<-\EOF > protocol=http > @@ -83,6 +119,42 @@ test_expect_success 'credential_fill invokes multiple helpers' ' > EOF > ' > > +test_expect_success 'credential_fill response does not get capabilities when helpers are incapable' ' > + check fill useless "verbatim foo bar" <<-\EOF > + capability[]=authtype > + protocol=http > + host=example.com > + -- > + protocol=http > + host=example.com > + username=foo > + password=bar > + -- > + useless: get > + useless: capability[]=authtype > + useless: protocol=http > + useless: host=example.com > + verbatim: get > + verbatim: capability[]=authtype > + verbatim: protocol=http > + verbatim: host=example.com > + EOF > +' > + > +test_expect_success 'credential_fill response does not get capabilities when caller is incapable' ' > + check fill "verbatim-cred Bearer token" <<-\EOF > + protocol=http > + host=example.com > + -- > + protocol=http > + host=example.com > + -- > + verbatim-cred: get > + verbatim-cred: protocol=http > + verbatim-cred: host=example.com > + EOF > +' > + > test_expect_success 'credential_fill stops when we get a full response' ' > check fill "verbatim one two" "verbatim three four" <<-\EOF > protocol=http > @@ -99,6 +171,25 @@ test_expect_success 'credential_fill stops when we get a full response' ' > EOF > ' > > +test_expect_success 'credential_fill thinks a credential is a full response' ' > + check fill "verbatim-cred Bearer token" "verbatim three four" <<-\EOF > + capability[]=authtype > + protocol=http > + host=example.com > + -- > + capability[]=authtype > + authtype=Bearer > + credential=token > + protocol=http > + host=example.com > + -- > + verbatim-cred: get > + verbatim-cred: capability[]=authtype > + verbatim-cred: protocol=http > + verbatim-cred: host=example.com > + EOF > +' > + > test_expect_success 'credential_fill continues through partial response' ' > check fill "verbatim one \"\"" "verbatim two three" <<-\EOF > protocol=http > @@ -175,6 +266,20 @@ test_expect_success 'credential_fill passes along metadata' ' > EOF > ' > > +test_expect_success 'credential_fill produces no credential without capability' ' > + check fill "verbatim-cred Bearer token" <<-\EOF > + protocol=http > + host=example.com > + -- > + protocol=http > + host=example.com > + -- > + verbatim-cred: get > + verbatim-cred: protocol=http > + verbatim-cred: host=example.com > + EOF > +' > + > test_expect_success 'credential_approve calls all helpers' ' > check approve useless "verbatim one two" <<-\EOF > protocol=http >
On 2024-03-27 at 08:02:39, Patrick Steinhardt wrote: > On Sun, Mar 24, 2024 at 01:12:53AM +0000, brian m. carlson wrote: > > +static int credential_has_capability(const struct credential_capability *capa, int op_type) > > +{ > > + /* > > + * We're checking here if each previous step indicated that we had the > > + * capability. If it did, then we want to pass it along; conversely, if > > + * it did not, we don't want to report that to our caller. > > + */ > > + switch (op_type) { > > + case CREDENTIAL_OP_HELPER: > > + return capa->request_initial; > > + case CREDENTIAL_OP_RESPONSE: > > + return capa->request_initial && capa->request_helper; > > + default: > > + return 0; > > + } > > +} > > I think I'm missing the bigger picture here, so please bear with me. > > What you provide here is simply an `op_type` that indicates the phase we > are currently in and thus allows us to check whether all of the > preceding phases had the capability set. But to me it seems like a phase > and the actual capability should be different things. So why is it that > the capability seems to be a mere boolean value instead of something > like a bitfield indicating whether a specific capability is supported or > not? Or is all of this infra really only to support a single capability, > namely the credential capability? > > I'm mostly coming from the angle of how capabilities work with remote > helpers. When asked, the helper will announce a set of capabilities that > it supports, e.g. "capabilities stateless-connect". So from thereon the > client of the helper knows that it can assume "stateless-connect" to be > understood by the helper. > > I would have expected capabilities to work similarly for the credential > helper, where it announces "I know to handle pre-encoded credentials". > But given that I have basically no clue at all for how the credential > helper works there may very well be good reasons why things work so > differently here. Let me explain a little bit. There are two possible flows that we can have for a credential request: git-credential input -> credential.c -> helper -> credential.c -> git-credential output git-http-backend -> credential.c -> helper -> credential.c -> git-http-backend Let's look at the first one (which might, say, come from Git LFS or another external tool), but the second one works similarly. When we get a request from `git credential fill`, we need to know first whether the requester supports the capability. If we're using an external tool from last decade, it's not going to do so. If it _does_ support that, then we want to pass that along to the helper, but if it doesn't, we don't. That's because if the caller doesn't support `credential` and `authtype`, the helper might legitimately want to provide a username and password (or token) instead, knowing that that's more likely to work. Similarly, in the final response, we want to indicate to the external caller whether the capability was in fact supported. That's useful to know in case we want to pass the response back to `git credential store`, and it also discloses functionality about what the credential helper in use supports. We can't assume that the helper does or doesn't support the same capabilities as Git because it might come from outside Git (e.g., Git Credential Manager Core, or a site-specific credential helper) or it just might not be capable of storing or handling that kind of credential. By not making the assumption that the helper is implicitly capable, we allow users to continue to use very simple shell scripts as credential helpers. As to why this functionality exists, it exists to support the two new capabilities in this series, `credential` and `state`. A pie in the sky goal for the future is to support additional request signing functionality, so it might learn things like method, URI, and TLS channel binding info, which would be an additional capability. (I might implement that, or I might not.) All of those are boolean: they either are supported, or not. If folks in the future want to expose non-boolean capabilities, I don't think that should be a problem. > > +/* > > + * These values define the kind of operation we're performing and the > > + * capabilities at each stage. The first is either an external request (via git > > + * credential fill) or an internal request (e.g., via the HTTP) code. The > > + * second is the call to the credential helper, and the third is the response > > + * we're providing. > > + * > > + * At each stage, we will emit the capability only if the previous stage > > + * supported it. > > + */ > > +#define CREDENTIAL_OP_INITIAL 1 > > +#define CREDENTIAL_OP_HELPER 2 > > +#define CREDENTIAL_OP_RESPONSE 3 > > Is there any specific reason why you're using defines instead of an enum > here? I think the latter would be more self-explanatory when you see > that functions take `enum credential_op` as input instead of an `int`. I think an enum would be a nice improvement. I'll include that in a reroll.
On Sun, Mar 24, 2024 at 01:12:53AM +0000, brian m. carlson wrote: > @@ -35,6 +41,16 @@ test_expect_success 'setup helper scripts' ' > test -z "$pass" || echo password=$pass > EOF > > + write_script git-credential-verbatim-cred <<-\EOF && > + authtype=$1; shift > + credential=$1; shift > + . ./dump > + echo capability[]=authtype > + test -z "${capability##*authtype*}" || return > + test -z "$authtype" || echo authtype=$authtype > + test -z "$credential" || echo credential=$credential > + EOF I think this "|| return" needs to be "|| exit 0" or similar. The Windows CI jobs fail with: --- a/expect-stderr +++ b/stderr @@ -2,3 +2,4 @@ verbatim-cred: get verbatim-cred: capability[]=authtype verbatim-cred: protocol=http verbatim-cred: host=example.com +D:\a\git\git\t\trash directory.t0300-credentials\git-credential-verbatim-cred: line 10: return: can only `return' from a function or sourced script (actually if you count the line numbers, I think this particular case is the similar "|| return" added to the script later, but both should be fixed). It doesn't show up elsewhere because only bash complains, but not dash. Even running the test script with bash isn't enough, because write_script uses $SHELL_PATH under the hood. But building with "make SHELL_PATH=/bin/bash test" shows the problem on other platforms. -Peff
Jeff King <peff@peff.net> writes: > It doesn't show up elsewhere because only bash complains, but not dash. > Even running the test script with bash isn't enough, because > write_script uses $SHELL_PATH under the hood. But building with "make > SHELL_PATH=/bin/bash test" shows the problem on other platforms. Can we sneak it in to the GitHub Actions CI, I wonder, so that we can catch tests that only fail with bash. Would this be sufficient, or can we just export it without using $use_bash to place it on the command line of make? ci/run-build-and-tests.sh | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git i/ci/run-build-and-tests.sh w/ci/run-build-and-tests.sh index c192bd613c..8fb1114bc5 100755 --- i/ci/run-build-and-tests.sh +++ w/ci/run-build-and-tests.sh @@ -11,6 +11,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";; esac run_tests=t +use_bash= case "$jobname" in linux-gcc) @@ -30,6 +31,7 @@ linux-TEST-vars) export GIT_TEST_NO_WRITE_REV_INDEX=1 export GIT_TEST_CHECKOUT_WORKERS=2 export GIT_TEST_PACK_USE_BITMAP_BOUNDARY_TRAVERSAL=1 + use_bash=/bin/bash ;; linux-clang) export GIT_TEST_DEFAULT_HASH=sha1 @@ -51,7 +53,8 @@ esac group Build make if test -n "$run_tests" then - group "Run tests" make test || + group "Run tests" \ + make ${use_bash:+SHELL_PATH="$use_bash"} test || handle_failed_tests group "Run unit tests" \ make DEFAULT_UNIT_TEST_TARGET=unit-tests-prove unit-tests
On Thu, Mar 28, 2024 at 09:13:38AM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > It doesn't show up elsewhere because only bash complains, but not dash. > > Even running the test script with bash isn't enough, because > > write_script uses $SHELL_PATH under the hood. But building with "make > > SHELL_PATH=/bin/bash test" shows the problem on other platforms. > > Can we sneak it in to the GitHub Actions CI, I wonder, so that we > can catch tests that only fail with bash. Would this be sufficient, > or can we just export it without using $use_bash to place it on the > command line of make? I think the sneaking has already been done, because Windows CI uses bash (which is after all how I noticed this). I'm not sure if using bash more places would be helpful. On the one hand, there are enough _other_ differences in Windows that it is not always immediately obvious that the shell is the culprit. On the other hand, I would probably forget that linux-gcc is the special one with bash, and just end up reading the test output anyway. So I dunno. As to your question, yes, I think you could just export SHELL_PATH; our Makefile uses "ifndef". -Peff
Jeff King <peff@peff.net> writes: > I think the sneaking has already been done, because Windows CI uses bash > (which is after all how I noticed this). I'm not sure if using bash more > places would be helpful. Yeah, you are absolutely right. Let's not pile another difference in the linux-TEST-vars that is already a kitchen sink. Thanks for a quick dose of sanity.
On 2024-03-28 at 10:20:53, Jeff King wrote: > I think this "|| return" needs to be "|| exit 0" or similar. The Windows > CI jobs fail with: > > --- a/expect-stderr > +++ b/stderr > @@ -2,3 +2,4 @@ verbatim-cred: get > verbatim-cred: capability[]=authtype > verbatim-cred: protocol=http > verbatim-cred: host=example.com > +D:\a\git\git\t\trash directory.t0300-credentials\git-credential-verbatim-cred: line 10: return: can only `return' from a function or sourced script > > (actually if you count the line numbers, I think this particular case is > the similar "|| return" added to the script later, but both should be > fixed). > > It doesn't show up elsewhere because only bash complains, but not dash. > Even running the test script with bash isn't enough, because > write_script uses $SHELL_PATH under the hood. But building with "make > SHELL_PATH=/bin/bash test" shows the problem on other platforms. I'll definitely make that change. I run Debian, and I've left the default dash as /bin/sh because it's faster, so I didn't notice.
On Wed, Mar 27, 2024 at 09:33:35PM +0000, brian m. carlson wrote: > On 2024-03-27 at 08:02:39, Patrick Steinhardt wrote: > > On Sun, Mar 24, 2024 at 01:12:53AM +0000, brian m. carlson wrote: > > > +static int credential_has_capability(const struct credential_capability *capa, int op_type) > > > +{ > > > + /* > > > + * We're checking here if each previous step indicated that we had the > > > + * capability. If it did, then we want to pass it along; conversely, if > > > + * it did not, we don't want to report that to our caller. > > > + */ > > > + switch (op_type) { > > > + case CREDENTIAL_OP_HELPER: > > > + return capa->request_initial; > > > + case CREDENTIAL_OP_RESPONSE: > > > + return capa->request_initial && capa->request_helper; > > > + default: > > > + return 0; > > > + } > > > +} > > > > I think I'm missing the bigger picture here, so please bear with me. > > > > What you provide here is simply an `op_type` that indicates the phase we > > are currently in and thus allows us to check whether all of the > > preceding phases had the capability set. But to me it seems like a phase > > and the actual capability should be different things. So why is it that > > the capability seems to be a mere boolean value instead of something > > like a bitfield indicating whether a specific capability is supported or > > not? Or is all of this infra really only to support a single capability, > > namely the credential capability? > > > > I'm mostly coming from the angle of how capabilities work with remote > > helpers. When asked, the helper will announce a set of capabilities that > > it supports, e.g. "capabilities stateless-connect". So from thereon the > > client of the helper knows that it can assume "stateless-connect" to be > > understood by the helper. > > > > I would have expected capabilities to work similarly for the credential > > helper, where it announces "I know to handle pre-encoded credentials". > > But given that I have basically no clue at all for how the credential > > helper works there may very well be good reasons why things work so > > differently here. > > Let me explain a little bit. There are two possible flows that we can > have for a credential request: > > git-credential input -> credential.c -> helper -> credential.c -> git-credential output > > git-http-backend -> credential.c -> helper -> credential.c -> git-http-backend > > Let's look at the first one (which might, say, come from Git LFS or > another external tool), but the second one works similarly. When we get > a request from `git credential fill`, we need to know first whether the > requester supports the capability. If we're using an external tool from > last decade, it's not going to do so. > > If it _does_ support that, then we want to pass that along to the > helper, but if it doesn't, we don't. That's because if the caller > doesn't support `credential` and `authtype`, the helper might > legitimately want to provide a username and password (or token) instead, > knowing that that's more likely to work. > > Similarly, in the final response, we want to indicate to the external > caller whether the capability was in fact supported. That's useful to > know in case we want to pass the response back to `git credential > store`, and it also discloses functionality about what the credential > helper in use supports. > > We can't assume that the helper does or doesn't support the same > capabilities as Git because it might come from outside Git (e.g., Git > Credential Manager Core, or a site-specific credential helper) or it > just might not be capable of storing or handling that kind of > credential. By not making the assumption that the helper is implicitly > capable, we allow users to continue to use very simple shell scripts as > credential helpers. The intent of this is quite clear to me, but thanks for re-explaining the bigger picture :) > As to why this functionality exists, it exists to support the two new > capabilities in this series, `credential` and `state`. A pie in the sky > goal for the future is to support additional request signing > functionality, so it might learn things like method, URI, and TLS > channel binding info, which would be an additional capability. (I might > implement that, or I might not.) All of those are boolean: they either > are supported, or not. If folks in the future want to expose > non-boolean capabilities, I don't think that should be a problem. I think you misunderstood my confusion. I didn't meant to say that there should be non-boolean capabilities. I was rather missing the picture of how exactly you can advertise multiple capabilities with the infra that currently exists, and why the infra supports per-phase capabilities. Basically, what I would have expected is a protocol where both Git and the credential helper initially did a single "handshake" that also announces capabilities. So something like: HELPER: capability foobar HELPER: capability barfoo GIT: capability foobar Git would only acknowledge capabilities that it both understands and that have been announced by the helper. So at the end of this both have agreed on a single capability "foobar". This is roughly how the remote helper capability subsystem works. What this patch is introducing seems quite a bit more complicated than that though because we have "staged" capabilities. I assume there is good reason for this complexity, but I didn't yet manage to figure out the reasoning behind it. To ask more specifically: why would one side ever announce a capability in phase 1, but not in phase 2? Is the reason that capabilities are in fact tied to credentials? Patrick > > > +/* > > > + * These values define the kind of operation we're performing and the > > > + * capabilities at each stage. The first is either an external request (via git > > > + * credential fill) or an internal request (e.g., via the HTTP) code. The > > > + * second is the call to the credential helper, and the third is the response > > > + * we're providing. > > > + * > > > + * At each stage, we will emit the capability only if the previous stage > > > + * supported it. > > > + */ > > > +#define CREDENTIAL_OP_INITIAL 1 > > > +#define CREDENTIAL_OP_HELPER 2 > > > +#define CREDENTIAL_OP_RESPONSE 3 > > > > Is there any specific reason why you're using defines instead of an enum > > here? I think the latter would be more self-explanatory when you see > > that functions take `enum credential_op` as input instead of an `int`. > > I think an enum would be a nice improvement. I'll include that in a > reroll. > -- > brian m. carlson (they/them or he/him) > Toronto, Ontario, CA
On 2024-04-02 at 10:04:31, Patrick Steinhardt wrote: > The intent of this is quite clear to me, but thanks for re-explaining > the bigger picture :) Sorry I misunderstood what you were getting it. > I think you misunderstood my confusion. I didn't meant to say that there > should be non-boolean capabilities. I was rather missing the picture of > how exactly you can advertise multiple capabilities with the infra that > currently exists, and why the infra supports per-phase capabilities. > > Basically, what I would have expected is a protocol where both Git and > the credential helper initially did a single "handshake" that also > announces capabilities. So something like: > > HELPER: capability foobar > HELPER: capability barfoo > GIT: capability foobar > > Git would only acknowledge capabilities that it both understands and > that have been announced by the helper. So at the end of this both have > agreed on a single capability "foobar". > > This is roughly how the remote helper capability subsystem works. What > this patch is introducing seems quite a bit more complicated than that > though because we have "staged" capabilities. I assume there is good > reason for this complexity, but I didn't yet manage to figure out the > reasoning behind it. > > To ask more specifically: why would one side ever announce a capability > in phase 1, but not in phase 2? Is the reason that capabilities are in > fact tied to credentials? More that they're tied to the credential helper. For example, say we have helpers A and B, in that order. B is incapable, but both A and Git understand the new authtype capability. If we announce the capability as in this series, we can get a new credential using the authtype capability if A is willing to provide something to us, but we can't if A has no credentials for us and B wants to provide them for us. This would also be true if we used your proposal of negotiation, but because we have external callers (e.g., Git LFS) who may invoke `git credential fill`, which would be a separate process from `git credential capability`, we'd still have to have some way to tell `git credential fill` what capabilities the external caller supported. The per-phase capabilities are such that we don't request functionality that our callers can't use. For example, if our external caller (phase 1) doesn't support the `authtype` credential, then we don't pass it to the helper (phase 2), since the external caller might not be able to use the result if we do. If the external caller (phase 1) _does_ support it, but the helper does not (phase 2), then we won't return the capability as the result of `git credential fill` (phase 3), so our external caller will know that this isn't supported. As a practical matter, that doesn't provide a great deal of useful information to the caller at the moment, but it definitely could in the future (say, if we had a capability for a certain form of data encoding). All of this is also true for internal (e.g., git-http-backend) callers, except that phase 1 has all the capabilities we know about automatically set, and phase 3 stores the data in the internal structure we'll use for the `store` and `erase` calls. I do, however, think some way to query capabilities more generically would be helpful, so I'll see if I can add such changes into a v2. I think we still need the current approach to make the use case I mentioned work, though.
On Thu, Apr 04, 2024 at 12:39:58AM +0000, brian m. carlson wrote: > On 2024-04-02 at 10:04:31, Patrick Steinhardt wrote: > > The intent of this is quite clear to me, but thanks for re-explaining > > the bigger picture :) > > Sorry I misunderstood what you were getting it. > > > I think you misunderstood my confusion. I didn't meant to say that there > > should be non-boolean capabilities. I was rather missing the picture of > > how exactly you can advertise multiple capabilities with the infra that > > currently exists, and why the infra supports per-phase capabilities. > > > > Basically, what I would have expected is a protocol where both Git and > > the credential helper initially did a single "handshake" that also > > announces capabilities. So something like: > > > > HELPER: capability foobar > > HELPER: capability barfoo > > GIT: capability foobar > > > > Git would only acknowledge capabilities that it both understands and > > that have been announced by the helper. So at the end of this both have > > agreed on a single capability "foobar". > > > > This is roughly how the remote helper capability subsystem works. What > > this patch is introducing seems quite a bit more complicated than that > > though because we have "staged" capabilities. I assume there is good > > reason for this complexity, but I didn't yet manage to figure out the > > reasoning behind it. > > > > To ask more specifically: why would one side ever announce a capability > > in phase 1, but not in phase 2? Is the reason that capabilities are in > > fact tied to credentials? > > More that they're tied to the credential helper. For example, say we > have helpers A and B, in that order. B is incapable, but both A and Git > understand the new authtype capability. If we announce the capability > as in this series, we can get a new credential using the authtype > capability if A is willing to provide something to us, but we can't if A > has no credentials for us and B wants to provide them for us. > > This would also be true if we used your proposal of negotiation, but > because we have external callers (e.g., Git LFS) who may invoke `git > credential fill`, which would be a separate process from `git credential > capability`, we'd still have to have some way to tell `git credential > fill` what capabilities the external caller supported. > > The per-phase capabilities are such that we don't request functionality > that our callers can't use. For example, if our external caller (phase > 1) doesn't support the `authtype` credential, then we don't pass it to > the helper (phase 2), since the external caller might not be able to use > the result if we do. If the external caller (phase 1) _does_ support > it, but the helper does not (phase 2), then we won't return the > capability as the result of `git credential fill` (phase 3), so our > external caller will know that this isn't supported. As a practical > matter, that doesn't provide a great deal of useful information to the > caller at the moment, but it definitely could in the future (say, if we > had a capability for a certain form of data encoding). > > All of this is also true for internal (e.g., git-http-backend) callers, > except that phase 1 has all the capabilities we know about automatically > set, and phase 3 stores the data in the internal structure we'll use for > the `store` and `erase` calls. > > I do, however, think some way to query capabilities more generically > would be helpful, so I'll see if I can add such changes into a v2. I > think we still need the current approach to make the use case I > mentioned work, though. Great. Thanks a lot for your explanations! Patrick
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c index 3a6a750a8e..ccbcf99ac1 100644 --- a/builtin/credential-cache--daemon.c +++ b/builtin/credential-cache--daemon.c @@ -115,7 +115,7 @@ static int read_request(FILE *fh, struct credential *c, return error("client sent bogus timeout line: %s", item.buf); *timeout = atoi(p); - if (credential_read(c, fh) < 0) + if (credential_read(c, fh, CREDENTIAL_OP_HELPER) < 0) return -1; return 0; } diff --git a/builtin/credential-store.c b/builtin/credential-store.c index 4a492411bb..494c809332 100644 --- a/builtin/credential-store.c +++ b/builtin/credential-store.c @@ -205,7 +205,7 @@ int cmd_credential_store(int argc, const char **argv, const char *prefix) if (!fns.nr) die("unable to set up default path; use --file"); - if (credential_read(&c, stdin) < 0) + if (credential_read(&c, stdin, CREDENTIAL_OP_HELPER) < 0) die("unable to read credential"); if (!strcmp(op, "get")) diff --git a/builtin/credential.c b/builtin/credential.c index 7010752987..5123dabcf1 100644 --- a/builtin/credential.c +++ b/builtin/credential.c @@ -17,12 +17,12 @@ int cmd_credential(int argc, const char **argv, const char *prefix UNUSED) usage(usage_msg); op = argv[1]; - if (credential_read(&c, stdin) < 0) + if (credential_read(&c, stdin, CREDENTIAL_OP_INITIAL) < 0) die("unable to read credential from stdin"); if (!strcmp(op, "fill")) { - credential_fill(&c); - credential_write(&c, stdout); + credential_fill(&c, 0); + credential_write(&c, stdout, CREDENTIAL_OP_RESPONSE); } else if (!strcmp(op, "approve")) { credential_approve(&c); } else if (!strcmp(op, "reject")) { diff --git a/credential.c b/credential.c index c521822e5a..f2a26b8672 100644 --- a/credential.c +++ b/credential.c @@ -34,6 +34,11 @@ void credential_clear(struct credential *c) credential_init(c); } +static void credential_set_all_capabilities(struct credential *c) +{ + c->capa_authtype.request_initial = 1; +} + int credential_match(const struct credential *want, const struct credential *have, int match_password) { @@ -210,7 +215,39 @@ static void credential_getpass(struct credential *c) PROMPT_ASKPASS); } -int credential_read(struct credential *c, FILE *fp) +static void credential_set_capability(struct credential_capability *capa, int op_type) +{ + switch (op_type) { + case CREDENTIAL_OP_INITIAL: + capa->request_initial = 1; + break; + case CREDENTIAL_OP_HELPER: + capa->request_helper = 1; + break; + case CREDENTIAL_OP_RESPONSE: + capa->response = 1; + break; + } +} + +static int credential_has_capability(const struct credential_capability *capa, int op_type) +{ + /* + * We're checking here if each previous step indicated that we had the + * capability. If it did, then we want to pass it along; conversely, if + * it did not, we don't want to report that to our caller. + */ + switch (op_type) { + case CREDENTIAL_OP_HELPER: + return capa->request_initial; + case CREDENTIAL_OP_RESPONSE: + return capa->request_initial && capa->request_helper; + default: + return 0; + } +} + +int credential_read(struct credential *c, FILE *fp, int op_type) { struct strbuf line = STRBUF_INIT; @@ -249,6 +286,8 @@ int credential_read(struct credential *c, FILE *fp) c->path = xstrdup(value); } else if (!strcmp(key, "wwwauth[]")) { strvec_push(&c->wwwauth_headers, value); + } else if (!strcmp(key, "capability[]") && !strcmp(value, "authtype")) { + credential_set_capability(&c->capa_authtype, op_type); } else if (!strcmp(key, "password_expiry_utc")) { errno = 0; c->password_expiry_utc = parse_timestamp(value, NULL, 10); @@ -288,14 +327,18 @@ static void credential_write_item(FILE *fp, const char *key, const char *value, fprintf(fp, "%s=%s\n", key, value); } -void credential_write(const struct credential *c, FILE *fp) +void credential_write(const struct credential *c, FILE *fp, int op_type) { + if (credential_has_capability(&c->capa_authtype, op_type)) { + credential_write_item(fp, "capability[]", "authtype", 0); + credential_write_item(fp, "authtype", c->authtype, 0); + credential_write_item(fp, "credential", c->credential, 0); + } credential_write_item(fp, "protocol", c->protocol, 1); credential_write_item(fp, "host", c->host, 1); 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, "credential", c->credential, 0); credential_write_item(fp, "oauth_refresh_token", c->oauth_refresh_token, 0); if (c->password_expiry_utc != TIME_MAX) { char *s = xstrfmt("%"PRItime, c->password_expiry_utc); @@ -304,7 +347,6 @@ void credential_write(const struct credential *c, FILE *fp) } for (size_t i = 0; i < c->wwwauth_headers.nr; i++) credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); - credential_write_item(fp, "authtype", c->authtype, 0); } static int run_credential_helper(struct credential *c, @@ -327,14 +369,14 @@ static int run_credential_helper(struct credential *c, fp = xfdopen(helper.in, "w"); sigchain_push(SIGPIPE, SIG_IGN); - credential_write(c, fp); + credential_write(c, fp, want_output ? CREDENTIAL_OP_HELPER : CREDENTIAL_OP_RESPONSE); fclose(fp); sigchain_pop(SIGPIPE); if (want_output) { int r; fp = xfdopen(helper.out, "r"); - r = credential_read(c, fp); + r = credential_read(c, fp, CREDENTIAL_OP_HELPER); fclose(fp); if (r < 0) { finish_command(&helper); @@ -367,7 +409,7 @@ static int credential_do(struct credential *c, const char *helper, return r; } -void credential_fill(struct credential *c) +void credential_fill(struct credential *c, int all_capabilities) { int i; @@ -375,6 +417,8 @@ void credential_fill(struct credential *c) return; credential_apply_config(c); + if (all_capabilities) + credential_set_all_capabilities(c); for (i = 0; i < c->helpers.nr; i++) { credential_do(c, c->helpers.items[i].string, "get"); diff --git a/credential.h b/credential.h index 9db892cf4d..2051d04c5a 100644 --- a/credential.h +++ b/credential.h @@ -93,6 +93,25 @@ * ----------------------------------------------------------------------- */ +/* + * These values define the kind of operation we're performing and the + * capabilities at each stage. The first is either an external request (via git + * credential fill) or an internal request (e.g., via the HTTP) code. The + * second is the call to the credential helper, and the third is the response + * we're providing. + * + * At each stage, we will emit the capability only if the previous stage + * supported it. + */ +#define CREDENTIAL_OP_INITIAL 1 +#define CREDENTIAL_OP_HELPER 2 +#define CREDENTIAL_OP_RESPONSE 3 + +struct credential_capability { + unsigned request_initial:1, + request_helper:1, + response:1; +}; /** * This struct represents a single username/password combination @@ -136,6 +155,8 @@ struct credential { use_http_path:1, username_from_proto:1; + struct credential_capability capa_authtype; + char *username; char *password; char *credential; @@ -174,8 +195,11 @@ void credential_clear(struct credential *); * returns, the username and password fields of the credential are * guaranteed to be non-NULL. If an error occurs, the function will * die(). + * + * If all_capabilities is set, this is an internal user that is prepared + * to deal with all known capabilities, and we should advertise that fact. */ -void credential_fill(struct credential *); +void credential_fill(struct credential *, int all_capabilities); /** * Inform the credential subsystem that the provided credentials @@ -198,8 +222,8 @@ void credential_approve(struct credential *); */ void credential_reject(struct credential *); -int credential_read(struct credential *, FILE *); -void credential_write(const struct credential *, FILE *); +int credential_read(struct credential *, FILE *, int); +void credential_write(const struct credential *, FILE *, int); /* * Parse a url into a credential struct, replacing any existing contents. diff --git a/http.c b/http.c index 1c2200da77..4f5df6fb14 100644 --- a/http.c +++ b/http.c @@ -569,7 +569,7 @@ static void init_curl_http_auth(CURL *result) return; } - credential_fill(&http_auth); + credential_fill(&http_auth, 0); curl_easy_setopt(result, CURLOPT_USERNAME, http_auth.username); curl_easy_setopt(result, CURLOPT_PASSWORD, http_auth.password); @@ -596,7 +596,7 @@ static void init_curl_proxy_auth(CURL *result) { if (proxy_auth.username) { if (!proxy_auth.password) - credential_fill(&proxy_auth); + credential_fill(&proxy_auth, 0); set_proxyauth_name_password(result); } @@ -630,7 +630,7 @@ static int has_cert_password(void) cert_auth.host = xstrdup(""); cert_auth.username = xstrdup(""); cert_auth.path = xstrdup(ssl_cert); - credential_fill(&cert_auth); + credential_fill(&cert_auth, 0); } return 1; } @@ -645,7 +645,7 @@ static int has_proxy_cert_password(void) proxy_cert_auth.host = xstrdup(""); proxy_cert_auth.username = xstrdup(""); proxy_cert_auth.path = xstrdup(http_proxy_ssl_cert); - credential_fill(&proxy_cert_auth); + credential_fill(&proxy_cert_auth, 0); } return 1; } @@ -2191,7 +2191,7 @@ static int http_request_reauth(const char *url, BUG("Unknown http_request target"); } - credential_fill(&http_auth); + credential_fill(&http_auth, 0); return http_request(url, result, target, options); } diff --git a/imap-send.c b/imap-send.c index f2e1947e63..8c89e866b6 100644 --- a/imap-send.c +++ b/imap-send.c @@ -944,7 +944,7 @@ static void server_fill_credential(struct imap_server_conf *srvc, struct credent cred->username = xstrdup_or_null(srvc->user); cred->password = xstrdup_or_null(srvc->pass); - credential_fill(cred); + credential_fill(cred, 1); if (!srvc->user) srvc->user = xstrdup(cred->username); diff --git a/remote-curl.c b/remote-curl.c index e37eaa17b7..f96bda2431 100644 --- a/remote-curl.c +++ b/remote-curl.c @@ -926,7 +926,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece do { err = probe_rpc(rpc, &results); if (err == HTTP_REAUTH) - credential_fill(&http_auth); + credential_fill(&http_auth, 0); } while (err == HTTP_REAUTH); if (err != HTTP_OK) return -1; @@ -1044,7 +1044,7 @@ static int post_rpc(struct rpc_state *rpc, int stateless_connect, int flush_rece rpc->any_written = 0; err = run_slot(slot, NULL); if (err == HTTP_REAUTH && !large_request) { - credential_fill(&http_auth); + credential_fill(&http_auth, 0); curl_slist_free_all(headers); goto retry; } diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 400f6bdbca..8477108b28 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -12,7 +12,13 @@ test_expect_success 'setup helper scripts' ' IFS== while read key value; do echo >&2 "$whoami: $key=$value" - eval "$key=$value" + if test -z "${key%%*\[\]}" + then + key=${key%%\[\]} + eval "$key=\"\$$key $value\"" + else + eval "$key=$value" + fi done IFS=$OIFS EOF @@ -35,6 +41,16 @@ test_expect_success 'setup helper scripts' ' test -z "$pass" || echo password=$pass EOF + write_script git-credential-verbatim-cred <<-\EOF && + authtype=$1; shift + credential=$1; shift + . ./dump + echo capability[]=authtype + test -z "${capability##*authtype*}" || return + test -z "$authtype" || echo authtype=$authtype + test -z "$credential" || echo credential=$credential + EOF + write_script git-credential-verbatim-with-expiry <<-\EOF && user=$1; shift pass=$1; shift @@ -64,6 +80,26 @@ test_expect_success 'credential_fill invokes helper' ' EOF ' +test_expect_success 'credential_fill invokes helper with credential' ' + check fill "verbatim-cred Bearer token" <<-\EOF + capability[]=authtype + protocol=http + host=example.com + -- + capability[]=authtype + authtype=Bearer + credential=token + protocol=http + host=example.com + -- + verbatim-cred: get + verbatim-cred: capability[]=authtype + verbatim-cred: protocol=http + verbatim-cred: host=example.com + EOF +' + + test_expect_success 'credential_fill invokes multiple helpers' ' check fill useless "verbatim foo bar" <<-\EOF protocol=http @@ -83,6 +119,42 @@ test_expect_success 'credential_fill invokes multiple helpers' ' EOF ' +test_expect_success 'credential_fill response does not get capabilities when helpers are incapable' ' + check fill useless "verbatim foo bar" <<-\EOF + capability[]=authtype + protocol=http + host=example.com + -- + protocol=http + host=example.com + username=foo + password=bar + -- + useless: get + useless: capability[]=authtype + useless: protocol=http + useless: host=example.com + verbatim: get + verbatim: capability[]=authtype + verbatim: protocol=http + verbatim: host=example.com + EOF +' + +test_expect_success 'credential_fill response does not get capabilities when caller is incapable' ' + check fill "verbatim-cred Bearer token" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + -- + verbatim-cred: get + verbatim-cred: protocol=http + verbatim-cred: host=example.com + EOF +' + test_expect_success 'credential_fill stops when we get a full response' ' check fill "verbatim one two" "verbatim three four" <<-\EOF protocol=http @@ -99,6 +171,25 @@ test_expect_success 'credential_fill stops when we get a full response' ' EOF ' +test_expect_success 'credential_fill thinks a credential is a full response' ' + check fill "verbatim-cred Bearer token" "verbatim three four" <<-\EOF + capability[]=authtype + protocol=http + host=example.com + -- + capability[]=authtype + authtype=Bearer + credential=token + protocol=http + host=example.com + -- + verbatim-cred: get + verbatim-cred: capability[]=authtype + verbatim-cred: protocol=http + verbatim-cred: host=example.com + EOF +' + test_expect_success 'credential_fill continues through partial response' ' check fill "verbatim one \"\"" "verbatim two three" <<-\EOF protocol=http @@ -175,6 +266,20 @@ test_expect_success 'credential_fill passes along metadata' ' EOF ' +test_expect_success 'credential_fill produces no credential without capability' ' + check fill "verbatim-cred Bearer token" <<-\EOF + protocol=http + host=example.com + -- + protocol=http + host=example.com + -- + verbatim-cred: get + verbatim-cred: protocol=http + verbatim-cred: host=example.com + EOF +' + test_expect_success 'credential_approve calls all helpers' ' check approve useless "verbatim one two" <<-\EOF protocol=http
We support the new credential and authtype fields, but we lack a way to indicate to a credential helper that we'd like them to be used. Without some sort of indication, the credential helper doesn't know if it should try to provide us a username and password, or a pre-encoded credential. For example, the helper might prefer a more restricted Bearer token if pre-encoded credentials are possible, but might have to fall back to more general username and password if not. Let's provide a simple way to indicate whether Git (or, for that matter, the helper) is capable of understanding the authtype and credential fields. We send this capability when we generate a request, and the other side may reply to indicate to us that it does, too. For now, don't enable sending capabilities for the HTTP code. In a future commit, we'll introduce appropriate handling for that code, which requires more in-depth work. The logic for determining whether a capability is supported may seem complex, but it is not. At each stage, we emit the capability to the following stage if all preceding stages have declared it. Thus, if the caller to git credential fill didn't declare it, then we won't send it to the helper, and if fill's caller did send but the helper doesn't understand it, then we won't send it on in the response. If we're an internal user, then we know about all capabilities and will request them. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- builtin/credential-cache--daemon.c | 2 +- builtin/credential-store.c | 2 +- builtin/credential.c | 6 +- credential.c | 58 ++++++++++++++-- credential.h | 30 +++++++- http.c | 10 +-- imap-send.c | 2 +- remote-curl.c | 4 +- t/t0300-credentials.sh | 107 ++++++++++++++++++++++++++++- 9 files changed, 197 insertions(+), 24 deletions(-)