diff mbox series

Documentation: clarify that cache forgets credentials if the system restarts

Message ID pull.1447.git.1671610994375.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series Documentation: clarify that cache forgets credentials if the system restarts | expand

Commit Message

M Hickford Dec. 21, 2022, 8:23 a.m. UTC
From: M Hickford <mirth.hickford@gmail.com>

Make it obvious to readers unfamiliar with Unix sockets.

Signed-off-by: M Hickford <mirth.hickford@gmail.com>
---
    Documentation: clarify that cache forgets credentials if the system
    restarts
    
    Make it obvious to readers unfamiliar with Unix sockets.
    
    Signed-off-by: M Hickford mirth.hickford@gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1447%2Fhickford%2Fpatch-2-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1447/hickford/patch-2-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1447

 Documentation/git-credential-cache.txt | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 7c2ef319c52c4997256f5807564523dfd4acdfc7

Comments

Junio C Hamano Dec. 21, 2022, 11:15 a.m. UTC | #1
"M Hickford via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: M Hickford <mirth.hickford@gmail.com>
>
> Make it obvious to readers unfamiliar with Unix sockets.

Is familiarity with sockets required?

Isn't the death of the daemon process that causes the credential
data cached in-core of the process?

>  This command caches credentials in memory for use by future Git
>  programs. The stored credentials never touch the disk, and are forgotten
> -after a configurable timeout.  The cache is accessible over a Unix
> +after a configurable timeout.  Credentials are forgotten sooner if you
> +log out or the system restarts.  The cache is accessible over a Unix

If we mention "if you log out" here, the readers would also want to
learn about credentialCache.ignoreSIGHUP configuration, no?

