Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upFixed #8594 -- use a more modern default cipher suite list by default #8611
Conversation
highfive
commented
Nov 20, 2015
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @SimonSapin (or someone else) soon. |
|
@bors-servo try |
Fixed #8594 -- use a more modern default cipher suite list by default <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8611) <!-- Reviewable:end -->
|
|
|
test failures look unrelated (@frewsxcv tells me they're known flaky?) |
|
Can you add some references to the comment by any chance? It'd be great if people who update this in the future (as well as reviewers) could have a link to some background reading as to best practices, since this is a security-critical configuration decision. |
|
@pcwalton Added! Thanks. |
|
Thanks, that was helpful! The string doesn't |
|
I believe so, yes. I use this cipher string (or ones very similar) in many libraries and TLS servers and have never seen it include things like export grade crypto. (If you go to |
|
OK, looks good from my end then once try comes back green. (We should get a security review from someone who's a professional as opposed to a dabbler like me before we ship any of this. But this is clearly an improvement and looks good based on my knowledge.) |
|
@bors-servo: try |
|
|
Fixed #8594 -- use a more modern default cipher suite list by default <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8611) <!-- Reviewable:end -->
|
Oh, why the leading underscore in |
|
@pcwalton Oh, it's a carryover from my Python days. Idiomatic rust would be to remove the |
|
|
|
Yeah, we try not to use leading underscores unless we're silencing an unused parameter compiler warning. |
|
r? @jvehent |
jvehent
commented
Nov 20, 2015
|
I would say r-, but a weak one. This is a secure ciphersuite that can be improved a bit. The list of ciphers it expands to is too long, mostly due to keywords like HIGH and RSA that expand to a lot of ciphers. For servo, I would recommend explicitly listing the ciphers that we want, as opposed to using keywords. Which expands to: This ciphersuite is closer to the prioritization rules described on the wiki, and is also mostly identical to what Firefox currently uses. |
|
Looks great to me, I'll update this PR to use that set later today. |
jvehent
commented
Nov 20, 2015
|
Looks good to me. r+ |
|
@bors-servo r=jvehent |
|
@bors-servo retry #8614 |
Fixed #8594 -- use a more modern default cipher suite list by default Fixes #8594. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8611) <!-- Reviewable:end -->
|
|
|
@bors-servo: retry |
Fixed #8594 -- use a more modern default cipher suite list by default Fixes #8594. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8611) <!-- Reviewable:end -->
|
|
|
@bors-servo: retry |
|
|
|
|
|
@bors-servo: retry |
Fixed #8594 -- use a more modern default cipher suite list by default Fixes #8594. <!-- Reviewable:start --> [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/8611) <!-- Reviewable:end -->
|
|
alex commentedNov 20, 2015
Fixes #8594.