| Message ID | 20200427125915.88667-1-carenas@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] git-credential-store: skip empty lines and comments from store | expand |
Hi Carlo, On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote: > with the added checks for invalid URLs in credentials, any locally > modified store files which might have empty lines or even comments > were reported[1] failing to parse as valid credentials. > > using the store file in this manner wasn't intended by the original > code and it had latent issues which the new code dutifully prevented > but since the strings used wouldn't had been valid credentials anyway > we could instead detect them and skip the matching logic and therefore > formalize this "feature". > > trim all lines as they are being read from the store file and skip the Does trimming affect any credentials that may have spaces either end? I've not looked at the format. > ones that will be otherwise empty or that start with "#" (therefore > assuming them to be comments) > > [1] https://stackoverflow.com/a/61420852/5005936 > > Reported-by: Dirk <dirk@ed4u.de> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > v3: > * avoid using q_to_cr as suggested by Peff > * a more verbose commit message and slightly more complete documentation > v2: > * use a here-doc for clarity as suggested by Eric > * improve commit message and include documentation > > Documentation/git-credential-store.txt | 7 +++++++ > credential-store.c | 3 +++ > t/t0302-credential-store.sh | 15 +++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 693dd9d9d7..48ab4b13e5 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned > to Git. See the discussion of configuration in linkgit:gitcredentials[7] > for more information. > > +Note that the file used is not a configuration file and should be ideally > +managed only through git, as any manually introduced typos will compromise > +the validation of credentials. > + > +The parser will ignore any lines starting with the '#' character during > +the processing of credentials for read, though. Should the trimming of leading/trailing spaces be mentioned? Also the git-credential page mentions that the credential must end with a blank line ("don't forget.."). Should that be mentioned here, or have I misunderstood? > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/credential-store.c b/credential-store.c > index c010497cb2..b2f160890d 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, > } > > while (strbuf_getline_lf(&line, fh) != EOF) { > + strbuf_trim(&line); > + if (line.len == 0 || *line.buf == '#') > + continue; > credential_from_url(&entry, line.buf); > if (entry.username && entry.password && > credential_match(c, &entry)) { > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index d6b54e8c65..5e6ace3a06 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi > test_must_be_empty "$HOME/.config/git/credentials" > ' > > +test_expect_success 'get: allow for empty lines or comments in store file' ' > + test_write_lines "#comment" " " "" \ > + https://user:pass@example.com >"$HOME/.git-credentials" && > + check fill store <<-\EOF > + protocol=https > + host=example.com > + -- > + protocol=https > + host=example.com > + username=user > + password=pass > + -- > + EOF > +' > + > test_done Philip
Thank you very much. That's correct and ideal in my eyes. Regarding the comments, this is a new feature, of course. I think it's a worthless discussion about the question if the correct handling of empty lines are a bugfix or a new feature. In fact empty lines were handled correcty (ignored). So this might be considered a feature. But it wasn't documented, so it's a bugfix... Anyway. Thank you all. Dirk Am 27.04.20 um 14:59 schrieb Carlo Marcelo Arenas Belón: > with the added checks for invalid URLs in credentials, any locally > modified store files which might have empty lines or even comments > were reported[1] failing to parse as valid credentials. > > using the store file in this manner wasn't intended by the original > code and it had latent issues which the new code dutifully prevented > but since the strings used wouldn't had been valid credentials anyway > we could instead detect them and skip the matching logic and therefore > formalize this "feature". > > trim all lines as they are being read from the store file and skip the > ones that will be otherwise empty or that start with "#" (therefore > assuming them to be comments) > > [1] https://stackoverflow.com/a/61420852/5005936 > > Reported-by: Dirk <dirk@ed4u.de> > Helped-by: Eric Sunshine <sunshine@sunshineco.com> > Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> > --- > v3: > * avoid using q_to_cr as suggested by Peff > * a more verbose commit message and slightly more complete documentation > v2: > * use a here-doc for clarity as suggested by Eric > * improve commit message and include documentation > > Documentation/git-credential-store.txt | 7 +++++++ > credential-store.c | 3 +++ > t/t0302-credential-store.sh | 15 +++++++++++++++ > 3 files changed, 25 insertions(+) > > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 693dd9d9d7..48ab4b13e5 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned > to Git. See the discussion of configuration in linkgit:gitcredentials[7] > for more information. > > +Note that the file used is not a configuration file and should be ideally > +managed only through git, as any manually introduced typos will compromise > +the validation of credentials. > + > +The parser will ignore any lines starting with the '#' character during > +the processing of credentials for read, though. > + > GIT > --- > Part of the linkgit:git[1] suite > diff --git a/credential-store.c b/credential-store.c > index c010497cb2..b2f160890d 100644 > --- a/credential-store.c > +++ b/credential-store.c > @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, > } > > while (strbuf_getline_lf(&line, fh) != EOF) { > + strbuf_trim(&line); > + if (line.len == 0 || *line.buf == '#') > + continue; > credential_from_url(&entry, line.buf); > if (entry.username && entry.password && > credential_match(c, &entry)) { > diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh > index d6b54e8c65..5e6ace3a06 100755 > --- a/t/t0302-credential-store.sh > +++ b/t/t0302-credential-store.sh > @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi > test_must_be_empty "$HOME/.config/git/credentials" > ' > > +test_expect_success 'get: allow for empty lines or comments in store file' ' > + test_write_lines "#comment" " " "" \ > + https://user:pass@example.com >"$HOME/.git-credentials" && > + check fill store <<-\EOF > + protocol=https > + host=example.com > + -- > + protocol=https > + host=example.com > + username=user > + password=pass > + -- > + EOF > +' > + > test_done
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: > with the added checks for invalid URLs in credentials, any locally s/with/With/ > modified store files which might have empty lines or even comments > were reported[1] failing to parse as valid credentials. These files are not supposed to be viewed or edited without the help of the credential helpers. Do these blank lines and comments even survive when a new credential is approved, or do we just overwrite and lose them? I'd rather not to do either, if we did not have to, but if it were necessary for us to do something, I am OK to ignore empty lines. But I'd prefer not to mix the new "# comment" feature in, if we did not have to. Also, triming the lines that are not empty is unwarranted. IIUC, what the "store" action writes encodes whitespaces, so as soon as you see whitespace on either end, (or anywhere on the line for that matter), it is a hand-edited cruft in the file. If you ignore comments, you probably should ignore those lines, too.
On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote: > > modified store files which might have empty lines or even comments > > were reported[1] failing to parse as valid credentials. > > These files are not supposed to be viewed or edited without the help > of the credential helpers. Do these blank lines and comments even > survive when a new credential is approved, or do we just overwrite > and lose them? That's a good question. In the older code we do save them, because credential-store passes through lines which don't match the credential we're operating on. But in Carlo's patch, the immediate "continue" means we wouldn't ever call "other_cb", which is what does that pass-through. > I'd rather not to do either, if we did not have to, but if it were > necessary for us to do something, I am OK to ignore empty lines. > But I'd prefer not to mix the new "# comment" feature in, if we did > not have to. > > Also, triming the lines that are not empty is unwarranted. IIUC, > what the "store" action writes encodes whitespaces, so as soon as > you see whitespace on either end, (or anywhere on the line for that > matter), it is a hand-edited cruft in the file. If you ignore > comments, you probably should ignore those lines, too. Yeah, all of that seems quite sensible. -Peff
Jeff King <peff@peff.net> writes: > On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote: > >> > modified store files which might have empty lines or even comments >> > were reported[1] failing to parse as valid credentials. >> >> These files are not supposed to be viewed or edited without the help >> of the credential helpers. Do these blank lines and comments even >> survive when a new credential is approved, or do we just overwrite >> and lose them? > > That's a good question. In the older code we do save them, because > credential-store passes through lines which don't match the credential > we're operating on. > > But in Carlo's patch, the immediate "continue" means we wouldn't ever > call "other_cb", which is what does that pass-through. So, does that mean the patch that started this thread will still help users who wrote custom comments and blank lines in their credential store by letting git-credential-store start up, but leaves a ticking bomb for them to lose these precious comments and blanks once they add a new site, change password, etc., at which point the user realizes that comments and blanks are not supported after all? >> I'd rather not to do either, if we did not have to, but if it were >> necessary for us to do something, I am OK to ignore empty lines. >> But I'd prefer not to mix the new "# comment" feature in, if we did >> not have to. >> >> Also, triming the lines that are not empty is unwarranted. IIUC, >> what the "store" action writes encodes whitespaces, so as soon as >> you see whitespace on either end, (or anywhere on the line for that >> matter), it is a hand-edited cruft in the file. If you ignore >> comments, you probably should ignore those lines, too. > > Yeah, all of that seems quite sensible. I think the first patch we need is a (belated) documentation patch, that adds to the existing "STORAGE FORMAT". We already say "Each credential is stored on its own line as a URL", but we do not say anything about allowing other cruft in the file. We probably should. Adding a "comment" feature, if anybody feels like it, is OK and we can loosen the paragraph when that happens. -- >8 -- Subject: credential-store: document the file format a bit more Reading a malformed credential URL line and silently ignoring it does not mean that we promise to torelate and/or keep empty lines and "# commented" lines forever. Some people seem to take anything that is not explicitly forbidden as allowed, but the world does not work that way. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- Documentation/git-credential-store.txt | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 693dd9d9d7..76b0798856 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -94,6 +94,10 @@ stored on its own line as a URL like: https://user:pass@example.com ------------------------------ +No other kinds of lines (e.g. empty lines or comment lines) are +allowed in the file, even though some may be silently ignored. Do +not view or edit the file with editors. + When Git needs authentication for a particular URL context, credential-store will consider that context a pattern to match against each entry in the credentials file. If the protocol, hostname, and
On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote: > >> These files are not supposed to be viewed or edited without the help > >> of the credential helpers. Do these blank lines and comments even > >> survive when a new credential is approved, or do we just overwrite > >> and lose them? > > > > That's a good question. In the older code we do save them, because > > credential-store passes through lines which don't match the credential > > we're operating on. > > > > But in Carlo's patch, the immediate "continue" means we wouldn't ever > > call "other_cb", which is what does that pass-through. > > So, does that mean the patch that started this thread will still help > users who wrote custom comments and blank lines in their credential > store by letting git-credential-store start up, but leaves a ticking > bomb for them to lose these precious comments and blanks once they > add a new site, change password, etc., at which point the user realizes > that comments and blanks are not supported after all? Yes, exactly. If I start with: cat >creds <<\EOF # this is a comment https://user:pass@example.com/ EOF then: echo url=https://other:pass@other.example.com | git credential-store --file=creds store with v2.26.0 results in: https://other:pass@other.example.com # this is a comment https://user:pass@example.com but applying the patch on top: https://other:pass@other.example.com https://user:pass@example.com/ That raises another issue about comments, too: we make no promises about where we write new entries. They could be inserted between a comment line and one it is meant to annotate (that line could even be moved if we reject and then re-approve a credential). > I think the first patch we need is a (belated) documentation patch, > that adds to the existing "STORAGE FORMAT". We already say "Each > credential is stored on its own line as a URL", but we do not say > anything about allowing other cruft in the file. We probably > should. Adding a "comment" feature, if anybody feels like it, is OK > and we can loosen the paragraph when that happens. The more I look into it, the more negative I am on adding such a comment feature. > -- >8 -- > Subject: credential-store: document the file format a bit more Yeah, this certainly clarifies things. I suppose one could ask why we'd bother documenting the format at all, then, though I do think it could be helpful for debugging. > Reading a malformed credential URL line and silently ignoring it > does not mean that we promise to torelate and/or keep empty lines > and "# commented" lines forever. s/torelate/tolerate/ -Peff
On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote: > Jeff King <peff@peff.net> writes: > > > On Mon, Apr 27, 2020 at 11:09:34AM -0700, Junio C Hamano wrote: > > > >> > modified store files which might have empty lines or even comments > >> > were reported[1] failing to parse as valid credentials. > >> > >> These files are not supposed to be viewed or edited without the help > >> of the credential helpers. Do these blank lines and comments even > >> survive when a new credential is approved, or do we just overwrite > >> and lose them? > > > > That's a good question. In the older code we do save them, because > > credential-store passes through lines which don't match the credential > > we're operating on. > > > > But in Carlo's patch, the immediate "continue" means we wouldn't ever > > call "other_cb", which is what does that pass-through. > > So, does that mean the patch that started this thread will still help > users who wrote custom comments and blank lines in their credential > store by letting git-credential-store start up, but leaves a ticking > bomb for them to lose these precious comments and blanks once they > add a new site, change password, etc., at which point the user realizes > that comments and blanks are not supported after all? yes, and it also helps users that might have added spaces or tabs around their credentials while editing them to still be able to use those instead of just failing to match them. IMHO the only "regression" I was fixing was the fact that the current code will get to throw a fatal error with an unhelpful message and prevent access to valid credentials as shown by : $ /git credential fill protocol=http host=example.com warning: url has no scheme: fatal: credential url cannot be parsed: Username for 'http://example.com': > >> I'd rather not to do either, if we did not have to, but if it were > >> necessary for us to do something, I am OK to ignore empty lines. > >> But I'd prefer not to mix the new "# comment" feature in, if we did > >> not have to. > >> > >> Also, triming the lines that are not empty is unwarranted. IIUC, > >> what the "store" action writes encodes whitespaces, so as soon as > >> you see whitespace on either end, (or anywhere on the line for that > >> matter), it is a hand-edited cruft in the file. If you ignore > >> comments, you probably should ignore those lines, too. > > > > Yeah, all of that seems quite sensible. > > I think the first patch we need is a (belated) documentation patch, > that adds to the existing "STORAGE FORMAT". We already say "Each > credential is stored on its own line as a URL", but we do not say > anything about allowing other cruft in the file. We probably > should. Adding a "comment" feature, if anybody feels like it, is OK > and we can loosen the paragraph when that happens. > > -- >8 -- > Subject: credential-store: document the file format a bit more > > Reading a malformed credential URL line and silently ignoring it > does not mean that we promise to torelate and/or keep empty lines > and "# commented" lines forever. > > Some people seem to take anything that is not explicitly forbidden > as allowed, but the world does not work that way. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Documentation/git-credential-store.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 693dd9d9d7..76b0798856 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -94,6 +94,10 @@ stored on its own line as a URL like: > https://user:pass@example.com > ------------------------------ > > +No other kinds of lines (e.g. empty lines or comment lines) are > +allowed in the file, even though some may be silently ignored. Do > +not view or edit the file with editors. view should be ok; mentioning that any typos or extraneous characters will compromise the validation of credentials like I mentioned in my proposed documentation update probably worth doing here instead, too. Carlo
On Mon, Apr 27, 2020 at 05:10:13PM -0400, Jeff King wrote: > On Mon, Apr 27, 2020 at 01:43:38PM -0700, Junio C Hamano wrote: > > >> These files are not supposed to be viewed or edited without the help > > >> of the credential helpers. Do these blank lines and comments even > > >> survive when a new credential is approved, or do we just overwrite > > >> and lose them? > > > > > > That's a good question. In the older code we do save them, because > > > credential-store passes through lines which don't match the credential > > > we're operating on. > > > > > > But in Carlo's patch, the immediate "continue" means we wouldn't ever > > > call "other_cb", which is what does that pass-through. > > > > So, does that mean the patch that started this thread will still help > > users who wrote custom comments and blank lines in their credential > > store by letting git-credential-store start up, but leaves a ticking > > bomb for them to lose these precious comments and blanks once they > > add a new site, change password, etc., at which point the user realizes > > that comments and blanks are not supported after all? > > The more I look into it, the more negative I am on adding such a comment > feature. FWIW I am not interested on implementing a comment feature (regardless of what the subject says, and indeed was going to use quotes around it in my original patch), but it was already too long ;) my objective was to allow people that were not upgrading because of this "regression" a path forward, but after all this discussion I am convinced too that my approach is misguided. if users are already manually editing this file, they should be able to fix the corrupted lines themselves but the error shown needs to be improved. and the failure mode corrected as well so they are not blocked. this also means that my "sneaky" way to fix formatting issues is gone and the passthrough is called so nothing will be lost, but as you also said the location is not warranted to be kept so this comment "feature" is as useless as intended. would something like the following (wouldn't pass the last test yet as it will need special handling to be able to handle the full path of the broken file as shown in the warning sent to stderr) on top of Junio's make more sense? it still "silently" fixes the formatting issues where a leading tab (test added) or spaces were added to easy the transition, but probably should also error out as Junio suggested originally. had also updated the commit message and what is left of the documentation. Carlo -- >8 -- Subject: [RFC PATCH v4] git-credential-store: skip empty lines and comments from store with the added checks for invalid URLs in credentials, any locally modified store files which might have empty lines or even comments were reported[1] failing to parse as valid credentials. using the store file in this manner wasn't intended by the original code and it had latent issues which the new code dutifully prevented but since the strings used wouldn't had been valid credentials anyway we could instead detect them and skip the matching logic and therefore prevent a fatal failure. trim all lines as they are being read from the store file and skip the ones that will be otherwise empty or that start with "#" (therefore assuming them to be comments) but warn users so they will be make aware of the issues introduced by their manual editing. [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- Documentation/git-credential-store.txt | 4 ++++ credential-store.c | 7 ++++++ t/t0302-credential-store.sh | 31 ++++++++++++++++++++++++++ 3 files changed, 42 insertions(+) diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 693dd9d9d7..e9a1993021 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -101,6 +101,10 @@ username (if we already have one) match, then the password is returned to Git. See the discussion of configuration in linkgit:gitcredentials[7] for more information. +The parser will ignore any lines that are empty or starting with '#' character +during the processing of credentials for read, but warn the user so they can +be corrected. + GIT --- Part of the linkgit:git[1] suite diff --git a/credential-store.c b/credential-store.c index c010497cb2..f1f016fec1 100644 --- a/credential-store.c +++ b/credential-store.c @@ -24,6 +24,13 @@ static int parse_credential_file(const char *fn, } while (strbuf_getline_lf(&line, fh) != EOF) { + strbuf_trim(&line); + if (line.len == 0 || *line.buf == '#') { + warning("ignoring corrupted credential entry \"%s\" in %s", line.buf, fn); + if (other_cb) + other_cb(&line); + continue; + } credential_from_url(&entry, line.buf); if (entry.username && entry.password && credential_match(c, &entry)) { diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..12a158c136 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,35 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' +test_expect_success 'get: attempt to workaround formatting isssues (ex: tab)' ' + q_to_tab >"$HOME/.git-credentials" <<-\EOF && + Qhttps://user:pass@example.com + EOF + check fill store <<-\EOF + protocol=https + host=example.com + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + +test_expect_success 'get: skip empty lines or "comments" in store file' ' + test_write_lines "#comment" " " "" \ + https://user:pass@example.com >"$HOME/.git-credentials" && + check fill store <<-\EOF + protocol=https + host=example.com + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + test_done
On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote: > On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote: > > with the added checks for invalid URLs in credentials, any locally > > modified store files which might have empty lines or even comments > > were reported[1] failing to parse as valid credentials. > > > > using the store file in this manner wasn't intended by the original > > code and it had latent issues which the new code dutifully prevented > > but since the strings used wouldn't had been valid credentials anyway > > we could instead detect them and skip the matching logic and therefore > > formalize this "feature". > > > > trim all lines as they are being read from the store file and skip the > > Does trimming affect any credentials that may have spaces either end? all credentials are url encoded so the only spaces that are affected are the ones that were added by careless editing of that file. as Eric pointed out, any tabs will be silently "cleaned" as well. > Should the trimming of leading/trailing spaces be mentioned? I wanted to keep that as an undocumented "feature" as it is just meant to help people to avoid a fatal error during the transition but didn't want to encourage people thinking it is a supported part or even worse encourage people to start editing their files to add tabs and spaces. Junio's suggested documentation fix[1] makes that clearer that I could > Also the git-credential page mentions that the credential must end with > a blank line ("don't forget.."). Should that be mentioned here, or have > I misunderstood? that is for the protocol part of it (which is used between git and the credential helper), the file format for this specific helper doesn't require that. Carlo [1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/
Junio C Hamano wrote: > -- >8 -- > Subject: credential-store: document the file format a bit more > > Reading a malformed credential URL line and silently ignoring it > does not mean that we promise to torelate and/or keep empty lines > and "# commented" lines forever. > > Some people seem to take anything that is not explicitly forbidden > as allowed, but the world does not work that way. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > Documentation/git-credential-store.txt | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt > index 693dd9d9d7..76b0798856 100644 > --- a/Documentation/git-credential-store.txt > +++ b/Documentation/git-credential-store.txt > @@ -94,6 +94,10 @@ stored on its own line as a URL like: > https://user:pass@example.com > ------------------------------ > > +No other kinds of lines (e.g. empty lines or comment lines) are > +allowed in the file, even though some may be silently ignored. Do > +not view or edit the file with editors. > + > When Git needs authentication for a particular URL context, I like this. I do suspect this is easy to run into accidentally. In $DAYJOB (in the context of [1]) there was a service that accidentally wrote a \n\n at the end of a line that was used by git-credential-store. Once the cause was tracked down, it was straightforward to fix, but I don't like the idea that others in a similar position may end up tempted to just not upgrade Git. Independently, there is the thread we are replying to. Independently, in Debian's bug tracking system, Stefan (cc-ed) reports[2]: | the vulnerability in CVE-2020-11008 is related to the handling | of credential helpers in git. In Buster this has been fixed in | 1:2.20.1-2+deb10u3. This broke my existing configuration where | repositories have credential.helper=store set. This is | documented in /usr/share/man/man1/git-credential-store.1.gz | and other files from git, git-doc etc. | I am unsure how to proceed... is this helper now unsupported? (Stefan, do you have more details? Did you manually populate your credential store? What error message do you get?) I wonder if in addition to the above documentation change we may want something guaranteed to catch all cases where people would have experienced a regression, like diff --git i/credential-store.c w/credential-store.c index c010497cb21..294e7716815 100644 --- i/credential-store.c +++ w/credential-store.c @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn, } while (strbuf_getline_lf(&line, fh) != EOF) { - credential_from_url(&entry, line.buf); - if (entry.username && entry.password && + if (!credential_from_url_gently(&entry, line.buf, 1) && + entry.username && entry.password && credential_match(c, &entry)) { found_credential = 1; if (match_cb) { And then we can tighten the handling of unrecognized lines to first warn and then error out, as a controlled change that doesn't lead people to regret updating git. Thoughts? Thanks, Jonathan [1] https://cs.opensource.google/copybara/copybara/+/master:java/com/google/copybara/git/GitOptions.java;drc=bc79a0b1ffe18f79dea0b75ba3a24b641a50a9fc;l=46 [2] https://bugs.debian.org/958929
On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote: > I wonder if in addition to the above documentation change we may want > something guaranteed to catch all cases where people would have > experienced a regression, like > > diff --git i/credential-store.c w/credential-store.c > index c010497cb21..294e7716815 100644 > --- i/credential-store.c > +++ w/credential-store.c > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn, > } > > while (strbuf_getline_lf(&line, fh) != EOF) { > - credential_from_url(&entry, line.buf); > - if (entry.username && entry.password && > + if (!credential_from_url_gently(&entry, line.buf, 1) && > + entry.username && entry.password && > credential_match(c, &entry)) { > found_credential = 1; > if (match_cb) { > > And then we can tighten the handling of unrecognized lines to first > warn and then error out, as a controlled change that doesn't lead > people to regret updating git. I like that solution, as it mostly brings us back to the original behavior, as weird or unexpected as it was. The only I think would be different is: ://user:pass@example.com which I think the old code would have treated as matching any protocol (and the new code will reject as a bogus URL). I wondered if you'd need to free the partially-parsed struct fields on failure, but the answer is no; we'd clear it the next time through the loop, or at the very end of the function. -Peff
On Tue, Apr 28, 2020 at 01:41:55AM -0400, Jeff King wrote: > On Mon, Apr 27, 2020 at 10:25:10PM -0700, Jonathan Nieder wrote: > > > I wonder if in addition to the above documentation change we may want > > something guaranteed to catch all cases where people would have > > experienced a regression, like > > > > diff --git i/credential-store.c w/credential-store.c > > index c010497cb21..294e7716815 100644 > > --- i/credential-store.c > > +++ w/credential-store.c > > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn, > > } > > > > while (strbuf_getline_lf(&line, fh) != EOF) { > > - credential_from_url(&entry, line.buf); > > - if (entry.username && entry.password && > > + if (!credential_from_url_gently(&entry, line.buf, 1) && > > + entry.username && entry.password && > > credential_match(c, &entry)) { > > found_credential = 1; > > if (match_cb) { > > > > And then we can tighten the handling of unrecognized lines to first > > warn and then error out, as a controlled change that doesn't lead > > people to regret updating git. > > I like that solution, as it mostly brings us back to the original > behavior, as weird or unexpected as it was. I like this version better as well, and we could even reuse my test case. it wouldn't cover cases where there were leading spaces/tabs around the credential which I have to admit I liked just because it is more robust to bad input, and there is no sane way now to tell the user that there is invalid data anyway, but I am ok eitherway. Carlo
On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote: > > > while (strbuf_getline_lf(&line, fh) != EOF) { > > > - credential_from_url(&entry, line.buf); > > > - if (entry.username && entry.password && > > > + if (!credential_from_url_gently(&entry, line.buf, 1) && > > > + entry.username && entry.password && > > > credential_match(c, &entry)) { > > > found_credential = 1; > > > if (match_cb) { > > > > > > And then we can tighten the handling of unrecognized lines to first > > > warn and then error out, as a controlled change that doesn't lead > > > people to regret updating git. > > > > I like that solution, as it mostly brings us back to the original > > behavior, as weird or unexpected as it was. > > I like this version better as well, and we could even reuse my test > case. > > it wouldn't cover cases where there were leading spaces/tabs around > the credential which I have to admit I liked just because it is > more robust to bad input, and there is no sane way now to tell the > user that there is invalid data anyway, but I am ok eitherway. I think if we're discouraging people from hand-editing the file, then that feature would be going in the wrong direction anyway. Did you or Jonathan want to wrap it up with the test and commit message? -Peff
On Mon, 27 Apr 2020 22:25:10 -0700 Jonathan Nieder <jrnieder@gmail.com> wrote: > Independently, in Debian's bug tracking system, Stefan (cc-ed) > reports[2]: > > | the vulnerability in CVE-2020-11008 is related to the handling > | of credential helpers in git. In Buster this has been fixed in > | 1:2.20.1-2+deb10u3. This broke my existing configuration where > | repositories have credential.helper=store set. This is > | documented in /usr/share/man/man1/git-credential-store.1.gz > | and other files from git, git-doc etc. > | I am unsure how to proceed... is this helper now unsupported? > > (Stefan, do you have more details? Did you manually populate your > credential store? What error message do you get?) I can't remember for sure - it's literally years ago - but I am pretty sure I did not *populate* it manually initially. However, I might have edited it to separate entries (https vs. smtp) and I might have added additional entries by c&p & editing. I have replaced the one instance of \n\n used for separation with a normal single line break between entries and how it works fine. Before that I got the following output (that's obviously not very helpful if you don't know the whole story :) > git pull > warning: url has no scheme: > fatal: credential url cannot be parsed: > Already up to date. > Current branch master is up to date.
On Tue, Apr 28, 2020 at 04:16:27AM -0400, Jeff King wrote: > On Tue, Apr 28, 2020 at 12:18:02AM -0700, Carlo Marcelo Arenas Belón wrote: > > > > it wouldn't cover cases where there were leading spaces/tabs around > > the credential which I have to admit I liked just because it is > > more robust to bad input, and there is no sane way now to tell the > > user that there is invalid data anyway, but I am ok eitherway. > > I think if we're discouraging people from hand-editing the file, then > that feature would be going in the wrong direction anyway. I think it will actually encourage them to edit the file (and complain of a regression), since they can see the credential is correct, but somehow git fails to work with it unlike their previous version. remember there is no warning that could explain why that happens and with this pach there is no error either. > Did you or Jonathan want to wrap it up with the test and commit message? I was going to say Jonathan will have to do it, since he was the only one that didn't provide a patch (and therefore a sign off) but guess I can save everyone some time and practice my git-send-email skills (which obviously failed me) Jonathan, make sure your patch[1] is complete and correct and to resend it with a sign off, feel free to add me as Tested-by if you feel like, but I added myself as Helped-by anyway ;) Junio, didn't fix the typo that Peff suggested fixed[2], because I forgot and I wasn't sure if that also means I should SOB it, but if you could also consider my suggestion[3] then at you get my SOB warranted ;) Carlo PS. guess I should had send it as an RFC instead [1] https://lore.kernel.org/git/20200428105254.28658-3-carenas@gmail.com/ [2] https://lore.kernel.org/git/20200427211013.GB1732530@coredump.intra.peff.net/ [3] https://lore.kernel.org/git/20200427234909.GC61348@Carlos-MBP/
Jonathan Nieder <jrnieder@gmail.com> writes: > I wonder if in addition to the above documentation change we may want > something guaranteed to catch all cases where people would have > experienced a regression, like > > diff --git i/credential-store.c w/credential-store.c > index c010497cb21..294e7716815 100644 > --- i/credential-store.c > +++ w/credential-store.c > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn, > } > > while (strbuf_getline_lf(&line, fh) != EOF) { > - credential_from_url(&entry, line.buf); > - if (entry.username && entry.password && > + if (!credential_from_url_gently(&entry, line.buf, 1) && > + entry.username && entry.password && > credential_match(c, &entry)) { > found_credential = 1; > if (match_cb) { Hmph, so the idea is, instead of ignoring the potential error detected by credential_from_url() and using credential when it is available, we loudly attempt to parse and give warning on malformed entries when we discard a line? I think that is an excellent idea. It would be nicer if we can somehow add where we found the offending line, e.g. /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path Do we feel it a bit disturbing that u:p is shown in the error message, without redacting, by the way?
On Tue, Apr 28, 2020 at 09:03:36AM -0700, Junio C Hamano wrote: > Jonathan Nieder <jrnieder@gmail.com> writes: > > > I wonder if in addition to the above documentation change we may want > > something guaranteed to catch all cases where people would have > > experienced a regression, like > > > > diff --git i/credential-store.c w/credential-store.c > > index c010497cb21..294e7716815 100644 > > --- i/credential-store.c > > +++ w/credential-store.c > > @@ -24,8 +24,8 @@ static int parse_credential_file(const char *fn, > > } > > > > while (strbuf_getline_lf(&line, fh) != EOF) { > > - credential_from_url(&entry, line.buf); > > - if (entry.username && entry.password && > > + if (!credential_from_url_gently(&entry, line.buf, 1) && > > + entry.username && entry.password && > > credential_match(c, &entry)) { > > found_credential = 1; > > if (match_cb) { > > Hmph, so the idea is, instead of ignoring the potential error > detected by credential_from_url() and using credential when it is > available, we loudly attempt to parse and give warning on malformed > entries when we discard a line? the idea was to silently ignore the line (notice quiet = 1), which is the "original behaviour" as Peff pointed out in his reply in 20200428054155.GB2376380@coredump.intra.peff.net[1] > I think that is an excellent idea. > > It would be nicer if we can somehow add where we found the offending > line, e.g. > > /home/me/.gitcredential:396: url has no scheme: ://u:p@host/path > > Do we feel it a bit disturbing that u:p is shown in the error > message, without redacting, by the way? we could do so with the following on top as a 5/4. most of the test changes would go away in a reroll, and are similar to what would have been needed for my original suggested patch[2] with the exception that in this series we won't support silently fixing credentials which is something you mentioned a preference on. Carlo [1] https://lore.kernel.org/git/20200428054155.GB2376380@coredump.intra.peff.net/ [2] https://lore.kernel.org/git/20200428013726.GD61348@Carlos-MBP/ -- >8 -- Subject: [RFC PATCH] credential-store: SQUASH!! add warnings Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- credential-store.c | 55 +++++++++++++++++++++++++++++++------ t/t0302-credential-store.sh | 42 ++++++++++++++++------------ 2 files changed, 70 insertions(+), 27 deletions(-) diff --git a/credential-store.c b/credential-store.c index 294e771681..f76292df20 100644 --- a/credential-store.c +++ b/credential-store.c @@ -6,6 +6,32 @@ static struct lock_file credential_lock; +static char *redact_credential(const struct strbuf *line) +{ + struct strbuf redacted_line = STRBUF_INIT; + char *at = strchr(line->buf, '@'); + char *colon; + int redacted = 0; + + if (at) { + strbuf_addf(&redacted_line, "%.*s", + (int)(at - line->buf), line->buf); + colon = strrchr(redacted_line.buf, ':'); + if (colon && *(colon + 1) != '/' && colon > redacted_line.buf) { + redacted_line.len = colon - redacted_line.buf + 1; + strbuf_addstr(&redacted_line, "<redacted>"); + strbuf_addstr(&redacted_line, at); + redacted = 1; + } + else + strbuf_reset(&redacted_line); + } + if (!redacted) + strbuf_addbuf(&redacted_line, line); + + return redacted_line.buf; +} + static int parse_credential_file(const char *fn, struct credential *c, void (*match_cb)(struct credential *), @@ -15,6 +41,7 @@ static int parse_credential_file(const char *fn, struct strbuf line = STRBUF_INIT; struct credential entry = CREDENTIAL_INIT; int found_credential = 0; + int lineno = 0; fh = fopen(fn, "r"); if (!fh) { @@ -24,17 +51,27 @@ static int parse_credential_file(const char *fn, } while (strbuf_getline_lf(&line, fh) != EOF) { - if (!credential_from_url_gently(&entry, line.buf, 1) && - entry.username && entry.password && - credential_match(c, &entry)) { - found_credential = 1; - if (match_cb) { - match_cb(&entry); - break; + lineno++; + if (!credential_from_url_gently(&entry, line.buf, 1)) { + if (entry.username && entry.password && + credential_match(c, &entry)) { + found_credential = 1; + if (match_cb) { + match_cb(&entry); + break; + } } + else if (other_cb) + other_cb(&line); + } + else { + if (other_cb) + other_cb(&line); + char *redacted = redact_credential(&line); + warning("%s:%d ignoring invalid credential: %s", + fn, lineno, redacted); + free(redacted); } - else if (other_cb) - other_cb(&line); } credential_clear(&entry); diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index 7c7a48303f..597a75903a 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,36 +120,42 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' -test_expect_success 'get: store file can contain empty/bogus lines' ' - test_write_lines "#comment" " " "" \ - https://user:pass@example.com >"$HOME/.git-credentials" && - check fill store <<-\EOF +test_expect_success 'get: credentials without scheme are invalid' ' + echo "://user:pass@example.com" >"$HOME/.git-credentials" && + cat >expect-stdout <<-\STDOUT && protocol=https host=example.com - -- + username=askpass-username + password=askpass-password + STDOUT + test_config credential.helper store && + git credential fill <<-\EOF >stdout 2>stderr && protocol=https host=example.com - username=user - password=pass - -- EOF + test_cmp expect-stdout stdout && + grep "askpass: Username for '\''https://example.com'\'':" stderr && + grep "askpass: Password for '\''https://askpass-username@example.com'\'':" stderr && + test_i18ngrep "ignoring invalid credential" stderr ' -test_expect_success 'get: ignore credentials without scheme' ' - echo "://user:pass@example.com" >"$HOME/.git-credentials" && - check fill store <<-\EOF +test_expect_success 'get: store file can contain empty/bogus lines' ' + test_write_lines "#comment" " " "" \ + https://user:pass@example.com >"$HOME/.git-credentials" && + cat >expect-stdout <<-\STDOUT && protocol=https host=example.com - -- + username=user + password=pass + STDOUT + test_config credential.helper store && + git credential fill <<-\EOF >stdout 2>stderr && protocol=https host=example.com - username=askpass-username - password=askpass-password - -- - askpass: Username for '\''https://example.com'\'': - askpass: Password for '\''https://askpass-username@example.com'\'': - -- EOF + test_cmp expect-stdout stdout && + test_i18ngrep "ignoring invalid credential" stderr && + test_line_count = 3 stderr ' test_done
Carlo Marcelo Arenas Belón <carenas@gmail.com> writes: >> > + if (!credential_from_url_gently(&entry, line.buf, 1) && >> > + entry.username && entry.password && >> > credential_match(c, &entry)) { >> > found_credential = 1; >> > if (match_cb) { >> >> Hmph, so the idea is, instead of ignoring the potential error >> detected by credential_from_url() and using credential when it is >> available, we loudly attempt to parse and give warning on malformed >> entries when we discard a line? > > the idea was to silently ignore the line (notice quiet = 1), which Ah, sorry, I apparently did not read the patch carefully enough. Thanks for a correction.
Hi Carlo, Thanks for the clarification. Philip On 28/04/2020 02:49, Carlo Marcelo Arenas Belón wrote: > On Mon, Apr 27, 2020 at 02:48:05PM +0100, Philip Oakley wrote: >> On 27/04/2020 13:59, Carlo Marcelo Arenas Belón wrote: >>> with the added checks for invalid URLs in credentials, any locally >>> modified store files which might have empty lines or even comments >>> were reported[1] failing to parse as valid credentials. >>> >>> using the store file in this manner wasn't intended by the original >>> code and it had latent issues which the new code dutifully prevented >>> but since the strings used wouldn't had been valid credentials anyway >>> we could instead detect them and skip the matching logic and therefore >>> formalize this "feature". >>> >>> trim all lines as they are being read from the store file and skip the >> Does trimming affect any credentials that may have spaces either end? > all credentials are url encoded so the only spaces that are affected are > the ones that were added by careless editing of that file. > > as Eric pointed out, any tabs will be silently "cleaned" as well. > >> Should the trimming of leading/trailing spaces be mentioned? > I wanted to keep that as an undocumented "feature" as it is just meant > to help people to avoid a fatal error during the transition but didn't > want to encourage people thinking it is a supported part or even worse > encourage people to start editing their files to add tabs and spaces. > > Junio's suggested documentation fix[1] makes that clearer that I could > >> Also the git-credential page mentions that the credential must end with >> a blank line ("don't forget.."). Should that be mentioned here, or have >> I misunderstood? > that is for the protocol part of it (which is used between git and the > credential helper), the file format for this specific helper doesn't > require that. > > Carlo > > [1] https://lore.kernel.org/git/xmqqv9lk7j7p.fsf@gitster.c.googlers.com/
diff --git a/Documentation/git-credential-store.txt b/Documentation/git-credential-store.txt index 693dd9d9d7..48ab4b13e5 100644 --- a/Documentation/git-credential-store.txt +++ b/Documentation/git-credential-store.txt @@ -101,6 +101,13 @@ username (if we already have one) match, then the password is returned to Git. See the discussion of configuration in linkgit:gitcredentials[7] for more information. +Note that the file used is not a configuration file and should be ideally +managed only through git, as any manually introduced typos will compromise +the validation of credentials. + +The parser will ignore any lines starting with the '#' character during +the processing of credentials for read, though. + GIT --- Part of the linkgit:git[1] suite diff --git a/credential-store.c b/credential-store.c index c010497cb2..b2f160890d 100644 --- a/credential-store.c +++ b/credential-store.c @@ -24,6 +24,9 @@ static int parse_credential_file(const char *fn, } while (strbuf_getline_lf(&line, fh) != EOF) { + strbuf_trim(&line); + if (line.len == 0 || *line.buf == '#') + continue; credential_from_url(&entry, line.buf); if (entry.username && entry.password && credential_match(c, &entry)) { diff --git a/t/t0302-credential-store.sh b/t/t0302-credential-store.sh index d6b54e8c65..5e6ace3a06 100755 --- a/t/t0302-credential-store.sh +++ b/t/t0302-credential-store.sh @@ -120,4 +120,19 @@ test_expect_success 'erase: erase matching credentials from both xdg and home fi test_must_be_empty "$HOME/.config/git/credentials" ' +test_expect_success 'get: allow for empty lines or comments in store file' ' + test_write_lines "#comment" " " "" \ + https://user:pass@example.com >"$HOME/.git-credentials" && + check fill store <<-\EOF + protocol=https + host=example.com + -- + protocol=https + host=example.com + username=user + password=pass + -- + EOF +' + test_done
with the added checks for invalid URLs in credentials, any locally modified store files which might have empty lines or even comments were reported[1] failing to parse as valid credentials. using the store file in this manner wasn't intended by the original code and it had latent issues which the new code dutifully prevented but since the strings used wouldn't had been valid credentials anyway we could instead detect them and skip the matching logic and therefore formalize this "feature". trim all lines as they are being read from the store file and skip the ones that will be otherwise empty or that start with "#" (therefore assuming them to be comments) [1] https://stackoverflow.com/a/61420852/5005936 Reported-by: Dirk <dirk@ed4u.de> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> --- v3: * avoid using q_to_cr as suggested by Peff * a more verbose commit message and slightly more complete documentation v2: * use a here-doc for clarity as suggested by Eric * improve commit message and include documentation Documentation/git-credential-store.txt | 7 +++++++ credential-store.c | 3 +++ t/t0302-credential-store.sh | 15 +++++++++++++++ 3 files changed, 25 insertions(+)