diff mbox series

[06/13] docs: indicate new credential protocol fields

Message ID 20240324011301.1553072-7-sandals@crustytoothpaste.net (mailing list archive)
State Superseded
Headers show
Series Support for arbitrary schemes in credentials | expand

Commit Message

brian m. carlson March 24, 2024, 1:12 a.m. UTC
Now that we have new fields (authtype and credential), let's document
them for users and credential helper implementers.

Indicate specifically what common values of authtype are and what values
are allowed.  Note that, while common, digest and NTLM authentication
are insecure because they require unsalted, uniterated password hashes
to be stored.

Tell users that they can continue to use a username and password even if
the new capability is supported.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 Documentation/git-credential.txt | 34 +++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

Comments

M Hickford March 25, 2024, 11:16 p.m. UTC | #1
> Tell users that they can continue to use a username and password even if
> the new capability is supported.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  Documentation/git-credential.txt | 34 +++++++++++++++++++++++++++++++-
>  1 file changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
> index 918a0aa42b..f3ed3a82fa 100644
> --- a/Documentation/git-credential.txt
> +++ b/Documentation/git-credential.txt
> @@ -178,6 +178,24 @@ empty string.
>  Components which are missing from the URL (e.g., there is no
>  username in the example above) will be left unset.
>  
> +`authtype`::
> +	This indicates that the authentication scheme in question should be used.
> +	Common values for HTTP and HTTPS include `basic`, `digest`, and `ntlm`,
> +	although the latter two are insecure and should not be used.  If `credential`
> +	is used, this may be set to an arbitrary string suitable for the protocol in
> +	question (usually HTTP).

How about adding 'bearer' to this list? Popular hosts Bitbucket https://bitbucket.org and Gitea/Forgejo (such as https://codeberg.org) support Bearer auth with OAuth tokens.

> ++
> +This value should not be sent unless the appropriate capability (see below) is
> +provided on input.
> +
> +`credential`::
> +	The pre-encoded credential, suitable for the protocol in question (usually
> +	HTTP).  If this key is sent, `authtype` is mandatory, and `username` and
> +	`password` are not used.

A credential protocol attribute named 'credential' is confusing. How about 'authorization' since it determines the HTTP Authorization header? This detail is surely worth mentioning too.

