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

Support TLS/Ciphersuite configuration options #249

Closed
ajlake opened this issue Mar 9, 2017 · 6 comments
Closed

Support TLS/Ciphersuite configuration options #249

ajlake opened this issue Mar 9, 2017 · 6 comments
Milestone

Comments

@ajlake
Copy link

ajlake commented Mar 9, 2017

We would like to be able to configure Fabio to only accept TLS 1.2 + certain ciphersuites. Would you accept a contribution for this? Looking for some guidance on an impl that would be accepted. Thanks!

Thoughts:

  1. Expose config options here: https://github.com/eBay/fabio/blob/4d8e2b1cce7edcf2c566ad7652d957cf32858efe/config/config.go#L29-L36
  • TLSMinVersion String ("1.0", "1.1", or "1.2" recognized options)
  • CipherSuites String (comma-separated list)
  1. Pass more options along here: https://github.com/eBay/fabio/blob/4d8e2b1cce7edcf2c566ad7652d957cf32858efe/listen.go#L92
  2. Take advantage of the tls.Config options here: https://github.com/eBay/fabio/blob/4d8e2b1cce7edcf2c566ad7652d957cf32858efe/cert/source.go#L89-L94
    // CipherSuites is a list of supported cipher suites. If CipherSuites
    // is nil, TLS uses a list of suites supported by the implementation.
    CipherSuites []uint16

   ...

    // MinVersion contains the minimum SSL/TLS version that is acceptable.
    // If zero, then TLS 1.0 is taken as the minimum.
    MinVersion uint16

from tls package common.go

const (
	VersionSSL30 = 0x0300
	VersionTLS10 = 0x0301
	VersionTLS11 = 0x0302
	VersionTLS12 = 0x0303
)

from tls package cipher_suites.go

// A list of cipher suite IDs that are, or have been, implemented by this
// package.
//
// Taken from http://www.iana.org/assignments/tls-parameters/tls-parameters.xml
const (
	TLS_RSA_WITH_RC4_128_SHA                uint16 = 0x0005
	TLS_RSA_WITH_3DES_EDE_CBC_SHA           uint16 = 0x000a
	TLS_RSA_WITH_AES_128_CBC_SHA            uint16 = 0x002f
	TLS_RSA_WITH_AES_256_CBC_SHA            uint16 = 0x0035
	TLS_RSA_WITH_AES_128_CBC_SHA256         uint16 = 0x003c
	TLS_RSA_WITH_AES_128_GCM_SHA256         uint16 = 0x009c
	TLS_RSA_WITH_AES_256_GCM_SHA384         uint16 = 0x009d
	TLS_ECDHE_ECDSA_WITH_RC4_128_SHA        uint16 = 0xc007
	TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA    uint16 = 0xc009
	TLS_ECDHE_ECDSA_WITH_AES_256_CBC_SHA    uint16 = 0xc00a
	TLS_ECDHE_RSA_WITH_RC4_128_SHA          uint16 = 0xc011
	TLS_ECDHE_RSA_WITH_3DES_EDE_CBC_SHA     uint16 = 0xc012
	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA      uint16 = 0xc013
	TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA      uint16 = 0xc014
	TLS_ECDHE_ECDSA_WITH_AES_128_CBC_SHA256 uint16 = 0xc023
	TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256   uint16 = 0xc027
	TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256   uint16 = 0xc02f
	TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 uint16 = 0xc02b
	TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384   uint16 = 0xc030
	TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384 uint16 = 0xc02c
	TLS_ECDHE_RSA_WITH_CHACHA20_POLY1305    uint16 = 0xcca8
	TLS_ECDHE_ECDSA_WITH_CHACHA20_POLY1305  uint16 = 0xcca9

	// TLS_FALLBACK_SCSV isn't a standard cipher suite but an indicator
	// that the client is doing version fallback. See
	// https://tools.ietf.org/html/rfc7507.
	TLS_FALLBACK_SCSV uint16 = 0x5600
)
@magiconair
Copy link
Contributor

That sounds like a good idea since it provides more control over the defaults. I'm inclined to consider this an option of the listener since it more about the TLS handshake than managing certificates.

I'd go with adding a tlsmin and ciphers option to the listener config. The challenge here is that ciphers would be a comma-sep list inside a semicolon sep list inside a comma-sep list, e.g.

proxy.addr = :9999,:443;cs=ssl;tlsmin=1.2;ciphers="TLS_RSA_WITH_AES_256_CBC_SHA,TLS_ECDHE_ECDSA_WITH_AES_256_GCM_SHA384,..."

So you would have to first update the flagset code to ignore commas in a quoted string. I'd also allow both the hex code and the named constants which you should try to resolve via reflection to avoid future maintenance.

@magiconair
Copy link
Contributor

For the values of the tlsmin option I suggest to use the lower-cased constants without the Version prefix, e.g. ssl30, tls10, tls11, tls12, ...

@magiconair
Copy link
Contributor

@ajlake Are you going to pick this up or shall I do this?

@kostyrev
Copy link

someone, please implement this feature

@magiconair
Copy link
Contributor

I'll have a look.

magiconair added a commit that referenced this issue May 28, 2017
Add support for configuring the min and max TLS versions
as well as the ciphers to be used.

Fixes #249
@magiconair
Copy link
Contributor

I've pushed a branch which contains a fix for this issue. You can add tlsmin, tlsmax and tlsciphers as listener options and the values are the ones defined in https://golang.org/pkg/crypto/tls/#pkg-constants. The tlsciphers are specified as a comma-separated list in a quoted string. This required replacing the simple string.Split approach of the old config parser to be replaced with a proper lexer and parser.

Example:

proxy.addr = :443;cs=ssl;tlsmin=0x301;tlsciphers="0xc00a,0xc02b"

I still need to add a test and update the documentation in fabio.properties but it would be great if someone could test whether this works.

magiconair added a commit that referenced this issue Jun 7, 2017
Add support for configuring the min and max TLS versions
as well as the ciphers to be used.

Fixes #249
@magiconair magiconair added this to the 1.5.0 milestone Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants