Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Squash leaks in t0000 #1092

Closed
wants to merge 150 commits into from
Closed

Squash leaks in t0000 #1092

wants to merge 150 commits into from

Conversation

ahunt
Copy link
Contributor

@ahunt ahunt commented Sep 18, 2021

Carlo points out that t0000 currently doesn't pass with leak-checking enabled in:
https://public-inbox.org/git/CAPUEsphMUNYRACmK-nksotP1RrMn09mNGFdEHLLuNEWH4AcU7Q@mail.gmail.com/T/#m7e40220195d98aee4be7e8593d30094b88a6ee71

Here's a series that I've sat on for a while, which adds some UNLEAK's to "fix" this situation - see the individual patches for a justification of why an UNLEAK seems appropriate.

ATB,
Andrzej

CC: Carlo Arenas carenas@gmail.com
cc: Ævar Arnfjörð Bjarmason avarab@gmail.com
cc: Eric Sunshine sunshine@sunshineco.com
cc: Jeff King peff@peff.net

FStelzer and others added 30 commits August 3, 2021 16:38
Openssh v8.2p1 added some new options to ssh-keygen for signature
creation and verification. These allow us to use ssh keys for git
signatures easily.

In our corporate environment we use PIV x509 Certs on Yubikeys for email
signing/encryption and ssh keys which I think is quite common
(at least for the email part). This way we can establish the correct
trust for the SSH Keys without setting up a separate GPG Infrastructure
(which is still quite painful for users) or implementing x509 signing
support for git (which lacks good forwarding mechanisms).
Using ssh agent forwarding makes this feature easily usable in todays
development environments where code is often checked out in remote VMs / containers.
In such a setup the keyring & revocationKeyring can be centrally
generated from the x509 CA information and distributed to the users.

To be able to implement new signing formats this commit:
 - makes the sigc structure more generic by renaming "gpg_output" to
   "output"
 - introduces function pointers in the gpg_format structure to call
   format specific signing and verification functions
 - moves format detection from verify_signed_buffer into the check_signature
   api function and calls the format specific verify
 - renames and wraps sign_buffer to handle format specific signing logic
   as well

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Generate some ssh keys and a allowedSignersFile for testing

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Implements the actual sign_buffer_ssh operation and move some shared
cleanup code into a strbuf function

Set gpg.format = ssh and user.signingkey to either a ssh public key
string (like from an authorized_keys file), or a ssh key file.
If the key file or the config value itself contains only a public key
then the private key needs to be available via ssh-agent.

gpg.ssh.program can be set to an alternative location of ssh-keygen.
A somewhat recent openssh version (8.2p1+) of ssh-keygen is needed for
this feature. Since only ssh-keygen is needed it can this way be
installed seperately without upgrading your system openssh packages.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
If user.signingkey is not set and a ssh signature is requested we call
gpg.ssh.defaultKeyCommand (typically "ssh-add -L") and use the first key we get

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
For ssh the user.signingkey can be a filename/path or even a literal ssh pubkey.
In push certs and textual output we prefer the ssh fingerprint instead.

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
To verify a ssh signature we first call ssh-keygen -Y find-principal to
look up the signing principal by their public key from the
allowedSignersFile. If the key is found then we do a verify. Otherwise
we only validate the signature but can not verify the signers identity.

Verification uses the gpg.ssh.allowedSignersFile (see ssh-keygen(1) "ALLOWED
SIGNERS") which contains valid public keys and a principal (usually
user@domain). Depending on the environment this file can be managed by
the individual developer or for example generated by the central
repository server from known ssh keys with push access. This file is usually
stored outside the repository, but if the repository only allows signed
commits/pushes, the user might choose to store it in the repository.

To revoke a key put the public key without the principal prefix into
gpg.ssh.revocationKeyring or generate a KRL (see ssh-keygen(1)
"KEY REVOCATION LISTS"). The same considerations about who to trust for
verification as with the allowedSignersFile apply.

Using SSH CA Keys with these files is also possible. Add
"cert-authority" as key option between the principal and the key to mark
it as a CA and all keys signed by it as valid for this CA.
See "CERTIFICATES" in ssh-keygen(1).

Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Fabian Stelzer <fs@gigacodes.de>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
"git bisect" spawned "git show-branch" only to pretty-print the
title of the commit after checking out the next version to be
tested; this has been rewritten in C.

