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

RC4 is dyning #400

Closed
kazu-yamamoto opened this issue Jul 17, 2015 · 17 comments

Comments

Projects
None yet
4 participants
@kazu-yamamoto
Copy link
Contributor

commented Jul 17, 2015

Now RFC 7465: "Prohibiting RC4 Cipher Suites" is published. We need to consider when we should remove RC4 from WarpTLS.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Jul 19, 2015

I'm OK removing it now. @vincenthz any thoughts on the matter?

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

@meteficha

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

Thanks for the pointer. +1 on removing.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

If we agree to remove RC4, we need to decide when we do it: WarpTLS 3.1.0 or 3.2.0? I will release WarpTLS 3.1.0 in this week.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

I'd say 3.1. This is just changing the default, not removing the capability
entirely, right?

On Mon, Jul 20, 2015 at 8:16 PM Kazu Yamamoto notifications@github.com
wrote:

If we agree to remove RC4, we need to decide when we do it: WarpTLS 3.1.0
or 3.2.0? I will release WarpTLS 3.1.0 in this week.


Reply to this email directly or view it on GitHub
#400 (comment).

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

Right.

So, what about providingdefaultTraditionalTlsSettings which includes RC4?

@snoyberg

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

Shrug. I'm fine with it, though I'm not convinced it's necessary. If you
want to go that route, feel free

On Mon, Jul 20, 2015, 8:44 PM Kazu Yamamoto notifications@github.com
wrote:

Right.

So, what about providingdefaultTraditionalTlsSettings which includes RC4?


Reply to this email directly or view it on GitHub
#400 (comment).

kazu-yamamoto added a commit that referenced this issue Jul 21, 2015

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

Please see the commit above to know what I want to do exactly.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

If we have defaultTlsSettings and traditionalTlsSettings, we can be more active to make defautlTlsSettings more modern. I would like to discuss to remove re-negociation from defaultTlsSettings, too.

@erikd

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2015

+1 on removing RC4
+1 on removing re-negociation from defaultTlsSettings

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2015

I was wrong. The smart constructors (e.g. tlsSettings) modifiy defaultTlsSettings. chainCertFiles etc are not exported. So, we cannot modify chainCertFiles in traditionalTlsSettings.

@snoyberg What is the intention to hide chainCertFiles etc? Can we export them?

@snoyberg

This comment has been minimized.

Copy link
Member

commented Jul 21, 2015

It's a backwards compatibility hack. A better approach would be to export setters like setChainCertFiles that will ensure that internal invariants are kept about in-memory vs in-file storage.

kazu-yamamoto added a commit that referenced this issue Jul 22, 2015

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

@snoyberg OK. We should not export these field names.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

Now, things are clear to me.

traditionalTlsSettings:

  • There is no way to control renegotiation of the "tls" lib so to satisfy the tests provided by https://www.ssllabs.com/ssltest/. I will bring this issue to the "tls" lib itself.
  • There is no smart constructors which can modify traditionalTlsSettings.
  • So, let's not define traditionalTlsSettings.

RC4:

  • Let's remove RC4 now.
  • The current doc is misleading. We should move the smart constructor part first, field names second. This leads users to use smart constructors first, then modify tlsCiphers if necessay.

I implemented this all and am satisfied. Please review the current code, everyone.

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

The renego issue now goes to vincenthz/hs-tls#112.

@snoyberg

This comment has been minimized.

Copy link
Member

commented Jul 22, 2015

LGTM

@kazu-yamamoto

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2015

Let's close this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.