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

["brian m. carlson" <sandals@crustytoothpaste.net>]

[-- Attachment #1: Type: text/plain, Size: 5213 bytes --]

On 2024-03-27 at 08:02:39, Patrick Steinhardt wrote:
> 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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]