[PATCH_v1] add 'git credential' plumbing command

From: Javier Roucher <jroucher@gmail.com>

The credential API is in C, and not available to scripting languages. Expose the functionalities of the API by wrapping them into a new plumbing command "git credentials".

Signed-off-by: Pavel Volek <Pavel.Volek@ensimag.imag.fr>
Signed-off-by: NGUYEN Kim Thuat <Kim-Thuat.Nguyen@ensimag.imag.fr>
Signed-off-by: ROUCHER IGLESIAS Javier <roucherj@ensimag.imag.fr>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>

---
 .gitignore                       |  1 +
 Documentation/git-credential.txt | 74 ++++++++++++++++++++++++++++++++++++++++
 Makefile                         |  1 +
 builtin.h                        |  1 +
 builtin/credential.c             | 40 ++++++++++++++++++++++
 git.c                            |  1 +
 6 files changed, 118 insertions(+)
 create mode 100644 Documentation/git-credential.txt
 create mode 100644 builtin/credential.c

Javier.Roucher-Iglesias@ensimag.imag.fr a écrit :

s/Providing/Provides/ & s/strore/store

s/have/has/

Wouldn't the note be "it's not git credential that decides if the password is correct or not. That part is done by the external system" ?

s/allready/already/

Braces for the last "else" part. In general, the structure should be

       if (...) {
                /*code*/
       } else if (...) {
                /*code*/
       } else {
                /*code*/
       }

If juste one block needs brances, all the other "else if"/"else" part need it too.

BTW, please be aware of the white spaces (here mostly in the doc) :).

Lucien Kong.

On Sat, 09 Jun 2012 21:52:36 +0200, konglu@minatec.inpg.fr wrote:

Thank's for the english writting corrections it will be change it for the next patch

I will remove brances.

roucherj <roucherj@telesun.imag.fr> writes:

The remark was about adding them, not removing them. There's one branch of the if/else if/ with several instructions, so we usually put braces everywhere.

Javier.Roucher-Iglesias@ensimag.imag.fr writes:

The above sounds like saying "A filesystem allows you to save pathname and contents". While it may not be _wrong_ per-se, usually you would think of a filesystem as something you store contents in; pathname is primarily used as a key to find the contents (i.e. you do not store it in the filesystem).

Isn't the credential mechanism for storing password for <user, protocol, host, path> tuple (i.e. the four-tuple is used as a look-up key)?

