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

Updating the Haskell stub #312

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kazu-yamamoto
Copy link

This patch updates the Haskell stub:

  • Using LTS 12.17 (the latest one) for Stackage
  • Using http-client >= 0.5 (the current one), which does not provide TlsExceptionHostPort
  • Using ciphersuite_strong instead of deprecated ciphersuite_all from Haskell TLS

The author of http-client has already checked if this fix is correct.
And note that I'm one of maintainers of Haskell TLS.

Thank you for creating a wonderful test tool!

@@ -84,6 +86,6 @@ injectCA caBundle p =

injectCiphers :: ClientParams -> ClientParams
injectCiphers p =
p { clientSupported = supported { supportedCiphers = ciphersuite_all } }
p { clientSupported = supported { supportedCiphers = ciphersuite_strong } }

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is ciphersuite_default also sufficient? Or is the default "too permissive"?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They result in the same. So, let's use default.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My intent when using ciphersuite_all is to let the TLS server select the appropriate ciphersuite for a particular test. In TLS Handshake ClientHello advertises all supported ciphersuites and ServerHello selects the one to be used. Changing it to more restrictive list of ciphersuites might limit the testing and provide skewed results.

It's worth testing every supported ciphersuite to find the problems they might contain.

Copy link

@vdukhovni vdukhovni Nov 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well some ciphersuites are implemented, but deprecated (not recommended) and should generally not be enabled. So the question comes down to what a particular test is intended to cover. If it is a test of the correctness of the implementation of even deprecated ciphers, then it should be "all", but if the test is to make sure that weak options are not enabled, then it should use "default" or "strong". So decide what the goal is and choose wisely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ciphersuite_all will be removed someday. It should not be used anyway.

@kazu-yamamoto
Copy link
Author

What is the status of this PR?

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.

3 participants