| Message ID | 20240324011301.1553072-9-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support for arbitrary schemes in credentials | expand |
On Sun, Mar 24, 2024 at 1:13 AM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > Until now, our credential code has mostly deal with usernames and > passwords and we've let libcurl deal with the variant of authentication > to be used. However, now that we have the credential value, the > credential helper can take control of the authentication, so the value > provided might be something that's generated, such as a Digest hash > value. > > In such a case, it would be helpful for a credential helper that gets an > erase or store command to be able to keep track of an identifier for the > original secret that went into the computation. Furthermore, some types > of authentication, such as NTLM and Kerberos, actually need two round > trips to authenticate, which will require that the credential helper > keep some state. > > In order to allow for these use cases and others, allow storing state in > a field called "state[]". This value is passed back to the credential > helper that created it, which avoids confusion caused by parsing values > from different helpers. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > Documentation/git-credential.txt | 29 ++++++++++++++++++----------- > credential.c | 20 +++++++++++++++++--- > credential.h | 7 +++++++ > t/t0300-credentials.sh | 29 +++++++++++++++++++++++++++++ > 4 files changed, 71 insertions(+), 14 deletions(-) > > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > index f3ed3a82fa..ef30c89c00 100644 > --- a/Documentation/git-credential.txt > +++ b/Documentation/git-credential.txt > @@ -196,6 +196,15 @@ provided on input. > This value should not be sent unless the appropriate capability (see below) is > provided on input. > > +`state[]`:: > + This value provides an opaque state that will be passed back to this helper > + if it is called again. Each different credential helper may specify this > + once. The value should include a prefix unique to the credential helper and > + should ignore values that don't match its prefix. Does Git ever populate state[] in 'store' or 'erase' requests, or only 'get' requests? It might be worthwhile to spell this out. This seems somewhat different to other multi-valued attributes, particularly the "set at most one value" constraint. As an alternative, how about a single-valued attribute stored independently for each helper (vector length equal to the number of configured helpers)? Then in repeat requests send the "nth state to the nth helper". This would avoid the complexity of the prefix mechanism. > ++ > +This value should not be sent unless the appropriate capability (see below) is > +provided on input. > + > `wwwauth[]`:: > > When an HTTP response is received by Git that includes one or more > @@ -208,18 +217,16 @@ they appear in the HTTP response. This attribute is 'one-way' from Git > to pass additional information to credential helpers. > > `capability[]`:: > - This signals that the caller supports the capability in question. > - This can be used to provide better, more specific data as part of the > - protocol. > + This signals that Git, or the helper, as appropriate, supports the > + capability in question. This can be used to provide better, more specific > + data as part of the protocol. > + > -The only capability currently supported is `authtype`, which indicates that the > -`authtype` and `credential` values are understood. It is not obligatory to use > -these values in such a case, but they should not be provided without this > -capability. > -+ > -Callers of `git credential` and credential helpers should emit the > -capabilities they support unconditionally, and Git will gracefully > -handle passing them on. > +There are two currently supported capabilities. The first is `authtype`, which > +indicates that the `authtype` and `credential` values are understood. The > +second is `state`, which indicates that the `state[]` value is understood. > > + > +It is not obligatory to use the additional features just because the capability > +is supported, but they should not be provided without this capability. > > Unrecognised attributes and capabilities are silently discarded. > > diff --git a/credential.c b/credential.c > index f2a26b8672..0cd7dd2a00 100644 > --- a/credential.c > +++ b/credential.c > @@ -30,6 +30,7 @@ void credential_clear(struct credential *c) > free(c->authtype); > string_list_clear(&c->helpers, 0); > strvec_clear(&c->wwwauth_headers); > + strvec_clear(&c->state_headers); > > credential_init(c); > } > @@ -286,8 +287,13 @@ int credential_read(struct credential *c, FILE *fp, int op_type) > 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, "state[]")) { > + strvec_push(&c->state_headers, value); > + } else if (!strcmp(key, "capability[]")) { > + if (!strcmp(value, "authtype")) > + credential_set_capability(&c->capa_authtype, op_type); > + else if (!strcmp(value, "state")) > + credential_set_capability(&c->capa_state, op_type); > } else if (!strcmp(key, "password_expiry_utc")) { > errno = 0; > c->password_expiry_utc = parse_timestamp(value, NULL, 10); > @@ -329,8 +335,12 @@ static void credential_write_item(FILE *fp, const char *key, const char *value, > > void credential_write(const struct credential *c, FILE *fp, int op_type) > { > - if (credential_has_capability(&c->capa_authtype, op_type)) { > + if (credential_has_capability(&c->capa_authtype, op_type)) > credential_write_item(fp, "capability[]", "authtype", 0); > + if (credential_has_capability(&c->capa_state, op_type)) > + credential_write_item(fp, "capability[]", "state", 0); > + > + if (credential_has_capability(&c->capa_authtype, op_type)) { > credential_write_item(fp, "authtype", c->authtype, 0); > credential_write_item(fp, "credential", c->credential, 0); > } > @@ -347,6 +357,10 @@ void credential_write(const struct credential *c, FILE *fp, int op_type) > } > for (size_t i = 0; i < c->wwwauth_headers.nr; i++) > credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); > + if (credential_has_capability(&c->capa_state, op_type)) { > + for (size_t i = 0; i < c->state_headers.nr; i++) > + credential_write_item(fp, "state[]", c->state_headers.v[i], 0); > + } > } > > static int run_credential_helper(struct credential *c, > diff --git a/credential.h b/credential.h > index 2051d04c5a..e2021455fe 100644 > --- a/credential.h > +++ b/credential.h > @@ -142,6 +142,11 @@ struct credential { > */ > struct strvec wwwauth_headers; > > + /** > + * A `strvec` of state headers from credential helpers. > + */ > + struct strvec state_headers; > + > > /** > * Internal use only. Keeps track of if we previously matched against a > * WWW-Authenticate header line in order to re-fold future continuation > @@ -156,6 +161,7 @@ struct credential { > username_from_proto:1; > > struct credential_capability capa_authtype; > + struct credential_capability capa_state; > > char *username; > char *password; > @@ -177,6 +183,7 @@ struct credential { > .helpers = STRING_LIST_INIT_DUP, \ > .password_expiry_utc = TIME_MAX, \ > .wwwauth_headers = STRVEC_INIT, \ > + .state_headers = STRVEC_INIT, \ > } > > /* Initialize a credential structure, setting all fields to empty. */ > diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh > index 8477108b28..aa56e0dc84 100755 > --- a/t/t0300-credentials.sh > +++ b/t/t0300-credentials.sh > @@ -46,9 +46,12 @@ test_expect_success 'setup helper scripts' ' > credential=$1; shift > . ./dump > echo capability[]=authtype > + echo capability[]=state > test -z "${capability##*authtype*}" || return > test -z "$authtype" || echo authtype=$authtype > test -z "$credential" || echo credential=$credential > + test -z "${capability##*state*}" || return > + echo state[]=verbatim-cred:foo > EOF > > write_script git-credential-verbatim-with-expiry <<-\EOF && > @@ -99,6 +102,29 @@ test_expect_success 'credential_fill invokes helper with credential' ' > EOF > ' > > +test_expect_success 'credential_fill invokes helper with credential and state' ' > + check fill "verbatim-cred Bearer token" <<-\EOF > + capability[]=authtype > + capability[]=state > + protocol=http > + host=example.com > + -- > + capability[]=authtype > + capability[]=state > + authtype=Bearer > + credential=token > + protocol=http > + host=example.com > + state[]=verbatim-cred:foo > + -- > + verbatim-cred: get > + verbatim-cred: capability[]=authtype > + verbatim-cred: capability[]=state > + 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 > @@ -122,6 +148,7 @@ test_expect_success 'credential_fill invokes multiple helpers' ' > test_expect_success 'credential_fill response does not get capabilities when helpers are incapable' ' > check fill useless "verbatim foo bar" <<-\EOF > capability[]=authtype > + capability[]=state > protocol=http > host=example.com > -- > @@ -132,10 +159,12 @@ test_expect_success 'credential_fill response does not get capabilities when hel > -- > useless: get > useless: capability[]=authtype > + useless: capability[]=state > useless: protocol=http > useless: host=example.com > verbatim: get > verbatim: capability[]=authtype > + verbatim: capability[]=state > verbatim: protocol=http > verbatim: host=example.com > EOF
On 2024-04-01 at 21:05:28, mirth hickford wrote: > On Sun, Mar 24, 2024 at 1:13 AM brian m. carlson > <sandals@crustytoothpaste.net> wrote: > > > > Until now, our credential code has mostly deal with usernames and > > passwords and we've let libcurl deal with the variant of authentication > > to be used. However, now that we have the credential value, the > > credential helper can take control of the authentication, so the value > > provided might be something that's generated, such as a Digest hash > > value. > > > > In such a case, it would be helpful for a credential helper that gets an > > erase or store command to be able to keep track of an identifier for the > > original secret that went into the computation. Furthermore, some types > > of authentication, such as NTLM and Kerberos, actually need two round > > trips to authenticate, which will require that the credential helper > > keep some state. > > > > In order to allow for these use cases and others, allow storing state in > > a field called "state[]". This value is passed back to the credential > > helper that created it, which avoids confusion caused by parsing values > > from different helpers. > > > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > > --- > > Documentation/git-credential.txt | 29 ++++++++++++++++++----------- > > credential.c | 20 +++++++++++++++++--- > > credential.h | 7 +++++++ > > t/t0300-credentials.sh | 29 +++++++++++++++++++++++++++++ > > 4 files changed, 71 insertions(+), 14 deletions(-) > > > > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > > index f3ed3a82fa..ef30c89c00 100644 > > --- a/Documentation/git-credential.txt > > +++ b/Documentation/git-credential.txt > > @@ -196,6 +196,15 @@ provided on input. > > This value should not be sent unless the appropriate capability (see below) is > > provided on input. > > > > +`state[]`:: > > + This value provides an opaque state that will be passed back to this helper > > + if it is called again. Each different credential helper may specify this > > + once. The value should include a prefix unique to the credential helper and > > + should ignore values that don't match its prefix. > > Does Git ever populate state[] in 'store' or 'erase' requests, or > only 'get' requests? It might be worthwhile to spell this out. Yes, it's populated with whatever the last state value was from `get`. > This seems somewhat different to other multi-valued attributes, > particularly the "set at most one value" constraint. As an > alternative, how about a single-valued attribute stored independently > for each helper (vector length equal to the number of configured > helpers)? Then in repeat requests send the "nth state to the nth > helper". This would avoid the complexity of the prefix mechanism. I originally tried that approach, but if you have external callers of `git credential` (like Git LFS), that doesn't work, since you need to make two separate calls: one (with `get`) to fetch the credentials that returns multiple state values, and one (with `store` or `erase`) that sends the data back to accept or reject the credentials. Since there's no internal state in Git between the two calls, it's not possible to only send certain data to certain helpers.
diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt index f3ed3a82fa..ef30c89c00 100644 --- a/Documentation/git-credential.txt +++ b/Documentation/git-credential.txt @@ -196,6 +196,15 @@ provided on input. This value should not be sent unless the appropriate capability (see below) is provided on input. +`state[]`:: + This value provides an opaque state that will be passed back to this helper + if it is called again. Each different credential helper may specify this + once. The value should include a prefix unique to the credential helper and + should ignore values that don't match its prefix. ++ +This value should not be sent unless the appropriate capability (see below) is +provided on input. + `wwwauth[]`:: When an HTTP response is received by Git that includes one or more @@ -208,18 +217,16 @@ they appear in the HTTP response. This attribute is 'one-way' from Git to pass additional information to credential helpers. `capability[]`:: - This signals that the caller supports the capability in question. - This can be used to provide better, more specific data as part of the - protocol. + This signals that Git, or the helper, as appropriate, supports the + capability in question. This can be used to provide better, more specific + data as part of the protocol. + -The only capability currently supported is `authtype`, which indicates that the -`authtype` and `credential` values are understood. It is not obligatory to use -these values in such a case, but they should not be provided without this -capability. -+ -Callers of `git credential` and credential helpers should emit the -capabilities they support unconditionally, and Git will gracefully -handle passing them on. +There are two currently supported capabilities. The first is `authtype`, which +indicates that the `authtype` and `credential` values are understood. The +second is `state`, which indicates that the `state[]` value is understood. + +It is not obligatory to use the additional features just because the capability +is supported, but they should not be provided without this capability. Unrecognised attributes and capabilities are silently discarded. diff --git a/credential.c b/credential.c index f2a26b8672..0cd7dd2a00 100644 --- a/credential.c +++ b/credential.c @@ -30,6 +30,7 @@ void credential_clear(struct credential *c) free(c->authtype); string_list_clear(&c->helpers, 0); strvec_clear(&c->wwwauth_headers); + strvec_clear(&c->state_headers); credential_init(c); } @@ -286,8 +287,13 @@ int credential_read(struct credential *c, FILE *fp, int op_type) 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, "state[]")) { + strvec_push(&c->state_headers, value); + } else if (!strcmp(key, "capability[]")) { + if (!strcmp(value, "authtype")) + credential_set_capability(&c->capa_authtype, op_type); + else if (!strcmp(value, "state")) + credential_set_capability(&c->capa_state, op_type); } else if (!strcmp(key, "password_expiry_utc")) { errno = 0; c->password_expiry_utc = parse_timestamp(value, NULL, 10); @@ -329,8 +335,12 @@ static void credential_write_item(FILE *fp, const char *key, const char *value, void credential_write(const struct credential *c, FILE *fp, int op_type) { - if (credential_has_capability(&c->capa_authtype, op_type)) { + if (credential_has_capability(&c->capa_authtype, op_type)) credential_write_item(fp, "capability[]", "authtype", 0); + if (credential_has_capability(&c->capa_state, op_type)) + credential_write_item(fp, "capability[]", "state", 0); + + if (credential_has_capability(&c->capa_authtype, op_type)) { credential_write_item(fp, "authtype", c->authtype, 0); credential_write_item(fp, "credential", c->credential, 0); } @@ -347,6 +357,10 @@ void credential_write(const struct credential *c, FILE *fp, int op_type) } for (size_t i = 0; i < c->wwwauth_headers.nr; i++) credential_write_item(fp, "wwwauth[]", c->wwwauth_headers.v[i], 0); + if (credential_has_capability(&c->capa_state, op_type)) { + for (size_t i = 0; i < c->state_headers.nr; i++) + credential_write_item(fp, "state[]", c->state_headers.v[i], 0); + } } static int run_credential_helper(struct credential *c, diff --git a/credential.h b/credential.h index 2051d04c5a..e2021455fe 100644 --- a/credential.h +++ b/credential.h @@ -142,6 +142,11 @@ struct credential { */ struct strvec wwwauth_headers; + /** + * A `strvec` of state headers from credential helpers. + */ + struct strvec state_headers; + /** * Internal use only. Keeps track of if we previously matched against a * WWW-Authenticate header line in order to re-fold future continuation @@ -156,6 +161,7 @@ struct credential { username_from_proto:1; struct credential_capability capa_authtype; + struct credential_capability capa_state; char *username; char *password; @@ -177,6 +183,7 @@ struct credential { .helpers = STRING_LIST_INIT_DUP, \ .password_expiry_utc = TIME_MAX, \ .wwwauth_headers = STRVEC_INIT, \ + .state_headers = STRVEC_INIT, \ } /* Initialize a credential structure, setting all fields to empty. */ diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh index 8477108b28..aa56e0dc84 100755 --- a/t/t0300-credentials.sh +++ b/t/t0300-credentials.sh @@ -46,9 +46,12 @@ test_expect_success 'setup helper scripts' ' credential=$1; shift . ./dump echo capability[]=authtype + echo capability[]=state test -z "${capability##*authtype*}" || return test -z "$authtype" || echo authtype=$authtype test -z "$credential" || echo credential=$credential + test -z "${capability##*state*}" || return + echo state[]=verbatim-cred:foo EOF write_script git-credential-verbatim-with-expiry <<-\EOF && @@ -99,6 +102,29 @@ test_expect_success 'credential_fill invokes helper with credential' ' EOF ' +test_expect_success 'credential_fill invokes helper with credential and state' ' + check fill "verbatim-cred Bearer token" <<-\EOF + capability[]=authtype + capability[]=state + protocol=http + host=example.com + -- + capability[]=authtype + capability[]=state + authtype=Bearer + credential=token + protocol=http + host=example.com + state[]=verbatim-cred:foo + -- + verbatim-cred: get + verbatim-cred: capability[]=authtype + verbatim-cred: capability[]=state + 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 @@ -122,6 +148,7 @@ test_expect_success 'credential_fill invokes multiple helpers' ' test_expect_success 'credential_fill response does not get capabilities when helpers are incapable' ' check fill useless "verbatim foo bar" <<-\EOF capability[]=authtype + capability[]=state protocol=http host=example.com -- @@ -132,10 +159,12 @@ test_expect_success 'credential_fill response does not get capabilities when hel -- useless: get useless: capability[]=authtype + useless: capability[]=state useless: protocol=http useless: host=example.com verbatim: get verbatim: capability[]=authtype + verbatim: capability[]=state verbatim: protocol=http verbatim: host=example.com EOF
Until now, our credential code has mostly deal with usernames and passwords and we've let libcurl deal with the variant of authentication to be used. However, now that we have the credential value, the credential helper can take control of the authentication, so the value provided might be something that's generated, such as a Digest hash value. In such a case, it would be helpful for a credential helper that gets an erase or store command to be able to keep track of an identifier for the original secret that went into the computation. Furthermore, some types of authentication, such as NTLM and Kerberos, actually need two round trips to authenticate, which will require that the credential helper keep some state. In order to allow for these use cases and others, allow storing state in a field called "state[]". This value is passed back to the credential helper that created it, which avoids confusion caused by parsing values from different helpers. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- Documentation/git-credential.txt | 29 ++++++++++++++++++----------- credential.c | 20 +++++++++++++++++--- credential.h | 7 +++++++ t/t0300-credentials.sh | 29 +++++++++++++++++++++++++++++ 4 files changed, 71 insertions(+), 14 deletions(-)