* jc/bisect-sans-show-branch:
  bisect: simplify return code from bisect_checkout()
  bisect: do not run show-branch just to show the current commit
"git add" can work better with the sparse index.

* ds/add-with-sparse-index:
  add: remove ensure_full_index() with --renormalize
  add: ignore outside the sparse-checkout in refresh()
  pathspec: stop calling ensure_full_index
  add: allow operating on a sparse-only index
  t1092: test merge conflicts outside cone
Support for ancient versions of cURL library has been dropped.

* ab/http-drop-old-curl:
  http: rename CURLOPT_FILE to CURLOPT_WRITEDATA
  http: drop support for curl < 7.19.3 and < 7.17.0 (again)
  http: drop support for curl < 7.19.4
  http: drop support for curl < 7.16.0
  http: drop support for curl < 7.11.1
Input validation of "git pack-objects --stdin-packs" has been
corrected.

* ab/pack-stdin-packs-fix:
  pack-objects: fix segfault in --stdin-packs option
  pack-objects tests: cover blindspots in stdin handling
Prepare the "ref-filter" machinery that drives the "--format"
option of "git for-each-ref" and its friends to be used in "git
cat-file --batch".

* zh/ref-filter-raw-data:
  ref-filter: add %(rest) atom
  ref-filter: use non-const ref_format in *_atom_parser()
  ref-filter: --format=%(raw) support --perl
  ref-filter: add %(raw) atom
  ref-filter: add obj-type check in grab contents
Doc update.

* ab/bundle-doc:
  bundle doc: replace "basis" with "prerequsite(s)"
  bundle doc: elaborate on rev<->ref restriction
  bundle doc: elaborate on object prerequisites
  bundle doc: rewrite the "DESCRIPTION" section
Pathname expansion (like "~username/") learned a way to specify a
location relative to Git installation (e.g. its $sharedir which is
$(prefix)/share), with "%(prefix)".

* js/expand-runtime-prefix:
  expand_user_path: allow in-flight topics to keep using the old name
  interpolate_path(): allow specifying paths relative to the runtime prefix
  Use a better name for the function interpolating paths
  expand_user_path(): clarify the role of the `real_home` parameter
  expand_user_path(): remove stale part of the comment
  tests: exercise the RUNTIME_PREFIX feature
Final batch for "merge -sort" optimization.

* en/ort-perf-batch-15:
  merge-ort: remove compile-time ability to turn off usage of memory pools
  merge-ort: reuse path strings in pool_alloc_filespec
  merge-ort: store filepairs and filespecs in our mem_pool
  diffcore-rename, merge-ort: add wrapper functions for filepair alloc/dealloc
  merge-ort: switch our strmaps over to using memory pools
  merge-ort: set up a memory pool
  merge-ort: add pool_alloc, pool_calloc, and pool_strndup wrappers
  diffcore-rename: use a mem_pool for exact rename detection's hashmap
  merge-ort: rename str{map,intmap,set}_func()
A handful of tests that assumed implementation details of files
backend for refs have been cleaned up.

* hn/refs-test-cleanup:
  t6001: avoid direct file system access
  t6500: use "ls -1" to snapshot ref database state
  t7064: use update-ref -d to remove upstream branch
  t1410: mark test as REFFILES
  t1405: mark test for 'git pack-refs' as REFFILES
  t1405: use 'git reflog exists' to check reflog existence
  t2402: use ref-store test helper to create broken symlink
  t3320: use git-symbolic-ref rather than filesystem access
  t6120: use git-update-ref rather than filesystem access
  t1503: mark symlink test as REFFILES
  t6050: use git-update-ref rather than filesystem access
trace2 logs learned to show parent process name to see in what
context Git was invoked.

* es/trace2-log-parent-process-name:
  tr2: log parent process name
  tr2: make process info collection platform-generic
Bugfix for common ancestor negotiation recently introduced in "git
push" codepath.

* jt/push-negotiation-fixes:
  fetch: die on invalid --negotiation-tip hash
  send-pack: fix push nego. when remote has refs
  send-pack: fix push.negotiate with remote helper
