* Re: [PATCH] credential-osxkeychain: load Security framework dynamically
From: Jeff King @ 2011-09-15 0:16 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, Junio C Hamano, John Szakmeister
In-Reply-To: <CAG+J_Dxenu7GDsTzUCCQiYg38Vto8CtD8ODX1J9mA8o1n2_YJA@mail.gmail.com>
On Wed, Sep 14, 2011 at 07:56:42PM -0400, Jay Soffian wrote:
> Okay, I think I was led astray by the fact that
> credential-{cache,store}.c (at least the latter of which is meant as
> nothing more than an example helper right?) links with git.
No, credential-store is meant to be used. It's just that it has a
security tradeoff that makes it the wrong choice for most cases. So it's
meant to be used sparingly. :)
As for those helpers being linked against git, I guess it doesn't make
them the best example code. But I wanted them to be always available as
a lowest common denominator (because even if you have a fancy local
keychain, it is likely that you'll end up at some point using git across
an ssh connection, and I wanted to provide _something_ there).
Not having any external dependencies, those helpers don't pollute our
code base too much. Building and testing them with the rest of git keeps
the code fresh and unbroken. Maybe it would be better if they provided a
clearer separation as an example. I'm open to that if people think it's
worth splitting them out. I suspect I could write credential-store as
something like 10 lines of perl.
-Peff
^ permalink raw reply
* Re: Helping on Git development
From: Jeff King @ 2011-09-15 0:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Andrew Ardill, Eduardo D'Avila
In-Reply-To: <7vd3f2snox.fsf@alter.siamese.dyndns.org>
On Wed, Sep 14, 2011 at 04:34:38PM -0700, Junio C Hamano wrote:
> > Is there such a thing as enough coders? :)
>
> Ever heard of the Mythical Man-Month ;-)?
I thought git was a silver bullet. :)
> I was simply saying that there already are many people who scratch his
> own real itch, and we are short of the bandwidth to review them all.
> It would not help the project at all to add more people who scratch
> some random non-itches that nobody is actually interested in (e.g. an
> entry in an unmaintained "bug tracker" that may list irrelevant and
> stale non issues).
Yeah, that may be. But I don't look at it as "we have enough
itch-scratchers, so we don't need more". I see it as survival of the
fittest. You may post a patch series that needs a lot of help, but
nobody else cares, and it dies off. Or your series may be interesting
enough that it draws attention, to the detriment of somebody else's
series (which may take longer to get reviewed and merged). But natural
selection only works if we have a diverse population to select from.
The downside, of course, is that somebody may end up wasting time going
down a fruitless road. But for a new contributor, hopefully they learn
something in the process.
> > 2. Read the list. People will report bugs. Try reproducing them,
> [...]
>
> Yes. In the earlier steps in the above, you may find out that the
> report was actually not a bug at all (e.g. old issue that has long
> been fixed, pebcak, or wrong expectation), but even in such a case,
> reporting your finding would help others.
Very much agreed. I think big organizations like mozilla have people who
do nothing but bug triage. We are not that big, but it has proven to be
one area that is easy to break out and distribute to other people.
-Peff
^ permalink raw reply
* Re: [PATCH] credential-osxkeychain: load Security framework dynamically
From: Jay Soffian @ 2011-09-14 23:56 UTC (permalink / raw)
To: Jeff King; +Cc: git, Junio C Hamano, John Szakmeister
In-Reply-To: <20110914230816.GA5754@sigill.intra.peff.net>
On Wed, Sep 14, 2011 at 7:08 PM, Jeff King <peff@peff.net> wrote:
> On Wed, Sep 14, 2011 at 06:55:26PM -0400, Jay Soffian wrote:
>
> To be honest, I was surprised to see you linking against git. I had
> always envisioned OS-specific helpers as living outside of the git.git
> repo. That's why I provided git-credential-getpass; it should be the
> only part of git that helpers really want to reuse.
Okay, I think I was led astray by the fact that
credential-{cache,store}.c (at least the latter of which is meant as
nothing more than an example helper right?) links with git.
> What are you getting from git that is useful? From my skim of your
> patch, it looks like xmalloc/die, parse_options, and credential_getpass.
Correct.
> The first can be replaced with a few trivial lines of code. The second
> is overkill, I think. The helper code will always hand you the
> "--option=value" form, and I always intended it to stay that way
> (whether that is well documented, I'm not sure). But a simple loop with
> strncmps would be fine.
>
> The hardest part is credential_getpass. You can call "git
> credential-getpass", but you'll have to read the output yourself (though
> it's quite simple to parse; see read_credential_response).
>
> I'm not a fan of cutting and pasting code, and this will add a number of
> lines to your C program. But I feel like keeping the build completely
> separate from core git is probably a good boundary (especially because
> this will not be getting built or tested all the time, as most of the
> core code is).
Okay.
j.
^ permalink raw reply
* Re: Fwd: [Survey] Signed push
From: Junio C Hamano @ 2011-09-14 23:44 UTC (permalink / raw)
To: Linus Torvalds
Cc: Philip Oakley, Johan Herland, Git List, Matthieu Moy,
Michael Haggerty, Junio C Hamano
In-Reply-To: <CA+55aFxFnAjpSAd+uB25BuZXBJGvN59qNMmF3fzvky8XK_DP0A@mail.gmail.com>
Linus Torvalds <torvalds@linux-foundation.org> writes:
> On Wed, Sep 14, 2011 at 3:51 PM, Philip Oakley <philipoakley@iee.org> wrote:
>>
>> Is one option to store the branch description (if any) on line two of the
>> <branch name> file in .git\refs\heads.
>
> Or even on line one.
>
> We already basically do that for the magic FETCH_HEAD branch, and use
> it to populate the merge commit. Extending that kind of thing to all
> branches might be a nice idea.
>
> Of course, then the question becomes "what about packed refs"? Do you
> just leave the unpacked ref in place for those?
Seriously, storage format is not an issue at all and you know it. The
semantics is.
What commands use the description and for what purpose, how it is updated
when the branch is repurposed, how it is propagated to other repositories,
if there is a situation where two descriptions need to be merged, and if
so how that merge happens, etc., etc.
We could store it in unused part of loose refs. We could add [branch
"master"] description = ... in the .git/config. The latter would even be
easier for humans to edit by hand.
If we want to use the description when merging locally, for example,
fmt-merge-msg needs to be taught to read it, which would mean we would
need an internal API "read_branch_description()", regardless of what
storage format we choose to use. If we want to use it for "git pull", then
the transport layer needs to become aware of it.
^ permalink raw reply
* Re: Helping on Git development
From: Junio C Hamano @ 2011-09-14 23:34 UTC (permalink / raw)
To: Jeff King; +Cc: git, Andrew Ardill, Eduardo D'Avila
In-Reply-To: <20110914231427.GA5611@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Sep 14, 2011 at 11:29:28AM -0700, Junio C Hamano wrote:
>
>> As to contributing to the project, right now, I think we have enough
>> people who want to write code and documentation for Git, but what we lack
>> are bandwidth to (this is not meant to be an exhaustive list):
>
> Is there such a thing as enough coders? :)
Ever heard of the Mythical Man-Month ;-)?
I thought it was clear from my message but probably I wasn't clear enough,
so let's make it clear. I didn't mean to ay "we have enough -- we need no
more -- we reject new applicants".
I was simply saying that there already are many people who scratch his own
real itch, and we are short of the bandwidth to review them all. It would
not help the project at all to add more people who scratch some random
non-itches that nobody is actually interested in (e.g. an entry in an
unmaintained "bug tracker" that may list irrelevant and stale non issues).
> 2. Read the list. People will report bugs. Try reproducing them,
> bisecting them, creating minimal test cases, narrowing the issues
> down to certain configurations or a certain bit of code, etc.
> Sometimes that will lead you to propose a solution. Sometimes
> you'll just add to the discussion, and then somebody with more
> familiarity can pick up the topic from there. But you'll have
> helped them by doing some of the work, and you'll have learned more
> about how git works.
Yes. In the earlier steps in the above, you may find out that the report
was actually not a bug at all (e.g. old issue that has long been fixed,
pebcak, or wrong expectation), but even in such a case, reporting your
finding would help others.
^ permalink raw reply
* Re: Fwd: [Survey] Signed push
From: Linus Torvalds @ 2011-09-14 23:30 UTC (permalink / raw)
To: Philip Oakley
Cc: Johan Herland, Git List, Matthieu Moy, Michael Haggerty,
Junio C Hamano
In-Reply-To: <E9E05FA85D0F4461BAE9ECAFE25CD84E@PhilipOakley>
On Wed, Sep 14, 2011 at 3:51 PM, Philip Oakley <philipoakley@iee.org> wrote:
>
> Is one option to store the branch description (if any) on line two of the
> <branch name> file in .git\refs\heads.
Or even on line one.
We already basically do that for the magic FETCH_HEAD branch, and use
it to populate the merge commit. Extending that kind of thing to all
branches might be a nice idea.
Of course, then the question becomes "what about packed refs"? Do you
just leave the unpacked ref in place for those?
Linus
^ permalink raw reply
* Re: Fwd: [Survey] Signed push
From: Philip Oakley @ 2011-09-14 22:51 UTC (permalink / raw)
To: Johan Herland, Linus Torvalds
Cc: Git List, Matthieu Moy, Michael Haggerty, Junio C Hamano
In-Reply-To: <201109141814.04752.johan@herland.net>
From: "Johan Herland" <johan@herland.net>
.
> _that_ SHA1. As I said, that _can_ be done with the notes
> infrastructure, but - as Ted noted - there might be better solutions to
> storing branch descriptions.
>
Is one option to store the branch description (if any) on line two of the
<branch name> file in .git\refs\heads.
That is, from byte 42 onward, after the 40 byte sha1 and its LF.
Older systems would simply overwrite it, while newer systems would be able
to read it. The fixed format of the first 41 chars alllows sensible checks
in the various places it is used.
Philip Oakley
^ permalink raw reply
* Re: [PATCH] credential-osxkeychain: load Security framework dynamically
From: Junio C Hamano @ 2011-09-14 23:18 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, Jeff King, John Szakmeister
In-Reply-To: <1316040926-89429-1-git-send-email-jaysoffian@gmail.com>
Jay Soffian <jaysoffian@gmail.com> writes:
> Something like this. I'm going to pause here for feedback. Is the (not yet
> existant) followup commit referenced above allowing git-credential-osxkeychain
> to be a hard link to git a worthwhile endeavor? Or would a better approach be
> to make git-credential-osxkeychain.c not use any git code?
Most definitely the latter, I would think.
The whole point of making the Git credential code talk with a defined
interface with external programs is so that these keychain helpers can be
written independently from the rest of Git.
If the reason why your keychain helper benefits from linking with the rest
of Git is because some pieces of information you need in order to respond
to the requests from credential interface is hard to get if your helper is
built as an independent program, that is a sign that we are not exposing
enough information to scripts, iow, the failure in the design of the
credential interface. If that is the case (and I doubt it is), we would
need to fix the interface (either the credential interface, or perhaps
"git config") so that such an independent program does not have to peek
inside the internals of Git.
If the reason is because you want to reuse some generic C API we have that
are not necessarily tied to Git (e.g. strbuf, string-list, etc.), on the
other hand, please resist the temptation to do so. It would not help your
program to serve as an example of independent external keychain helpers,
i.e. a demonstration of how simple to write them.
^ permalink raw reply
* Re: [PATCH 3/7] refactor argv_array into generic code
From: Jeff King @ 2011-09-14 23:18 UTC (permalink / raw)
To: Christian Couder; +Cc: Junio C Hamano, Jens Lehmann, git
In-Reply-To: <CAP8UFD1vxP9ABgJpM99hxDWWLeGO_QW7QLVFq1f-teu1fiCftA@mail.gmail.com>
On Wed, Sep 14, 2011 at 07:54:48AM +0200, Christian Couder wrote:
> In sha1-array you called the "push" function "sha1_array_append"
> instead of "sha1_array_push", so I wonder why here you call them
> "*_push*" instead of "*_append*"?
I dunno. It just seemed natural to write "push" in the context of argv.
Maybe too much perl (push, pop, shift, unshift).
argv_array_append does make sense. One could argue that
sha1_array_append actually doesn't. True, it does append to the end of
the array, but after writing the docs for it yesterday, I realized that
it less of an array, and more of a set container. Because the point of
using it is the optimized lookup/unique function, which is going to sort
it. The array is really just an implementation detail.
So arguably it should be "struct sha1_set", and "sha1_set_insert" or
something. I'm not sure if it's really worth changing (because this is
C, our data structures tend to be a little leaky, anyway, and you _can_
use sha1_array as an ordered list if you want; just don't call the
lookup or sorting functions).
-Peff
^ permalink raw reply
* Re: Helping on Git development
From: Jeff King @ 2011-09-14 23:14 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Andrew Ardill, Eduardo D'Avila
In-Reply-To: <7vehzjugdz.fsf@alter.siamese.dyndns.org>
On Wed, Sep 14, 2011 at 11:29:28AM -0700, Junio C Hamano wrote:
> As to contributing to the project, right now, I think we have enough
> people who want to write code and documentation for Git, but what we lack
> are bandwidth to (this is not meant to be an exhaustive list):
Is there such a thing as enough coders? :)
Two things that got me started on git, and that I think are still
relevant today:
1. Scratch your own itch. Surely git doesn't do something that you
wish it did. Or did it faster. Or whatever. Try to dig up past
discussions on the list to make sure you're not doing something
that has already been tried and rejected, and then start hacking.
Your patches may be terrible at first, but I think there are people
willing to guide you if you actually have running code.
2. Read the list. People will report bugs. Try reproducing them,
bisecting them, creating minimal test cases, narrowing the issues
down to certain configurations or a certain bit of code, etc.
Sometimes that will lead you to propose a solution. Sometimes
you'll just add to the discussion, and then somebody with more
familiarity can pick up the topic from there. But you'll have
helped them by doing some of the work, and you'll have learned more
about how git works.
-Peff
^ permalink raw reply
* Re: [PATCH] credential-osxkeychain: load Security framework dynamically
From: Jeff King @ 2011-09-14 23:08 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, Junio C Hamano, John Szakmeister
In-Reply-To: <1316040926-89429-1-git-send-email-jaysoffian@gmail.com>
On Wed, Sep 14, 2011 at 06:55:26PM -0400, Jay Soffian wrote:
> Something like this. I'm going to pause here for feedback. Is the (not yet
> existant) followup commit referenced above allowing git-credential-osxkeychain
> to be a hard link to git a worthwhile endeavor? Or would a better approach be
> to make git-credential-osxkeychain.c not use any git code?
To be honest, I was surprised to see you linking against git. I had
always envisioned OS-specific helpers as living outside of the git.git
repo. That's why I provided git-credential-getpass; it should be the
only part of git that helpers really want to reuse.
What are you getting from git that is useful? From my skim of your
patch, it looks like xmalloc/die, parse_options, and credential_getpass.
The first can be replaced with a few trivial lines of code. The second
is overkill, I think. The helper code will always hand you the
"--option=value" form, and I always intended it to stay that way
(whether that is well documented, I'm not sure). But a simple loop with
strncmps would be fine.
The hardest part is credential_getpass. You can call "git
credential-getpass", but you'll have to read the output yourself (though
it's quite simple to parse; see read_credential_response).
I'm not a fan of cutting and pasting code, and this will add a number of
lines to your C program. But I feel like keeping the build completely
separate from core git is probably a good boundary (especially because
this will not be getting built or tested all the time, as most of the
core code is).
-Peff
^ permalink raw reply
* [PATCH] credential-osxkeychain: load Security framework dynamically
From: Jay Soffian @ 2011-09-14 22:55 UTC (permalink / raw)
To: git; +Cc: Jay Soffian, Junio C Hamano, Jeff King, John Szakmeister
In-Reply-To: <1316023117-84755-1-git-send-email-jaysoffian@gmail.com>
Use dlopen() / dysym() instead of dynmically linking to the
Security framework. A followup commit will refactor things such
that git-credential-osxkeychain can be hardlinked to git.
Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
> Hmm, maybe it can be if I dlopen the security framework instead of linking
> against it.
Something like this. I'm going to pause here for feedback. Is the (not yet
existant) followup commit referenced above allowing git-credential-osxkeychain
to be a hard link to git a worthwhile endeavor? Or would a better approach be
to make git-credential-osxkeychain.c not use any git code?
contrib/credential-osxkeychain/Makefile | 12 +++-
.../credential-osxkeychain/generate_security.py | 73 ++++++++++++++++++++
2 files changed, 82 insertions(+), 3 deletions(-)
create mode 100755 contrib/credential-osxkeychain/generate_security.py
diff --git a/contrib/credential-osxkeychain/Makefile b/contrib/credential-osxkeychain/Makefile
index dc6bbbc3f9..001d695cb8 100644
--- a/contrib/credential-osxkeychain/Makefile
+++ b/contrib/credential-osxkeychain/Makefile
@@ -25,11 +25,17 @@ ifndef V
endif
endif
-git-credential-osxkeychain: git-credential-osxkeychain.o $(GIT_LIBS)
- $(QUIET_LINK)$(CC) -o $@ $< $(LIBS) -Wl,-framework -Wl,Security
+git-credential-osxkeychain: git-credential-osxkeychain.o security.o $(GIT_LIBS)
+ $(QUIET_LINK)$(CC) -o $@ $< security.o $(LIBS)
git-credential-osxkeychain.o: git-credential-osxkeychain.c
$(QUIET_CC)$(CC) -c $(CFLAGS) $<
+security.o: security.c
+ $(QUIET_CC)$(CC) -c $(CFLAGS) $<
+
+security.c: generate_security.py
+ python generate_security.py
+
clean:
- $(RM) git-credential-osxkeychain git-credential-osxkeychain.o
+ $(RM) git-credential-osxkeychain git-credential-osxkeychain.o security.?
diff --git a/contrib/credential-osxkeychain/generate_security.py b/contrib/credential-osxkeychain/generate_security.py
new file mode 100755
index 0000000000..db94672e95
--- /dev/null
+++ b/contrib/credential-osxkeychain/generate_security.py
@@ -0,0 +1,73 @@
+#!/usr/bin/python
+
+import re
+
+func_decls = """
+OSStatus SecKeychainAddInternetPassword(SecKeychainRef keychain, UInt32 serverNameLength, const char *serverName, UInt32 securityDomainLength, const char *securityDomain, UInt32 accountNameLength, const char *accountName, UInt32 pathLength, const char *path, UInt16 port, SecProtocolType protocol, SecAuthenticationType authenticationType, UInt32 passwordLength, const void *passwordData, SecKeychainItemRef *itemRef);
+OSStatus SecKeychainFindInternetPassword(CFTypeRef keychainOrArray, UInt32 serverNameLength, const char *serverName, UInt32 securityDomainLength, const char *securityDomain, UInt32 accountNameLength, const char *accountName, UInt32 pathLength, const char *path, UInt16 port, SecProtocolType protocol, SecAuthenticationType authenticationType, UInt32 *passwordLength, void **passwordData, SecKeychainItemRef *itemRef);
+OSStatus SecKeychainItemCopyContent(SecKeychainItemRef itemRef, SecItemClass *itemClass, SecKeychainAttributeList *attrList, UInt32 *length, void **outData);
+OSStatus SecKeychainItemDelete(SecKeychainItemRef itemRef);
+OSStatus SecKeychainItemFreeContent(SecKeychainAttributeList *attrList, void *data);
+OSStatus SecKeychainItemModifyContent(SecKeychainItemRef itemRef, const SecKeychainAttributeList *attrList, UInt32 length, const void *data);
+"""
+
+header = r"""
+#include <dlfcn.h>
+#include <Security/Security.h>
+#include "cache.h"
+
+const char *security_framework =
+ "/System/Library/Frameworks/Security.framework/Security";
+
+void *load_security()
+{
+ static void *security;
+ if (!security) {
+ if (!(security = dlopen(security_framework, RTLD_LAZY)))
+ die(_("dlopen(\"%s\") failed: %s"),
+ security_framework, dlerror());
+ }
+ return security;
+}
+"""
+
+func_tmpl = """
+%(func_decl)s
+{
+ %(func_rv)s (*func)(%(arg_types)s) =
+ dlsym(load_security(), "%(func_name)s");
+ if (!func)
+ die(_("dlsym(%(func_name)s) failed: %%s"), dlerror());
+ return func(%(arg_names)s);
+}
+"""
+
+def generate_func(decl):
+ func_rv, func_name, func_args = re.search(
+ r'^(.*?)\s+([^(]+)\((.*)\);$', decl).groups()
+ func_args = [s.strip() for s in func_args.split(',')]
+ arg_types = []
+ arg_names = []
+ for arg in func_args:
+ arg_type, arg_name = re.search(r'^(.*?)([a-zA-Z]+)$', arg).groups()
+ arg_types.append(arg_type.strip())
+ arg_names.append(arg_name.strip())
+ return func_tmpl % dict(
+ func_decl=decl.rstrip(';'),
+ func_name=func_name,
+ func_rv=func_rv,
+ arg_types=', '.join(arg_types),
+ arg_names=', '.join(arg_names),
+ )
+
+def main():
+ f = open('security.c', 'w')
+ f.write(header)
+ for decl in func_decls.splitlines():
+ decl = decl.strip()
+ if decl:
+ f.write(generate_func(decl))
+ f.close()
+
+if __name__ == '__main__':
+ main()
--
1.7.7.rc1.1.g011e1
^ permalink raw reply related
* Re: [Survey] Signed push
From: Nguyen Thai Ngoc Duy @ 2011-09-14 22:42 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <20110914210512.GA20294@elie>
On Thu, Sep 15, 2011 at 7:05 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Nguyen Thai Ngoc Duy wrote:
>> On Wed, Sep 14, 2011 at 2:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>>> An alternative that I am considering is to let the requester say this
>>> instead:
>>>
>>> are available in the git repository at:
>>> git://git.kernel.org/pub/flobar.git/ 5738c9c21e53356ab5020912116e7f82fd2d428f
> [...]
>> Stupid question, if we agree to go with signed push, can we also sign
>> pull requests and verify them when we pull? I suppose most of the
>> time, pulling can be done automatically by extracting pull url from
>> the request. This would make pull/push both signed.
>>
>> BTW, there's a third way (rsync is obsolete) to carry changes away in
>> human-unreadable way: bundles. Should we also sign the bundles too (I
>> guess we could just do the same as in signed push).
>
> If I understand you correctly, then ordinary PGP email signing[1]
> should work for that already. In your first example, the receiver can
> make sure whatever process grabs a pull request verifies it, and in
> the second example, the receiver checks the signature on her email
> before saving a bundle and passing it to "git fetch".
Yes, I think we can do that already. It's just more convenient to
teach "git fetch/pull" to take pull requests and automatically verify
them. Some repositories may also want to enforce signing and we can do
that by setting config file and fetch/pull refuses if pull requests
are not signed. We can also store the sign as git notes, just like in
git-push (extra work if it has to be done manually).
> [1] http://www.phildev.net/pgp/gpgmua.html
--
Duy
^ permalink raw reply
* Re: [PATCH] contrib: add a credential helper for Mac OS X's keychain
From: John Szakmeister @ 2011-09-14 22:23 UTC (permalink / raw)
To: Jay Soffian; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <CAG+J_Dw-vf7FtyT-vPpj-LHBo0rCBJi39bHh=8vWjc52QBMM2A@mail.gmail.com>
On Wed, Sep 14, 2011 at 9:31 AM, Jay Soffian <jaysoffian@gmail.com> wrote:
[snip]
> Nope, you snipped out too much context. That command is turned into a
> string and then sent to /usr/bin/security on its stdin. It is
> absolutely not passed on the command-line.
Bah. I see it now. Thanks.
-John
^ permalink raw reply
* Re: [PATCH 0/5] credential helper super fun pak
From: Junio C Hamano @ 2011-09-14 21:37 UTC (permalink / raw)
To: Jeff King; +Cc: git, Thomas Rast, Brian Gernhardt
In-Reply-To: <20110914191704.GA23201@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Here's a mixed bag of bugfix commits to go on top of what's in
> jk/http-auth-keyring.
Heh, it indeed is a super-fun-pak. Will queue.
^ permalink raw reply
* Re: Helping on Git development
From: Jonathan Nieder @ 2011-09-14 21:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Andrew Ardill, Eduardo D'Avila, git, git
In-Reply-To: <7vehzjugdz.fsf@alter.siamese.dyndns.org>
Junio C Hamano wrote:
> As to contributing to the project, right now, I think we have enough
> people who want to write code and documentation for Git, but what we lack
> are bandwidth to (this is not meant to be an exhaustive list):
[...]
> - distilling random wishes from the end user community while winnowing
> chaffs that are unrealistic or do not fit well with the grand scheme of
> things, to come up with a concrete proposal and a patch series
[...]
> - dig list archives to point people at age-old discussions to non-issues
> that have long been resolved to squelch noise; and
>
> - remind original submitter, people who were involved in the discussion,
> and people who should have been involved but who weren't, of a worthy
> but stalled topics from time to time.
I also should (reluctantly) mention that the Debian bug tracker has
been accepting bugs from outsiders and provides a service like this.
Caveats:
- it only tracks bugs that affect Debian (usually meaning
platform-independent bugs). Occasionally bugs from Windows users
have been reported there and it's been okay.
- the interface might seem quirky if you're not used to it.
Documentation is at http://www.debian.org/Bugs/
- no guarantee of a quick response. When there is a response,
usually it is "here are some thoughts; now let's take this to
git@vger.kernel.org".
- if the bugtracker gets swamped with reports from outside without
manpower to match, the policy re bugs from outside Debian might
change.
Bug listing: [1].
To subscribe to receive bug reports by email: [2].
Thoughts welcome, as always.
Jonathan
[1] http://bugs.debian.org/cgi-bin/pkgreport.cgi?src=git;include=tags:upstream;exclude=tags:fixed-upstream;exclude=tags:moreinfo;exclude=severity:wishlist
[2] http://www.debian.org/doc/manuals/developers-reference/resources.html#pts-commands
Summary: an email to pts@qa.debian.org whose body contains the two
lines "subscribe git", "keyword git = bts" would do the trick.
^ permalink raw reply
* Re: [Survey] Signed push
From: Jonathan Nieder @ 2011-09-14 21:05 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Junio C Hamano, Git Mailing List
In-Reply-To: <CACsJy8Dwu2U-7eEZU-VYmcrA7JwtvUkJS5SywXjZWoE1twchhQ@mail.gmail.com>
Nguyen Thai Ngoc Duy wrote:
> On Wed, Sep 14, 2011 at 2:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> An alternative that I am considering is to let the requester say this
>> instead:
>>
>> are available in the git repository at:
>> git://git.kernel.org/pub/flobar.git/ 5738c9c21e53356ab5020912116e7f82fd2d428f
[...]
> Stupid question, if we agree to go with signed push, can we also sign
> pull requests and verify them when we pull? I suppose most of the
> time, pulling can be done automatically by extracting pull url from
> the request. This would make pull/push both signed.
>
> BTW, there's a third way (rsync is obsolete) to carry changes away in
> human-unreadable way: bundles. Should we also sign the bundles too (I
> guess we could just do the same as in signed push).
If I understand you correctly, then ordinary PGP email signing[1]
should work for that already. In your first example, the receiver can
make sure whatever process grabs a pull request verifies it, and in
the second example, the receiver checks the signature on her email
before saving a bundle and passing it to "git fetch".
[1] http://www.phildev.net/pgp/gpgmua.html
^ permalink raw reply
* Re: What's cooking in git.git (Sep 2011, #04; Mon, 12)
From: Junio C Hamano @ 2011-09-14 20:57 UTC (permalink / raw)
To: Jonathon Mah; +Cc: Dan McGee, David Aguilar, git
In-Reply-To: <D3CA81F2-647B-4AD0-A4FC-4C22772FD791@JonathonMah.com>
Jonathon Mah <me@JonathonMah.com> writes:
> On 2011-09-12, at 14:59, Junio C Hamano wrote:
>
>>> [Stalled]
>>>
>>> * jm/mergetool-pathspec (2011-06-22) 2 commits
>>> - mergetool: Don't assume paths are unmerged
>>> - mergetool: Add tests for filename with whitespace
>>
>> What's the plan for this series? Do we still want to pursue it within the
>> timeframe for the next round?
>>
>> Is there any mergetool/difftool expert who volunteers to help moving this
>> topic forward?
>
> I'd love this to stay alive. As I've mentioned before, my relationship
> with shell is tenuous. My biggest problem is...
Hopefully volunteers can help moving this forward with "coding".
http://thread.gmane.org/gmane.comp.version-control.git/176215
http://thread.gmane.org/gmane.comp.version-control.git/176216
There were issues with the patches that were _not_ about the coding but
about the documentation and design.
Topics with a stale version in 'pu' that are stalled are not even alive--
they are zombies that need to be wiped at some point, or replaced with
updated series, whichever comes first.
^ permalink raw reply
* Re: Fwd: [Survey] Signed push
From: Sam Vilain @ 2011-09-14 20:52 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7vobynui8a.fsf@alter.siamese.dyndns.org>
[re-send as text-only. sorry, new system]
On 9/14/11 10:49 AM, Junio C Hamano wrote:
>> The extra line in the pull request is cheap - it's not like we need to
>> > ration them. The above format, in contrast, requires that the person
>> > doing the*pull* have a recent enough git client, otherwise the merge
>> > commit message will be just horrible.
> In a re-roll patch I've added ";# branch-name" at the end of that line for
> people with older git, but existing git wouldn't allow you to fetch anything
> but refs so you won't risk getting "just horrible" merge message;-)
>
If the system is watertight enough, then you could have verified pushes
ONLY under some new refspace, like:
refs/forks/forkid/branch
"forkid" is set up when you choose to "follow" someone's pushes. ie,
you say something like:
git fork follow linustorvalds@osdl.org
<http://pgp.mit.edu:11371/pks/lookup?op=vindex&search=0x17762C4676E21CBB>
"torvalds@osdl.org" here representing a PGP key ID search string. It
could be instead, "0x17762c4676e21cbb"
And then the "refs/forks/linus/xxx" space gets populated as signed
pushes are seen on the network (perhaps when fetching from a pull
request, or perhaps via a service). A configuration option can be set
to warn if you are not merging a signed branch. If the new "pull
request/push" object is a distinct object type or tag object, then it
could be the destination of the 'refs/forks/...' ref, and the branch
desription and hence the default merge message be retrieved from that.
Sam
^ permalink raw reply
* Re: [Survey] Signed push
From: Andrew Lutomirski @ 2011-09-14 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git
In-Reply-To: <7vwrdasvr7.fsf@alter.siamese.dyndns.org>
On Wed, Sep 14, 2011 at 1:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Andy Lutomirski <luto@MIT.EDU> writes:
>
>>> An alternative that I am considering is to let the requester say this
>>> instead:
>>>
>>> are available in the git repository at:
>>> git://git.kernel.org/pub/flobar.git/ 5738c9c21e53356ab5020912116e7f82fd2d428f
>>>
>>> without adding the extra line.
>>>
>>> That is, to allow fetching the history up to an explicitly named commit
>>> object. This would only involve a change to fetch-pack at the receiving
>>> end; just match the commit object name given from the command line against
>>> the ls-remote response and ask upload-pack to give the history leading to
>>> it....
>>
>> I would love this feature on the pull/fetch interface, but for a
>> completely different reason. Sometimes I want to pull a particular
>> object (usually a commit, but sometimes just a tree or blob) from
>> *myself*, and having to stick it on a branch is annoying.
>
> I am afraind that it is not going to happen; see
>
> http://article.gmane.org/gmane.comp.version-control.git/181317
>
> for a rationale.
>
Do you mean that it's a security feature? What if a .git/config
option existed to allow this use? Or even a git upload-pack option
that turned it *on* and was stripped by git-shell?
--Andy
^ permalink raw reply
* Re: [Survey] Signed push
From: Junio C Hamano @ 2011-09-14 20:40 UTC (permalink / raw)
To: Andy Lutomirski; +Cc: Linus Torvalds, git
In-Reply-To: <4E7101F3.1090204@mit.edu>
Andy Lutomirski <luto@MIT.EDU> writes:
>> An alternative that I am considering is to let the requester say this
>> instead:
>>
>> are available in the git repository at:
>> git://git.kernel.org/pub/flobar.git/ 5738c9c21e53356ab5020912116e7f82fd2d428f
>>
>> without adding the extra line.
>>
>> That is, to allow fetching the history up to an explicitly named commit
>> object. This would only involve a change to fetch-pack at the receiving
>> end; just match the commit object name given from the command line against
>> the ls-remote response and ask upload-pack to give the history leading to
>> it....
>
> I would love this feature on the pull/fetch interface, but for a
> completely different reason. Sometimes I want to pull a particular
> object (usually a commit, but sometimes just a tree or blob) from
> *myself*, and having to stick it on a branch is annoying.
I am afraind that it is not going to happen; see
http://article.gmane.org/gmane.comp.version-control.git/181317
for a rationale.
^ permalink raw reply
* Re: [PATCH 1/3] make-static: master
From: Junio C Hamano @ 2011-09-14 20:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Sixt, Ramsay Jones, GIT Mailing-list
In-Reply-To: <7vlitrt08n.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Johannes Sixt <j.sixt@viscovery.net> writes:
>
>> Am 9/14/2011 1:46, schrieb Junio C Hamano:
>>> This kind of breakage report was exactly what I was looking for by
>>> merging it early to 'next'. Hopefully no other (function / platform) combo
>>> has such dependencies...
>>
>> There is! prepare_git_cmd is used from the Windows section in run-command.c.
>> Therefore, the following hunks of the patch should be reverted.
Thanks; this fix-up will be queued on top.
-- >8 --
Subject: [PATCH] exec_cmd.c: prepare_git_cmd() is sometimes used
Add warning to prevent people from making the same mistake.
Noticed by Johannes Sixt.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
exec_cmd.c | 7 ++++++-
exec_cmd.h | 1 +
2 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/exec_cmd.c b/exec_cmd.c
index 1dddbf4..9c784db 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -113,7 +113,12 @@ void setup_path(void)
strbuf_release(&new_path);
}
-static const char **prepare_git_cmd(const char **argv)
+/*
+ * This symbol may be unreferenced from outside this file in some
+ * builds, but run-command.c on Windows does use it. Do not make it
+ * static without checking!
+ */
+const char **prepare_git_cmd(const char **argv)
{
int argc;
const char **nargv;
diff --git a/exec_cmd.h b/exec_cmd.h
index f5d2cdd..e2b546b 100644
--- a/exec_cmd.h
+++ b/exec_cmd.h
@@ -5,6 +5,7 @@ extern void git_set_argv_exec_path(const char *exec_path);
extern const char *git_extract_argv0_path(const char *path);
extern const char *git_exec_path(void);
extern void setup_path(void);
+extern const char **prepare_git_cmd(const char **argv);
extern int execv_git_cmd(const char **argv); /* NULL terminated */
extern int execl_git_cmd(const char *cmd, ...);
extern const char *system_path(const char *path);
--
1.7.7.rc1.1.g1e5814
^ permalink raw reply related
* Re: [PATCH 7/7] run_hook: use argv_array API
From: Junio C Hamano @ 2011-09-14 20:01 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Christian Couder, Jens Lehmann, git
In-Reply-To: <20110914185658.GA21352@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Wed, Sep 14, 2011 at 11:54:11AM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > @@ -609,26 +610,23 @@ int finish_async(struct async *async)
>> > int run_hook(const char *index_file, const char *name, ...)
>> > {
>> > struct child_process hook;
>> > - const char **argv = NULL, *env[2];
>> > + struct argv_array argv = ARGV_ARRAY_INIT;
>> > + const char *p, *env[2];
>>
>> Given that in argv-array.h you define it as
>>
>> #define ARGV_ARRAY_INIT { empty_argv, 0, 0 };
>>
>> the above will introduce decl-after-stmt.
>
> Oops. Can you squash in removing the semicolon from the macro
> definition?
Yes, I did. Thanks.
^ permalink raw reply
* Re: [Survey] Signed push
From: Andy Lutomirski @ 2011-09-14 19:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git, linux-kernel
In-Reply-To: <7vaaa8xufi.fsf@alter.siamese.dyndns.org>
On 09/13/2011 09:45 AM, Junio C Hamano wrote:
>
> An alternative that I am considering is to let the requester say this
> instead:
>
> are available in the git repository at:
> git://git.kernel.org/pub/flobar.git/ 5738c9c21e53356ab5020912116e7f82fd2d428f
>
> without adding the extra line.
>
> That is, to allow fetching the history up to an explicitly named commit
> object. This would only involve a change to fetch-pack at the receiving
> end; just match the commit object name given from the command line against
> the ls-remote response and ask upload-pack to give the history leading to
> it. The released versions of Git already will happily oblige, as long as
> the commit object named in the request message still sits at the tip of
> the intended branch.
I would love this feature on the pull/fetch interface, but for a
completely different reason. Sometimes I want to pull a particular
object (usually a commit, but sometimes just a tree or blob) from
*myself*, and having to stick it on a branch is annoying.
One use-case is when applying a patch in git's extended format. If I
know where it came from, I ought to be able to pull the blobs it depends
on to enable three-way merge. I think that this is essentially
impossible remotely right now.
Of course, merging with the result of the pull will result in terrible
automatically-generated messages, but it's easy to fix that up manually.
This is one thing that I think Mercurial handles better than git. (And
apologies for the noise if I've missed a way to do this with current
git. I've looked, but maybe I missed some magic way to do this.)
--Andy
^ permalink raw reply
* [PATCH 5/5] credential-store: use a better storage format
From: Jeff King @ 2011-09-14 19:21 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Thomas Rast, Brian Gernhardt
In-Reply-To: <20110914191704.GA23201@sigill.intra.peff.net>
In the name of simplicity, the credential-store helper
re-used git's config file format for on-disk storage of
credentials. The scheme looked something like this:
[credential "unique-context"]
username = foo
password = bar
This works fine if you have only one credential for each
context. But there's no good way to store separate passwords
for user1@unique-context versus user2@unique-context. For
end users, this meant that:
1. Trying to access user2@host when you already have a
cached credential for user1@host would return user2's
username with user1's password.
2. The stored result would also mix the credentials.
3. Rejection would remove all entries for the host,
regardless of username.
You can hack around it by assuming that order is important,
and that:
[credential "unique-context"]
username = user1
password = pass1
username = user2
password = pass2
refers to two distinct credentials. But the config code
isn't well setup for that. You have to keep track of the
last username field you parsed, and assume the next password
field after it is associated with it. Which isn't too bad.
But removing just one credential (which we need to do for
--reject) is impossible. You can say "delete the username
field that is user2", but there is no way to say "delete
the password field that comes right after the username field
that is user2" (it's tempting to find the password for user2
and say "delete the password field that is pass2", but
that's not strictly correct either).
Instead, let's define a new, very simple storage format.
Each line is a credential containing three tokens: unique,
username, and password. Each is separated by a space, and
shell-quoted to avoid ambiguity.
There's now an additional test that we can differentiate the credentials
correctly (matching the similar test we already have for
credential-cache). The test for removing credentials is also enhanced to
check that removing one credential leaves an unrelated one stored.
Signed-off-by: Jeff King <peff@peff.net>
---
credential-store.c | 119 ++++++++++++++++++++++++++++++++++++++++++------
t/t0300-credentials.sh | 38 ++++++++++++++-
2 files changed, 140 insertions(+), 17 deletions(-)
diff --git a/credential-store.c b/credential-store.c
index 8ab8582..aae8e0c 100644
--- a/credential-store.c
+++ b/credential-store.c
@@ -2,40 +2,129 @@
#include "credential.h"
#include "string-list.h"
#include "parse-options.h"
+#include "quote.h"
+
+static struct lock_file credential_lock;
+
+static int parse_credential_file(const char *fn,
+ struct credential *c,
+ int (*match_cb)(const char *username,
+ const char *password,
+ struct credential *c),
+ int (*other_cb)(const char *buf))
+{
+ FILE *fh;
+ struct strbuf buf = STRBUF_INIT;
+ const char **argv = NULL;
+ int alloc = 0;
+ int retval = 0;
+
+ fh = fopen(fn, "r");
+ if (!fh)
+ return errno == ENOENT ? 0 : -1;
+
+ while (strbuf_getwholeline(&buf, fh, '\n') != EOF) {
+ int nr = 0;
+ char *pristine = xstrdup(buf.buf);
+
+ strbuf_trim(&buf);
+ if (!sq_dequote_to_argv(buf.buf, &argv, &nr, &alloc) &&
+ nr == 3 &&
+ !strcmp(c->unique, argv[0]) &&
+ (!c->username || !strcmp(c->username, argv[1]))) {
+ if (match_cb(argv[1], argv[2], c) < 0) {
+ retval = -1;
+ break;
+ }
+ }
+ else if (other_cb) {
+ if (other_cb(pristine) < 0) {
+ retval = -1;
+ break;
+ }
+ }
+ free(pristine);
+ }
+
+ free(argv);
+ strbuf_release(&buf);
+ fclose(fh);
+ return retval;
+}
+
+
+static int copy_credential(const char *username, const char *password,
+ struct credential *c)
+{
+ if (!c->username)
+ c->username = xstrdup(username);
+ free(c->password);
+ c->password = xstrdup(password);
+ return 0;
+}
static int lookup_credential(const char *fn, struct credential *c)
{
- config_exclusive_filename = fn;
- credential_from_config(c);
+ if (!c->unique)
+ return 0;
+ parse_credential_file(fn, c, copy_credential, NULL);
return c->username && c->password;
}
-static void store_item(const char *fn, const char *unique,
- const char *item, const char *value)
+static int skip_match(const char *username, const char *password,
+ struct credential *c)
{
- struct strbuf key = STRBUF_INIT;
+ return 0;
+}
- if (!unique)
- return;
+static int print_entry(const char *buf)
+{
+ return write_in_full(credential_lock.fd, buf, strlen(buf));
+}
- config_exclusive_filename = fn;
+static int rewrite_credential_file(const char *fn, struct credential *c,
+ int replace)
+{
umask(077);
+ if (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)
+ return -1;
+ if (parse_credential_file(fn, c, skip_match, print_entry) < 0) {
+ rollback_lock_file(&credential_lock);
+ return -1;
+ }
+ if (replace) {
+ struct strbuf buf = STRBUF_INIT;
+ int r;
- strbuf_addf(&key, "credential.%s.%s", unique, item);
- git_config_set(key.buf, value);
- strbuf_release(&key);
+ sq_quote_buf(&buf, c->unique);
+ strbuf_addch(&buf, ' ');
+ sq_quote_buf(&buf, c->username);
+ strbuf_addch(&buf, ' ');
+ sq_quote_buf(&buf, c->password);
+ strbuf_addch(&buf, '\n');
+
+ r = write_in_full(credential_lock.fd, buf.buf, buf.len);
+ strbuf_release(&buf);
+ if (r < 0) {
+ rollback_lock_file(&credential_lock);
+ return -1;
+ }
+ }
+ return commit_lock_file(&credential_lock);
}
static void store_credential(const char *fn, struct credential *c)
{
- store_item(fn, c->unique, "username", c->username);
- store_item(fn, c->unique, "password", c->password);
+ if (!c->unique || !c->username || !c->password)
+ return;
+ rewrite_credential_file(fn, c, 1);
}
static void remove_credential(const char *fn, struct credential *c)
{
- store_item(fn, c->unique, "username", NULL);
- store_item(fn, c->unique, "password", NULL);
+ if (!c->unique)
+ return;
+ rewrite_credential_file(fn, c, 0);
}
int main(int argc, const char **argv)
diff --git a/t/t0300-credentials.sh b/t/t0300-credentials.sh
index 41ab8cc..613c3fb 100755
--- a/t/t0300-credentials.sh
+++ b/t/t0300-credentials.sh
@@ -314,6 +314,30 @@ test_expect_success 'credential-store requires matching unique token' '
EOF
'
+test_expect_success 'credential-store requires matching usernames' '
+ test_when_finished "rm -f .git-credentials" &&
+ check --unique=host store <<-\EOF &&
+ username=askpass-username
+ password=askpass-password
+ --
+ askpass: Username:
+ askpass: Password:
+ EOF
+ test_when_finished "rm -f askpass-password" &&
+ echo other-password >askpass-password &&
+ check --unique=host --username=other store <<-\EOF &&
+ username=other
+ password=other-password
+ --
+ askpass: Password:
+ EOF
+ check --unique=host --username=askpass-username store <<-\EOF
+ username=askpass-username
+ password=askpass-password
+ --
+ EOF
+'
+
test_expect_success 'credential-store removes rejected credentials' '
test_when_finished "rm -f .git-credentials" &&
check --unique=host store <<-\EOF &&
@@ -323,16 +347,26 @@ test_expect_success 'credential-store removes rejected credentials' '
askpass: Username:
askpass: Password:
EOF
+ check --unique=host --username=other store <<-\EOF &&
+ username=other
+ password=askpass-password
+ --
+ askpass: Password:
+ EOF
check --reject --unique=host --username=askpass-username store <<-\EOF &&
--
EOF
- check --unique=host store <<-\EOF
+ check --unique=host --username=askpass-username store <<-\EOF &&
username=askpass-username
password=askpass-password
--
- askpass: Username:
askpass: Password:
EOF
+ check --unique=host --username=other store <<-\EOF
+ username=other
+ password=askpass-password
+ --
+ EOF
'
test_done
--
1.7.6.252.ge9c18
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox