8000 Pass hostkey & port to host verify callback by fxcoudert · Pull Request #6503 · libgit2/libgit2 · GitHub
[go: up one dir, main page]
Skip to content
/ libgit2 Public

Pass hostkey & port to host verify callback #6503

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Feb 24, 2023
Merged

Pass hostkey & port to host verify callback #6503

merged 3 commits into from
Feb 24, 2023

Conversation

fxcoudert
Copy link
Contributor
@fxcoudert fxcoudert commented Feb 24, 2023 •
edited
Loading

This is a patch that has been applied for 2 years in Julia. Quoting from @StefanKarpinski in his original pull request:

It seems that no one actually verifies SSH host identity with libgit2 because the callback doesn't give enough information do so correctly:

  • It doesn't give the actual host key fingerprint, but rather three
    different hashes thereof. This means we cannot distinguish a known
    hosts entry that has a different type (ssh-rsa, ssh-dsa, etc.)
    from an entry with a matching type and a fingerprint mismatch: the
    former should be treated as an unknown host whereas the latter is a
    host key mismatch; they cannot be distinguished without this patch.
  • If the user connects on a non-default port (i.e. not 22), this is not
    passed to the callback in any way. Since there can be different known
    host entries for different ports and they should be treated as
    distinct, this also means the current API cannot be used to verify
    hosts serving SSH on non-standard ports. This patch passes the port.

I will try to upstream some version of this patch to libgit2.

I have adapted the patch slightly. I do not know libgit2 internals enough to know whether snprintf and alloca are the best way to handle the memory allocation/string printing in that place.

@fxcoudert fxcoudert mentioned this pull request Feb 24, 2023
Merged
@fxcoudert
Copy link
Contributor Author
  • Make SSH_DEFAULT_PORT an integer instead of a string (there are no current users of that macro in the file)
  • Avoid alloca() which seems to be problematic on Windows builders

ethomson
ethomson reviewed Feb 24, 2023
View reviewed changes
src/libgit2/transports/ssh.c
size_t n = strlen(host) + 5 + 2;
host_and_port = malloc(n);
snprintf(host_and_port, n, "%s:%d", host, port);
}
Copy link
Member
@ethomson ethomson Feb 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should use our git_str class here, and we should avoid the unnecessary strdup in the common case. Something more like:

const char *host_ptr = host;
git_str host_and_port = GIT_STR_INIT;

if (port != SSH_DEFAULT_PORT) {
    git_str_printf(&host_and_port, "%s:%d", host, port);
    host_ptr = host_and_port.ptr;
}

// use host_ptr

git_str_free(&host_and_port);

boretrk
boretrk reviewed Feb 24, 2023
View reviewed changes
src/libgit2/transports/ssh.c
const char *host_ptr = host;
git_str host_and_port = GIT_STR_INIT;

if (port != SSH_DEFAULT_PORT) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_git_ssh_setup_conn sets port to -1 to indicate default but here it is compared to SSH_DEFAULT_PORT that is defined as 22.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does. But git_net_url_parse and friends should enforce that the port is a number, and set them to the default if the port number is omitted. So the port = -1 should never be reached. As a result, I changed the strtol failure to an actual error. Good catch, thanks!

@ethomson
Copy link
Member

Thanks @fxcoudert ! I'm sorry to hear that Julia has been carrying patches around for a few years. 😢

@ethomson ethomson merged commit f7325c4 into libgit2:main Feb 24, 2023
@fxcoudert fxcoudert deleted the hostandport branch February 25, 2023 10:07
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request Feb 25, 2023
@0-wiz-0
libgit2: update to 1.6.0.
f199d3f 
v1.6
----

This is release v1.6.0, "Hubbeliges Krokodil". This release adds experimental SHA256 support and includes many new
8000
 features and bugfixes.

## What's Changed

### New features

* **Support for bare repositories with SHA256 support (experimental)** by @ethomson in libgit2/libgit2#6191
   You can configure experimental SHA256 support in libgit2 with `cmake -DEXPERIMENTAL_SHA256=ON` during project setup. This is useful for considering future integrations, work on clients, and work on language bindings. At present, working with bare repositories should largely work, including remote operations. But many pieces of functionality - including working with the index - are not yet supported. As a result, **libgit2 with SHA256 support should not be used in production or released with package distribution.**

* **Support the notion of a home directory separately from global configuration directory** by @ethomson in libgit2/libgit2#6455 and libgit2/libgit2#6456
  Callers and language bindings can now configure the home directory that libgit2 uses for file lookups (eg, the `.ssh` directory). This configuration is separate from the git global configuration path.

* **stash: partial stash specific files** by @gitkraken-jacobw in libgit2/libgit2#6330
  A stash can be created with only specific files, using a pathspec. This is similar to the `git stash push` command.

* **push: revparse refspec source, so you can push things that are not refs** by @sven-of-cord in libgit2/libgit2#6362
  Pushes can be performed using refspecs instead of only references.

* **Support OpenSSL3** by @ethomson in libgit2/libgit2#6464 and libgit2/libgit2#6471
  OpenSSL 3 is now supported, both when compiled directly and dynamically loaded.

