[PATCH 5/4] wincred: port to generic credential helper (UNTESTED)

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: "Philipp A. Hartmann" <pah@xxxxx>

This patch is an experiment to port the wincred helper
to the generic implementation.  As of know, it is
completely untested.

In addition to porting the helper to the generic API,
this patch clears up all passwords from memory, which
reduces the total amount to saved lines.

Signed-off-by: Philipp A. Hartmann <pah@xxxxx>
---

The porting was fairly easy, but due to the lack of a testing
platform, it is completely untested.

Erik: Can you try to have look?

The patch is against the current 'next' with my series applied.
A github repository with all the helpers can also be found at
  git://github.com/pah/git-credential-helper.git

In order to keep things simple, I decided to add the setup of
the binary pipes in the generic helper's main() already.  This
was the only platform-specific change needed in the helper.

Other changes to the generic helper were
 - the addition of an URI field in the credential struct
   (can be reused in the GNOME implementation)
 - the early stop in case of incomplete credentials
   (simplifies handling this case in all three helpers)

The overall code reduction could be increased (also in
the osxkeychain helper), if we add init/cleanup functions
to the generic helper.  I skipped this for now.

Some additions to the wincred code were needed (or at least
motivated) to clean up the memory correctly.  Especially for
the password strings, this seems to be a sane thing to do.
Since the wincred helper duplicates parts of these buffers to
comply with the wchar_t-based Windows API, this required more
manual free's than in the other helpers.

In summary, although the platform and API looked fairly different
from the other platforms, the generic code could be used without
too much pain.

Thanks,
  Philipp

 contrib/credential/helper/credential_helper.c      |   41 ++-
 contrib/credential/helper/credential_helper.h      |    3 +-
 contrib/credential/wincred/.gitignore              |    1 +
 contrib/credential/wincred/Makefile                |   19 +-
 .../credential/wincred/git-credential-wincred.c    |  310 +++++++++-----------
 5 files changed, 199 insertions(+), 175 deletions(-)
 create mode 100644 contrib/credential/wincred/.gitignore

diff --git a/contrib/credential/helper/credential_helper.c b/contrib/credential/helper/credential_helper.c
index e99c2ec..5ddf5a5 100644
--- a/contrib/credential/helper/credential_helper.c
+++ b/contrib/credential/helper/credential_helper.c
@@ -11,6 +11,11 @@
 
 #include <credential_helper.h>
 
+#ifdef WIN32
+#include <fcntl.h>
+#include <io.h>
+#endif
+
 void credential_init(struct credential *c)
 {
 	memset(c, 0, sizeof(*c));
@@ -23,6 +28,7 @@ void credential_clear(struct credential *c)
 	free(c->path);
 	free(c->username);
 	free_password(c->password);
+	free(c->uri);
 
 	credential_init(c);
 }
@@ -79,6 +85,29 @@ int credential_read(struct credential *c)
 		 * learn new lines, and the helpers are updated to match.
 		 */
 	}
+
+	/* Rebuild URI from parts */
+	*buf = '\0';
+	if (c->protocol) {
+		strncat(buf, c->protocol, sizeof(buf));
+		strncat(buf, "://", sizeof(buf));
+	}
+	if (c->username) {
+		strncat(buf, c->username, sizeof(buf));
+		strncat(buf, "@", sizeof(buf));
+	}
+	if (c->host)
+		strncat(buf, c->host, sizeof(buf));
+	if (c->port) {
+		value = buf + strlen(buf);
+		snprintf(value, sizeof(buf)-(value-buf), ":%hd", c->port);
+	}
+	if (c->path) {
+		strncat(buf, "/", sizeof(buf));
+		strncat(buf, c->path, sizeof(buf));
+	}
+	c->uri = xstrdup(buf);
+
 	return 0;
 }
 
@@ -126,6 +155,12 @@ int main(int argc, char *argv[])
 		goto out;
 	}
 
+#ifdef WIN32
+	/* git on Windows uses binary pipes to avoid CRLF-issues */
+	_setmode(_fileno(stdin), _O_BINARY);
+	_setmode(_fileno(stdout), _O_BINARY);
+#endif
+
 	/* lookup operation callback */
 	while(try_op->name && strcmp(argv[1], try_op->name))
 		try_op++;
@@ -138,11 +173,15 @@ int main(int argc, char *argv[])
 	if(ret)
 		goto out;
 
