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@xxxxxxxxxxxxxxxxxxxx>
> ---
> 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
>
Attachment:
signature.asc
Description: PGP signature
