Re: [PATCH 06/14] introduce credentials API

From: Jeff King <peff@peff.net>
Date: Jul 22, 2011, 20:39 UTC
Message-ID: <20110722203901.GA11922@sigill.intra.peff.net>
In-Reply-To: <7vy5zs4mxt.fsf@alter.siamese.dyndns.org>

On Wed, Jul 20, 2011 at 03:17:02PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > +`struct credential`::
> > +
> > +	This struct represents a single username/password combination.
> > +	The `username` and `password` fields should be heap-allocated
> > +	strings (or NULL if they are not yet known). The `unique` field,
> > +	if non-NULL, should be a heap-allocated string indicating a
> > +	unique context for this credential (e.g., a protocol and server
> > +	name for a remote credential). The `description` field, if
> > +	non-NULL, should point to a string containing a human-readable
> > +	description of this credential.
> 
> Am I confused to say "unique" is for code/machine and "description" is for
> human to identify what remote resource is being accessed using the
> credential?

No, you're not confused. That is the intent.

> Can two credentials have the same description but different unique,
> and if so what happens?

Yes, they could. In fact, you can do it with the code in my series. The URL "http://example.com" will have unique context "http:example.com", and a description of "example.com". The ssl-enabled version, "https://example.com", will have the same description, but a different unique context ("https:example.com").

I wanted the machine-readable token to be complete and unambiguous, since it may be sent automatically. Deciding to send a password over unencrypted http versus https is a policy decision that only the user can make, and we should err on the side of caution.

The human-readable portion, on the other hand, can assume that the user has some context for the request, and can make the right decision. So we can afford to put less information it, if it makes the output prettier to human eyes.

One could argue that we still need to provide as much information as possible to a human user. For example, if I cut-and-paste a URL with "http" instead of "https", I might fail to notice and provide my password accidentally over an unencrypted link. Including the missing information in the human-readable part could help with that case, at the cost of making the prompt a little longer than necessary for other cases.

Another example is that we say only "Password for 'certificate':" when unlocking a certificate, even though the certificate's unique token mentions the filename. This is the same case: the user has the extra context to know which certificate we mean, so we can make the message prettier. Again, I think one could argue that it's better to provide the context to the user, in case they don't have it.

In both of the above paragraphs, my "one could argue..." statements can be translated as "I argued with myself about this, and am on the fence. If you feel strongly, it's easy to make it so."

> A plausible answer may be "the user cannot tell what username/password
> pair to give, but it is expected that such a nondescriptive context is
> only for certificate and it also is expected that only one certificate
> is used at a time", perhaps?

Right.

> I think "unique" vs "description" are secondary aspects of these things,
> and the primary aspect that they share is that they are about "context",
> as [11/14] describes.  Perhaps "context-id" vs "context-description" may
> be better names to reduce confusion?  I dunno.

Yeah, it might be better to unify the concepts by giving them similar names. It might even make sense to call them "context_id" and "context_human" to make the difference more clear.

Another option would be to drop the concept entirely, and simply give the helpers as many descriptive tokens as we can. So instead of:

  unique=https:example.com
  description=example.com

and

  unique=cert:/path/to/cert.pem
  description=certificate

we would give:

  type=network
  proto=https
  host=example.com
  path=repo.git

and

  type=certificate
  file=/path/to/cert.pem