+	if (!cred.protocol || !(cred.host || cred.path)) {
+		ret = EXIT_FAILURE;
+		goto out;
+	}
+
 	/* perform credential operation */
 	ret = (*try_op->op)(&cred);
 
 	credential_write(&cred);
-
 out:
 	credential_clear(&cred);
 	return ret;
diff --git a/contrib/credential/helper/credential_helper.h b/contrib/credential/helper/credential_helper.h
index 76b6e50..822b471 100644
--- a/contrib/credential/helper/credential_helper.h
+++ b/contrib/credential/helper/credential_helper.h
@@ -25,10 +25,11 @@ struct credential
 	char          *path;
 	char          *username;
 	char          *password;
+	char          *uri;     /* <protocol>://[username@][host[:port]][/path] */
 };
 
 #define CREDENTIAL_INIT \
-  { NULL,NULL,0,NULL,NULL,NULL }
+  { NULL,NULL,0,NULL,NULL,NULL,NULL }
 
 void credential_init(struct credential *c);
 void credential_clear(struct credential *c);
diff --git a/contrib/credential/wincred/.gitignore b/contrib/credential/wincred/.gitignore
new file mode 100644
index 0000000..4780e4e
--- /dev/null
+++ b/contrib/credential/wincred/.gitignore
@@ -0,0 +1 @@
+git-credential-wincred.exe
diff --git a/contrib/credential/wincred/Makefile b/contrib/credential/wincred/Makefile
index bad45ca..ee7a8ef 100644
--- a/contrib/credential/wincred/Makefile
+++ b/contrib/credential/wincred/Makefile
@@ -1,4 +1,5 @@
-all: git-credential-wincred.exe
+MAIN:=git-credential-wincred
+all:: $(MAIN)
 
 CC = gcc
 RM = rm -f
@@ -7,8 +8,18 @@ CFLAGS = -O2 -Wall
 -include ../../../config.mak.autogen
 -include ../../../config.mak
 
-git-credential-wincred.exe : git-credential-wincred.c
-	$(LINK.c) $^ $(LOADLIBES) $(LDLIBS) -o $@
+HELPER:=../helper
+VPATH +=$(HELPER)
+
+SRCS:=$(MAIN).c
+SRCS+=credential_helper.c
+OBJS:=$(SRCS:.c=.o)
+
+%.o: %.c
+	$(CC) $(CFLAGS) $(CPPFLAGS) -I$(HELPER) -o $@ -c $<
+
+$(MAIN).exe: $(OBJS)
+	$(LINK.o) $^ $(LOADLIBES) $(LDLIBS) -o $@
 
 clean:
-	$(RM) git-credential-wincred.exe
+	$(RM) $(MAIN).exe $(OBJS)
diff --git a/contrib/credential/wincred/git-credential-wincred.c b/contrib/credential/wincred/git-credential-wincred.c
index cbaec5f..721e59f 100644
--- a/contrib/credential/wincred/git-credential-wincred.c
+++ b/contrib/credential/wincred/git-credential-wincred.c
@@ -7,37 +7,6 @@
 #include <io.h>
 #include <fcntl.h>
 
-/* common helpers */
-
-static void die(const char *err, ...)
-{
-	char msg[4096];
-	va_list params;
-	va_start(params, err);
-	vsnprintf(msg, sizeof(msg), err, params);
-	fprintf(stderr, "%s\n", msg);
-	va_end(params);
-	exit(1);
-}
-
-static void *xmalloc(size_t size)
-{
-	void *ret = malloc(size);
-	if (!ret && !size)
-		ret = malloc(1);
-	if (!ret)
-		 die("Out of memory");
-	return ret;
-}
-
-static char *xstrdup(const char *str)
-{
-	char *ret = strdup(str);
-	if (!ret)
-		die("Out of memory");
-	return ret;
-}
-
 /* MinGW doesn't have wincred.h, so we need to define stuff */
 
 typedef struct _CREDENTIAL_ATTRIBUTEW {
@@ -84,13 +53,16 @@ static CredPackAuthenticationBufferWT CredPackAuthenticationBufferW;
 static CredFreeT CredFree;
 static CredDeleteWT CredDeleteW;
 
-static void load_cred_funcs(void)
+
+static int load_cred_funcs(void)
 {
 	/* load DLLs */
 	advapi = LoadLibrary("advapi32.dll");
 	credui = LoadLibrary("credui.dll");
-	if (!advapi || !credui)
-		die("failed to load DLLs");
+	if (!advapi || !credui) {
+		error("failed to load DLLs");
+		return EXIT_FAILURE;
+	}
 
 	/* get function pointers */
 	CredWriteW = (CredWriteWT)GetProcAddress(advapi, "CredWriteW");
@@ -104,28 +76,58 @@ static void load_cred_funcs(void)
 	CredDeleteW = (CredDeleteWT)GetProcAddress(advapi, "CredDeleteW");
 	if (!CredWriteW || !CredUnPackAuthenticationBufferW ||
 	    !CredEnumerateW || !CredPackAuthenticationBufferW || !CredFree ||
-	    !CredDeleteW)
-		die("failed to load functions");
+	    !CredDeleteW) {
+		error("failed to load functions");
+		return EXIT_FAILURE;
+	}
+	return EXIT_SUCCESS;
 }
 
 static char target_buf[1024];
