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

Improve Android compatibility #636

Merged
merged 2 commits into from
Oct 20, 2020

Conversation

fmeum
Copy link
Contributor

@fmeum fmeum commented Oct 6, 2020

  • Instead of only counting BouncyCastle as being registered if it is set as the explicit security provider used by SSHJ, count it as
    registered if it is available as a provider. This improves Android compatibility, which requires not specifying an explicit provider. This is meant to address Make KeyType compatible with Android Keystore #586 (comment)

@oakkitten Could you verify that this fixes the issue with ECDSA host keys?

  • The ECNamedCurveGenParameterSpec is a BC-specific workaround for missing curve tables in Java 1.4 and earlier. For the sake of Android compatibility, where Conscrypt can't deal with this custom spec class, replace it with the standard ECGenParameterSpec and update the curve names to the standard identifiers.

Instead of only counting BouncyCastle as being registered if it
is set as the explicit security provider used by SSHJ, count it as
registered if it is available as a provider.

This commit improves Android compatibility, which requires not
specifying an explicit provider.
@fmeum fmeum requested a review from hierynomus as a code owner October 6, 2020 08:42
@oakkitten
Copy link

i'll test the code later, but why check for BC in the first place? why not let it fail? same with the order places where the check for BC happens

@oakkitten
Copy link

oakkitten commented Oct 6, 2020

hm jitpack doesn't seem to be able to build this

edit: i'm getting the same errors. i don't know how to build this so i can't say if this fixes it or not

@fmeum
Copy link
Contributor Author

fmeum commented Oct 6, 2020

Let's wait for @hierynomus to give his opinion. My first attempt would also have been to just remove all the explicit BC checks, but I do not know whether this has the potential to obscure bugs.

The ECNamendCurveGenParameterSpec is a BC-specific workaround for
missing curve tables in Java 1.4 and earlier. For the sake of Android
compatibility, where Conscrypt can't deal with this custom spec class,
replace it with the standard ECGenParameterSpec and update the curve
names to the standard identifiers.
@fmeum fmeum changed the title Loop through security providers to check for BC Improve Android compatibility Oct 8, 2020
@fmeum
Copy link
Contributor Author

fmeum commented Oct 8, 2020

I added another commit to fix failing ECDSA key exchanges on Android.

@hierynomus hierynomus merged commit 2edaf07 into hierynomus:master Oct 20, 2020
@fmeum
Copy link
Contributor Author

fmeum commented Nov 12, 2020

Would it be possible to get a release with this fix and AES-GCM support?

@fmeum fmeum deleted the android_compatibility branch November 12, 2020 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants