Re: [PATCH 05/13] credential: gate new fields on capability

From: brian m. carlson <sandals@crustytoothpaste.net>
Date: Mar 27, 2024, 21:33 UTC
Message-ID: <ZgSQr7uQdIA8oVNn@tapette.crustytoothpaste.net>
In-Reply-To: <ZgPSn_D8luXQyZGf@tanuki>

On 2024-03-27 at 08:02:39, Patrick Steinhardt wrote:

Show 40 quoted lines

> On Sun, Mar 24, 2024 at 01:12:53AM +0000, brian m. carlson wrote:
> > +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.

Let me explain a little bit. There are two possible flows that we can have for a credential request:

  git-credential input -> credential.c -> helper -> credential.c -> git-credential output

  git-http-backend -> credential.c -> helper -> credential.c -> git-http-backend

Let's look at the first one (which might, say, come from Git LFS or another external tool), but the second one works similarly. When we get a request from `git credential fill`, we need to know first whether the requester supports the capability. If we're using an external tool from last decade, it's not going to do so.

If it _does_ support that, then we want to pass that along to the helper, but if it doesn't, we don't. That's because if the caller doesn't support `credential` and `authtype`, the helper might legitimately want to provide a username and password (or token) instead, knowing that that's more likely to work.

Similarly, in the final response, we want to indicate to the external caller whether the capability was in fact supported. That's useful to know in case we want to pass the response back to `git credential store`, and it also discloses functionality about what the credential helper in use supports.

We can't assume that the helper does or doesn't support the same capabilities as Git because it might come from outside Git (e.g., Git Credential Manager Core, or a site-specific credential helper) or it just might not be capable of storing or handling that kind of credential. By not making the assumption that the helper is implicitly capable, we allow users to continue to use very simple shell scripts as credential helpers.

As to why this functionality exists, it exists to support the two new capabilities in this series, `credential` and `state`. A pie in the sky goal for the future is to support additional request signing functionality, so it might learn things like method, URI, and TLS channel binding info, which would be an additional capability. (I might implement that, or I might not.) All of those are boolean: they either are supported, or not. If folks in the future want to expose non-boolean capabilities, I don't think that should be a problem.

Show 17 quoted lines

> > +/*
> > + * 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`.

I think an enum would be a nice improvement. I'll include that in a reroll.

-- 
brian m. carlson (they/them or he/him)
Toronto, Ontario, CA