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

Added option to resolve root ca and simplify Examine SSL option #466

Closed
wants to merge 1 commit into from
Closed

Conversation

Hakky54
Copy link

@Hakky54 Hakky54 commented Dec 29, 2023

This PR adds the capability to KeyStore Explorer to import certificates from an url. It uses sslcontext-kickstart library under the covers which I have built. It calls the server and gets the certificate chain which will be presented to the user, so the user can decided which he/she wants to import as demonstrated in the video below.

certificate.importer.demo.mov

@kaikramer
Copy link
Owner

Thanks for the PR, but what's the difference to "Examine SSL"?

@Hakky54
Copy link
Author

Hakky54 commented Dec 30, 2023

Ah, my bad. I didn't know about the existence of Examine SSL option. Thank you for pointing out as it is similar. We can close this PR.

There is only one difference in the implementation, but it is up to you whether you think it is worth to adjust. I am curious what your opinion is. I am also resolving the root ca in the chain, see below for an example. I used google.com and in the chain it included GlobalSign Root CA. Right side I used Examine SSL, left side I used my implementation.

Screenshot 2023-12-30 at 10 34 17

@Hakky54
Copy link
Author

Hakky54 commented Jan 1, 2024

I did an attempt to refactor the existing examine ssl option.

I noticed it had the option client authentication, altough it is nice to have when calling a server with mutual authentication enabled, it is not required to get the server certificates. So it can be obtained without client keystore. So I removed that part and it still works. You probably noticed that I hooked up my own library. I added it because it contains the logic of obtaining the certificate chain of the server itself and resolving the root certificate, which is not sent by the server but it can be resolved as demonstrated in my previous post #466 (comment)

Because I delegated the call to obtain the server certificates to the library some code was not needed anymore, so I removed it. I also noticed most of the properties of SSLConnectionInfo was not being used, so that was the reason why I thought it could be removed. The only property which was really being used was the server certificates.

I am curious what you think of the refactored solution. The code changes for this PR has changed by the way.

@Hakky54 Hakky54 changed the title Added option to import certificates from an url Added option to resolve root ca and simplify Examine SSL option Jan 1, 2024
@kaikramer
Copy link
Owner

Sorry for the late reply and I want to assure you that I really appreciate your efforts to improve KSE! I hope my answer does not discourage you from further PRs.

That being said, everything in the current implementation of the "Examine SSL" feature has good reasons to be as it is:

The reason why the root certificate is not shown is simply that it was not returned by the server. If you automatically add certificates from the local truststore (usually cacerts) to the result then you lose the information what the server exactly delivers. It is important that it does not deliver the root certificate, therefore I want to check with "Examine SSL" what exactly is in the server's response.

This also mirrors the behaviour of openssl s_client ....

If you want to make sure that server certificates chain matches your local trust anchor, then you should use the "Verify" button, which does a complete chain validation.

There is also a ticket suggesting to augment the chain in the certificate details with certificates found somewhere else:

rel55_transfer

This would be a reasonable middle ground between showing the actual content and providing additional infos, but it's a general solution, not only for "Exmaine SSL".

The "Client Authentication" part has also its use case: Let's say you have created a keystore for inter-service communication and want to do a quick test to see if all the certificate extensions etc. are correct. Then you can select this keystore and connect with client authentication. It's not needed to retrieve the server certificates, that's right, but it has other uses.

And the idea behind SslConnectionInfos was to show not only the certificates but also connection related infos like the cipher suite that was negotiated between client and server or whether SNI is enabled. Unfortunately I did not have the time to complete this feature so far.

Again, your PR is appreciated, but I cannot accept it.

@Hakky54
Copy link
Author

Hakky54 commented Jan 2, 2024

Well explained, thank you! I appreciate for taking your time for for providing the context for every remark I had. It makes sense now. I thought it would be a nice addition to include the resolved root ca, however your use case is different and I can understand that you only want to show what the server returns which is also the case with openssl s_client.

It was fun walking through the code. It has been well written and great to see also how you used swing/javafx in this project. Then let's close the PR. Have a nice day!

@Hakky54 Hakky54 closed this Jan 2, 2024
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

2 participants