kernel/git - git - PowerEL Git System

Commit Graph

Author	SHA1	Message	Date
brian m. carlson	9b27b49240	gpg-interface: remove other signature headers before verifying When we have a multiply signed commit, we need to remove the signature in the header before verifying the object, since the trailing signature will not be over both pieces of data. Do so, and verify that we validate the signature appropriately. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
brian m. carlson	482c119186	gpg-interface: improve interface for parsing tags We have a function which parses a buffer with a signature at the end, parse_signature, and this function is used for signed tags. However, we'll need to store values for multiple algorithms, and we'll do this by using a header for the non-default algorithm. Adjust the parse_signature interface to store the parsed data in two strbufs and turn the existing function into parse_signed_buffer. The latter is still used in places where we know we always have a signed buffer, such as push certs. Adjust all the callers to deal with this new interface. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	4 years ago
Jeff King	f6d8942b1f	strvec: fix indentation in renamed calls Code which split an argv_array call across multiple lines, like: argv_array_pushl(&args, "one argument", "another argument", "and more", NULL); was recently mechanically renamed to use strvec, which results in mis-matched indentation like: strvec_pushl(&args, "one argument", "another argument", "and more", NULL); Let's fix these up to align the arguments with the opening paren. I did this manually by sifting through the results of: git jump grep 'strvec_.*,$' and liberally applying my editor's auto-format. Most of the changes are of the form shown above, though I also normalized a few that had originally used a single-tab indentation (rather than our usual style of aligning with the open paren). I also rewrapped a couple of obvious cases (e.g., where previously too-long lines became short enough to fit on one), but I wasn't aggressive about it. In cases broken to three or more lines, the grouping of arguments is sometimes meaningful, and it wasn't worth my time or reviewer time to ponder each case individually. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Jeff King	ef8d7ac42a	strvec: convert more callers away from argv_array name We eventually want to drop the argv_array name and just use strvec consistently. There's no particular reason we have to do it all at once, or care about interactions between converted and unconverted bits. Because of our preprocessor compat layer, the names are interchangeable to the compiler (so even a definition and declaration using different names is OK). This patch converts remaining files from the first half of the alphabet, to keep the diff to a manageable size. The conversion was done purely mechanically with: git ls-files '.c' '.h' \| xargs perl -i -pe ' s/ARGV_ARRAY/STRVEC/g; s/argv_array/strvec/g; ' and then selectively staging files with "git add '[abcdefghjkl]*'". We'll deal with any indentation/style fallouts separately. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Hans Jerry Illikainen	6794898198	gpg-interface: prefer check_signature() for GPG verification This commit refactors the use of verify_signed_buffer() outside of gpg-interface.c to use check_signature() instead. It also turns verify_signed_buffer() into a file-local function since it's now only invoked internally by check_signature(). There were previously two globally scoped functions used in different parts of Git to perform GPG signature verification: verify_signed_buffer() and check_signature(). Now only check_signature() is used. The verify_signed_buffer() function doesn't guard against duplicate signatures as described by Michał Górny [1]. Instead it only ensures a non-erroneous exit code from GPG and the presence of at least one GOODSIG status field. This stands in contrast with check_signature() that returns an error if more than one signature is encountered. The lower degree of verification makes the use of verify_signed_buffer() problematic if callers don't parse and validate the various parts of the GPG status message themselves. And processing these messages seems like a task that should be reserved to gpg-interface.c with the function check_signature(). Furthermore, the use of verify_signed_buffer() makes it difficult to introduce new functionality that relies on the content of the GPG status lines. Now all operations that does signature verification share a single entry point to gpg-interface.c. This makes it easier to propagate changed or additional functionality in GPG signature verification to all parts of Git, without having odd edge-cases that don't perform the same degree of verification. [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Junio C Hamano	0106b1d4be	Revert "gpg-interface: prefer check_signature() for GPG verification" This reverts commit `72b006f4bf`, which breaks the end-user experience when merging a signed tag without having the public key. We should report "can't check because we have no public key", but the code with this change claimed that there was no signature.	5 years ago
Hans Jerry Illikainen	54887b4689	gpg-interface: add minTrustLevel as a configuration option Previously, signature verification for merge and pull operations checked if the key had a trust-level of either TRUST_NEVER or TRUST_UNDEFINED in verify_merge_signature(). If that was the case, the process die()d. The other code paths that did signature verification relied entirely on the return code from check_commit_signature(). And signatures made with a good key, irregardless of its trust level, was considered valid by check_commit_signature(). This difference in behavior might induce users to erroneously assume that the trust level of a key in their keyring is always considered by Git, even for operations where it is not (e.g. during a verify-commit or verify-tag). The way it worked was by gpg-interface.c storing the result from the key/signature status and the lowest-two trust levels in the `result` member of the signature_check structure (the last of these status lines that were encountered got written to `result`). These are documented in GPG under the subsection `General status codes` and `Key related`, respectively [1]. The GPG documentation says the following on the TRUST_ status codes [1]: """ These are several similar status codes: - TRUST_UNDEFINED <error_token> - TRUST_NEVER <error_token> - TRUST_MARGINAL [0 [<validation_model>]] - TRUST_FULLY [0 [<validation_model>]] - TRUST_ULTIMATE [0 [<validation_model>]] For good signatures one of these status lines are emitted to indicate the validity of the key used to create the signature. The error token values are currently only emitted by gpgsm. """ My interpretation is that the trust level is conceptionally different from the validity of the key and/or signature. That seems to also have been the assumption of the old code in check_signature() where a result of 'G' (as in GOODSIG) and 'U' (as in TRUST_NEVER or TRUST_UNDEFINED) were both considered a success. The two cases where a result of 'U' had special meaning were in verify_merge_signature() (where this caused git to die()) and in format_commit_one() (where it affected the output of the %G? format specifier). I think it makes sense to refactor the processing of TRUST_ status lines such that users can configure a minimum trust level that is enforced globally, rather than have individual parts of git (e.g. merge) do it themselves (except for a grace period with backward compatibility). I also think it makes sense to not store the trust level in the same struct member as the key/signature status. While the presence of a TRUST_ status code does imply that the signature is good (see the first paragraph in the included snippet above), as far as I can tell, the order of the status lines from GPG isn't well-defined; thus it would seem plausible that the trust level could be overwritten with the key/signature status if they were stored in the same member of the signature_check structure. This patch introduces a new configuration option: gpg.minTrustLevel. It consolidates trust-level verification to gpg-interface.c and adds a new `trust_level` member to the signature_check structure. Backward-compatibility is maintained by introducing a special case in verify_merge_signature() such that if no user-configurable gpg.minTrustLevel is set, then the old behavior of rejecting TRUST_UNDEFINED and TRUST_NEVER is enforced. If, on the other hand, gpg.minTrustLevel is set, then that value overrides the old behavior. Similarly, the %G? format specifier will continue show 'U' for signatures made with a key that has a trust level of TRUST_UNDEFINED or TRUST_NEVER, even though the 'U' character no longer exist in the `result` member of the signature_check structure. A new format specifier, %GT, is also introduced for users that want to show all possible trust levels for a signature. Another approach would have been to simply drop the trust-level requirement in verify_merge_signature(). This would also have made the behavior consistent with other parts of git that perform signature verification. However, requiring a minimum trust level for signing keys does seem to have a real-world use-case. For example, the build system used by the Qubes OS project currently parses the raw output from verify-tag in order to assert a minimum trust level for keys used to sign git tags [2]. [1] https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/doc/DETAILS;h=bd00006e933ac56719b1edd2478ecd79273eae72;hb=refs/heads/master [2] `9674c1991d/scripts/verify-git-tag (L43)` Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Hans Jerry Illikainen	72b006f4bf	gpg-interface: prefer check_signature() for GPG verification This commit refactors the use of verify_signed_buffer() outside of gpg-interface.c to use check_signature() instead. It also turns verify_signed_buffer() into a file-local function since it's now only invoked internally by check_signature(). There were previously two globally scoped functions used in different parts of Git to perform GPG signature verification: verify_signed_buffer() and check_signature(). Now only check_signature() is used. The verify_signed_buffer() function doesn't guard against duplicate signatures as described by Michał Górny [1]. Instead it only ensures a non-erroneous exit code from GPG and the presence of at least one GOODSIG status field. This stands in contrast with check_signature() that returns an error if more than one signature is encountered. The lower degree of verification makes the use of verify_signed_buffer() problematic if callers don't parse and validate the various parts of the GPG status message themselves. And processing these messages seems like a task that should be reserved to gpg-interface.c with the function check_signature(). Furthermore, the use of verify_signed_buffer() makes it difficult to introduce new functionality that relies on the content of the GPG status lines. Now all operations that does signature verification share a single entry point to gpg-interface.c. This makes it easier to propagate changed or additional functionality in GPG signature verification to all parts of Git, without having odd edge-cases that don't perform the same degree of verification. [1] https://dev.gentoo.org/~mgorny/articles/attack-on-git-signature-verification.html Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Hans Jerry Illikainen	67a6ea6300	gpg-interface: limit search for primary key fingerprint The VALIDSIG status line from GnuPG with --status-fd is documented to have 9 required and 1 optional fields [1]. The final, and optional, field is used to specify the fingerprint of the primary key that made the signature in case it was made by a subkey. However, this field is only available for OpenPGP signatures; not for CMS/X.509. If the VALIDSIG status line does not have the optional 10th field, the current code will continue reading onto the next status line. And this is the case for non-OpenPGP signatures [1]. The consequence is that a subsequent status line may be considered as the "primary key" for signatures that does not have an actual primary key. Limit the search of these 9 or 10 fields to the single line to avoid this problem. If the 10th field is missing, report that there is no primary key fingerprint. [Reference] [1] GnuPG Details, General status codes https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=6ce340e8c04794add995e84308bb3091450bd28f;hb=HEAD#l483 The documentation says: VALIDSIG <args> The args are: - <fingerprint_in_hex> - <sig_creation_date> - <sig-timestamp> - <expire-timestamp> - <sig-version> - <reserved> - <pubkey-algo> - <hash-algo> - <sig-class> - [ <primary-key-fpr> ] This status indicates that the signature is cryptographically valid. [...] PRIMARY-KEY-FPR is the fingerprint of the primary key or identical to the first argument. The primary-key-fpr parameter is used for OpenPGP and not available for CMS signatures. [...] Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Hans Jerry Illikainen	392b862e9a	gpg-interface: refactor the free-and-xmemdupz pattern Introduce a static replace_cstring() function to simplify repeated pattern of free-and-xmemdupz() for GPG status line parsing. This also helps us avoid potential memleaks if parsing of new status lines are introduced in the future. Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	5 years ago
Steven Roberts	64c45dc72e	gpg-interface: do not scan past the end of buffer If the GPG output ends with trailing blank lines, after skipping them over inside the loop to find the terminating NUL at the end, the loop ends up looking for the next line, starting past the end. Signed-off-by: Steven Roberts <sroberts@fenderq.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	6 years ago
Michał Górny	4de9394dcb	gpg-interface.c: obtain primary key fingerprint as well Obtain the primary key fingerprint off VALIDSIG status message, and expose it via %GP format. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>	6 years ago
Michał Górny	3daaaabe7e	gpg-interface.c: support getting key fingerprint via %GF format Support processing VALIDSIG status that provides additional information for valid signatures. Use this information to propagate signing key fingerprint and expose it via %GF pretty format. This format can be used to build safer key verification systems that verify the key via complete fingerprint rather than short/long identifier provided by %GK. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>	6 years ago
Michał Górny	0b11a84e1b	gpg-interface.c: use flags to determine key/signer info presence Replace the logic used to determine whether key and signer information is present to use explicit flags in sigcheck_gpg_status[] array. This is more future-proof, since it makes it possible to add additional statuses without having to explicitly update the conditions. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>	6 years ago
Michał Górny	da6cf1b336	gpg-interface.c: detect and reject multiple signatures on commits GnuPG supports creating signatures consisting of multiple signature packets. If such a signature is verified, it outputs all the status messages for each signature separately. However, git currently does not account for such scenario and gets terribly confused over getting multiple *SIG statuses. For example, if a malicious party alters a signed commit and appends a new untrusted signature, git is going to ignore the original bad signature and report untrusted commit instead. However, %GK and %GS format strings may still expand to the data corresponding to the original signature, potentially tricking the scripts into trusting the malicious commit. Given that the use of multiple signatures is quite rare, git does not support creating them without jumping through a few hoops, and finally supporting them properly would require extensive API improvement, it seems reasonable to just reject them at the moment. Signed-off-by: Michał Górny <mgorny@gentoo.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>	6 years ago
Junio C Hamano	4e5dc9ca17	gpg-interface: propagate exit status from gpg back to the callers When gpg-interface API unified support for signature verification codepaths for signed tags and signed commits in mid 2015 at around v2.6.0-rc0~114, we accidentally loosened the GPG signature verification. Before that change, signed commits were verified by looking for "G"ood signature from GPG, while ignoring the exit status of "gpg --verify" process, while signed tags were verified by simply passing the exit status of "gpg --verify" through. The unified code we currently have ignores the exit status of "gpg --verify" and returns successful verification when the signature matches an unexpired key regardless of the trust placed on the key (i.e. in addition to "G"ood ones, we accept "U"ntrusted ones). Make these commands signal failure with their exit status when underlying "gpg --verify" (or the custom command specified by "gpg.program" configuration variable) does so. This essentially changes their behaviour in a backward incompatible way to reject signatures that have been made with untrusted keys even if they correctly verify, as that is how "gpg --verify" behaves. Note that the code still overrides a zero exit status obtained from "gpg" (or gpg.program) if the output does not say the signature is good or computes correctly but made with untrusted keys, to catch a poorly written wrapper around "gpg" the user may give us. We could exclude "U"ntrusted support from this fallback code, but that would be making two backward incompatible changes in a single commit, so let's avoid that for now. A follow-up change could do so if desired. Helped-by: Vojtech Myslivec <vojtech.myslivec@nic.cz> Helped-by: brian m. carlson <sandals@crustytoothpaste.net> Helped-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Henning Schild	1e7adb9756	gpg-interface: introduce new signature format "x509" using gpgsm This commit allows git to create and check x509 type signatures using gpgsm. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Henning Schild	b02f51b196	gpg-interface: introduce new config to select per gpg format program Supporting multiple signing formats we will have the need to configure a custom program each. Add a new config value to cater for that. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Henning Schild	42149d7f4b	gpg-interface: do not hardcode the key string len anymore gnupg does print the keyid followed by a space and the signer comes next. The same pattern is also used in gpgsm, but there the key length would be 40 instead of 16. Instead of hardcoding the expected length, find the first space and calculate it. Input that does not match the expected format will be ignored now, before we jumped to found+17 which might have been behind the end of an unexpected string. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Henning Schild	58af57e1c8	gpg-interface: introduce an abstraction for multiple gpg formats Create a struct that holds the format details for the supported formats. At the moment that is still just "openpgp". This commit prepares for the introduction of more formats, that might use other programs and match other signatures. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Henning Schild	57a8dd75df	gpg-interface: add new config to select how to sign a commit Add "gpg.format" where the user can specify which type of signature to use for commits. At the moment only "openpgp" is supported and the value is not even used. This commit prepares for a new types of signatures. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Henning Schild	fbd0f16610	gpg-interface: make parse_gpg_output static and remove from interface header Turn parse_gpg_output into a static function, the only outside user was migrated in an earlier commit. Signed-off-by: Henning Schild <henning.schild@siemens.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	8b44b2be89	gpg-interface: find the last gpg signature line A signed tag has a detached signature like this: object ... [...more header...] This is the tag body. -----BEGIN PGP SIGNATURE----- [opaque gpg data] -----END PGP SIGNATURE----- Our parser finds the _first_ line that appears to start a PGP signature block, meaning we may be confused by a signature (or a signature-like line) in the actual body. Let's keep parsing and always find the final block, which should be the detached signature over all of the preceding content. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	f68f2dd57f	gpg-interface: extract gpg line matching helper Let's separate the actual line-by-line parsing of signatures from the notion of "is this a gpg signature line". That will make it easier to do more refactoring of this loop in future patches. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	17ef3a421e	gpg-interface: fix const-correctness of "eol" pointer We accidentally shed the "const" of our buffer by passing it through memchr. Let's fix that, and while we're at it, move our variable declaration inside the loop, which is the only place that uses it. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	e6fa6cde5b	gpg-interface: use size_t for signature buffer size Even though our object sizes (from which these buffers would come) are typically "unsigned long", this is something we'd like to eventually fix (since it's only 32-bits even on 64-bit Windows). It makes more sense to use size_t when taking an in-memory buffer. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	f80bee27e3	gpg-interface: modernize function declarations Let's drop "extern" from our declarations, which brings us in line with our modern style guidelines. While we're here, let's wrap some of the overly long lines, and move docstrings for public functions to their declarations, since they document the interface. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	1b0eeec3f3	gpg-interface: handle bool user.signingkey The config handler for user.signingkey does not check for a boolean value, and thus: git -c user.signingkey tag will segfault. We could fix this and even shorten the code by using git_config_string(). But our set_signing_key() helper is used by other code outside of gpg-interface.c, so we must keep it (and we may as well use it, because unlike git_config_string() it does not leak when we overwrite an old value). Ironically, the handler for gpg.program just below _could_ use git_config_string() but doesn't. But since we're going to touch that in a future patch, we'll leave it alone for now. We will add some whitespace and returns in preparation for adding more config keys, though. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Ben Toews <mastahyeti@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	7 years ago
Jeff King	076aa2cbda	tempfile: auto-allocate tempfiles on heap The previous commit taught the tempfile code to give up ownership over tempfiles that have been renamed or deleted. That makes it possible to use a stack variable like this: struct tempfile t; create_tempfile(&t, ...); ... if (!err) rename_tempfile(&t, ...); else delete_tempfile(&t); But doing it this way has a high potential for creating memory errors. The tempfile we pass to create_tempfile() ends up on a global linked list, and it's not safe for it to go out of scope until we've called one of those two deactivation functions. Imagine that we add an early return from the function that forgets to call delete_tempfile(). With a static or heap tempfile variable, the worst case is that the tempfile hangs around until the program exits (and some functions like setup_shallow_temporary rely on this intentionally, creating a tempfile and then leaving it for later cleanup). But with a stack variable as above, this is a serious memory error: the variable goes out of scope and may be filled with garbage by the time the tempfile code looks at it. Let's see if we can make it harder to get this wrong. Since many callers need to allocate arbitrary numbers of tempfiles, we can't rely on static storage as a general solution. So we need to turn to the heap. We could just ask all callers to pass us a heap variable, but that puts the burden on them to call free() at the right time. Instead, let's have the tempfile code handle the heap allocation _and_ the deallocation (when the tempfile is deactivated and removed from the list). This changes the return value of all of the creation functions. For the cleanup functions (delete and rename), we'll add one extra bit of safety: instead of taking a tempfile pointer, we'll take a pointer-to-pointer and set it to NULL after freeing the object. This makes it safe to double-call functions like delete_tempfile(), as the second call treats the NULL input as a noop. Several callsites follow this pattern. The resulting patch does have a fair bit of noise, as each caller needs to be converted to handle: 1. Storing a pointer instead of the struct itself. 2. Passing the pointer instead of taking the struct address. 3. Handling a "struct tempfile " return instead of a file descriptor. We could play games to make this less noisy. For example, by defining the tempfile like this: struct tempfile { struct heap_allocated_part_of_tempfile { int fd; ...etc } actual_data; } Callers would continue to have a "struct tempfile", and it would be "active" only when the inner pointer was non-NULL. But that just makes things more awkward in the long run. There aren't that many callers, so we can simply bite the bullet and adjust all of them. And the compiler makes it easy for us to find them all. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	49bd0fc222	tempfile: do not delete tempfile on failed close When close_tempfile() fails, we delete the tempfile and reset the fields of the tempfile struct. This makes it easier for callers to return without cleaning up, but it also makes this common pattern: if (close_tempfile(tempfile)) return error_errno("error closing %s", tempfile->filename.buf); wrong, because the "filename" field has been reset after the failed close. And it's not easy to fix, as in many cases we don't have another copy of the filename (e.g., if it was created via one of the mks_tempfile functions, and we just have the original template string). Let's drop the feature that a failed close automatically deletes the file. This puts the burden on the caller to do the deletion themselves, but this isn't that big a deal. Callers which do: if (write(...) \|\| close_tempfile(...)) { delete_tempfile(...); return -1; } already had to call delete when the write() failed, and so aren't affected. Likewise, any caller which just calls die() in the error path is OK; we'll delete the tempfile during the atexit handler. Because this patch changes the semantics of close_tempfile() without changing its signature, all callers need to be manually checked and converted to the new scheme. This patch covers all in-tree callers, but there may be others for not-yet-merged topics. To catch these, we rename the function to close_tempfile_gently(), which will attract compile-time attention to new callers. (Technically the original could be considered "gentle" already in that it didn't die() on errors, but this one is even more so). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	45c6b1ed24	always check return value of close_tempfile If close_tempfile() encounters an error, then it deletes the tempfile and resets the "struct tempfile". But many code paths ignore the return value and continue to use the tempfile. Instead, we should generally treat this the same as a write() error. Note that in the postimage of some of these cases our error message will be bogus after a failed close because we look at tempfile->filename (either directly or via get_tempfile_path). But after the failed close resets the tempfile object, this is guaranteed to be the empty string. That will be addressed in a future patch (because there are many more cases of the same problem than just these instances). Note also in the hunk in gpg-interface.c that it's fine to call delete_tempfile() in the error path, even if close_tempfile() failed and already deleted the file. The tempfile code is smart enough to know the second deletion is a noop. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Jeff King	d88ef66051	verify_signed_buffer: prefer close_tempfile() to close() We do a manual close() on the descriptor provided to us by mks_tempfile. But this runs contrary to the advice in tempfile.h, which notes that you should always use close_tempfile(). Otherwise the descriptor may be reused without the tempfile object knowing it, and the later call to delete_tempfile() could close a random descriptor. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Ævar Arnfjörð Bjarmason	88ce3ef636	*.[ch] refactoring: make use of the FREE_AND_NULL() macro Replace occurrences of `free(ptr); ptr = NULL` which weren't caught by the coccinelle rule. These fall into two categories: - free/NULL assignments one after the other which coccinelle all put on one line, which is functionally equivalent code, but very ugly. - manually spotted occurrences where the NULL assignment isn't right after the free() call. Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Brandon Williams	b2141fc1d2	config: don't include config.h by default Stop including config.h by default in cache.h. Instead only include config.h in those files which require use of the config system. Signed-off-by: Brandon Williams <bmwill@google.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Michael J Gruber	661a180681	gpg-interface: use more status letters According to gpg2's doc/DETAILS: For each signature only one of the codes GOODSIG, BADSIG, EXPSIG, EXPKEYSIG, REVKEYSIG or ERRSIG will be emitted. gpg1 ("classic") behaves the same (although doc/DETAILS differs). Currently, we parse gpg's status output for GOODSIG, BADSIG and trust information and translate that into status codes G, B, U, N for the %G? format specifier. git-verify-* returns success in the GOODSIG case only. This is somewhat in disagreement with gpg, which considers the first 5 of the 6 above as VALIDSIG, but we err on the very safe side. Introduce additional status codes E, X, Y, R for ERRSIG, EXPSIG, EXPKEYSIG, and REVKEYSIG so that a user of %G? gets more information about the absence of a 'G' on first glance. Requested-by: Alex <agrambot@gmail.com> Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	8 years ago
Linus Torvalds	b624a3e67f	gpg-interface: prefer "long" key format output when verifying pgp signatures Yes, gpg2 already uses the long format by default, but most distributions seem to still have "gpg" be the older 1.x version due to compatibility reasons. And older versions of gpg only show the 32-bit short ID, which is quite insecure. This doesn't actually matter for the _verification_ itself: if the verification passes, the pgp signature is good. But if you don't actually have the key yet, and want to fetch it, or you want to check exactly which key was used for verification and want to check it, we should specify the key with more precision. In fact, we should preferentially specify the whole key fingerprint, but gpg doesn't actually support that. Which is really quite sad. Showing the "long" format improves things to at least show 64 bits of the fingerprint. That's a lot better, even if it's not perfect. This change the log format for "git log --show-signature" from commit `2376d31787` merged tag 'v2.9.3' gpg: Signature made Fri 12 Aug 2016 09:17:59 AM PDT using RSA key ID 96AFE6CB gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" gpg: aka "Junio C Hamano <jch@google.com>" gpg: aka "Junio C Hamano <junio@pobox.com>" Merge: `2807cd7b25` `e0c1ceafc5` Author: Junio C Hamano <gitster@pobox.com> Date: Fri Aug 12 10:02:18 2016 -0700 to commit `2376d31787` merged tag 'v2.9.3' gpg: Signature made Fri 12 Aug 2016 09:17:59 AM PDT gpg: using RSA key B0B5E88696AFE6CB gpg: Good signature from "Junio C Hamano <gitster@pobox.com>" gpg: aka "Junio C Hamano <jch@google.com>" gpg: aka "Junio C Hamano <junio@pobox.com>" Merge: `2807cd7b25` `e0c1ceafc5` Author: Junio C Hamano <gitster@pobox.com> Date: Fri Aug 12 10:02:18 2016 -0700 (note the longer key ID, but also the reflowing of the text) and also changes the format in the merge messages when merging a signed tag. If you already use gpg2 (either because it's installed by default, or because you have set your gpg_program configuration to point to gpg2), that already used the long format, you'll also see a change: it will now have the same formatting as gpg 1.x, and the verification string looks something like gpg: Signature made Sun 24 Jul 2016 12:24:02 PM PDT gpg: using RSA key 79BE3E4300411886 gpg: Good signature from "Linus Torvalds <torvalds@linux-foundation.org>" [ultimate] where it used to be on one line: gpg: Signature made Sun 24 Jul 2016 12:24:02 PM PDT using RSA key ID 79BE3E4300411886 gpg: Good signature from "Linus Torvalds <torvalds@linux-foundation.org>" [ultimate] so there is certainly a chance this could break some automated scripting. But the 32-bit key ID's really are broken. Also note that because of the differences between gpg-1.x and gpg-2.x, hopefully any scripted key ID parsing code (if such code exists) is already flexible enough to not care. This was triggered by the fact that the "evil32" project keys ended up leaking to the public key servers, so now there are 32-bit aliases for just about every open source developer that you can easily get by mistake if you use the 32-bit short ID format. Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Michael J Gruber	efee9553a4	gpg-interface: check gpg signature creation status When we create a signature, it may happen that gpg returns with "success" but not with an actual detached signature on stdout. Check for the correct signature creation status to catch these cases better. Really, --status-fd parsing is the only way to check gpg status reliably. We do the same for verify already. 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>	9 years ago
Jeff King	0581b54641	sign_buffer: use pipe_command Similar to the prior commit for verify_signed_buffer, the motivation here is both to make the code simpler, and to avoid any possible deadlocks with gpg. In this case we have the same "write to stdin, then read from stdout" that the verify case had. This is unlikely to be a problem in practice, since stdout has the detached signature, which it cannot compute until it has read all of stdin (if it were a non-detached signature, that would be a problem, though). We don't read from stderr at all currently. However, we will want to in a future patch, so this also prepares us there (and in that case gpg _does_ write before reading all of the input, though again, it is unlikely that a key uid will fill up a pipe buffer). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Jeff King	0d2b664efd	verify_signed_buffer: use pipe_command This is shorter and should make the function easier to follow. But more importantly, it removes the possibility of any deadlocks based on reading or writing to gpg. It's not clear if such a deadlock is possible in practice. We do write the whole payload before reading anything, so we could deadlock there. However, in practice gpg will need to read our whole input to verify the signature, so it will drain our payload first. It could write an error to stderr before reading, but it's unlikely that such an error wouldn't be followed by it immediately exiting, or that the error would actually be larger than a pipe buffer. On the writing side, we drain stderr (with the human-readable output) in its entirety before reading stdout (with the status-fd data). Running strace on "gpg --verify" does show interleaved output on the two descriptors: write(2, "gpg: ", 5) = 5 write(2, "Signature made Thu 16 Jun 2016 0"..., 73) = 73 write(1, "[GNUPG:] SIG_ID tQw8KGcs9rBfLvAj"..., 66) = 66 write(1, "[GNUPG:] GOODSIG 69808639F9430ED"..., 60) = 60 write(2, "gpg: ", 5) = 5 write(2, "Good signature from \"Jeff King <"..., 47) = 47 write(2, "\n", 1) = 1 write(2, "gpg: ", 5) = 5 write(2, " aka \"Jeff King <"..., 49) = 49 write(2, "\n", 1) = 1 write(1, "[GNUPG:] VALIDSIG C49CE24156AF08"..., 135) = 135 write(1, "[GNUPG:] TRUST_ULTIMATE\n", 24) = 24 The second line written to stdout there contains the signer's UID, which can be arbitrarily long. If it fills the pipe buffer, then gpg would block writing to its stdout, while we are blocked trying to read its stderr. In practice, GPG seems to limit UIDs to 2048 bytes, so unless your pipe buffer size is quite small, or unless gpg does not enforce the limit under some conditions, this seems unlikely in practice. Still, it is not hard for us to be cautious and just use pipe_command. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Jeff King	4322353bfb	verify_signed_buffer: use tempfile object We use git_mkstemp to create a temporary file, and try to clean it up in all exit paths from the function. But that misses any cases where we die by signal, or by calling die() in a sub-function. In addition, we missed one of the exit paths. Let's convert to using a tempfile object, which handles the hard cases for us, and add the missing cleanup call. Note that we would not simply want to rely on program exit to catch our missed cleanup, as this function may be called many times in a single program (for the same reason, we use a static tempfile instead of heap-allocating a new one; that gives an upper bound on our memory usage). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Jeff King	c752fcc8e0	verify_signed_buffer: drop pbuf variable If our caller gave us a non-NULL gpg_status parameter, we write the gpg status into their strbuf. If they didn't, then we write it to a temporary local strbuf (since we still need to look at it). The variable "pbuf" adds an extra layer of indirection so that the rest of the function can just access whichever is appropriate. However, the name "pbuf" isn't very descriptive, and it's easy to get confused about what is supposed to be in it (especially because we are reading both "status" and "output" from gpg). Rather than give it a more descriptive name, we can just use gpg_status as our indirection pointer. Either it points to the caller's input, or we can point it directly to our temporary buffer. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Jeff King	aedb5dc343	gpg-interface: use child_process.args Our argv allocations are relatively straightforward, but this avoids us having to manually keep the count up to date (or create new to-be-replaced slots in the declaration) when we add new arguments. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Nguyễn Thái Ngọc Duy	ddf362a2a9	gpg-interface.c: use error_errno() Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
Santiago Torres	d281b45d75	builtin/verify-tag.c: ignore SIGPIPE in gpg-interface The verify_signed_buffer() function may trigger a SIGPIPE when the GPG child process terminates early (due to a bad keyid, for example) and Git tries to write to it afterwards. Previously, ignoring SIGPIPE was done in builtin/verify-tag.c to avoid this issue. However, any other caller who wants to call verify_signed_buffer() would have to do the same. Use sigchain_push(SIGPIPE, SIG_IGN) in verify_signed_buffer(), pretty much like in sign_buffer(), so that any caller is not required to perform this task. This will avoid possible mistakes by further developers using verify_signed_buffer(). Signed-off-by: Santiago Torres <santiago@nyu.edu> Reviewed-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>	9 years ago
brian m. carlson	aeff29dd4d	verify-commit: add option to print raw gpg status information verify-commit by default displays human-readable output on standard error. However, it can also be useful to get access to the raw gpg status information, which is machine-readable, allowing automated implementation of signing policy. Add a --raw option to make verify-commit produce the gpg status information on standard error instead of the human-readable format. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	10 years ago
brian m. carlson	ca194d50b8	gpg: centralize printing signature buffers The code to handle printing of signature data from a struct signature_check is very similar between verify-commit and verify-tag. Place this in a single function. verify-tag retains its special case behavior of printing the tag even when no valid signature is found. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	10 years ago
brian m. carlson	434060ec6d	gpg: centralize signature check verify-commit and verify-tag both share a central codepath for verifying commits: check_signature. However, verify-tag exited successfully for untrusted signature, while verify-commit exited unsuccessfully. Centralize this signature check and make verify-commit adopt the older verify-tag behavior. This behavior is more logical anyway, as the signature is in fact valid, whether or not there's a path of trust to the author. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	10 years ago
brian m. carlson	a4cc18f293	verify-tag: share code with verify-commit verify-tag was executing an entirely different codepath than verify-commit, except for the underlying verify_signed_buffer. Move much of the code from check_commit_signature to a generic check_signature function and adjust both codepaths to call it. Update verify-tag to explicitly output the signature text, as we now call verify_signed_buffer with strbufs to catch the output, which prevents it from being printed automatically. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>	10 years ago
Junio C Hamano	d7c67668fe	gpg-interface: move parse_signature() to where it should be Our signed-tag objects set the standard format used by Git to store GPG-signed payload (i.e. the payload followed by its detached signature) [1], and it made sense to have a helper to find the boundary between the payload and its signature in tag.c back then. Newer code added later to parse other kinds of objects that learned to use the same format to store GPG-signed payload (e.g. signed commits), however, kept using the helper from the same location. Move it to gpg-interface; the helper is no longer about signed tag, but it is how our code and data interact with GPG. [Reference] 1 http://thread.gmane.org/gmane.linux.kernel/297998/focus=1383 Signed-off-by: Junio C Hamano <gitster@pobox.com>	11 years ago
Junio C Hamano	a50e7ca321	gpg-interface: move parse_gpg_output() to where it should be Earlier, `ffb6d7d5` (Move commit GPG signature verification to commit.c, 2013-03-31) moved this helper that used to be in pretty.c (i.e. the output code path) to commit.c for better reusability. It was a good first step in the right direction, but still suffers from a myopic view that commits will be the only thing we would ever want to sign---we would actually want to be able to reuse it even wider. The function interprets what GPG said; gpg-interface is obviously a better place. Move it there. Signed-off-by: Junio C Hamano <gitster@pobox.com>	11 years ago

1 2

74 Commits (3ed77c47922f6c8806fd83121dbacf90e89ded46)