-static char *protocol, *host, *path, *username;
-static WCHAR *wusername, *password, *target;
+static char port_buf[8];
+static WCHAR *wusername, *wpassword;
 
-static void write_item(const char *what, WCHAR *wbuf)
+static WCHAR *utf8_to_utf16_dup(const char *str)
 {
-	char *buf;
-	int len = WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, NULL, 0, NULL,
-	    FALSE);
-	buf = xmalloc(len);
-
-	if (!WideCharToMultiByte(CP_UTF8, 0, wbuf, -1, buf, len, NULL, FALSE))
-		die("WideCharToMultiByte failed!");
-
-	printf("%s=", what);
-	fwrite(buf, 1, len - 1, stdout);
-	putchar('\n');
-	free(buf);
+	int wlen = MultiByteToWideChar(CP_UTF8, 0, str, -1, NULL, 0);
+	WCHAR *wstr = xmalloc(sizeof(WCHAR) * wlen);
+	if (!MultiByteToWideChar(CP_UTF8, 0, str, -1, wstr, wlen))
+		die("MultiByteToWideChar failed");
+	return wstr;
+}
+
+static char *utf16_to_utf8_dup(const WCHAR *wstr)
+{
+	int len = WideCharToMultiByte(CP_UTF8, 0, wstr, -1, NULL, 0, NULL,
+		FALSE);
+	char *str = xmalloc(len);
+	if (!WideCharToMultiByte(CP_UTF8, 0, wstr, -1, str, len, NULL, FALSE))
+		die("WideCharToMultiByte failed");
+	return str;
+}
+
+static void free_wpassword(WCHAR *pass)
+{
+	WCHAR *w = pass;
+	if(!pass)
+		return;
+	while(*w) *w++ = L'\0';
+	free(pass);
+}
+
+static int prepare_credential(struct credential *c)
+{
+	if (load_cred_funcs() )
+		return EXIT_FAILURE;
+
+	if (c->username)
+		wusername = utf8_to_utf16_dup(c->username);
+	if (c->password)
+		wpassword = utf8_to_utf16_dup(c->password);
+	if (c->port) {
+		snprintf(port_buf,"%hd",c->port);
+	}
+	return EXIT_SUCCESS;
 }
 
 static int match_attr(const CREDENTIALW *cred, const WCHAR *keyword,
@@ -143,24 +145,28 @@ static int match_attr(const CREDENTIALW *cred, const WCHAR *keyword,
 	return 0; /* not found */
 }
 
-static int match_cred(const CREDENTIALW *cred)
+static int match_cred(const CREDENTIALW *cred, const struct credential *c)
 {
 	return (!wusername || !wcscmp(wusername, cred->UserName)) &&
-	    match_attr(cred, L"git_protocol", protocol) &&
-	    match_attr(cred, L"git_host", host) &&
-	    match_attr(cred, L"git_path", path);
+	    match_attr(cred, L"git_protocol", c->protocol) &&
+	    match_attr(cred, L"git_host", c->host) &&
+	    (c->port && match_attr(cred, L"git_port", port_buf)) &&
+	    match_attr(cred, L"git_path", c->path);
 }
 
-static void get_credential(void)
+static int get_credential(struct credential *c)
 {
-	WCHAR *user_buf, *pass_buf;
+	WCHAR *user_buf = NULL, *pass_buf = NULL;
 	DWORD user_buf_size = 0, pass_buf_size = 0;
 	CREDENTIALW **creds, *cred = NULL;
 	DWORD num_creds;
-	int i;
+	int i, ret = EXIT_SUCCESS;
+
+	if (prepare_credential(c))
+		return EXIT_FAILURE;
 
 	if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
-		return;
+		return EXIT_FAILURE;
 
 	/* search for the first credential that matches username */
 	for (i = 0; i < num_creds; ++i)
@@ -169,7 +175,7 @@ static void get_credential(void)
 			break;
 		}
 	if (!cred)
-		return;
+		goto out;
 
 	CredUnPackAuthenticationBufferW(0, cred->CredentialBlob,
 	    cred->CredentialBlobSize, NULL, &user_buf_size, NULL, NULL,
@@ -180,20 +186,29 @@ static void get_credential(void)
 
 	if (!CredUnPackAuthenticationBufferW(0, cred->CredentialBlob,
 	    cred->CredentialBlobSize, user_buf, &user_buf_size, NULL, NULL,
-	    pass_buf, &pass_buf_size))
-		die("CredUnPackAuthenticationBuffer failed");
-
-	CredFree(creds);
+	    pass_buf, &pass_buf_size)) {
+		error("CredUnPackAuthenticationBuffer failed");
+		ret = EXIT_FAILURE;
+		goto out;
+	}
 
 	/* zero-terminate (sizes include zero-termination) */
 	user_buf[user_buf_size - 1] = L'\0';
 	pass_buf[pass_buf_size - 1] = L'\0';
 
