Add cert key type checking to chooseClientAlias

[nmburgan]

Previously, these functions ignored the keyType (or 'strings') argument to the chooseClientAlias function. Some libraries (e.g. Bouncy Castle) expect that when chooseClientAlias is called and key types are passed in, that it will return null if the cert doesn't use one of the given key types. For example, if ['EC'] was passed in for keyType and the cert contained an RSA key, since this would return 'user' rather than null in that case, it would cause Bouncy Castle to assume using an ECDSA signing algorithm was okay, and cause problems during the Certificate Verify part of the handshake.

This modifies these functions to only return 'user' if keyType is passed in and the cert contains a key of that type. If keyType is empty or null, it will ignore this and continue to check only the issuer.

All Submissions:

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same update/change?

Changes to Existing Features:

• Does this break existing behaviour? If so please explain.
  If keyType is being passed in, it will now return null if the cert does not contain that keyType. Previously, this argument was being ignored.

• Have you added an explanation of what your changes do and why you'd like us to include them?

• Have you written new tests for your core changes, as applicable?

• Have you successfully run tests with your changes locally?

[davecramer]

@nmburgan I'm wondering if there is a way to test this ?

[nmburgan]

@davecramer We've tested with our particular setup with Bouncy Castle and it works for our use case. Not sure if there would be other libraries passing in keyType and expecting different results.

Did you mean unit tests? Could possibly throw something in LazyKeyManagerTest and PKCS12KeyTest. It didn't look like that was currently exercising chooseClientAlias but I didn't dig into it too much.

Let me know if you have any suggestions. I'd be happy to do whatever makes sense!

[davecramer]

Yes, I meant unit tests.

Ideally we'd like features tested so that in the future some change doesn't break them

[nmburgan]

Sure, I can add those!

[nmburgan]

Tests added

[nmburgan]

This may actually not work correctly against non-RSA key types. I'm not sure if getAlgorithm() gives you EC for ECC keys. Might need to find something better to compare keyTypes against.

Edit: I think it does, actually, but this still probably isn't the most robust way. Not sure what else we can do since the function takes in strings rather than more detailed key type information.

[nmburgan]

Changed to use equalsIgnoreCase

[davecramer]

https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html
Does that help ?

[nmburgan]

Yeah, I think based on https://docs.oracle.com/en/java/javase/11/docs/specs/security/standard-names.html#keyfactory-algorithms we're okay.

[nmburgan]

Any idea when the next release might go out? We're mulling whether we want to fork the repo to incorporate the fix in the near-term, or just wait for the next release.

[davecramer]

Any idea when the next release might go out? We're mulling whether we want to fork the repo to incorporate the fix in the near-term, or just wait for the next release.

I'm working on. that now. I'd like to get a release out this week.