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.

Sign up for GitHub

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

Jump to bottom

🔒 Update supported TLS cipher suites #3962

Merged
merged 4 commits into from
Mar 19, 2018
Merged

🔒 Update supported TLS cipher suites #3962

merged 4 commits into from
Mar 19, 2018

Conversation

canterberry
Copy link
Contributor

@canterberry canterberry commented Mar 13, 2018 •
edited
Loading

The list of cipher suites included in this commit are consistent with the values and precedence in the Golang TLS documentation.

Removed

No cipher suites were removed in this PR. For existing cipher suites, only the order has changed.

Added

The following cipher suites were added in this PR:

  • TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256
  • TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305
  • TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256
  • TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305
  • TLS_RSA_WITH_AES_128_CBC_SHA256

Security Note

Cipher suites with RC4 are still included within the list of accepted values for compatibility, but these cipher suites are not safe to use and should be deprecated with warnings and subsequently removed. Support for RC4 ciphers has already been removed or disabled by default in many prominent browsers and tools, including Golang.

References

@pearkes
Copy link
Contributor

pearkes commented Mar 15, 2018

Thanks for the PR @canterberry!

Prior to review it'd be worth investigating out the source of the test failures here.

--- FAIL: TestFullConfig (0.04s)
    runtime_test.go:3624: "*RuntimeConfig.AdvertiseAddrLAN.IP[0]" is zero value
--- FAIL: TestFullConfig/json (0.02s)
    values.go:22: verification for runtime_config at runtime_test.go:3658:

You can run tests locally with make test or view the CI failures linked in this PR.

@pearkes pearkes added the waiting-reply Waiting on response from Original Poster or another individual in the thread label Mar 15, 2018
@canterberry
Copy link
Contributor Author

canterberry commented Mar 15, 2018 •
edited
Loading

Thanks for the comment @pearkes! At the time I forked for this PR, the master branch was failing. I'm fairly confident that is the cause of this PR's failure as well. It looks like master is passing now, so I'll merge that in now.

Edit: Ah, it does look like it might be a legitimate failure. Investigating…

@canterberry
Copy link
Contributor Author

@pearkes Fixed. The failing tests were due to differences in the three different configuration definitions in the test suite, which all need to be in sync for the expectations to work out properly. The error reported by the failing test is misleading -- it seems to occur if any definition within the JSON or HCL payloads do not match up with the corresponding values in the RuntimeConfig definition.

The most recent failing Travis build appears to be sporadic. Local builds are failing for me due to the HTTP test suite creating too many open files, but the tests shown by Travis CI as failing do pass for me locally.

@canterberry
🔒 Update supported TLS cipher suites
c901307 
The list of cipher suites included in this commit are consistent with
the values and precedence in the [Golang TLS documentation](https://golang.org/src/crypto/tls/cipher_suites.go).

> **Note:** Cipher suites with RC4 are still included within the list
> of accepted values for compatibility, but **these cipher suites are
> not safe to use** and should be deprecated with warnings and
> subsequently removed. Support for RC4 ciphers has already been
> removed or disabled by default in many prominent browsers and tools,
> including Golang.
>
> **References:**
>
>  * [RC4 on Wikipedia](https://en.wikipedia.org/wiki/RC4)
>  * [Mozilla Security Blog](https://blog.mozilla.org/security/2015/09/11/deprecating-the-rc4-cipher/)
@canterberry
🐛 Formatting changes only; add missing trailing commas
a61abcd
@canterberry
✅ Match expectation of TLSCipherSuites to values of tls_cipher_suites
7236c95
@canterberry
Copy link
Contributor Author

(squashing latest two commits to coerce a Travis CI rebuild)

canterberry
canterberry commented Mar 15, 2018
View reviewed changes
agent/consul/server_test.go
testrpc.WaitForLeader(t, s1.RPC, "dc1")

err:= s1.revokeLeadership()
err := s1.revokeLeadership()
Copy link
Contributor Author

@canterberry canterberry Mar 15, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Note: The seemingly spurious changes in this file result from changes applied by make format. An identical diff for this file can be achieved by running this command against the current HEAD of the master branch.

@banks
Copy link
Member

banks commented Mar 19, 2018

Thanks @canterberry!

@banks banks merged commit a8f7681 into hashicorp:master Mar 19, 2018
@pearkes pearkes added this to the 1.0.7 milestone Mar 19, 2018
@canterberry canterberry deleted the upgrade/tls-cipher-suites branch March 19, 2018 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
waiting-reply Waiting on response from Original Poster or another individual in the thread
Projects
None yet
Milestone
1.0.7
Development

Successfully merging this pull request may close these issues.
3 participants
@canterberry @pearkes @banks