-
Notifications
You must be signed in to change notification settings - Fork 5
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
Backwards-compatible support for @cert-authority #9
Conversation
The previous commit d314bf3 added support for @cert-authority lines, but technically broke backwards compatibility due to changing the return type of one exported method. This commit adjusts that previous commit's new logic to restore backwards compatibility, and makes additional changes as follows: * Introduce new exported type HostKeyDB, which handles @cert-authority lines correctly and is returned by NewDB; old exported type HostKeyCallback (which is returned by New) omits that handling. Git-specific use-cases can likely remain with using New, since Git forges typically don't support CAs. Non-Git use-cases, such as general-purpose SSH clients, should consider switching to NewDB to get the CA logic. * When NewDB re-reads the known_hosts files to implement the CA support, it only re-reads each file a single time (vs potentially multiple times at callback execution time in d314bf3), and it reads using buffered IO similar to x/crypto/ssh/knownhosts. * This package's PublicKey struct now exports its Cert boolean field, vs keeping it private in d314bf3. * Refactor the RSA-to-algo expansion logic to simplify its handling in the CA situation. * Add test coverage for all new behaviors and @cert-authority logic.
Hi @evanelias, Thank you so much for taking the initiative to improve my original proposal. I do think this is a better choice going forward considering the number of users that depend on this repository. Making the functionality opt-in seems sensible to me. Curiously enough, I do need this functionality for a git ssh backend that uses certificate authorities. We have an internal tool that was recently migrated from plain I will test this during the next couple of days and give you some feedback if it all seems to work as intended, but on first sight, it looks good to me.
I agree with you. I tend to really dislike globals for many reasons, like concurrency issues (to avoid concurrent mutations you'd probably introduce an undesired lock) and just the plain fact that over the runtime of a program, the host key callbacks can also change, so keeping them around doesn't sound like the best idea. I like this alternative best. |
It is correct that the returned result from |
Oh interesting, I didn't realize anyone used CAs for Git use-cases. I'll revise the package and method doc comments later today to remove the stuff about Git use-cases being fine to remain on the old
Sounds good. Probably the best path would be to just have go-git switch to using Just to be safe, I'll add a method to make this easier, allowing conversion from
Thank you, that sounds great. |
@abakum yes, that is correct and intentional. After switching to |
* Add new exported method HostKeyCallback.ToDB, to provide a mechanism for callers who want to conditionally enable or disable CA support, while still using a *HostKeyDB for both cases. * Clarify many doc string comments. * Add new exported function WriteKnownHostCA for writing a @cert-authority line to a known_hosts file. Previously this logic was in a test helper, but it could be useful to others, so let's export it outside of the tests.
Thanks, evanelias, it works for me! |
@evanelias, if line in known_hosts like
then kh.HostKeyAlgorithms("127.0.0.1:22") return [ecdsa-sha2-nistp256-cert-v01@openssh.com] |
@abakum Interesting catch, thanks. But the core host-matching logic is still handled by x/crypto/ssh/knownhosts, we don't re-implement or change that here. The match logic in x/crypto/ssh/knownhosts appears to only apply wildcards like * to the hostname portion, not the port, per https://cs.opensource.google/go/x/crypto/+/refs/tags/v0.24.0:ssh/knownhosts/knownhosts.go;l=110 return wildcardMatch([]byte(p.addr.host), []byte(a.host)) && p.addr.port == a.port So in order to match I just searched the issue tracker for https://github.com/golang/go and found golang/go#52056 which seems to describe the root of the problem. I've commented there now too. |
On second thought, it might be possible to build a work-around here in skeema/knownhosts for that problem with wildcards and non-standard ports. The callback logic I'm envisioning will need to be nested and tricky, so I'm not 100% certain this is feasible, but I can give this a try sometime in the next few days. |
This comment was marked as outdated.
This comment was marked as outdated.
This is a good point, thank you. I drafted here how it would look like to use the knownhosts DB. Javier-varez/go-git@0879ef1
On this front, I tested the change, and it works great for my usecase. Thank you! |
I'm trying to re-check https://github.com/abakum/knownhosts/blob/0280d4dc9533ee3f92de6b57a5efb45087cbf3e0/cmd/main.go
|
@Javier-varez awesome, thank you! re: proposed go-git changes, makes sense and looks good. Might need to leave the old NewKnownHostsCallback in place too though as deprecated, since it's exported and could be used by third-party code, unless their versioning policy isn't strict about this. Inside go-git itself that function is also called from a unit test -- see plumbing/transport/ssh/auth_method_test.go @abakum can you please summarize the finding? Is it just confirming that * wildcards are not working with non-standard ports, or something else? Thanks! |
Yes golang not working but OpenSSH working |
Thank you both again for the testing assistance! I'm going to merge this momentarily. Early next week, I'll do a separate branch / pull request with a fix for the wildcards on non-standard port issue. And then once that one looks good and gets merged too, I'll tag a new release. |
Thanks, @evanelias ! |
*** This is a work-in-progress commit, which will be amended/rewritten ***
Just opened #10 for the wildcard host matching fix, along with some additional documentation/README tweaks. |
I've just tagged v1.3.0 which includes this PR, as well as the wildcard matching fix in #10. Thanks again @Javier-varez and @abakum for all your assistance here! |
skeema/knownhosts v1.3.0 introduced a HostKeyDB type that extends the HostKeyCallback functionality to support @cert-authority algorithms. `known_hosts` files may contain lines with @cert-authority markers to indicate that a line corresponds to a certificate instead of a key. If a git remote uses cert authorities as the preferred host identification mechanism, the functionality added in skeema/knownhosts v1.3.0 is needed so that go-git can interact with this remote. See skeema/knownhosts#9 for details.
skeema/knownhosts v1.3.0 introduced a HostKeyDB type that extends the HostKeyCallback functionality to support @cert-authority algorithms. `known_hosts` files may contain lines with @cert-authority markers to indicate that a line corresponds to a certificate instead of a key. If a git remote uses cert authorities as the preferred host identification mechanism, the functionality added in skeema/knownhosts v1.3.0 is needed so that go-git can interact with this remote. See skeema/knownhosts#9 for details.
skeema/knownhosts v1.3.0 introduced a HostKeyDB type that extends the HostKeyCallback functionality to support @cert-authority algorithms. `known_hosts` files may contain lines with @cert-authority markers to indicate that a line corresponds to a certificate instead of a key. If a git remote uses cert authorities as the preferred host identification mechanism, the functionality added in skeema/knownhosts v1.3.0 is needed so that go-git can interact with this remote. See skeema/knownhosts#9 for details.
| Package | Package file | Manager | Update | Change | |---|---|---|---|---| | [skeema](https://github.com/skeema/skeema) | misk/bin/hermit | hermit | patch | `1.12.0` -> `1.12.1` | --- ### Release Notes <details> <summary>skeema/skeema (skeema)</summary> ### [`v1.12.1`](https://github.com/skeema/skeema/releases/tag/v1.12.1) - **MySQL 9.0 and 9.1** support ([`7c94fcb`](skeema/skeema@7c94fcb), [`47bfc0a`](skeema/skeema@47bfc0a), [`a0a060a`](skeema/skeema@a0a060a)) - The new `VECTOR` column type is supported. In [`skeema diff`](https://www.skeema.io/docs/commands/diff/) and [`skeema push`](https://www.skeema.io/docs/commands/push/), altering a column type between `VECTOR` and any other sufficiently-large binary type is permitted as a [safe operation](https://www.skeema.io/docs/features/safety/#unsafe-change-detection) since the conversion is non-lossy. - MySQL 9 finally processes "inline" foreign key definitions (that is, `REFERENCES` clause in a column definition). These are supported as-is in Skeema. - **MariaDB 11.5 and 11.6** support ([`6165c90`](skeema/skeema@6165c90), [`f24ad30`](skeema/skeema@f24ad30)) - MariaDB 11.5 changes the default collation for Unicode charsets to use uca1400\_ai_ci collations, which are fully supported in Skeema. - MariaDB 11.5 solves the `TIMESTAMP` col type's previous Y2K38 limitation, and Skeema's [lint-has-time](https://www.skeema.io/docs/options/#lint-has-time) annotation message has been adjusted accordingly. - **`CHECK` constraint improvement**: When a diff only affects the *name* of a `CHECK` constraint without modifying its check expression, [`skeema diff`](https://www.skeema.io/docs/commands/diff/) and [`skeema push`](https://www.skeema.io/docs/commands/push/) now ignore this cosmetic change by default. This improves compatibility with external [OSC tools](https://www.skeema.io/docs/features/osc/), which inherently need to rename `CHECK` constraints as part of their operation. This new behavior can be overridden by enabling the [--exact-match](https://www.skeema.io/docs/options/#exact-match) option. ([`f000616`](skeema/skeema@f000616)) - **[Event](https://www.skeema.io/docs/features/events/) handling** improvements and fixes ([Skeema Premium](https://www.skeema.io/download/)) - When an event diff only included a change to the `DEFINER` clause, and no other differences, the `ALTER EVENT` emitted by Skeema was not valid SQL (despite conforming to syntax in the MySQL and MariaDB manuals). To fix this situation, the SQL will now also include an additional no-op clause, such as `ENABLE` for an event that is already enabled. - Several dump normalizations for `CREATE EVENT` statements were inadvertently omitted the first time an event was dumped by [`skeema init`](https://www.skeema.io/docs/commands/init/) or [`skeema pull`](https://www.skeema.io/docs/commands/pull/). - If any workspace query failed (e.g. query timeout), and any events were present in the \*.sql definitions, a panic would result instead of the intended workspace query failure error message. ([#​229](skeema/skeema#229)) - **[SSH tunnel](https://www.skeema.io/docs/features/ssh/) enhancements** ([Skeema Premium](https://www.skeema.io/download/)) - CAs, which are indicated in the known_hosts file using `@cert-authority` lines, are now fully supported. ([skeema/knownhosts#8](skeema/knownhosts#8), [skeema/knownhosts#9](skeema/knownhosts#9)) - known_hosts lines using non-default ports are now matched properly. ([skeema/knownhosts#10](skeema/knownhosts#10)) - If any hand-written \*.sql files use the optional **`CREATE OR REPLACE` SQL syntax**, Skeema now parses and ignores the `OR REPLACE` clause. Previously, use of this syntax would prevent Skeema from parsing the statement. ([`6805737`](skeema/skeema@6805737)) - Enhancements for [Docker workspaces](https://www.skeema.io/docs/features/workspaces/#docker-workspaces) - Significant performance improvements for several common situations ([`d348249`](skeema/skeema@d348249), [`ca85df7`](skeema/skeema@ca85df7), [`7a40155`](skeema/skeema@7a40155)) - When using Percona Server 8.x, the Docker image / point release selection logic has been improved ([`e09350c`](skeema/skeema@e09350c), [`fe55d62`](skeema/skeema@fe55d62), [`af1b3b5`](skeema/skeema@af1b3b5)) - When a redundant non-unique index is flagged by [lint-dupe-index](https://www.skeema.io/docs/options/#lint-dupe-index), the annotation message is now clearer (since false positives may be possible) and suggests making the index be `INVISIBLE` / `IGNORED` before dropping ([#​238](skeema/skeema#238), [#​237](skeema/skeema#237)) - MariaDB's August 2024 point releases have changed the formatting of compressed columns in `SHOW CREATE TABLE`, which affected Skeema's [diff logic safeguards](https://www.skeema.io/docs/features/safety/#table-introspection-validation). This change is now handled and compressed columns are fully supported again. ([`49aed41`](skeema/skeema@49aed41)) - Minor wording changes in log messages and help text, for consistency. ([`4f8fa44`](skeema/skeema@4f8fa44), [`5f7598e`](skeema/skeema@5f7598e)) **Thank you** to all code contributors and issue reporters! An [installation guide](https://www.skeema.io/docs/install/) and [full documentation](https://www.skeema.io/docs/) are available on our website [skeema.io](https://www.skeema.io/). </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 6pm every weekday,before 2am every weekday" in timezone Australia/Melbourne, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). GitOrigin-RevId: 90a37c8a0c3c86b3fa245502cfeefd429129ebed
This pull request adds support for @cert-authority lines in known_hosts files in an optional, backwards-compatible manner. Fixes #7.
With this PR's changes, users can switch from
knownhosts.New()
to the newly-introducedknownhosts.NewDB()
in order to opt-in to the @cert-authority support. This returns a newHostKeyDB
struct instead of aHostKeyCallback
. It has a very slight performance penalty as it requires re-reading the known_hosts file, and it has a different return type of itsHostKeys()
method, but otherwise it should be a drop-in replacement for users who require CA support. This makes sense for use-cases such as general-purpose SSH clients.Git-specific SSH use-cases can likely stay on[Edit: not necessarily true, see discussion in comments below.]knownhosts.New()
, since public Git forges (GitHub, Gitlab, etc) don't seem to use CAs for host keys anyway.All previous
HostKeyCallback
logic remains backwards-compatible and avoids any functionality changes. [Edit to clarify: When usingknownhosts.New
orknownhosts.HostKeyCallback
directly, there is no CA support. To get the CA support, your calling code must switch to usingknownhosts.NewDB
instead.]Implementation
This includes @Javier-varez's commit from #8 as-is, and then adds an additional commit on top to adjust the following:
Move the CA support to new
HostKeyDB
struct, making it opt-in. This avoids changing the function signature of theHostKeyCallback.HostKeys()
method, in order to retain backwards compatibility and avoiding a v2 version bump for the module.When re-reading the known_hosts files to implement the CA support, it only re-reads each file a single time, at constructor time instead of in the callback. It reads using buffered IO similar to x/crypto/ssh/knownhosts which should ensure its line-counting behavior matches.
Add test coverage for all new behaviors and @cert-authority logic.
Alternatives considered
Conceptually the information on @cert-authority lines needs to be tracked somewhere, but the difficulty with the previous design is that
New()
returns aHostKeyCallback
which is just a function, rather than a struct. So the chosen solution here leaves that type as-is, and instead introduces a separate new struct which supports adding more fields.Instead of introducing a separate new struct, an alternative approach would have been to use a non-exported package global of the form map[*HostKeyCallback]CertInfo, in order to track additional information on each HostKeyCallback. This would result in simpler user-facing logic, however it would then require a separate function to "de-register" a callback to avoid a memory leak. Overall that seems hackier, and less extensible if additional metadata fields are needed in the future.
Testing and feedback
The PR includes unit test coverage, but it could use some further real-world testing to ensure it properly solves #7. I will keep this PR open a few days, and hugely appreciate any community feedback.
cc @lonnywong @abakum @Javier-varez