### Bug fixes
* winhttp: support long custom headers by @kcsaul in libgit2/libgit2#6363
* Fix memory leak by @csware in libgit2/libgit2#6382
* Don't fail the whole clone if you can't find a default branch by @torvalds in libgit2/libgit2#6369
* #6366: When a worktree is missing, return `GIT_ENOTFOUND`. by @arroz in libgit2/libgit2#6395
* commit-graph: only verify csum on `git_commit_graph_open()`. by @derrickstolee in libgit2/libgit2#6420
* Ignore missing 'safe.directory' config during ownership checks by @kcsaul in libgit2/libgit2#6408
* Fix leak in `git_tag_create_from_buffer` by @julianmesa-gitkraken in libgit2/libgit2#6421
* http: Update httpclient options when reusing an existing connection. by @slackner in libgit2/libgit2#6416
* Add support for `safe.directory *` by @csware in libgit2/libgit2#6429
* URL parsing for google-compatible URLs by @ethomson in libgit2/libgit2#6326
* Fixes #6433: `git_submodule_update` fails to update configured but missing submodule by @tagesuhu in libgit2/libgit2#6434
* transport: fix capabilities calculation by @russell in libgit2/libgit2#6435
* push: use resolved oid as the source by @ethomson in libgit2/libgit2#6452
* Use `git_clone__submodule` to avoid file checks in workdir by @abizjak in libgit2/libgit2#6444
* #6422: handle dangling symbolic refs gracefully by @arroz in libgit2/libgit2#6423
* `diff_file`: Fix crash when freeing a patch representing an empty untracked file by @jorio in libgit2/libgit2#6475
* clone: clean up options on failure by @ethomson in libgit2/libgit2#6479
* stash: update strarray usage by @ethomson in libgit2/libgit2#6487
* #6491: Sets `oid_type` on repos open with `git_repository_open_bare` by @arroz in libgit2/libgit2#6492
* Handle Win32 shares by @ethomson in libgit2/libgit2#6493
* Make failure to connect to ssh-agent non-fatal by @fxcoudert in libgit2/libgit2#6497
* odb: don't unconditionally add `oid_type` to stream by @ethomson in libgit2/libgit2#6499
* Pass hostkey & port to host verify callback by @fxcoudert in libgit2/libgit2#6503

### Security fixes

### Code cleanups
* meta: update version number to v1.6.0-alpha by @ethomson in libgit2/libgit2#6352
* sha256: indirection for experimental functions by @ethomson in libgit2/libgit2#6354
* Delete `create.c.bak` by @lrm29 in libgit2/libgit2#6398
* Support non-cmake builds with an in-tree `experimental.h` by @ethomson in libgit2/libgit2#6405

### Build and CI improvements
* tests: skip flaky-ass googlesource tests by @ethomson in libgit2/libgit2#6353
* clar: remove ftrunacte from libgit2 tests by @boretrk in libgit2/libgit2#6357
* CI Improvements by @ethomson in libgit2/libgit2#6403
* fix compile on Windows with `-DWIN32_LEAN_AND_MEAN` by @christoph-cullmann in libgit2/libgit2#6373
* Fixes #6365 : Uppercase windows.h include fails build in case-sensitive OS by @Vinz2008 in libgit2/libgit2#6377
* ci: update version numbers of actions by @ethomson in libgit2/libgit2#6448
* thread: avoid warnings when building without threads by @ethomson in libgit2/libgit2#6432
* src: hide unused hmac() prototype by @0-wiz-0 in libgit2/libgit2#6458
* tests: update clar test runner by @ethomson in libgit2/libgit2#6459
* ci: always create test summaries, even on failure by @ethomson in libgit2/libgit2#6460
* Fix build failure with `-DEMBED_SSH_PATH` by @vicr123 in libgit2/libgit2#6374
* Define correct `off64_t` for AIX by @bzEq in libgit2/libgit2#6376
* Fix some warnings in main by @ethomson in libgit2/libgit2#6480
* strarray: remove deprecated declaration by @ethomson in libgit2/libgit2#6486
* tests: always unset `HTTP_PROXY` before starting tests by @ethomson in libgit2/libgit2#6498

### Documentation improvements
* add 2-clause BSD licen
8000
se to COPYING by @martinvonz in libgit2/libgit2#6413
* Add new PHP bindings project to language bindings section of README.md by @RogerGee in libgit2/libgit2#6473
* README: clarify the linking exception by @ethomson in libgit2/libgit2#6494
* Correct the definition of "empty" in the docs for `git_repository_is_empty` by @timrogers in libgit2/libgit2#6500
@ehuss ehuss mentioned this pull request Feb 26, 2023
Closed
@ethomson ethomson mentioned this pull request Feb 27, 2023
Merged
ethomson added a commit that referenced this pull request Feb 27, 2023
@ethomson
Merge pull request #6511 from libgit2/ethomson/hostandport
c0dc97b 
Revert #6503
@ethomson ethomson mentioned this pull request Feb 28, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethomson ethomson ethomson left review comments

@boretrk boretrk boretrk left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fxcoudert @ethomson @boretrk
0