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

Cipher List Setting #13

Merged
merged 2 commits into from
May 9, 2016
Merged

Cipher List Setting #13

merged 2 commits into from
May 9, 2016

Conversation

tomohisa
Copy link
Contributor

@tomohisa tomohisa commented May 9, 2016

Problem

can not connect some site. e.g. https://echo.websocket.org
https://www.ssllabs.com/ssltest/analyze.html?d=echo.websocket.org
Safari can respond 404 but HTTPSClient reset connection by peer

Reason

default cipher list "HIGH:!aNULL:!kRSA:!PSK:!SRP:!MD5:!RC4" is strict
so some server does not reach its requirement.

Fix

Able to set cipherList from HTTPSClient it can pass to TCPSSL and then
OpenSSL

it does not break current program because it uses cipherList: String? = nil so exsist code runs exactly it was before.

# Problem
can not connect some site. e.g. https://echo.websocket.org
https://www.ssllabs.com/ssltest/analyze.html?d=echo.websocket.org
Safari can respond 404 but HTTPSClient reset connection by peer

# Reason
default cipher list `"HIGH:!aNULL:!kRSA:!PSK:!SRP:!MD5:!RC4"` is strict
so some server does not reach its requirement.

# Fix
Able to set cipherList from HTTPSClient it can pass to TCPSSL and then
OpenSSL

it does not break current program because it uses `cipherList: String?
= nil` so exsist code runs exactly it was before.
@@ -27,17 +27,26 @@ import COpenSSL
typealias CCallback = @convention(c) Void -> Void

public final class SSLClientContext: Context {
public init(verifyBundle: String? = nil, certificate: String? = nil, privateKey: String? = nil, certificateChain: String? = nil) throws {
public init(verifyBundle: String? = nil,

Choose a reason for hiding this comment

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

Cyclomatic Complexity Violation: Function should have complexity 10 or less: currently complexity equals 12 (cyclomatic_complexity)

@tomohisa
Copy link
Contributor Author

tomohisa commented May 9, 2016

related:
ZewoGraveyard/TCPSSL#8
ZewoGraveyard/HTTPSClient#17
those depend on this PR

Just removed Cipher List Specification otherwise we find clean solution.
@tomohisa
Copy link
Contributor Author

tomohisa commented May 9, 2016

Just removed Cipher List Specification otherwise we find clean solution. It would be best we figure clean solution like ContextConfiguration.

@tomohisa
Copy link
Contributor Author

tomohisa commented May 9, 2016

related issue
Zewo/Zewo#102

@tomohisa tomohisa merged commit 78629d6 into master May 9, 2016
@tomohisa tomohisa deleted the cipher_can_be_specify branch May 9, 2016 15:19
try super.init(method: .SSLv23, type: .Client)

SSL_CTX_set_verify(context, SSL_VERIFY_PEER, nil)
SSL_CTX_set_verify_depth(context, 4)
SSL_CTX_set_options(context, SSL_OP_NO_SSLv2 | SSL_OP_NO_SSLv3 | SSL_OP_NO_COMPRESSION)

if SSL_CTX_set_cipher_list(context, "HIGH:!aNULL:!kRSA:!PSK:!SRP:!MD5:!RC4") != 1 {
throw Context.Error.Certificate(description: lastSSLErrorDescription)
Copy link
Contributor

Choose a reason for hiding this comment

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

So what is it being set to now?

Copy link
Contributor

Choose a reason for hiding this comment

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

@czechboy0 the Open SSL default.. which I don't know what is haha

Copy link
Contributor

Choose a reason for hiding this comment

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

That doesn't sound good 😬 We should be kept at high security and only lower it on demand by the client, not the other way around. Can you please find out what setting we have here now, @tomohisa? I'd like to know how secure my servers are after this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

But only the client changed. Not the server. And yes, @tomohisa created an issue so we find a way to set the cyphers in a type-safe way. Using option set or enums, etc. You can take the front if you're interested.

Copy link
Contributor

Choose a reason for hiding this comment

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

I use this code on servers to fetch data from other servers over https :) And since it was changed I'd like to know what the security setting is now.

Copy link
Contributor

Choose a reason for hiding this comment

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

https://hynek.me/articles/hardening-your-web-servers-ssl-ciphers/

@tomohisa can you see if the server you were targeting works with the cipher list on the link I sent, please?

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.

4 participants