-	write_item("username", user_buf);
-	write_item("password", pass_buf);
+	if (!c->username)
+		c->username = utf16_to_utf8_dup(user_buf);
+	free_password(c->password);
+	c->password = utf16_to_utf8_dup(pass_buf);
+
+out:
+	CredFree(creds);
 
 	free(user_buf);
-	free(pass_buf);
+	free(wusername);
+	free_wpassword(pass_buf);
+
+	return ret;
 }
 
 static void write_attr(CREDENTIAL_ATTRIBUTEW *attr, const WCHAR *keyword,
@@ -205,29 +220,42 @@ static void write_attr(CREDENTIAL_ATTRIBUTEW *attr, const WCHAR *keyword,
 	attr->Value = (LPBYTE)value;
 }
 
-static void store_credential(void)
+static int store_credential(struct credential *c)
 {
 	CREDENTIALW cred;
 	BYTE *auth_buf;
 	DWORD auth_buf_size = 0;
 	CREDENTIAL_ATTRIBUTEW attrs[CRED_MAX_ATTRIBUTES];
+	WCHAR *wtarget;
+	int ret = EXIT_SUCCESS;
 
-	if (!wusername || !password)
-		return;
+	if (prepare_credential(c))
+		return EXIT_FAILURE;
+
+	/* prepare 'target', the unique key for the credential */
+	strncat(target_buf,"git:",sizeof(target_buf));
+	strncat(target_buf,c->uri,sizeof(target_buf));
+	wtarget = utf8_to_utf16_dup(target_buf);
+
+	if (!wusername || !wpassword)
+		return EXIT_FAILURE;
 
 	/* query buffer size */
-	CredPackAuthenticationBufferW(0, wusername, password,
+	CredPackAuthenticationBufferW(0, wusername, wpassword,
 	    NULL, &auth_buf_size);
 
 	auth_buf = xmalloc(auth_buf_size);
 
-	if (!CredPackAuthenticationBufferW(0, wusername, password,
-	    auth_buf, &auth_buf_size))
-		die("CredPackAuthenticationBuffer failed");
+	if (!CredPackAuthenticationBufferW(0, wusername, wpassword,
+	    auth_buf, &auth_buf_size)) {
+		error("CredPackAuthenticationBuffer failed");
+		ret = EXIT_FAILURE;
+		goto out;
+	}
 
 	cred.Flags = 0;
 	cred.Type = CRED_TYPE_GENERIC;
-	cred.TargetName = target;
+	cred.TargetName = wtarget;
 	cred.Comment = L"saved by git-credential-wincred";
 	cred.CredentialBlobSize = auth_buf_size;
 	cred.CredentialBlob = auth_buf;
@@ -237,121 +265,65 @@ static void store_credential(void)
 	cred.TargetAlias = NULL;
 	cred.UserName = wusername;
 
-	write_attr(attrs, L"git_protocol", protocol);
+	write_attr(attrs, L"git_protocol", c->protocol);
 
-	if (host) {
-		write_attr(attrs + cred.AttributeCount, L"git_host", host);
+	if (c->host) {
+		write_attr(attrs + cred.AttributeCount, L"git_host", c->host);
 		cred.AttributeCount++;
 	}
 
-	if (path) {
-		write_attr(attrs + cred.AttributeCount, L"git_path", path);
+	if (c->port) {
+		write_attr(attrs + cred.AttributeCount, L"git_port", port_buf);
 		cred.AttributeCount++;
 	}
 
-	if (!CredWriteW(&cred, 0))
-		die("CredWrite failed");
+	if (c->path) {
+		write_attr(attrs + cred.AttributeCount, L"git_path", c->path);
+		cred.AttributeCount++;
+	}
+
+	if (!CredWriteW(&cred, 0)) {
+		error("CredWrite failed");
+		ret = EXIT_FAILURE;
+	}
+
+out:
+	free(auth_buf);
+	free(wusername);
+	free_wpassword(wpassword);
+	free(wtarget);
+	return ret;
 }
 
-static void erase_credential(void)
+static int erase_credential(struct credential *c)
 {
 	CREDENTIALW **creds;
 	DWORD num_creds;
 	int i;
 
+	if (prepare_credential(c))
+		return EXIT_FAILURE;
+
 	if (!CredEnumerateW(L"git:*", 0, &num_creds, &creds))
-		return;
+		return EXIT_FAILURE;
 
 	for (i = 0; i < num_creds; ++i) {
-		if (match_cred(creds[i]))
+		if (match_cred(creds[i],c))
 			CredDeleteW(creds[i]->TargetName, creds[i]->Type, 0);
 	}
 
 	CredFree(creds);
+	return EXIT_SUCCESS;
 }
 
-static WCHAR *utf8_to_utf16_dup(const char *str)
-{
-	int wlen = MultiByteToWideChar(CP_UTF8, 0, str, -1, NULL, 0);
-	WCHAR *wstr = xmalloc(sizeof(WCHAR) * wlen);
-	MultiByteToWideChar(CP_UTF8, 0, str, -1, wstr, wlen);
-	return wstr;
-}
-
-static void read_credential(void)
-{
-	char buf[1024];
-
-	while (fgets(buf, sizeof(buf), stdin)) {
-		char *v;
-
-		if (!strcmp(buf, "\n"))
-			break;
-		buf[strlen(buf)-1] = '\0';
-
-		v = strchr(buf, '=');
-		if (!v)
-			die("bad input: %s", buf);
-		*v++ = '\0';
-
-		if (!strcmp(buf, "protocol"))
-			protocol = xstrdup(v);
-		else if (!strcmp(buf, "host"))
-			host = xstrdup(v);
-		else if (!strcmp(buf, "path"))
-			path = xstrdup(v);
-		else if (!strcmp(buf, "username")) {
-			username = xstrdup(v);
-			wusername = utf8_to_utf16_dup(v);
-		} else if (!strcmp(buf, "password"))
-			password = utf8_to_utf16_dup(v);
-		else
-			die("unrecognized input");
-	}
-}
-
-int main(int argc, char *argv[])
+/*
+ * Table with helper operation callbacks, used by generic
+ * credential helper main function.
+ */
+struct credential_operation const credential_helper_ops[] =
 {
-	const char *usage =
-	    "Usage: git credential-wincred <get|store|erase>\n";
-
-	if (!argv[1])
-		die(usage);
-
-	/* git use binary pipes to avoid CRLF-issues */
-	_setmode(_fileno(stdin), _O_BINARY);
-	_setmode(_fileno(stdout), _O_BINARY);
-
-	read_credential();
-
-	load_cred_funcs();
-
-	if (!protocol || !(host || path))
-		return 0;
-
-	/* prepare 'target', the unique key for the credential */
-	strncat(target_buf, "git:", sizeof(target_buf));
-	strncat(target_buf, protocol, sizeof(target_buf));
-	strncat(target_buf, "://", sizeof(target_buf));
-	if (username) {
-		strncat(target_buf, username, sizeof(target_buf));
-		strncat(target_buf, "@", sizeof(target_buf));
-	}
-	if (host)
-		strncat(target_buf, host, sizeof(target_buf));
-	if (path) {
-		strncat(target_buf, "/", sizeof(target_buf));
-		strncat(target_buf, path, sizeof(target_buf));
-	}
-
-	target = utf8_to_utf16_dup(target_buf);
-
-	if (!strcmp(argv[1], "get"))
-		get_credential();
-	else if (!strcmp(argv[1], "store"))
-		store_credential();
-	else if (!strcmp(argv[1], "erase"))
-		erase_credential();
-	/* otherwise, ignore unknown action */
-	return 0;
-}
+	{ "get",   get_credential   },
+	{ "store", store_credential },
+	{ "erase", erase_credential },
+	CREDENTIAL_OP_END
+};
-- 
1.7.10.4

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]