Loading of ref tips to prepare for common ancestry negotiation in
"git fetch-pack" has been optimized by taking advantage of the
commit graph when available.

* ps/fetch-pack-load-refs-optim:
  fetch-pack: speed up loading of refs via commit graph
"git pull" had various corner cases that were not well thought out
around its --rebase backend, e.g. "git pull --ff-only" did not stop
but went ahead and rebased when the history on other side is not a
descendant of our history.  The series tries to fix them up.

* en/pull-conflicting-options:
  pull: fix handling of multiple heads
  pull: update docs & code for option compatibility with rebasing
  pull: abort by default when fast-forwarding is not possible
  pull: make --rebase and --no-rebase override pull.ff=only
  pull: since --ff-only overrides, handle it first
  pull: abort if --ff-only is given and fast-forwarding is impossible
  t7601: add tests of interactions with multiple merge heads and config
  t7601: test interaction of merge/rebase/fast-forward flags and options
Documentation updates.

* en/merge-strategy-docs:
  Update error message and code comment
  merge-strategies.txt: add coverage of the `ort` merge strategy
  git-rebase.txt: correct out-of-date and misleading text about renames
  merge-strategies.txt: fix simple capitalization error
  merge-strategies.txt: avoid giving special preference to patience algorithm
  merge-strategies.txt: do not imply using copy detection is desired
  merge-strategies.txt: update wording for the resolve strategy
  Documentation: edit awkward references to `git merge-recursive`
  directory-rename-detection.txt: small updates due to merge-ort optimizations
  git-rebase.txt: correct antiquated claims about --rebase-merges
Use `ort` instead of `recursive` as the default merge strategy.

* en/ort-becomes-the-default:
  Update docs for change of default merge backend
  Change default merge backend from recursive to ort
Debugging aid.

* js/log-protocol-version:
  connect, protocol: log negotiated protocol version
The revision traversal API has been optimized by taking advantage
of the commit-graph, when available, to determine if a commit is
reachable from any of the existing refs.

* ps/connectivity-optim:
  revision: avoid hitting packfiles when commits are in commit-graph
  commit-graph: split out function to search commit position
  revision: stop retrieving reference twice
  connected: do not sort input revisions
  revision: separate walk and unsorted flags
Code clean-up.

* cb/builtin-merge-format-string-fix:
  builtin/merge: avoid -Wformat-extra-args from ancient Xcode
Remind developers that the userdiff patterns should be kept simple
and permissive, assuming that the contents they apply are always
syntactically correct.

* jc/userdiff-pattern-hint:
  userdiff: comment on the builtin patterns
"git apply" miscounted the bytes and failed to read to the end of
binary hunks.

* jk/apply-binary-hunk-parsing-fix:
  apply: keep buffer/size pair in sync when parsing binary hunks
"git range-diff" code clean-up.

* jk/range-diff-fixes:
  range-diff: use ssize_t for parsed "len" in read_patches()
  range-diff: handle unterminated lines in read_patches()
  range-diff: drop useless "offset" variable from read_patches()
The userdiff pattern for "java" language has been updated.

* th/userdiff-more-java:
  userdiff: improve java hunk header regex
@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

There was a status update in the "Discarded" section about the branch ah/unleak-revisions on the Git mailing list:

Mark a few structures with UNLEAK() to help leak detection CI jobs.

Retracted.
cf. <05754f9c-cd58-30f5-e2d3-58b9221d2770@ahunt.org>

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

3 similar comments
@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

There was a status update in the "Discarded" section about the branch ah/unleak-revisions on the Git mailing list:

Mark a few structures with UNLEAK() to help leak detection CI jobs.

Retracted.
cf. <05754f9c-cd58-30f5-e2d3-58b9221d2770@ahunt.org>

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

19 similar comments
@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@gitgitgadget-git
Copy link

Found multiple candidates in gitster/git:
refs/remotes/gitster/ah/unleak-revisions
refs/remotes/gitster/ah/unreak-revisions;

Using the first one.

@ahunt
Copy link
Contributor Author

ahunt commented Dec 4, 2021

Closing this PR because I think it's obsolete (I haven't had enough time to keep up with the latest developments, but I suspect the leak has been fixed properly by now given all the recent work in that area).

@ahunt ahunt closed this Dec 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
3 participants