kernel/git - git - PowerEL Git System

Commit Graph

Author	SHA1	Message	Date
Johannes Schindelin	9a121b0d22	credential: handle `credential.<partial-URL>.<key>` again In the patches for CVE-2020-11008, the ability to specify credential settings in the config for partial URLs got lost. For example, it used to be possible to specify a credential helper for a specific protocol: [credential "https://"] helper = my-https-helper Likewise, it used to be possible to configure settings for a specific host, e.g.: [credential "dev.azure.com"] useHTTPPath = true Let's reinstate this behavior. While at it, increase the test coverage to document and verify the behavior with a couple other categories of partial URLs. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Reviewed-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
brian m. carlson	b44d0118ac	credential: fix matching URLs with multiple levels in path `46fd7b3900` ("credential: allow wildcard patterns when matching config", 2020-02-20) introduced support for matching credential helpers using urlmatch. In doing so, it introduced code to percent-encode the paths we get from the credential helper so that they could be effectively matched by the urlmatch code. Unfortunately, that code had a bug: it percent-encoded the slashes in the path, resulting in any URL path that contained multiple levels (i.e., a directory component) not matching. We are currently the only caller of the percent-encoding code and could simply change it not to encode slashes. However, we still want to encode slashes in the username component, so we need to have both behaviors available. So instead, let's add a flag to control encoding slashes, which is the behavior we want here, and use it when calling the code in this case. Add a test for credential helper URLs using multiple slashes in the path, which our test suite previously lacked, as well as one ensuring that we handle usernames with slashes gracefully. Since we're testing other percent-encoding handling, let's add one for non-ASCII UTF-8 characters as well. Reported-by: Ilya Tretyakov <it@it3xl.ru> Signed-off-by: Carlo Marcelo Arenas Belón <carenas@gmail.com> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Johannes Schindelin	12294990c9	credential: handle `credential.<partial-URL>.<key>` again In the patches for CVE-2020-11008, the ability to specify credential settings in the config for partial URLs got lost. For example, it used to be possible to specify a credential helper for a specific protocol: [credential "https://"] helper = my-https-helper Likewise, it used to be possible to configure settings for a specific host, e.g.: [credential "dev.azure.com"] useHTTPPath = true Let's reinstate this behavior. While at it, increase the test coverage to document and verify the behavior with a couple other categories of partial URLs. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Jeff King	fe29a9b7b0	credential: die() when parsing invalid urls When we try to initialize credential loading by URL and find that the URL is invalid, we set all fields to NULL in order to avoid acting on malicious input. Later when we request credentials, we diagonse the erroneous input: fatal: refusing to work with credential missing host field This is problematic in two ways: - The message doesn't tell the user why we are missing the host field, so they can't tell from this message alone how to recover. There can be intervening messages after the original warning of bad input, so the user may not have the context to put two and two together. - The error only occurs when we actually need to get a credential. If the URL permits anonymous access, the only encouragement the user gets to correct their bogus URL is a quiet warning. This is inconsistent with the check we perform in fsck, where any use of such a URL as a submodule is an error. When we see such a bogus URL, let's not try to be nice and continue without helpers. Instead, die() immediately. This is simpler and obviously safe. And there's very little chance of disrupting a normal workflow. It's _possible_ that somebody has a legitimate URL with a raw newline in it. It already wouldn't work with credential helpers, so this patch steps that up from an inconvenience to "we will refuse to work with it at all". If such a case does exist, we should figure out a way to work with it (especially if the newline is only in the path component, which we normally don't even pass to helpers). But until we see a real report, we're better off being defensive. Reported-by: Carlo Arenas <carenas@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>	5 years ago
Jeff King	8ba8ed568e	credential: refuse to operate when missing host or protocol The credential helper protocol was designed to be very flexible: the fields it takes as input are treated as a pattern, and any missing fields are taken as wildcards. This allows unusual things like: echo protocol=https \| git credential reject to delete all stored https credentials (assuming the helpers themselves treat the input that way). But when helpers are invoked automatically by Git, this flexibility works against us. If for whatever reason we don't have a "host" field, then we'd match _any_ host. When you're filling a credential to send to a remote server, this is almost certainly not what you want. Prevent this at the layer that writes to the credential helper. Add a check to the credential API that the host and protocol are always passed in, and add an assertion to the credential_write function that speaks credential helper protocol to be doubly sure. There are a few ways this can be triggered in practice: - the "git credential" command passes along arbitrary credential parameters it reads from stdin. - until the previous patch, when the host field of a URL is empty, we would leave it unset (rather than setting it to the empty string) - a URL like "example.com/foo.git" is treated by curl as if "http://" was present, but our parser sees it as a non-URL and leaves all fields unset - the recent fix for URLs with embedded newlines blanks the URL but otherwise continues. Rather than having the desired effect of looking up no credential at all, many helpers will return _any_ credential Our earlier test for an embedded newline didn't catch this because it only checked that the credential was cleared, but didn't configure an actual helper. Configuring the "verbatim" helper in the test would show that it is invoked (it's obviously a silly helper which doesn't look at its input, but the point is that it shouldn't be run at all). Since we're switching this case to die(), we don't need to bother with a helper. We can see the new behavior just by checking that the operation fails. We'll add new tests covering partial input as well (these can be triggered through various means with url-parsing, but it's simpler to just check them directly, as we know we are covered even if the url parser changes behavior in the future). [jn: changed to die() instead of logging and showing a manual username/password prompt] Reported-by: Carlo Arenas <carenas@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>	5 years ago
Jeff King	24036686c4	credential: parse URL without host as empty host, not unset We may feed a URL like "cert:///path/to/cert.pem" into the credential machinery to get the key for a client-side certificate. That credential has no hostname field, which is about to be disallowed (to avoid confusion with protocols where a helper _would_ expect a hostname). This means as of the next patch, credential helpers won't work for unlocking certs. Let's fix that by doing two things: - when we parse a url with an empty host, set the host field to the empty string (asking only to match stored entries with an empty host) rather than NULL (asking to match _any_ host). - when we build a cert:// credential by hand, similarly assign an empty string It's the latter that is more likely to impact real users in practice, since it's what's used for http connections. But we don't have good infrastructure to test it. The url-parsing version will help anybody using git-credential in a script, and is easy to test. Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>	5 years ago
Jeff King	73aafe9bc2	t0300: use more realistic inputs Many of the tests in t0300 give partial inputs to git-credential, omitting a protocol or hostname. We're checking only high-level things like whether and how helpers are invoked at all, and we don't care about specific hosts. However, in preparation for tightening up the rules about when we're willing to run a helper, let's start using input that's a bit more realistic: pretend as if http://example.com is being examined. This shouldn't change the point of any of the tests, but do note we have to adjust the expected output to accommodate this (filling a credential will repeat back the protocol/host fields to stdout, and the helper debug messages and askpass prompt will change on stderr). Signed-off-by: Jeff King <peff@peff.net> Reviewed-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>	5 years ago
Jeff King	a88dbd2f8c	t0300: make "quit" helper more realistic We test a toy credential helper that writes "quit=1" and confirms that we stop running other helpers. However, that helper is unrealistic in that it does not bother to read its stdin at all. For now we don't send any input to it, because we feed git-credential a blank credential. But that will change in the next patch, which will cause this test to racily fail, as git-credential will get SIGPIPE writing to the helper rather than exiting because it was asked to. Let's make this one-off helper more like our other sample helpers, and have it source the "dump" script. That will read stdin, fixing the SIGPIPE problem. But it will also write what it sees to stderr. We can make the test more robust by checking that output, which confirms that we do run the quit helper, don't run any other helpers, and exit for the reason we expected. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>	5 years ago
Jeff King	4c5971e18a	credential: treat "?" and "#" in URLs as end of host It's unusual to see: https://example.com?query-parameters without an intervening slash, like: https://example.com/some-path?query-parameters or even: https://example.com/?query-parameters but it is a valid end to the hostname (actually "authority component") according to RFC 3986. Likewise for "#". And curl will parse the URL according to the standard, meaning it will contact example.com, but our credential code would ask about a bogus hostname with a "?" in it. Let's make sure we follow the standard, and more importantly ask about the same hosts that curl will be talking to. It would be nice if we could just ask curl to parse the URL for us. But it didn't grow a URL-parsing API until 7.62, so we'd be stuck with fallback code either way. Plus we'd need this code in the main Git binary, where we've tried to avoid having a link dependency on libcurl. But let's at least fix our parser. Moving to curl's parser would prevent other potential discrepancies, but this gives us immediate relief for the known problem, and would help our fallback code if we eventually use curl. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Jeff King	c716fe4bd9	credential: detect unrepresentable values when parsing urls The credential protocol can't represent newlines in values, but URLs can embed percent-encoded newlines in various components. A previous commit taught the low-level writing routines to die() when encountering this, but we can be a little friendlier to the user by detecting them earlier and handling them gracefully. This patch teaches credential_from_url() to notice such components, issue a warning, and blank the credential (which will generally result in prompting the user for a username and password). We blank the whole credential in this case. Another option would be to blank only the invalid component. However, we're probably better off not feeding a partially-parsed URL result to a credential helper. We don't know how a given helper would handle it, so we're better off to err on the side of matching nothing rather than something unexpected. The die() call in credential_write() is _probably_ impossible to reach after this patch. Values should end up in credential structs only by URL parsing (which is covered here), or by reading credential protocol input (which by definition cannot read a newline into a value). But we should definitely keep the low-level check, as it's our final and most accurate line of defense against protocol injection attacks. Arguably it could become a BUG(), but it probably doesn't matter much either way. Note that the public interface of credential_from_url() grows a little more than we need here. We'll use the extra flexibility in a future patch to help fsck catch these cases.	5 years ago
Jeff King	9a6bbee800	credential: avoid writing values with newlines The credential protocol that we use to speak to helpers can't represent values with newlines in them. This was an intentional design choice to keep the protocol simple, since none of the values we pass should generally have newlines. However, if we _do_ encounter a newline in a value, we blindly transmit it in credential_write(). Such values may break the protocol syntax, or worse, inject new valid lines into the protocol stream. The most likely way for a newline to end up in a credential struct is by decoding a URL with a percent-encoded newline. However, since the bug occurs at the moment we write the value to the protocol, we'll catch it there. That should leave no possibility of accidentally missing a code path that can trigger the problem. At this level of the code we have little choice but to die(). However, since we'd not ever expect to see this case outside of a malicious URL, that's an acceptable outcome. Reported-by: Felix Wilhelm <fwilhelm@google.com>	5 years ago
brian m. carlson	46fd7b3900	credential: allow wildcard patterns when matching config In some cases, a user will want to use a specific credential helper for a wildcard pattern, such as https://*.corp.example.com. We have code that handles this already with the urlmatch code, so let's use that instead of our custom code. Since the urlmatch code is a superset of our current matching in terms of capabilities, there shouldn't be any cases of things that matched previously that don't match now. However, in addition to wildcard matching, we now use partial path matching, which can cause slightly different behavior in the case that a helper applies to the prefix (considering path components) of the remote URL. While different, this is probably the behavior people were wanting anyway. Since we're using the urlmatch code, we need to encode the components we've gotten into a URL to match, so add a function to percent-encode data and format the URL with it. We now also no longer need to the custom code to match URLs, so let's remove it. Additionally, the urlmatch code always looks for the best match, whereas we want all matches for credential helpers to preserve existing behavior. Let's add an optional field, select_fn, that lets us control which items we want (in this case, all of them) and default it to the best-match code that already exists for other users. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
brian m. carlson	82eb249853	credential: use the last matching username in the config Everywhere else in the codebase, we use the rule that the last matching configuration option is the one that takes effect. This is helpful because it allows more specific configuration settings (e.g., per-repo configuration) to override less specific settings (e.g., per-user configuration). However, in the credential code, we didn't honor this setting, and instead picked the first setting we had, and stuck with it. This was likely to ensure we picked the value from the URL, which we want to honor over the configuration. It's possible to do both, though, so let's check if the value is the one we've gotten over our protocol connection, which if present will have come from the URL, and keep it if so. Otherwise, let's overwrite the value with the latest version we've got from the configuration, so we keep the last configuration value. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
brian m. carlson	588c70e10f	t0300: add tests for some additional cases There are some tricky cases in our credential helpers that we don't have test cases for. To help prevent regressions, let's add some for these cases: * If there are multiple configured credential helpers, one without a path and one with a path, we want to invoke both of them. * If there are percent-encoded values in the URL, we handle them properly. * And finally, if there is a username in the remote URL, we want to honor that over what the configuration tells us. Finally, there's an additional case that we'd like to test for as well, but that currently fails. In all other situations in our configuration, we pick the last configuration setting that's provided. However, we fail to do that for credential.username, where we pick the first setting instead. Let's add a failing test that we have the consistent behavior here, since that's the documented, expected behavior. Signed-off-by: brian m. carlson <bk2204@github.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Ævar Arnfjörð Bjarmason	d3c6751b18	tests: make use of the test_must_be_empty function Change various tests that use an idiom of the form: >expect && test_cmp expect actual To instead use: test_must_be_empty actual The test_must_be_empty() wrapper was introduced in `ca8d148daf` ("test: test_must_be_empty helper", 2013-06-09). Many of these tests have been added after that time. This was mostly found with, and manually pruned from: git grep '^\s+>.expect. &&$' t Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	24321375cd	credential: let empty credential specs reset helper list Sine the credential.helper key is a multi-valued config list, there's no way to "unset" a helper once it's been set. So if your system /etc/gitconfig sets one, you can never avoid running it, but only add your own helpers on top. Since an empty value for credential.helper is nonsensical (it would just try to run "git-credential-"), we can assume nobody is using it. Let's define it to reset the helper list, letting you override lower-priority instances which have come before. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Jeff King	59b386526a	credential: let helpers tell us to quit When we are trying to fill a credential, we loop over the set of defined credential-helpers, then fall back to running askpass, and then finally prompt on the terminal. Helpers which cannot find a credential are free to tell us nothing, but they cannot currently ask us to stop prompting. This patch lets them provide a "quit" attribute, which asks us to stop the process entirely (avoiding running more helpers, as well as the askpass/terminal prompt). This has a few possible uses: 1. A helper which prompts the user itself (e.g., in a dialog) can provide a "cancel" button to the user to stop further prompts. 2. Some helpers may know that prompting cannot possibly work. For example, if their role is to broker a ticket from an external auth system and that auth system cannot be contacted, there is no point in continuing (we need a ticket to authenticate, and the user cannot provide one by typing it in). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	10 years ago
Elia Pinto	dd64267fe2	t0300-credentials.sh: use the $( ... ) construct for command substitution The Git CodingGuidelines prefer the $(...) construct for command substitution instead of using the backquotes `...`. The backquoted form is the traditional method for command substitution, and is supported by POSIX. However, all but the simplest uses become complicated quickly. In particular, embedded command substitutions and/or the use of double quotes require careful escaping with the backslash character. The patch was generated by: for _f in $(find . -name ".sh") do sed -i 's@`$.$`@$(\1)@g' ${_f} done and then carefully proof-read. Signed-off-by: Elia Pinto <gitter.spiros@gmail.com> Reviewed-by: Matthieu Moy <Matthieu.Moy@imag.fr> Signed-off-by: Junio C Hamano <gitster@pobox.com>	11 years ago
Matthieu Moy	2d6dc182b8	git credential fill: output the whole 'struct credential' Instead of outputing only the username and password, print all the attributes, even those that already appeared in the input. This is closer to what the C API does, and allows one to take the exact output of "git credential fill" as input to "git credential approve" or "git credential reject". Signed-off-by: Junio C Hamano <gitster@pobox.com>	13 years ago
Michael J Gruber	78ed1d2d63	t0300: work around bug in dash 0.5.6 The construct 'while IFS== read' makes dash 0.5.6 execute read without changing IFS, which results in test breakages all over the place in t0300. Neither dash 0.5.5.1 and older nor dash 0.5.7 and newer are affected: The problem was introduded resp. fixed by the commits 55c46b7 ([BUILTIN] Honor tab as IFS whitespace when splitting fields in readcmd, 2009-08-11) 1d806ac ([VAR] Do not poplocalvars prematurely on regular utilities, 2010-05-27) in http://git.kernel.org/?p=utils/dash/dash.git Putting 'IFS==' before that line makes all versions of dash work. This looks like a dash bug, not a misinterpretation of the standard. However, it's worth working around for two reasons. One, this version of dash was released in Fedora 14-16, so the bug is found in the wild. And two, at least one other shell, Solaris /bin/sh, choked on this by persisting IFS after the read invocation. That is not a shell we usually care about, and I think this use of IFS is acceptable by POSIX (which allows other behavior near "special builtins", but "read" is not one of those). But it seems that this may be a subtle, not-well-tested case for some shells. Given that the workaround is so simple, it's worth just being defensive. Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	13 years ago
Jeff King	3d9f5b674f	t0300: use write_script helper t0300 creates some helper shell scripts, and marks them with "!/bin/sh". Even though the scripts are fairly simple, they can fail on broken shells (specifically, Solaris /bin/sh will persist a temporary assignment to IFS in a "read" command). Rather than work around the problem for Solaris /bin/sh, using write_script will make sure we point to a known-good shell that the user has given us. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	13 years ago
Jeff King	a78fbb4fb6	credential: make relevance of http path configurable When parsing a URL into a credential struct, we carefully record each part of the URL, including the path on the remote host, and use the result as part of the credential context. This had two practical implications: 1. Credential helpers which store a credential for later access are likely to use the "path" portion as part of the storage key. That means that a request to https://example.com/foo.git would not use the same credential that was stored in an earlier request for: https://example.com/bar.git 2. The prompt shown to the user includes all relevant context, including the path. In most cases, however, users will have a single password per host. The behavior in (1) will be inconvenient, and the prompt in (2) will be overly long. This patch introduces a config option to toggle the relevance of http paths. When turned on, we use the path as before. When turned off, we drop the path component from the context: helpers don't see it, and it does not appear in the prompt. This is nothing you couldn't do with a clever credential helper at the start of your stack, like: [credential "http://"] helper = "!f() { grep -v ^path= ; }; f" helper = your_real_helper But doing this: [credential] useHttpPath = false is way easier and more readable. Furthermore, since most users will want the "off" behavior, that is the new default. Users who want it "on" can set the variable (either for all credentials, or just for a subset using credential.*.useHttpPath). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	13 years ago
Jeff King	d5742425eb	credential: add credential.*.username Credential helpers can help users avoid having to type their username and password over and over. However, some users may not want a helper for their password, or they may be running a helper which caches for a short time. In this case, it is convenient to provide the non-secret username portion of their credential via config. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	13 years ago
Jeff King	118250728e	credential: apply helper config The functionality for credential storage helpers is already there; we just need to give the users a way to turn it on. This patch provides a "credential.helper" configuration variable which allows the user to provide one or more helper strings. Rather than simply matching credential.helper, we will also compare URLs in subsection headings to the current context. This means you can apply configuration to a subset of credentials. For example: [credential "https://example.com"] helper = foo would match a request for "https://example.com/foo.git", but not one for "https://kernel.org/foo.git". This is overkill for the "helper" variable, since users are unlikely to want different helpers for different sites (and since helpers run arbitrary code, they could do the matching themselves anyway). However, future patches will add new config variables where this extra feature will be more useful. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	13 years ago
Jeff King	abca927dbe	introduce credentials API There are a few places in git that need to get a username and password credential from the user; the most notable one is HTTP authentication for smart-http pushing. Right now the only choices for providing credentials are to put them plaintext into your ~/.netrc, or to have git prompt you (either on the terminal or via an askpass program). The former is not very secure, and the latter is not very convenient. Unfortunately, there is no "always best" solution for password management. The details will depend on the tradeoff you want between security and convenience, as well as how git can integrate with other security systems (e.g., many operating systems provide a keychain or password wallet for single sign-on). This patch provides an abstract notion of credentials as a data item, and provides three basic operations: - fill (i.e., acquire from external storage or from the user) - approve (mark a credential as "working" for further storage) - reject (mark a credential as "not working", so it can be removed from storage) These operations can be backed by external helper processes that interact with system- or user-specific secure storage. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	13 years ago

32 Commits (59f0d5073fd9f0497b9ff9a48fb3a7a8d82d1f9d)