On 2024-06-28 at 18:16:43, Junio C Hamano wrote:
> "brian m. carlson" <sandals@xxxxxxxxxxxxxxxxxxxx> writes:
>
> > diff --git a/Documentation/config/http.txt b/Documentation/config/http.txt
> > index 2d4e0c9b86..2bacb2b862 100644
> > --- a/Documentation/config/http.txt
> > +++ b/Documentation/config/http.txt
> > @@ -56,6 +56,21 @@ http.emptyAuth::
> > a username in the URL, as libcurl normally requires a username for
> > authentication.
> >
> > +http.proactiveAuth::
> > + Attempt authentication without first making an unauthenticated attempt and
> > + receiving a 401 response. This can be used to ensure that all requests are
> > + authenticated. If `http.emptyAuth` is set to true, this value has no effect.
>
> Well written.
>
> > ++
> > +If the credential helper used specifies an authentication scheme (i.e., via the
> > +`authtype` field), that value will be used; if a username and password is
> > +provided without a scheme, then Basic authentication is used. The value of the
> > +option determines the scheme requested from the helper. Possible values are:
> > ++
> > +--
> > +* `basic` - Request Basic authentication from the helper.
> > +* `auto` - Don't request any scheme from the helper.
> > +--
>
> What does "don't request" exactly mean? It is not like we are
> telling the helper "Don't give us anything", right? Are we telling
> the helper "Give us any username/password for the URL in any
> authentication scheme you know about?"
It means we don't send a `wwwauth[]` entry in the request. We are
giving the helper carte blanche to decide what scheme is best (maybe it
knows we want Bearer, for example).
> As we are not getting an initial "401 Unauthorized", which is a good
> channel to convey WWW-Authenticate, Digest is not available to us in
> this context; we may end up using Basic---if the other side then
> says "No, I do not like basic, please use Diest in this realm with
> this nonce" with a "401 Unauthorized" with WWW-Authenticate, then
> all we gained was a chance to expose the username/password in
> plaintext (ok, that's still TLS protected in practice so it may not
> be a huge deal). Hopefully that wouldn't be a problem, but perhaps
> we would want to suggest that this mechanism should primarily be
> used when the user _knows_ that the other side is happy accepting
> you with Basic, or something, in the documentation?
I think it might suffice to say that TLS should always be used with this
configuration. The documentation is very clear that it will use Basic
if no scheme is provided, so presumably the user is either very
confident in their helper configuration (e.g., they know for certain
that the helper will always emit an `authtype` field because they've so
configured it), or they're willing to accept the risk that the password
is sent with basic auth.
> PROACTIVE_AUTH_NONE being at the first position to be assigned 0
> in this enum has significance, because ...
>
> > static struct credential proxy_auth = CREDENTIAL_INIT;
> > static const char *curl_proxyuserpwd;
> > static char *curl_cookie_file;
> > static int curl_save_cookies;
> > struct credential http_auth = CREDENTIAL_INIT;
> > -static int http_proactive_auth;
> > +static enum proactive_auth http_proactive_auth;
>
> ... this implicitly initializes the variable to *_NONE and we do
> rely on that value. It may clarify what we are doing, if we are a
> bit more explicit, i.e.,
>
> enum proactive_auth {
> PROACTIVE_AUTH_NONE = 0,
> ...
>
> It would give a hint to future developers that they shouldn't
> reorder the enum def without thinking.
Sure, that seems like a good idea.
> > +static int always_auth_proactively(void)
> > +{
> > + return http_proactive_auth != PROACTIVE_AUTH_NONE && http_proactive_auth != PROACTIVE_AUTH_IF_CREDENTIALS;
> > +}
>
> An overly long line.
Will fix.
> > size_t fread_buffer(char *ptr, size_t eltsize, size_t nmemb, void *buffer_)
> > {
> > size_t size = eltsize * nmemb;
> > @@ -537,6 +549,18 @@ static int http_options(const char *var, const char *value,
> > return 0;
> > }
> >
> > + if (!strcmp("http.proactiveauth", var)) {
> > + if (value && !strcmp(value, "auto"))
> > + http_proactive_auth = PROACTIVE_AUTH_AUTO;
> > + else if (value && !strcmp(value, "basic"))
> > + http_proactive_auth = PROACTIVE_AUTH_BASIC;
> > + else if (!value)
> > + http_proactive_auth = PROACTIVE_AUTH_NONE;
>
> This is how you "reset" the variable that is set in lower-precedence
> configuration source back to the default, but use of "I exist
> therefore I represent true without giving any value" for that
> purpose is rather unusual. Even if this were about resetting a
> multi-valued configuration variable to empty, don't we usually allow
> an empty string to serve that purpose as well?
>
> And more importantly we apparently do not use this variable as
> multi-valued one---the above callback is a bog-standard "last one
> wins" single value variable.
>
> So instead of using the "I exist" true, I think it is a lot easier
> to see if you used an explicit string "none" for this purpose.
Okay, I think that seems reasonable.
> > credential_fill(&http_auth, 1);
> >
> > if (http_auth.password) {
> > + if (always_auth_proactively()) {
>
> But when http.proactiveauth is set to either "auto" or "basic",
> always_auth_proactively() returns true (because it is not set to
> *_NONE and it is not *_IF_CREDENTIALS) and we come here, to do
> curl_easy_setop() to use basic anyway.
Yes, iff there's a password. The reason is that it isn't sufficient for
libcurl to have a username and password to send authentication. Either
we have to explicitly fill in the Authorization header (which we do with
`authtype` and `credential`) or we have to specify exactly one
authentication scheme; otherwise, libcurl doesn't auth. Note that
entire branch is not taken if we have `authtype` and `credential`; that
is handled elsewhere.
This is subtle, so I'll mention it in the commit message on a reroll.
> > + /*
> > + * We got a credential without an authtype and we don't
> > + * know what's available. Since our only two options at
> > + * the moment are auto (which defaults to basic) and
> > + * basic, use basic for now.
> > + */
> > + curl_easy_setopt(result, CURLOPT_HTTPAUTH, CURLAUTH_BASIC);
>
> So users with "auto" will not see "Basic" in .wwwauth_headers
> strvec, while those with "basic" will. Is this intended? What is
> the expected difference in behaviour coming from this difference?
Yes. The difference is that a credential helper supporting multiple
schemes will (or at least _should_) read the `wwwauth[]` entry and say,
"Oh, the server only supports Basic auth, so I must send Basic auth",
whereas not sending anything tells it that it may choose freely. For
example, a helper might legitimately return Bearer for the case where we
don't send a `wwwauth[]` entry (which would be handled differently),
whereas it probably would not want to do that when the option is set to
`basic`.
> > @@ -1292,7 +1331,8 @@ void http_init(struct remote *remote, const char *url, int proactive_auth)
> > if (curl_global_init(CURL_GLOBAL_ALL) != CURLE_OK)
> > die("curl_global_init failed");
> >
> > - http_proactive_auth = proactive_auth;
> > + if (proactive_auth && http_proactive_auth == PROACTIVE_AUTH_NONE)
> > + http_proactive_auth = PROACTIVE_AUTH_IF_CREDENTIALS;
>
> The webdav http-push is the only caller of http_init() with the
> proactive_auth parameter set to true. In such a case, if we do not
> see the configuration variable left/set to the default, we force the
> "if-credentials" behaviour. IOW, if http.proactive_auth is set to
> some value, we leave it as-is even for http-push (which wants to use
> the "if-credentials" behaviour).
Correct. When this option was a boolean, the if-credentials behaviour
was what happened when the option was true.
> I wonder if there are cases where you would want to do the
> proactive-auth for fetches (which you couldn't do before), but you
> want to drive http-push with if-credentials, and if so, if this
> changes the behaviour in a way that is visible to the end-user.
I think that's remarkably unlikely. Pushing effectively always requires
some sort of authentication because you don't want random people
modifying your codebase/writing project/other collection of bits.
That's like security 101.
Therefore, you're _going_ to authenticate sooner or later on push. The
only case where if-credentials is helpful is what I mentioned about
t5540, where we actually have an open push. That isn't a real world use
case for security reasons, but using TLS certificates to authenticate
and not otherwise sending credentials, which _looks_ the same to this
code, is legitimate.
However, you wouldn't configure TLS certificates for push only and use
regular auth for fetch, since you'd already have the TLS certificate on
the client and you'd have already requested the certificate when
establishing the TLS connection. Moreover, there's no reason that you
_would_ want to auth for fetches or clones and not for pushes; if your
goal is to make it easier to throttle heavy users, say, pushes are more
expensive because you have to run pre-receive hooks and fsck the data.
Perhaps someone has come up with such an unusual use case and is still
using the WebDAV approach, and in such a case, we can simply let them
send a nice patch to implement http.pushProactiveAuth for that case. In
the mean time, I'm happy to let one option control both.
> Or perhaps http-push over webdav outlived its usefulness and we
> should send a patch to Documentation/BreakingChanges.txt to declare
> its deprecation and removal, in which case there is nothing to
> wonder or be worried here ;-)
I think cgit still supports it because it's easier to implement in a
CGI-style piece of code, and maybe that's useful. While I don't use it,
it can be easier to set up in certain environments, and I don't really
want to be the one to announce its demise.
Anyway, thanks for the review, and I'll try to get a reroll out
relatively soon. I have a couple days of vacation with the Canada Day
holiday, so hopefully I can find some time to get one out then.
--
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA
Attachment:
signature.asc
Description: PGP signature
