From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-ASN: AS3215 2.6.0.0/16 X-Spam-Status: No, score=-10.4 required=3.0 tests=AWL,BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from out1.vger.email (out1.vger.email [IPv6:2620:137:e000::1:20]) by dcvr.yhbt.net (Postfix) with ESMTP id 670881F910 for ; Wed, 9 Nov 2022 23:41:22 +0000 (UTC) Authentication-Results: dcvr.yhbt.net; dkim=pass (2048-bit key; unprotected) header.d=google.com header.i=@google.com header.b="iSXFog81"; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231980AbiKIXlN (ORCPT ); Wed, 9 Nov 2022 18:41:13 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:43904 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232003AbiKIXlF (ORCPT ); Wed, 9 Nov 2022 18:41:05 -0500 Received: from mail-yw1-x114a.google.com (mail-yw1-x114a.google.com [IPv6:2607:f8b0:4864:20::114a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id A2C2C19016 for ; Wed, 9 Nov 2022 15:40:51 -0800 (PST) Received: by mail-yw1-x114a.google.com with SMTP id 00721157ae682-36fde8f2cdcso494917b3.23 for ; Wed, 09 Nov 2022 15:40:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:from:to:cc:subject:date:message-id:reply-to; bh=rmlz5tg0zy2sATbFFLlqyGcE0uV2JD/jygCl03wkGXE=; b=iSXFog81glmH8n6hp3oOiLWK4KtS4utBYb7v53mpnxGaLxFWzI++Jm3dtzGTJUwQ9H La1KojZkUr/J0sV4rRgXPNC4LLY6vFjNR0rF411ZBAAzK/YVthzZZgwwh1AZGTDpjFoX wjZwmS+CvNbnM8SEF/uKMRnCk2So4sskLszdvCQqBwBb5d24/wUxXkPJguDgasFKvVz8 JVRZVb6orJESHvijPLFcMd+TZBCTQ8V+H1Ev7oil5EOnvu7dIY7CzwKmOhzbTC6XQyaq L8/jvMX7D0Tlzj5TAkSnQKsN/FuHaPMHR0Y2IX06gxgUKzQirX2UZfhmeptPsnExqNEJ Sh7A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:from:subject:message-id:references:mime-version:in-reply-to :date:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=rmlz5tg0zy2sATbFFLlqyGcE0uV2JD/jygCl03wkGXE=; b=wChqszRjg2CyIH3GwL/A+HM5dhsplhcbsfRt44+7SbDtAwyseHyqX+WP28Ks5IFhnj I8oa7cPTg8ma9AVWPm0BtWGG7L0nx7V6KAYwz1l6hgrsJit21dr6UdL0p6zIIWIZV/Bb ioU41S7h3HDQiU7Te2UMB0W+SLVRVj4ae2ve3O/bnXC/7loHFEh1rrZZs8+AcLTYXEgW mfnPSTZuZpp9V63Y6ELkYHiXi3DsaI5V9z/Xk0P80hkKzQ7+2G1mG7DrcVbP9IkEetSW TxPivZbgLd9PEiQaONiyMZBgKPv972+52Z10ToapxFC3XBhVmtvVvfm025znp0RkXs7C l6YQ== X-Gm-Message-State: ACrzQf3oyzi4LpSlu+Lq/3XKbiB1oTd5Cpr13CLC1QGwSfsYbFMxoypi cauYjRELrbECjn0wRGrcDatIs4gHM1FlNA== X-Google-Smtp-Source: AMsMyM4rtx+aXVC14TUeAYCcZECNcGgkBgCOMIBvewXdf9IXnMUzB3HQJIAMoUsD1zLT8JAV1b6D4zGMvzDiNQ== X-Received: from chooglen.c.googlers.com ([fda3:e722:ac3:cc00:24:72f4:c0a8:26d9]) (user=chooglen job=sendgmr) by 2002:a05:6902:4ec:b0:6cb:86b1:a5cc with SMTP id w12-20020a05690204ec00b006cb86b1a5ccmr61559409ybs.551.1668037250709; Wed, 09 Nov 2022 15:40:50 -0800 (PST) Date: Wed, 09 Nov 2022 15:40:49 -0800 In-Reply-To: <2f38427aa8db188060d153d8ece9503e1b604e91.1667426970.git.gitgitgadget@gmail.com> Mime-Version: 1.0 References: <2f38427aa8db188060d153d8ece9503e1b604e91.1667426970.git.gitgitgadget@gmail.com> Message-ID: Subject: Re: [PATCH v3 05/11] http: set specific auth scheme depending on credential From: Glen Choo To: Matthew John Cheetham via GitGitGadget , git@vger.kernel.org Cc: Derrick Stolee , Lessley Dennington , Matthew John Cheetham , M Hickford , Jeff Hostetler , Matthew John Cheetham Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org "Matthew John Cheetham via GitGitGadget" writes: > From: Matthew John Cheetham > > Introduce a new credential field `authtype` that can be used by > credential helpers to indicate the type of the credential or > authentication mechanism to use for a request. > > Modify http.c to now specify the correct authentication scheme or > credential type when authenticating the curl handle. If the new > `authtype` field in the credential structure is `NULL` or "Basic" then > use the existing username/password options. If the field is "Bearer" > then use the OAuth bearer token curl option. Otherwise, the `authtype` > field is the authentication scheme and the `password` field is the > raw, unencoded value. > > Signed-off-by: Matthew John Cheetham > --- > Documentation/git-credential.txt | 12 ++++++++++++ > credential.c | 5 +++++ > credential.h | 1 + > git-curl-compat.h | 10 ++++++++++ > http.c | 24 +++++++++++++++++++++--- > 5 files changed, 49 insertions(+), 3 deletions(-) > > diff --git a/Documentation/git-credential.txt b/Documentation/git-credential.txt > index 791a57dddfb..9069bfb2d50 100644 > --- a/Documentation/git-credential.txt > +++ b/Documentation/git-credential.txt > @@ -175,6 +175,18 @@ username in the example above) will be left unset. > attribute 'wwwauth[]', where the order of the attributes is the same as > they appear in the HTTP response. > > +`authtype`:: > + > + Indicates the type of authentication scheme that should be used by Git. > + Credential helpers may reply to a request from Git with this attribute, > + such that subsequent authenticated requests include the correct > + `Authorization` header. > + If this attribute is not present, the default value is "Basic". > + Known values include "Basic", "Digest", and "Bearer". > + If an unknown value is provided, this is taken as the authentication > + scheme for the `Authorization` header, and the `password` field is > + used as the raw unencoded authorization parameters of the same header. > + [...] > @@ -525,8 +526,25 @@ static void init_curl_http_auth(struct active_request_slot *slot) > > credential_fill(&http_auth); > > - curl_easy_setopt(slot->curl, CURLOPT_USERNAME, http_auth.username); > - curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, http_auth.password); > + if (!http_auth.authtype || !strcasecmp(http_auth.authtype, "basic") > + || !strcasecmp(http_auth.authtype, "digest")) { > + curl_easy_setopt(slot->curl, CURLOPT_USERNAME, > + http_auth.username); > + curl_easy_setopt(slot->curl, CURLOPT_PASSWORD, > + http_auth.password); > +#ifdef GIT_CURL_HAVE_CURLAUTH_BEARER > + } else if (!strcasecmp(http_auth.authtype, "bearer")) { > + curl_easy_setopt(slot->curl, CURLOPT_HTTPAUTH, CURLAUTH_BEARER); > + curl_easy_setopt(slot->curl, CURLOPT_XOAUTH2_BEARER, > + http_auth.password); > +#endif > + } else { > + struct strbuf auth = STRBUF_INIT; > + strbuf_addf(&auth, "Authorization: %s %s", > + http_auth.authtype, http_auth.password); > + slot->headers = curl_slist_append(slot->headers, auth.buf); > + strbuf_release(&auth); > + } As expected, a "Bearer" authtype doesn't require passing a username to curl, but as you noted in the cover letter, credential helpers were designed with username-password authentication in mind, which raises the question of what a credential helper should do with "Bearer" credentials. e.g. it is not clear to me where the "username" comes from in the tests, e.g. +test_expect_success 'http auth www-auth headers to credential helper basic valid' ' + test_when_finished "per_test_cleanup" && + # base64("alice:secret-passwd") + USERPASS64=YWxpY2U6c2VjcmV0LXBhc3N3ZA== && + export USERPASS64 && + + start_http_server \ + --auth=bearer:authority=\"id.example.com\"\ q=1\ p=0 \ + --auth=basic:realm=\"example.com\" \ + --auth-token=basic:$USERPASS64 && + + cat >get-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + wwwauth[]=bearer authority="id.example.com" q=1 p=0 + wwwauth[]=basic realm="example.com" + EOF + + cat >store-expected.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + authtype=basic + EOF + + cat >get-response.cred <<-EOF && + protocol=http + host=$HOST_PORT + username=alice + password=secret-passwd + authtype=basic + EOF + + git -c credential.helper="$CREDENTIAL_HELPER" ls-remote $ORIGIN_URL && + + test_cmp get-expected.cred get-actual.cred && + test_cmp store-expected.cred store-actual.cred +' I'm not sure how we plan to handle this. Some approaches I can see are: - We require that credential helpers set a reasonable value for "username". Presumably most credential helpers generating bearer tokens have some idea of user identity, so this might be reasonable, though it is wasteful, since we never use it in a meaningul way, e.g. I don't think Git asks the credential helper for "username=alice" and the credential helper decides to return the 'alice' credential instead of the 'bob' credential (but I could be mistaken). - We require that credential helpers set _some_ value for "username", even if it is bogus. If so, we should communicate this explicitly. - It is okay for "username" to be missing. This seems like the most elegant approach for credential helpers. I'm not sure if we're there yet with this series, e.g. http.c::handle_curl_result() reads: else if (results->http_code == 401) { if (http_auth.username && http_auth.password) { credential_reject(&http_auth); return HTTP_NOAUTH; which seems to assume both a username _and_ password. If the username is missing, we presumably don't send "erase", which might be a problem for revoked access tokens (though presumably not an issue for OIDC id tokens).