Use known host algorithm #707
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 2 commits
August 12, 2021 14:00
Try to find the Algorithm that was used when a known_host entry was created and make that the first choice for the current connection attempt. If the current connection algorithm matches the algorithm used when the known_host entry was created we can get a fair verification.
This code base has a lot of instances where an exception is thrown as part of the logic path. That makes it very hard to follow and debug. In Scala I use built in Types to encapsulate the results of a method calls that may have non-determinate results. I wrote something like this version of PossibleResult for a Java project recently, it used Optional instead of null. You may want to include this and use it, or not.
|
This works as far as I can tell. I didn't add any tests. All your unit tests still pass. Since I haven't use Gradle in years I didn't try to get into those tests at all. It fixes my issue with accessing hosts with varying known host algorithms. |
The hostnames saved in known_hosts are forced to lower case, we need to do the same to match.
|
After bringing this up to date with master, it breaks the integration tests. |
|
I'll pull and look at it
…On September 20, 2021 7:53:03 AM Jeroen van Erp ***@***.***> wrote:
After bringing this up to date with master, it breaks the integration tests.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
I looked to pull on github, but it says I am up to date. I did add one
thing which wasn't in the pull request. Known_hosts has forced lowercase
host names.
I am just looking at the tests that run when gradlew starts. Is there some
other way I need to test?
…On September 20, 2021 7:53:03 AM Jeroen van Erp ***@***.***> wrote:
After bringing this up to date with master, it breaks the integration tests.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
Sorry for being out of the norm on this. I don't get to do github much. But
now I see the failure and the method. Github really has a lot to offer.
…On September 20, 2021 7:53:03 AM Jeroen van Erp ***@***.***> wrote:
After bringing this up to date with master, it breaks the integration tests.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
|
We all learn ;) BTW, I think the PossibleResult class that you added is dead code? Can you remove it from the PR? |
|
Yeah, I get that.
…On September 21, 2021 9:53:47 AM Jeroen van Erp ***@***.***> wrote:
We all learn ;)
BTW, I think the PossibleResult class that you added is dead code? Can you
remove it from the PR?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
If key-type from known_hosts isn't in the configured key algorithm list, don't add it to the list of algorithms to try.
This kind of thing might really make it easier to follow the logic path, but as there is already a huge base of exception throwing code, retrofitting something like this would be a lot of work. Still a little in the right spot might make things a lot easier for a log time to come.
The hostnames saved in known_hosts are forced to lower case, we need to do the same to match.
* Handle @cert-authority in known_hosts. * Fix ClassCastException when receiving an ECDSA-CERT host key. * Mention what exactly is not negotiated. * Verify host key certificates during key exchange. * Unit and integration tests for host key verification. * Show sshd logs when integration test finishes. * Review fixes: extract to private method, change strings.
* Enhanced PKCS8 parsing to support PEM ASN.1 Private Keys * Corrected copyright year to match existing license headers
- Added unit tests for encrypted PKCS8 RSA Private Key
- Renamed BCryptTest and updated using JUnit Test annotations
* Support v3 PuTTY keys * add test for putty v3 key * Format PuTTYKeyFile to fix Codacy warnings Signed-off-by: Jeroen van Erp <jeroen@hierynomus.com> Co-authored-by: Jeroen van Erp <jeroen@hierynomus.com>
If key-type from known_hosts isn't in the configured key algorithm list, don't add it to the list of algorithms to try.
|
Is there a better way to communicate with you?
I don't see a way to remove the
PossibleResult commit, I committed the deletion of it, but that doesn't
seem like the goal. I rebased, but got grief about merging that.
Unless you have a better idea I'll just make the changes again in a clean repo.
…On September 21, 2021 9:53:47 AM Jeroen van Erp ***@***.***> wrote:
We all learn ;)
BTW, I think the PossibleResult class that you added is dead code? Can you
remove it from the PR?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.
Triage notifications on the go with GitHub Mobile for iOS or Android.
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
#642 #635 etc.
There are a lot of issues with this, in various words. I hit it first thing when I tried an example, and as I really need a new SSH library and my outside fix was dirty, here is a patch.
I haven't done this github thing very much and that was a little a long time ago. So help me out. Reading the pull request how to, it sounds like I should do this in a feature branch. I started out like that but messed up and didn't want to push a dirty commit. So I can easily do this again if that's better.