> ++
> +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
> @@ -189,7 +207,21 @@ attribute 'wwwauth[]', where the order of the attributes is the same as
>  they appear in the HTTP response. This attribute is 'one-way' from Git
>  to pass additional information to credential helpers.
>  
> -Unrecognised attributes are silently discarded.
> +`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.
> ++
> +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.
> +> +Unrecognised attributes and capabilities are silently discarded.
brian m. carlson March 25, 2024, 11:37 p.m. UTC | #2
On 2024-03-25 at 23:16:09, M Hickford wrote:
> > +`authtype`::
> > +	This indicates that the authentication scheme in question should be used.
> > +	Common values for HTTP and HTTPS include `basic`, `digest`, and `ntlm`,
> > +	although the latter two are insecure and should not be used.  If `credential`
> > +	is used, this may be set to an arbitrary string suitable for the protocol in
> > +	question (usually HTTP).
> 
> How about adding 'bearer' to this list? Popular hosts Bitbucket
> https://bitbucket.org and Gitea/Forgejo (such as https://codeberg.org)
> support Bearer auth with OAuth tokens.

Sure, I can do that.

> > ++
> > +This value should not be sent unless the appropriate capability (see below) is
> > +provided on input.
> > +
> > +`credential`::
> > +	The pre-encoded credential, suitable for the protocol in question (usually
> > +	HTTP).  If this key is sent, `authtype` is mandatory, and `username` and
> > +	`password` are not used.
> 
> A credential protocol attribute named 'credential' is confusing. How
> about 'authorization' since it determines the HTTP Authorization
> header? This detail is surely worth mentioning too.

I don't want this to be very specific to HTTP, so I don't think that's a
great name.  As I mentioned in the cover letter, I might well extend
this to IMAP and SMTP for our mail handling in the future, and that name
wouldn't work well there.

I named it `credential` because, well, it's the credential that's used
in the protocol.  I feel like saying that the field represents "the
authorization" sounds unnatural.  It's not wrong, per se, but it sounds
confusing.

I'm open to other ideas if you or others have them, but between these
two, I think I'd prefer to stick with `credential`.
M Hickford March 30, 2024, 1 p.m. UTC | #3
On Mon, 25 Mar 2024 at 23:37, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
>
> On 2024-03-25 at 23:16:09, M Hickford wrote:
> > > +`authtype`::
> > > +   This indicates that the authentication scheme in question should be used.
> > > +   Common values for HTTP and HTTPS include `basic`, `digest`, and `ntlm`,
> > > +   although the latter two are insecure and should not be used.  If `credential`
> > > +   is used, this may be set to an arbitrary string suitable for the protocol in
> > > +   question (usually HTTP).
> >
> > How about adding 'bearer' to this list? Popular hosts Bitbucket
> > https://bitbucket.org and Gitea/Forgejo (such as https://codeberg.org)
> > support Bearer auth with OAuth tokens.
>
> Sure, I can do that.
>
> > > ++
> > > +This value should not be sent unless the appropriate capability (see below) is
> > > +provided on input.
> > > +
> > > +`credential`::
> > > +   The pre-encoded credential, suitable for the protocol in question (usually
> > > +   HTTP).  If this key is sent, `authtype` is mandatory, and `username` and
> > > +   `password` are not used.
> >
> > A credential protocol attribute named 'credential' is confusing. How
> > about 'authorization' since it determines the HTTP Authorization
> > header? This detail is surely worth mentioning too.

Would it be accurate to add "For HTTP, Git concatenates the authtype
and credential attributes to determine the Authorization header"?

>
> I don't want this to be very specific to HTTP, so I don't think that's a
> great name.  As I mentioned in the cover letter, I might well extend
> this to IMAP and SMTP for our mail handling in the future, and that name
> wouldn't work well there.

Good point, you've dissuaded me against 'authorization'.

>
> I named it `credential` because, well, it's the credential that's used
> in the protocol.  I feel like saying that the field represents "the
> authorization" sounds unnatural.  It's not wrong, per se, but it sounds
> confusing.

We already use 'credential' to describe the whole collection of
attributes, as in "The credential is split into a set of named
attributes".

>
> I'm open to other ideas if you or others have them, but between these
> two, I think I'd prefer to stick with `credential`.

Ideas anyone?


> --
> brian m. carlson (they/them or he/him)
> Toronto, Ontario, CA
brian m. carlson March 31, 2024, 9:43 p.m. UTC | #4
On 2024-03-30 at 13:00:00, M Hickford wrote:
> Would it be accurate to add "For HTTP, Git concatenates the authtype
> and credential attributes to determine the Authorization header"?

Yes, I think that would be accurate.
diff mbox series

Patch

diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt
index 918a0aa42b..f3ed3a82fa 100644
--- a/Documentation/git-credential.txt
+++ b/Documentation/git-credential.txt
@@ -178,6 +178,24 @@  empty string.
 Components which are missing from the URL (e.g., there is no
 username in the example above) will be left unset.
 
+`authtype`::
+	This indicates that the authentication scheme in question should be used.
+	Common values for HTTP and HTTPS include `basic`, `digest`, and `ntlm`,
+	although the latter two are insecure and should not be used.  If `credential`
+	is used, this may be set to an arbitrary string suitable for the protocol in
+	question (usually HTTP).
++
+This value should not be sent unless the appropriate capability (see below) is
+provided on input.
+
+`credential`::
+	The pre-encoded credential, suitable for the protocol in question (usually
+	HTTP).  If this key is sent, `authtype` is mandatory, and `username` and
+	`password` are not used.
++
+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
@@ -189,7 +207,21 @@  attribute 'wwwauth[]', where the order of the attributes is the same as
 they appear in the HTTP response. This attribute is 'one-way' from Git
 to pass additional information to credential helpers.
 
-Unrecognised attributes are silently discarded.
+`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.
++
+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.
+
+Unrecognised attributes and capabilities are silently discarded.
 
 GIT
 ---