code review 7163043: crypto/tls: allow the server to enforce it's ciphersuit...

crypto/tls: allow the server to enforce its ciphersuite preferences.

Previously, Go TLS servers always took the client's preferences into
account when selecting a ciphersuite. This change adds the option of
using the server's preferences, which can be expressed by setting
tls.Config.CipherSuites.

This mirrors Apache's SSLHonorCipherOrder directive.

[agl1, 2013-01-17 20:24:13]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[ioe, 2013-01-17 21:03:35]

Just a nit.

https://codereview.appspot.com/7163043/diff/4001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

https://codereview.appspot.com/7163043/diff/4001/src/pkg/crypto/tls/common.go...
src/pkg/crypto/tls/common.go:191: ServerCipherSuitePreference bool
s/ServerCipherSuitePreference/UseServersPreferredCipherSuite/g makes more clear
that this is a yes/no question instead of a variable containing a list of
preferred cipher suites.

[bradfitz, 2013-01-17 21:34:46]

s/it's/its/

On Thu, Jan 17, 2013 at 12:24 PM, <agl@golang.org> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> crypto/tls: allow the server to enforce it's ciphersuite preferences.
>
> Previously, Go TLS servers always took the client's preferences into
> account when selecting a ciphersuite. This change adds the option of
> using the server's preferences, which can be expressed by setting
> tls.Config.CipherSuites.
>
> This mirrors Apache's SSLHonorCipherOrder directive.
>
> Please review this at
https://codereview.appspot.**com/7163043/<https://codereview.appspot.com/7163...
>
> Affected files:
>   M src/pkg/crypto/tls/common.go
>   M src/pkg/crypto/tls/handshake_**server.go
>   M src/pkg/crypto/tls/handshake_**server_test.go
>
>
> Index: src/pkg/crypto/tls/common.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/tls/common.go
> +++ b/src/pkg/crypto/tls/common.go
> @@ -184,6 +184,12 @@
>         // is nil, TLS uses a list of suites supported by the
> implementation.
>         CipherSuites []uint16
>
> +       // ServerCipherSuitePreference controls whether the server selects
> the
> +       // client's most preferred ciphersuite, or the server's most
> preferred
> +       // ciphersuite. If true then the server's preference, as expressed
> in
> +       // the order of elements in CipherSuites, is used.
> +       ServerCipherSuitePreference bool
> +
>         // SessionTicketsDisabled may be set to true to disable session
> ticket
>         // (resumption) support.
>         SessionTicketsDisabled bool
> Index: src/pkg/crypto/tls/handshake_**server.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/tls/**handshake_server.go
> +++ b/src/pkg/crypto/tls/**handshake_server.go
> @@ -180,8 +180,17 @@
>                 return true, nil
>         }
>
> -       for _, id := range hs.clientHello.cipherSuites {
> -               if hs.suite = c.tryCipherSuite(id, hs.ellipticOk);
> hs.suite != nil {
> +       var preferenceList, supportedList []uint16
> +       if c.config.**ServerCipherSuitePreference {
> +               preferenceList = c.config.cipherSuites()
> +               supportedList = hs.clientHello.cipherSuites
> +       } else {
> +               preferenceList = hs.clientHello.cipherSuites
> +               supportedList = c.config.cipherSuites()
> +       }
> +
> +       for _, id := range preferenceList {
> +               if hs.suite = c.tryCipherSuite(id, supportedList,
> hs.ellipticOk); hs.suite != nil {
>                         break
>                 }
>         }
> @@ -222,7 +231,7 @@
>         }
>
>         // Check that we also support the ciphersuite from the session.
> -       hs.suite = c.tryCipherSuite(hs.**sessionState.cipherSuite,
> hs.ellipticOk)
> +       hs.suite = c.tryCipherSuite(hs.**sessionState.cipherSuite,
> c.config.cipherSuites(), hs.ellipticOk)
>         if hs.suite == nil {
>                 return false
>         }
> @@ -568,8 +577,8 @@
>
>  // tryCipherSuite returns a cipherSuite with the given id if that cipher
> suite
>  // is acceptable to use.
> -func (c *Conn) tryCipherSuite(id uint16, ellipticOk bool) *cipherSuite {
> -       for _, supported := range c.config.cipherSuites() {
> +func (c *Conn) tryCipherSuite(id uint16, supportedCipherSuites []uint16,
> ellipticOk bool) *cipherSuite {
> +       for _, supported := range supportedCipherSuites {
>                 if id == supported {
>                         var candidate *cipherSuite
>
> Index: src/pkg/crypto/tls/handshake_**server_test.go
> ==============================**==============================**=======
> --- a/src/pkg/crypto/tls/**handshake_server_test.go
> +++ b/src/pkg/crypto/tls/**handshake_server_test.go
> @@ -125,6 +125,50 @@
>         }
>  }
>
> +func testHandshake(clientConfig, serverConfig *Config) (state
> ConnectionState, err error) {
> +       c, s := net.Pipe()
> +       go func() {
> +               cli := Client(c, clientConfig)
> +               cli.Handshake()
> +               c.Close()
> +       }()
> +       server := Server(s, serverConfig)
> +       err = server.Handshake()
> +       if err == nil {
> +               state = server.ConnectionState()
> +       }
> +       s.Close()
> +       return
> +}
> +
> +func TestCipherSuitePreference(t *testing.T) {
> +       serverConfig := &Config{
> +               CipherSuites: []uint16{TLS_RSA_WITH_RC4_128_**SHA,
> TLS_RSA_WITH_AES_128_CBC_SHA, TLS_ECDHE_RSA_WITH_RC4_128_**SHA},
> +               Certificates: testConfig.Certificates,
> +       }
> +       clientConfig := &Config{
> +               CipherSuites:       []uint16{TLS_RSA_WITH_AES_128_**CBC_SHA,
> TLS_RSA_WITH_RC4_128_SHA},
> +               InsecureSkipVerify: true,
> +       }
> +       state, err := testHandshake(clientConfig, serverConfig)
> +       if err != nil {
> +               t.Fatalf("handshake failed: %s", err)
> +       }
> +       if state.CipherSuite != TLS_RSA_WITH_AES_128_CBC_SHA {
> +               // By default the server should use the client's
> preference.
> +               t.Fatalf("Client's preference was not used, got %x",
> state.CipherSuite)
> +       }
> +
> +       serverConfig.**ServerCipherSuitePreference = true
> +       state, err = testHandshake(clientConfig, serverConfig)
> +       if err != nil {
> +               t.Fatalf("handshake failed: %s", err)
> +       }
> +       if state.CipherSuite != TLS_RSA_WITH_RC4_128_SHA {
> +               t.Fatalf("Server's preference was not used, got %x",
> state.CipherSuite)
> +       }
> +}
> +
>  func testServerScript(t *testing.T, name string, serverScript [][]byte,
> config *Config, peers []*x509.Certificate) {
>         c, s := net.Pipe()
>         srv := Server(s, config)
>
>
>

[bradfitz, 2013-01-19 16:55:42]

https://codereview.appspot.com/7163043/diff/4001/src/pkg/crypto/tls/common.go
File src/pkg/crypto/tls/common.go (right):

https://codereview.appspot.com/7163043/diff/4001/src/pkg/crypto/tls/common.go...
src/pkg/crypto/tls/common.go:191: ServerCipherSuitePreference bool
On 2013/01/17 21:03:35, ioe wrote:
> s/ServerCipherSuitePreference/UseServersPreferredCipherSuite/g makes more
clear
> that this is a yes/no question instead of a variable containing a list of
> preferred cipher suites.
> 

Or:

PreferServerCipherSuites

... which also ends in "CipherSuites", the name of the field, and begins with an
imperative verb. And "Prefer server" is together, rather than splitting it with
"CipherSuite" in the middle.

[ality, 2013-01-20 02:23:03]

bradfitz@golang.org once said:
> PreferServerCipherSuites

I like this name.

  Anthony

[agl1, 2013-01-21 16:21:36]

Thanks all. Have updated with Brad's suggested name.

[bradfitz, 2013-01-21 16:39:57]

LGTM
 On Jan 21, 2013 8:21 AM, <agl@golang.org> wrote:

> Thanks all. Have updated with Brad's suggested name.
>
>
https://codereview.appspot.**com/7163043/<https://codereview.appspot.com/7163...
>

[agl1, 2013-01-22 15:10:52]

*** Submitted as https://code.google.com/p/go/source/detail?r=b68f084eaba7 ***

crypto/tls: allow the server to enforce its ciphersuite preferences.

Previously, Go TLS servers always took the client's preferences into
account when selecting a ciphersuite. This change adds the option of
using the server's preferences, which can be expressed by setting
tls.Config.CipherSuites.

This mirrors Apache's SSLHonorCipherOrder directive.

R=golang-dev, nightlyone, bradfitz, ality
CC=golang-dev
https://codereview.appspot.com/7163043