git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv3 10/13] credentials: add "cache" helper

From
Jeff King <peff@peff.net>
Date
Jan 10, 2012, 17:53 UTC
Message-ID
<20120110175312.GA7289@sigill.intra.peff.net>
In-Reply-To
<20120110174420.GA22184@burratino>
On Tue, Jan 10, 2012 at 11:44:21AM -0600, Jonathan Nieder wrote:
Show 18 quoted lines
> Jeff King wrote:
> 
> >   2. A hack in credential-cache won't help any unix-socket
> >      users who come along later.
> >
> >   3. The chdir trickery isn't that likely to fail (basically
> >      it's only a problem if your cwd is missing or goes away
> >      while you're running).  And because we only enable the
> >      hack when we get a too-long name, it can only fail in
> >      cases that would have failed under the previous code
> >      anyway.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
> 
> A part of me worries that this assumption (3), and the additional
> assumption that nothing running concurrently cares about the cwd,
> might come back to bite us when the future (2) arrives.  But I don't
> see a better approach.

The problem is that when future (2) arrives, a credential-cache specific hack won't be helping it at all. :)

To be honest, I don't really expect a lot of future unix-socket users. It's not portable, and most of our IPC happens over pipes. The design of the cache daemon is very specific in requiring a true many-clients-to-one-server model, but also caring deeply about access controls (making TCP sockets less good[1]).

[1] One could in theory do a loopback TCP socket and provide a random
    token read from an access-controlled file. But that ends up a lot
    more complicated and introduces new attack surfaces. Which is a bad
    thing for security-sensitive code like this.
Show 11 quoted lines
> Follow-on ideas: on platforms that support it, it would be nice to use
> 
> 	ctx->orig_dirfd = open(".", O_RDONLY);
> 	if (ctx->orig_dirfd < 0)
> 		... error handling ...
> 	...
> 	if (ctx->orig_dirfd >= 0) {
> 		if (fchdir(ctx->orig_dirfd))
> 			die_errno("unable to restore original working directory");
> 		close(ctx->orig_dirfd);
> 	}

Yeah, I started by using fchdir, but noticed that we don't use it anywhere else and didn't want to create a portability problem. It does fix the "somebody deleted your cwd while you were gone from it" problem. But if you have no cwd at all, the open call will still fail.

Still, it would be slightly more robust. I wonder how portable fchdir is in practice (I guess we could always fall back to the getcwd code path). Do you want to prepare a patch on top?

Show 11 quoted lines
> [...]
> > +		dir = path;
> > +		dir_len = slash - path;
> [...]
> > +		if (chdir_len(dir, dir_len) < 0)
> > +			return -1;
> 
> Could avoid some complication by eliminating the dir_len var.
> 
> 		if (chdir_len(dir, slash - dir))
> 			return -1;

Ah, yeah. Originally I had written it so that "slash" didn't survive untouched to there, but in the current code, that would work.

Junio, if you haven't merged it to next yet, it might be worth squashing in the patch below. Otherwise I wouldn't worry about it.

> Neither seems worth holding up the patch, so
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for the review (and for, as usual, a well-written bug report).
---
diff --git a/unix-socket.c b/unix-socket.c
index 91ed9dc..e8f19c6 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -43,7 +43,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 	if (size > sizeof(sa->sun_path)) {
 		const char *slash = find_last_dir_sep(path);
 		const char *dir;
-		int dir_len;
 
 		if (!slash) {
 			errno = ENAMETOOLONG;
@@ -51,8 +50,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 		}
 
 		dir = path;
-		dir_len = slash - path;
-
 		path = slash + 1;
 		size = strlen(path) + 1;
 		if (size > sizeof(sa->sun_path)) {
@@ -64,7 +61,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
 			errno = ENAMETOOLONG;
 			return -1;
 		}
-		if (chdir_len(dir, dir_len) < 0)
+		if (chdir_len(dir, slash - dir) < 0)
 			return -1;
 	}