Style:

        void cmd_credential(int argc, char **argv, const char *prefix)
        {

Does "git credential" need to have a git repository (i.e. run in a git repository or in a working tree that is controlled by one)? A scripted Porcelain you would write using "git credential" may want to implement something like "git clone" or "git ls-remote" where you do not have to be in an existing repository.

Junio C Hamano <gitster@pobox.com> writes:

It shouldn't (hence, should use RUN_SETUP_GENTLY).

("git clone" might not be the best example, as the authentication is usually done in the "git fetch" part of "git clone", but never mind)

Actually, "git credential" has very little to do with Git, and could even be used in a git-unrelated script.

Matthieu Moy wrote:

Rather, that means it should use 0:

			{ "credential", cmd_credential },

[...]

I suspect it would be simplest to make it a non-builtin. There are some examples to take inspiration from in the PROGRAM_OBJS variable of the Makefile.

Hope that helps, Jonathan

Jonathan Nieder wrote:

... and it turns out I'm talking nonsense. RUN_SETUP_GENTLY would be a sensible choice indeed, to allow the command to discover the current repository and read .git/config from there indeed.

Sorry for the confusion, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

Yeah, I think "it is OK to be called outside a repository, but if inside we read and honor the configuration" is a sensible behaviour for this command.

On Sat, Jun 09, 2012 at 08:45:02PM +0200, Javier.Roucher-Iglesias@ensimag.imag.fr wrote:

I wonder if this description starts off a bit too literal, and should start with the big picture. Tell people what it is generally, how it fits into git, and in what circumstances they would want to run it.

Like:

  Git has an internal interface for storing and retrieving credentials
  from system-specific helpers, as well as prompting the user for
  usernames and passwords. The git-credential command exposes this
  interface to scripts which may want to retrieve, store, or prompt for
  credentials in the same manner as git. The design of this scriptable
  interface models the internal C API; see
  link:technical/api-credentials.txt[the git credential API] for more
  background on the concepts.

It's nice to have an example like this, but there's much detail missing in how the format is specified. However, this format is already documented in the "helpers" section of api-credentials.txt, so it probably makes sense to refer to that document.

I'd rather see this broken up into a reference specification and separate examples. Then the reference part can be very specific about what happens and the behavior in each case. For example:

  git-credential takes an "action" option on the command-line (one of
  "fill", "approve", or "reject") and reads a credential description
  on stdin. The format of the description is the same as that given to
  credential helpers; see link:technical/api-credential.txt[the git
  credential API] for a specification.

  If the action is "fill", git-credential will attempt to add "username"
  and "password" fields to the description by reading config files, by
  contacting any configured credential helpers, or by prompting the
  user. The username and password fields of the credential description
  are then printed to stdout.

  If the action is "approve", git-credential will send the description
  to any configured credential helpers, which may store the credential
  for later use. 

  If the action is "reject", git-credential will send the description to
  any configured credential helpers, which may erase any stored
  credential matching the description.

And now that we've introduced the actions and said what they've done, we can go on to describe the expected workflow, which is a good place to put examples:

  An application using git-credential will typically follow this
  workflow:

    1. Generate a credential description based on the context.

       For example, if we want a password for
       `https://example.com/foo.git`, we might generate the following
       credential description:

           protocol=https
           host=example.com
           path=foo.git

    2. Ask git-credential to give us a username and password for this
       description. This is done by running `git credential fill`,
       feeding the description from step (1) to its stdin. The username
       and password will be produced on stdout, like:

           username=bob
           password=secr3t

    3. Try to use the credential (e.g., by accessing the URL with the
       username and password from step (2)).

    4. Report on the success or failure of the password. If the
       credential allowed the operation to complete successfully, then
       it can be marked with an "approve" action. If the credential was
       rejected during the operation, use the "reject" action. In either
       case, `git credential` should be fed with the credential
       description from step (1) concatenated with the attempted
       credential (i.e., the output you got in step (2)).

Note in the above proposed documentation I was trying to describe the behavior of the command in your patch. It does not reflect changes which I will propose to the command's behavior below. :)

So arguments past the first one become helpers that we try, regardless of the credential.helper config settings. Is there a reason for this to be in the user-facing command?

I assume this got copied by looking at test-credential. I'd be OK with including this feature in git-credential (and converting our test scripts to use it, so we can drop test-credential entirely). But probably it should not soak up all of the command-line arguments, and instead should be a hidden option like:

  git credential --helper=cache fill

That will give us more flexibility later down the road.

I think we want to provide a straight credential-reader like this, because it is the most flexible form of describing a credential. But I suspect many callers will have a URL, and we are creating extra work for them to break it down into components themselves.

Perhaps it would be simpler to accept a URL on the command line, and also provide a --stdin option for callers that want to feed it directly. So:

  git credential fill https://example.com/foo.git

would be identical to:

  git credential --stdin fill <<\EOF
  protocol=https
  host=example.com
  path=foo.git
  EOF

I am tempted to suggest that this actually output the _whole_ credential, not just the username and password. Coupled with the above behavior, you would get:

  $ git credential fill https://example.com/foo.git
  protocol=https
  host=example.com
  path=foo.git
  username=bob
  password=secr3t

which happens to be exactly what you want to feed back to the "approve" and "reject" actions (and it is not really any harder to parse).

We _could_ get by with allowing:

  git credential --stdin approve https://example.com/foo.git <<\EOF
  username=bob
  password=secr3t
  EOF

and having it combine the URL on the command-line with the entries on stdin (and indeed, I think that is the only sane thing to do when --stdin and a URL are both given).

But that implies that there will never be any extra attributes that are relevant to the approve/reject that are not in the URL (e.g., elsewhere there has been talk about helpers being able to add arbitrary fields to the descriptions in order to communicate with each other). So it may be that we want to encourage callers to save the result of "fill" and feed it back to "approve" or "reject" verbatim.

-Peff

Jeff King <peff@peff.net> writes:

and "git credential fill https://peff@example.com/foo.git" would be identical to the latter one with user=peff already filled in?

All good suggestions ;-).

On Mon, Jun 11, 2012 at 08:34:55AM -0700, Junio C Hamano wrote:

Exactly. Though see my other response to Matthieu, which notes that:

  git credential fill https://peff:supersecret@example.com/foo.git

is problematic. :(

Probably it should be spelled:

  git credential fill <<\EOF
  url=https://peff:supersecret@example.com/foo.git
  EOF

and then:

  git credential fill <<\EOF
  url=https://peff:supersecret@example.com/foo.git
  username=junio
  password=othersecret

would do what you expect (break the URL out into its components, and then override particular fields).

-Peff

Jeff King <peff@peff.net> writes:

I'd do it the other way around. api-credentials.txt is in technical/, while the document we're writing will end-up in a man page, which cannot link to technical/.

So, it makes more sense to move the format specification to git-credential.txt, and link to it from api-credentials.txt (now that we have a nice way to link to manpages from technical/ ;-) ).

Actually, this should already be possible with

  git -c credential.helper=cache credential fill

I suspect that this feature will never be used outside tests, and if so, I don't think it deserves a command-line option.

I like that, yes.

On Mon, Jun 11, 2012 at 08:02:06PM +0200, Matthieu Moy wrote:

We do so already in a few places:

  $ cd Documentation && git grep 'link:technical/'
  git.txt:link:technical/api-index.html[GIT API documentation].
  gitcredentials.txt:link:technical/api-credentials.html[credentials API] for details.
  user-manual.txt:found in link:technical/pack-format.txt[technical/pack-format.txt].

I think the rationale is that you would have the HTML documentation installed into /usr/share/doc/git-doc or similar, and asciidoc does correctly generate footnote references from those links. However, I would be fine with putting the meat of it into git-credential, and having api-credentials refer back to it. It's easier on the user that way.

Yeah, that is even better. The tests could also use test_config. I would pick whichever of the two is more convenient for a particular test.

-Peff

Javier.Roucher-Iglesias@ensimag.imag.fr writes:

[...]

This helpers argument is still there, and is now totally undocumented. I shouldn't have to repeat that so many times.

Your "git credential" runs cmd_count_objects. A little bit of testing should have found that (my remarks about lack of testing still apply).