{"thread":{"id":"55656","subject":"git-credential-cache inner workings","startedAt":"2021-05-10T06:33:51Z","lastAt":"2021-05-10T21:25:30Z","messageCount":2,"participants":["Thijs Cramer","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"424003","messageId":"CAKWuzOrga4eqXhrw11xMsxJCVAPJRTTa7FncNYW5PE5QXCwAJw@mail.gmail.com","threadId":"55656","inReplyTo":null,"subject":"git-credential-cache inner workings","fromName":"Thijs Cramer","fromEmail":"thijs.cramer@gmail.com","sentAt":"2021-05-10T06:33:38Z","receivedAt":"2021-05-10T06:33:51Z","isPatch":false,"sender":{"key":"thijs.cramer@gmail.com","avatar":null},"body":"During debugging of a nasty recurring GIT code 128 error in our CI/CD\nenvironment, we've found some design choices that have raised some\nquestions about the inner workings of the credential cache in GIT.\nFirst, a little sketch of the situation.\n\nWe are using GitHub Enterprise Cloud on Jenkins using the Github\nBranch Source Plugin.\nThis plugin uses \"GitHub Apps\" (Service Accounts if you will), which\nuse Application (Installation) Tokens to authenticate with GitHub. The\ntokens are used as regular passwords in this case.\n\nWhen a Jenkins job starts, it checks out the code from GitHub using\nthe GIT command line, passing it a token with a lifetime between 45\nand 60 minutes remaining (tokens have a max lifetime of 60m). This\ntoken is passed via the GIT_ASKPASS variable to give the commandline\nCI/CD job the ability to use the token for subsequent GIT actions\nlocally during the job.\n\nWe are seeing GIT code 128's in rare cases when jobs follow each other\nup at a fairly high tempo. The reason is:\nhttps://github.com/git/git/blob/master/http.c#L1637 . This line\n'approves' a credential that gives a successful result. This means\nthat Tokens that have been cached in the git-credential-cache-daemon\nwill have it's timeout refreshed even though they might expire\nshortly.\nThe credential cache timeout is a user-input timeout, and not a token\ntimeout, and this structure collides with the functional lifetime of a\ntoken.\n\nThere are several solutions to circumvent this issue:\n1) We could ask the Plugin that manages the tokens to support managing\nthe Credential Cache as well. This makes it possible for the plugin to\nactively manage tokens and revoke tokens from the cache that have\nexpired. Downside is that making plugins aware of the cache in git,\nmight not be a good idea, and there might even be cases where caching\nis disabled completely, which would still require the ASKPASS method.\n2) We could add an option to the GIT client that has the same\nfunctionality as the ASKPASS variable, but one that takes precedence\nbefore the cache. This way, one can preseed the GIT client with\ncredentials that will *always* be used, even if there is a cache that\nhas a credential for the given URL.\n3) We could alter the cache daemon (or build a new one) that has more\nelaborate options regarding it's timeout. Perhaps a daemon option like\n\"--keep-timeout-on-approve\".\nThis would mean that if a credential is approved for a second time the\ntimeout is not refreshed.\nCurrently the daemon just removes any old credentials from it's cache\nand adds the new one without questioning the intention of the user. We\ncould add a check that tests if the incoming credential is equal to\nthe currently cached credential, and skip updating the expiration date\nif they are the same. (With an exception when receiving a new\npassword, then we should forcefully update). This means that the\n--timeout option from the daemon will become a 'hard' timeout, instead\nof a user-action timeout. Of course this should be done with a command\nline option on the daemon to not change the default behaviour.\n\nMy C is a bit rusty, but I can help with adding one of the features, I\nwould just like to know some of your thoughts on this issue.\n\nThanks a lot,\n\n- Thijs\n"},{"id":"424078","messageId":"YJmkxkqz2k+K3Dso@coredump.intra.peff.net","threadId":"55656","inReplyTo":"CAKWuzOrga4eqXhrw11xMsxJCVAPJRTTa7FncNYW5PE5QXCwAJw@mail.gmail.com","subject":"Re: git-credential-cache inner workings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-05-10T21:25:26Z","receivedAt":"2021-05-10T21:25:30Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, May 10, 2021 at 08:33:38AM +0200, Thijs Cramer wrote:\n\n> We are seeing GIT code 128's in rare cases when jobs follow each other\n> up at a fairly high tempo. The reason is:\n> https://github.com/git/git/blob/master/http.c#L1637 . This line\n> 'approves' a credential that gives a successful result. This means\n> that Tokens that have been cached in the git-credential-cache-daemon\n> will have it's timeout refreshed even though they might expire\n> shortly.\n> The credential cache timeout is a user-input timeout, and not a token\n> timeout, and this structure collides with the functional lifetime of a\n> token.\n\nRight. This was discussed a few months ago in this thread:\n\n  https://lore.kernel.org/git/CAP8UukiL0niGSm3o7uYNBzL3FP-UEgfOuq-Tu=fksWJkerT5fg@mail.gmail.com/\n\nwhich also links back to a much older discussion of various solutions.\n\n> There are several solutions to circumvent this issue:\n> 1) We could ask the Plugin that manages the tokens to support managing\n> the Credential Cache as well. This makes it possible for the plugin to\n> actively manage tokens and revoke tokens from the cache that have\n> expired. Downside is that making plugins aware of the cache in git,\n> might not be a good idea, and there might even be cases where caching\n> is disabled completely, which would still require the ASKPASS method.\n\nYes, I agree it's much nicer if the plugin doesn't have to fiddle with\n(possibly) configured helpers itself.\n\n> 2) We could add an option to the GIT client that has the same\n> functionality as the ASKPASS variable, but one that takes precedence\n> before the cache. This way, one can preseed the GIT client with\n> credentials that will *always* be used, even if there is a cache that\n> has a credential for the given URL.\n\nYou can do that already by setting up a helper that provides the\ncredentials; Git will stop asking as soon as it has an answer. Inserting\na helper at the front of the list is tricky, but you can provide an\nempty helper to reset the list. E.g.:\n\n  git \\\n    -c credential.helper = \\\n    -c credential.helper = your-real-helper \\\n    fetch ...\n\nwould work.\n\n> 3) We could alter the cache daemon (or build a new one) that has more\n> elaborate options regarding it's timeout. Perhaps a daemon option like\n> \"--keep-timeout-on-approve\".\n> This would mean that if a credential is approved for a second time the\n> timeout is not refreshed.\n\nYes, that would mostly work. There is a race condition where the cache\ntimes out _during_ a client request. So the cache daemon sees:\n\n  1. Client asks for credential; we provide it.\n\n  2. Cache times out; credential is dropped.\n\n  3. Client provides credential to store.\n\nThen it appears \"new\". We could keep a tombstone entry for the timed-out\ncredential, but part of the point of timing out is that we drop the\nsecret data. You could perhaps do something clever with one-way hashes,\nbut my preference is to keep things as simple as possible.\n\n> Currently the daemon just removes any old credentials from it's cache\n> and adds the new one without questioning the intention of the user. We\n> could add a check that tests if the incoming credential is equal to\n> the currently cached credential, and skip updating the expiration date\n> if they are the same. (With an exception when receiving a new\n> password, then we should forcefully update). This means that the\n> --timeout option from the daemon will become a 'hard' timeout, instead\n> of a user-action timeout. Of course this should be done with a command\n> line option on the daemon to not change the default behaviour.\n\nThe variant discussed in the thread I linked is to ask Git not to ask to\nstore credentials which came from a helper in the first place. I think\nthat solves the problem more directly, and gives the cache helper more\npredictable semantics. But some people expressed a preference for the\ncurrent behavior in that thread. We could perhaps make it optional,\nthough (credential.storeFromHelpers or similar).\n\n-Peff\n"}]}