Previous: Jonathan NiederNext: Jonathan Nieder
Message 23 of 54 in “[PATCHv3 0/13] credential helpers”
  1. Jeff KingDec 10, 2011
  2. 01/13 test-lib: add test_config_global variantJeff King, Dec 10, 2011
  3. 02/13 t5550: fix typoJeff King, Dec 10, 2011
  4. 03/13 introduce credentials APIJeff King, Dec 10, 2011
  5. Jakub NarebskiDec 10, 2011
  6. Jeff KingDec 10, 2011
  7. 04/13 credential: add function for parsing url componentsJeff King, Dec 10, 2011
  8. 05/13 http: use credential API to get passwordsJeff King, Dec 10, 2011
  9. 06/13 credential: apply helper configJeff King, Dec 10, 2011
  10. 07/13 credential: add credential.*.usernameJeff King, Dec 10, 2011
  11. 08/13 credential: make relevance of http path configurableJeff King, Dec 10, 2011
  12. Jakub NarebskiDec 10, 2011
  13. Jeff KingDec 10, 2011
  14. 09/13 docs: end-user documentation for the credential subsystemJeff King, Dec 10, 2011
  15. 10/13 credentials: add "cache" helperJeff King, Dec 10, 2011
  16. Jonathan NiederJan 10, 2012
  17. Jeff KingJan 10, 2012
  18. Jeff KingJan 10, 2012
  19. Junio C HamanoJan 10, 2012
  20. Jeff KingJan 17, 2012
  21. Junio C HamanoJan 17, 2012
  22. Jonathan NiederJan 10, 2012
  23. Jeff KingJan 10, 2012
  24. Jonathan NiederJan 11, 2012
  25. Jeff KingJan 12, 2012
  26. 11/13 strbuf: add strbuf_add*_urlencodeJeff King, Dec 10, 2011
  27. Jakub NarebskiDec 10, 2011
  28. Jeff KingDec 10, 2011
  29. 12/13 credentials: add "store" helperJeff King, Dec 10, 2011
  30. 13/13 t: add test harness for external credential helpersJeff King, Dec 10, 2011
  31. 0/9 echo characters in username promptJeff King, Dec 10, 2011
  32. 1/9 imap-send: avoid buffer overflowJeff King, Dec 10, 2011
  33. 2/9 imap-send: don't check return value of git_getpassJeff King, Dec 10, 2011
  34. 3/9 move git_getpass to its own source fileJeff King, Dec 10, 2011
  35. 4/9 refactor git_getpass into generic prompt functionJeff King, Dec 10, 2011
  36. 5/9 add generic terminal prompt functionJeff King, Dec 10, 2011
  37. Pete WyckoffDec 15, 2011
  38. Jeff KingDec 15, 2011
  39. Pete WyckoffDec 15, 2011
  40. 6/9 prompt: use git_terminal_promptJeff King, Dec 10, 2011
  41. 7/9 credential: use git_prompt instead of git_getpassJeff King, Dec 10, 2011
  42. 8/9 Makefile: linux has /dev/ttyJeff King, Dec 10, 2011
  43. 9/9 Makefile: OS X has /dev/ttyJeff King, Dec 10, 2011
  44. 1/2 Makefile: Windows lacks /dev/ttyJohannes Sixt, Dec 12, 2011
  45. 2/2 Makefile: optionally exclude code that needs Unix socketsJohannes Sixt, Dec 12, 2011
  46. Jeff KingDec 12, 2011
  47. Junio C HamanoDec 12, 2011
  48. Junio C HamanoDec 13, 2011
  49. Jeff KingDec 13, 2011
  50. Johannes SixtDec 13, 2011
  51. Junio C HamanoDec 14, 2011
  52. Jeff KingDec 12, 2011
  53. Johannes SixtDec 12, 2011
  54. 1/1 contrib: add credential helper for OS X KeychainJeff King, Dec 10, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.