This is not a new issue, but I am not sure if "never touch the disk"
is a honest thing to say (I know there is no "write this in a file"
done by the cache daemon, but the running daemon can be swapped out
and I do not think we do anything to prevent the in-core structure
credential_cache_entry from getting written to the swap.

Taking all of the above together, perhaps something like this?

    ... caches credentials for use by future Git programs.  The
    stored credentials are kept in memory of the cache-daemon
    process (instead of written to a file) and are forgotten after a
    configuarble timeout.  The cache-daemon dies with the cached
    credentials upon a system shutdown/restart, or when it receives
    SIGHUP (i.e. by logging out, you disconnect from the terminal
    the daemon was started from); the latter can be disabled with
    credentialCache.ignoreSIGHUP configuration.  The cache is
    accessible over a Unix domain socket, ...
brian m. carlson Dec. 21, 2022, 10:09 p.m. UTC | #2
On 2022-12-21 at 08:23:14, M Hickford via GitGitGadget wrote:
> From: M Hickford <mirth.hickford@gmail.com>
> 
> Make it obvious to readers unfamiliar with Unix sockets.
> 
> Signed-off-by: M Hickford <mirth.hickford@gmail.com>
> ---
>     Documentation: clarify that cache forgets credentials if the system
>     restarts
>     
>     Make it obvious to readers unfamiliar with Unix sockets.
>     
>     Signed-off-by: M Hickford mirth.hickford@gmail.com
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1447%2Fhickford%2Fpatch-2-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1447/hickford/patch-2-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1447
> 
>  Documentation/git-credential-cache.txt | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
> index 432e159d952..83fb4d4c4dc 100644
> --- a/Documentation/git-credential-cache.txt
> +++ b/Documentation/git-credential-cache.txt
> @@ -16,7 +16,8 @@ DESCRIPTION
>  
>  This command caches credentials in memory for use by future Git
>  programs. The stored credentials never touch the disk, and are forgotten
> -after a configurable timeout.  The cache is accessible over a Unix
> +after a configurable timeout.  Credentials are forgotten sooner if you
> +log out or the system restarts.  The cache is accessible over a Unix
>  domain socket, restricted to the current user by filesystem permissions.

I don't think it's accurate to say that the credentials are forgotten
sooner if you log out.  That may be the case on Windows, or it may be
the case if you or your distro have configured systemd to gratuitously
murder all your local processes when your session exits[0], but it
hasn't traditionally been the case on Unix that processes exit when your
session or shell exits.  For example, I don't believe that the statement
is accurate on Debian, Ubuntu, or the BSDs by default, which constitute
a substantial number of deployed Unix systems.

[0] Such as with KillUserProcesses=yes.
Jeff King Dec. 22, 2022, 2:41 a.m. UTC | #3
On Wed, Dec 21, 2022 at 08:15:59PM +0900, Junio C Hamano wrote:

> This is not a new issue, but I am not sure if "never touch the disk"
> is a honest thing to say (I know there is no "write this in a file"
> done by the cache daemon, but the running daemon can be swapped out
> and I do not think we do anything to prevent the in-core structure
> credential_cache_entry from getting written to the swap.

Right, we don't do anything like mlock(), mostly because of the
portability problems (though obviously we could make an optional
wrapper, which is strictly better than the status quo). On the other
hand, neither does git itself, so we're only holding credential-cache to
the same standard. Arguably the cache holds credentials longer, but a
fetch or push may run for quite a while bottle-necked on network or pack
generation/indexing (and both of those operations create memory pressure
which may trigger swap).

But I agree that it is more accurate to say "does not touch the
filesystem" or your "instead of written to a file".

> Taking all of the above together, perhaps something like this?
> 
>     ... caches credentials for use by future Git programs.  The
>     stored credentials are kept in memory of the cache-daemon
>     process (instead of written to a file) and are forgotten after a
>     configuarble timeout.  The cache-daemon dies with the cached
>     credentials upon a system shutdown/restart, or when it receives
>     SIGHUP (i.e. by logging out, you disconnect from the terminal
>     the daemon was started from); the latter can be disabled with
>     credentialCache.ignoreSIGHUP configuration.  The cache is
>     accessible over a Unix domain socket, ...

That seems reasonable. I was going to suggest also mentioning that we
can ask the daemon to exit manually, but that is pretty well covered
later in the document. On the other hand, it may make sense to put all
of this together in the description.

As brian mentioned, not every system behaves the same with respect to
SIGHUP here. So we may need to be a little more vague here.

So maybe more like:

  ...are forgotten after a configurable timeout, or if the daemon exits.

  You can ask the daemon to exit manually, forgetting all cached
  credentials before their timeout, by running:

    git credential-cache exit

  The daemon will also exit when it receives a signal. Depending on the
  configuration of your system, this may happen automatically when you
  log out. If you want to inhibit this behavior (and let items time out
  as normal even when you're logged out), you can set the
  credentialCache.ignoreSIGHUP configuration variable to `true`.

There are many possible variations, of course. I was mostly just trying
to get across the point that:

  - there are several ways for the daemon to exit

  - sighup / logout handling may depend on your system

And I am happy with any text that says so.

-Peff
M Hickford Jan. 28, 2023, 8:08 p.m. UTC | #4
Thanks Junio, Jeff and Brian for your replies. I'll send an updated patch based on Junio's text. To me, most important is to explain that credentials are forgotten early if the daemon exits, so that setting a timeout of 1 year is unlikely to work.
diff mbox series

Patch

diff --git a/Documentation/git-credential-cache.txt b/Documentation/git-credential-cache.txt
index 432e159d952..83fb4d4c4dc 100644
--- a/Documentation/git-credential-cache.txt
+++ b/Documentation/git-credential-cache.txt
@@ -16,7 +16,8 @@  DESCRIPTION
 
 This command caches credentials in memory for use by future Git
 programs. The stored credentials never touch the disk, and are forgotten
-after a configurable timeout.  The cache is accessible over a Unix
+after a configurable timeout.  Credentials are forgotten sooner if you
+log out or the system restarts.  The cache is accessible over a Unix
 domain socket, restricted to the current user by filesystem permissions.
 
 You probably don't want to invoke this command directly; it is meant to