and then let the helper generate an appropriate token or description from those fields. That provides more flexibility for a helper to make a decision. I avoided doing that in the first place for two reasons:

  1. It pushes more work onto the helpers to generate a token, or
     to generate a nice human-readable prompt. So we may end up with
     inconsistency between helpers, both in terms of messages shown to
     the user and in security properties (e.g., helpers may decide
     differently on policy issues like "are http and https similar
     enough to share passwords? What about imaps and https?"). Not to
     mention the duplicated effort among helper writers.

  2. I didn't know where it would end. Right now, those examples above
     are enough for what git uses. But when we add new credential
     locations (certainly imap-send is a place we could use it; probably
     send-email could also use it for smtp auth), will it be enough? How
     will already-written credential helpers cope with the new fields?

The Gnome Keyring API seems to just provide space for a free-form set of key/value pairs. But I haven't found a micro-format which says which fields should be used, so I guess it's up to the application to use them consistently. The OS X keychain API has very specific fields for protocol, hostname, etc (and even has different classes of "this is an internet password" versus "this is a generic password").

So maybe it is simply better to break the information down as much as possible and let the password wallet helpers do what they will with it.

Show 12 quoted lines

> > +`credential_reject`::
> > +
> > +	Inform the credential subsystem that the provided credentials
> > +	have been rejected. This will clear the username and password
> > +	fields in `struct credential`, as well as notify any helpers of
> > +	the rejection (which may, for example, purge the invalid
> > +	credentials from storage).
> 
> What hints do helpers get when this is called? Do they get username,
> unique and description to allow them to selectively purge the bad ones
> while keeping good ones, or the username is already cleared by the time
> the helpers are notified and they have no clue?

They get username, unique, and description, to let them purge the minimal amount (they are always free to ignore the username and purge more, of course, if they are backed by less-capable storage).

We could also hold open a connection to the helper that gave us information, try the credential, and then say either "OK" or "FAIL". I specifically tried to avoid bi-directional interactive communication in this protocol, though, in the name of simplicity. The helper gets everything from git in one shot via the command line, and then dumps everything back in one shot via stdout.

>From looking at a few APIs of system password wallets, it seems that

many expose functionality that is just "get" and "put". So normal use would be something like:

  while (1) {
    if (!wallet_get(&cred))
      ask_user(&cred);
    if (request_with_cred(&cred))
      break;
  }
  wallet_put(&cred);

and it is up to the program to implement "ask_user". So another option would be to just expose "get" and "put" via credential helpers, which keeps things pretty simple. In my proposal, helpers are actually responsible for asking the user.

I did that intentionally to provide a mechanism for system-specific prompts. For example, you could pop up a single dialog with both username and password fields. The username field might be filled in already, but the user could override it. You can't do anything quite as nice with the askpass interface, which receives the questions one at a time.

I also wanted to make sure we were flexible to systems which do incorporate the "ask the user" step automatically. From my brief reading of the freedesktop Secrets API, it does so.

Show 10 quoted lines

> > +`credential_getpass`::
> > +
> > +	Fetch credentials from the user either using an "askpass" helper
> > +	(see the discussion of core.askpass and GIT_ASKPASS in
> > +	linkgit:git-config[1] and linkgit:git[1], respectively) or by
> > +	prompting the user via the terminal.
> 
> It sounds like that users of the API should call fill_credential() and let
> the machinery fall back to this function as needed. Does this need to be
> part of the public API functions?

The "getpass" helper calls it, though I suppose it could also just use credential_fill with an empty list. I mainly left it as a convenience function if there is something that doesn't want to use the regular "fill" process, but I don't think it's critical.

Show 21 quoted lines

> > +static int read_credential_response(struct credential *c, FILE *fp)
> > +{
> > +	struct strbuf response = STRBUF_INIT;
> > +
> > +	while (strbuf_getline(&response, fp, '\n') != EOF) {
> > +		char *key = response.buf;
> > +		char *value = strchr(key, '=');
> > +
> > +		if (!value) {
> > +			warning("bad output from credential helper: %s", key);
> > +			strbuf_release(&response);
> > +			return -1;
> > +		}
> > +		*value++ = '\0';
> > +
> > +		if (!strcmp(key, "username")) {
> > +			free(c->username);
> > +			c->username = xstrdup(value);
> 
> The document did not say anything about escaping/quoting of values, but it
> may not be a bad idea to make it more explicit over there.

There is no quoting or escaping. As the document says, "the value may contain any bytes except a newline". It doesn't mention that keys cannot contain an "=" or a newline, though that is also true.

Again, I went for simplicity of helper implementation, in the hope that usernames and passwords could get by without newline characters. We could switch it to "\0" if it matters. That makes shell implementations of helpers a little tougher. But it's much easier than quoting.

-Peff