Repository: oauth2-proxy/oauth2-proxy
Pull request #1723 (2022-07-13 16:06:43)

Added ability to specify allowed TLS cipher suites.
## Description

A new `--tls-cipher-suite` / `tls_cipher_suites` / `CipherSuites` option has been added which allows the default TLS cipher suite list to be overridden.

Without specifying the new option, behavior will be unchanged- allowing Go to determine which TLS cipher suites to support. However, when specified this option will restrict cipher suites only to the list provided. Names for cipher suites are pulled from the [`crypto/tls`](https://pkg.go.dev/crypto/tls#pkg-constants) package.

Example usage:
CLI:
```
--tls-cipher-suite=TLS_RSA_WITH_RC4_128_SHA
--tls-cipher-suite=TLS_RSA_WITH_3DES_EDE_CBC_SHA
```

Config:
```
tls_cipher_suites = [ "TLS_RSA_WITH_RC4_128_SHA", "TLS_RSA_WITH_3DES_EDE_CBC_SHA" ]
```

Alpha Config:
```
server:
  TLS:
    CipherSuites:
      - TLS_RSA_WITH_RC4_128_SHA
      - TLS_RSA_WITH_3DES_EDE_CBC_SHA
```

## Motivation and Context

Per #1704:
> OAuth2-Proxy currently relies on Go's crypto/tls package to maintain a list of safe cipher suites. Interestingly enough, this default list does not appear to be the same as tls.CipherSuites() and even seems to partially overlap with tls.InsecureCipherSuites().
> 
> As this list is not directly published and is tied to the particular version of Go in use, it seems reasonable that users might want to narrow down the list of enabled cipher suites as security requirements change over time.

## How Has This Been Tested?

New unit tests have been added to ensure the settings are properly parsed and propagated.

Additionally, manual testing was performed using [cipherscan](https://github.com/mozilla/cipherscan).
With `tls_cipher_suites = [ "TLS_RSA_WITH_AES_128_CBC_SHA256", "TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA256" ]`, the results were as expected:
```
prio  ciphersuite              protocols  pfs                 curves
1     ECDHE-RSA-AES128-SHA256  TLSv1.2    ECDH,P-521,521bits  prime256v1,secp384r1,secp521r1
2     AES128-SHA256            TLSv1.2    None                None
```

Without any value specified, the default cipher suite list appears to be used:
```
prio  ciphersuite                  protocols  pfs                 curves
1     ECDHE-RSA-AES128-GCM-SHA256  TLSv1.2    ECDH,P-521,521bits  prime256v1,secp384r1,secp521r1
2     ECDHE-RSA-AES256-GCM-SHA384  TLSv1.2    ECDH,P-521,521bits  prime256v1,secp384r1,secp521r1
3     ECDHE-RSA-AES128-SHA         TLSv1.2    ECDH,P-521,521bits  prime256v1,secp384r1,secp521r1
4     ECDHE-RSA-AES256-SHA         TLSv1.2    ECDH,P-521,521bits  prime256v1,secp384r1,secp521r1
5     AES128-GCM-SHA256            TLSv1.2    None                None
6     AES256-GCM-SHA384            TLSv1.2    None                None
7     AES128-SHA                   TLSv1.2    None                None
8     AES256-SHA                   TLSv1.2    None                None
9     ECDHE-RSA-DES-CBC3-SHA       TLSv1.2    ECDH,P-521,521bits  prime256v1,secp384r1,secp521r1
10    DES-CBC3-SHA                 TLSv1.2    None                None
```

## Checklist:

- [x] My change requires a change to the documentation or CHANGELOG.
- [x] I have updated the documentation/CHANGELOG accordingly.
- [x] I have created a feature (non-master) branch for my PR.


crbednarz: I did what I could to verify this change, however I'm a little less certain on how to verify the formatting of documentation changes. 
JoelSpeed: Please make sure to rebase on the latest master as well and then once the nit is fixed we can get this merged
crbednarz: Thanks for the feedback. I updated the error message per your recommendation and rebased off master.

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 85fe35c4..94aeaae6 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -18,6 +18,7 @@ N/A
 - [#1669](https://github.com/oauth2-proxy/oauth2-proxy/pull/1699) Fix method deprecated error in lint (@t-katsumura)
 
 - [#1709](https://github.com/oauth2-proxy/oauth2-proxy/pull/1709) Show an alert message when basic auth credentials are invalid (@aiciobanu)
+- [#1723](https://github.com/oauth2-proxy/oauth2-proxy/pull/1723) Added ability to specify allowed TLS cipher suites. (@crbednarz)
 
 - [#1720](https://github.com/oauth2-proxy/oauth2-proxy/pull/1720) Extract roles from authToken, to allow using allowed roles with Keycloak.
 
diff --git a/docs/docs/configuration/alpha_config.md b/docs/docs/configuration/alpha_config.md
index f0e9d182..f3fe9634 100644
--- a/docs/docs/configuration/alpha_config.md
+++ b/docs/docs/configuration/alpha_config.md
@@ -478,6 +478,7 @@ as well as an optional minimal TLS version that is acceptable.
 | `Key` | _[SecretSource](#secretsource)_ | Key is the TLS key data to use.<br/>Typically this will come from a file. |
 | `Cert` | _[SecretSource](#secretsource)_ | Cert is the TLS certificate data to use.<br/>Typically this will come from a file. |
 | `MinVersion` | _string_ | MinVersion is the minimal TLS version that is acceptable.<br/>E.g. Set to "TLS1.3" to select TLS version 1.3 |
+| `CipherSuites` | _[]string_ | CipherSuites is a list of TLS cipher suites that are allowed.<br/>E.g.:<br/>- TLS_RSA_WITH_RC4_128_SHA<br/>- TLS_RSA_WITH_AES_256_GCM_SHA384<br/>If not specified, the default Go safe cipher list is used.<br/>List of valid cipher suites can be found in the [crypto/tls documentation](https://pkg.go.dev/crypto/tls#pkg-constants). |
 
 ### URLParameterRule
 
diff --git a/docs/docs/configuration/overview.md b/docs/docs/configuration/overview.md
index 49303228..524e3414 100644
--- a/docs/docs/configuration/overview.md
+++ b/docs/docs/configuration/overview.md
@@ -196,6 +196,7 @@ An example [oauth2-proxy.cfg](https://github.com/oauth2-proxy/oauth2-proxy/blob/
 | `--standard-logging` | bool | Log standard runtime information | true |
 | `--standard-logging-format` | string | Template for standard log lines | see [Logging Configuration](#logging-configuration) |
 | `--tls-cert-file` | string | path to certificate file | |
+| `--tls-cipher-suite` | string \| list | Restricts TLS cipher suites used by server to those listed (e.g. TLS_RSA_WITH_RC4_128_SHA) (may be given multiple times). If not specified, the default Go safe cipher list is used. List of valid cipher suites can be found in the [crypto/tls documentation](https://pkg.go.dev/crypto/tls#pkg-constants). | |
 | `--tls-key-file` | string | path to private key file | |
 | `--tls-min-version` | string | minimum TLS version that is acceptable, either `"TLS1.2"` or `"TLS1.3"` | `"TLS1.2"` |
 | `--upstream` | string \| list | the http url(s) of the upstream endpoint, file:// paths for static files or `static://<status_code>` for static response. Routing is based on the path | |
diff --git a/docs/docs/configuration/tls.md b/docs/docs/configuration/tls.md
index 1efdc5fa..21aa6f3b 100644
--- a/docs/docs/configuration/tls.md
+++ b/docs/docs/configuration/tls.md
@@ -32,8 +32,9 @@ There are two recommended configurations:
     The defaults set `TLS1.2` as the minimal version. 
     Regardless of the minimum version configured, `TLS1.3` is currently always used as the maximal version.
 
-    The server side cipher suites are the defaults from [`crypto/tls`](https://pkg.go.dev/crypto/tls#CipherSuites) of 
-    the currently used `go` version for building `oauth2-proxy`.
+    TLS server side cipher suites can be specified with `--tls-cipher-suite=TLS_RSA_WITH_RC4_128_SHA`.
+    If not specified, the defaults from [`crypto/tls`](https://pkg.go.dev/crypto/tls#CipherSuites) of the currently used `go` version for building `oauth2-proxy` will be used.
+    A complete list of valid TLS cipher suite names can be found in [`crypto/tls`](https://pkg.go.dev/crypto/tls#pkg-constants).
 
 ### Terminate TLS at Reverse Proxy, e.g. Nginx
 
diff --git a/pkg/apis/options/legacy_options.go b/pkg/apis/options/legacy_options.go
index 16297e00..b3e1f2b5 100644
--- a/pkg/apis/options/legacy_options.go
+++ b/pkg/apis/options/legacy_options.go
@@ -447,15 +447,16 @@ func getXAuthRequestAccessTokenHeader() Header {
 }
 
 type LegacyServer struct {
-	MetricsAddress       string `flag:"metrics-address" cfg:"metrics_address"`
-	MetricsSecureAddress string `flag:"metrics-secure-address" cfg:"metrics_secure_address"`
-	MetricsTLSCertFile   string `flag:"metrics-tls-cert-file" cfg:"metrics_tls_cert_file"`
-	MetricsTLSKeyFile    string `flag:"metrics-tls-key-file" cfg:"metrics_tls_key_file"`
-	HTTPAddress          string `flag:"http-address" cfg:"http_address"`
-	HTTPSAddress         string `flag:"https-address" cfg:"https_address"`
-	TLSCertFile          string `flag:"tls-cert-file" cfg:"tls_cert_file"`
-	TLSKeyFile           string `flag:"tls-key-file" cfg:"tls_key_file"`
-	TLSMinVersion        string `flag:"tls-min-version" cfg:"tls_min_version"`
+	MetricsAddress       string   `flag:"metrics-address" cfg:"metrics_address"`
+	MetricsSecureAddress string   `flag:"metrics-secure-address" cfg:"metrics_secure_address"`
+	MetricsTLSCertFile   string   `flag:"metrics-tls-cert-file" cfg:"metrics_tls_cert_file"`
+	MetricsTLSKeyFile    string   `flag:"metrics-tls-key-file" cfg:"metrics_tls_key_file"`
+	HTTPAddress          string   `flag:"http-address" cfg:"http_address"`
+	HTTPSAddress         string   `flag:"https-address" cfg:"https_address"`
+	TLSCertFile          string   `flag:"tls-cert-file" cfg:"tls_cert_file"`
+	TLSKeyFile           string   `flag:"tls-key-file" cfg:"tls_key_file"`
+	TLSMinVersion        string   `flag:"tls-min-version" cfg:"tls_min_version"`
+	TLSCipherSuites      []string `flag:"tls-cipher-suite" cfg:"tls_cipher_suites"`
 }
 
 func legacyServerFlagset() *pflag.FlagSet {
@@ -470,6 +471,7 @@ func legacyServerFlagset() *pflag.FlagSet {
 	flagSet.String("tls-cert-file", "", "path to certificate file")
 	flagSet.String("tls-key-file", "", "path to private key file")
 	flagSet.String("tls-min-version", "", "minimal TLS version for HTTPS clients (either \"TLS1.2\" or \"TLS1.3\")")
+	flagSet.StringSlice("tls-cipher-suite", []string{}, "restricts TLS cipher suites to those listed (e.g. TLS_RSA_WITH_RC4_128_SHA) (may be given multiple times)")
 
 	return flagSet
 }
@@ -600,6 +602,9 @@ func (l LegacyServer) convert() (Server, Server) {
 			},
 			MinVersion: l.TLSMinVersion,
 		}
+		if len(l.TLSCipherSuites) != 0 {
+			appServer.TLS.CipherSuites = l.TLSCipherSuites
+		}
 		// Preserve backwards compatibility, only run one server
 		appServer.BindAddress = ""
 	} else {
diff --git a/pkg/apis/options/server.go b/pkg/apis/options/server.go
index 704e133e..f423ef2c 100644
--- a/pkg/apis/options/server.go
+++ b/pkg/apis/options/server.go
@@ -29,4 +29,12 @@ type TLS struct {
 	// MinVersion is the minimal TLS version that is acceptable.
 	// E.g. Set to "TLS1.3" to select TLS version 1.3
 	MinVersion string
+
+	// CipherSuites is a list of TLS cipher suites that are allowed.
+	// E.g.:
+	// - TLS_RSA_WITH_RC4_128_SHA
+	// - TLS_RSA_WITH_AES_256_GCM_SHA384
+	// If not specified, the default Go safe cipher list is used.
+	// List of valid cipher suites can be found in the [crypto/tls documentation](https://pkg.go.dev/crypto/tls#pkg-constants).
+	CipherSuites []string
 }
diff --git a/pkg/http/server.go b/pkg/http/server.go
index b44d5b10..9daadf39 100644
--- a/pkg/http/server.go
+++ b/pkg/http/server.go
@@ -81,6 +81,27 @@ func (s *server) setupListener(opts Opts) error {
 	return nil
 }
 
+func parseCipherSuites(names []string) ([]uint16, error) {
+	cipherNameMap := make(map[string]uint16)
+
+	for _, cipherSuite := range tls.CipherSuites() {
+		cipherNameMap[cipherSuite.Name] = cipherSuite.ID
+	}
+	for _, cipherSuite := range tls.InsecureCipherSuites() {
+		cipherNameMap[cipherSuite.Name] = cipherSuite.ID
+	}
+
+	result := make([]uint16, len(names))
+	for i, name := range names {
+		id, present := cipherNameMap[name]
+		if !present {
+			return nil, fmt.Errorf("unknown TLS cipher suite name specified %q", name)
+		}
+		result[i] = id
+	}
+	return result, nil
+}
+
 // setupTLSListener sets the server TLS listener if the HTTPS server is enabled.
 // The HTTPS server can be disabled by setting the SecureBindAddress to "-" or by
 // leaving it empty.
@@ -104,6 +125,14 @@ func (s *server) setupTLSListener(opts Opts) error {
 	}
 	config.Certificates = []tls.Certificate{cert}
 
+	if len(opts.TLS.CipherSuites) > 0 {
+		cipherSuites, err := parseCipherSuites(opts.TLS.CipherSuites)
+		if err != nil {
+			return fmt.Errorf("could not parse cipher suites: %v", err)
+		}
+		config.CipherSuites = cipherSuites
+	}
+
 	if len(opts.TLS.MinVersion) > 0 {
 		switch opts.TLS.MinVersion {
 		case "TLS1.2":


Test patch:
diff --git a/pkg/apis/options/legacy_options_test.go b/pkg/apis/options/legacy_options_test.go
index da5545d6..03f874ca 100644
--- a/pkg/apis/options/legacy_options_test.go
+++ b/pkg/apis/options/legacy_options_test.go
@@ -804,6 +804,7 @@ var _ = Describe("Legacy Options", func() {
 			keyPath             = "tls.key"
 			minVersion          = "TLS1.3"
 		)
+		cipherSuites := []string{"TLS_RSA_WITH_AES_128_GCM_SHA256", "TLS_RSA_WITH_AES_256_GCM_SHA384"}
 
 		var tlsConfig = &TLS{
 			Cert: &SecretSource{
@@ -820,6 +821,15 @@ var _ = Describe("Legacy Options", func() {
 			MinVersion: minVersion,
 		}
 
+		var tlsConfigCipherSuites = &TLS{
+			Cert: tlsConfig.Cert,
+			Key:  tlsConfig.Key,
+			CipherSuites: []string{
+				"TLS_RSA_WITH_AES_128_GCM_SHA256",
+				"TLS_RSA_WITH_AES_256_GCM_SHA384",
+			},
+		}
+
 		DescribeTable("should convert to app and metrics servers",
 			func(in legacyServersTableInput) {
 				appServer, metricsServer := in.legacyServer.convert()
@@ -860,6 +870,19 @@ var _ = Describe("Legacy Options", func() {
 					TLS:               tlsConfigMinVersion,
 				},
 			}),
+			Entry("with TLS options specified with CipherSuites", legacyServersTableInput{
+				legacyServer: LegacyServer{
+					HTTPAddress:     insecureAddr,
+					HTTPSAddress:    secureAddr,
+					TLSKeyFile:      keyPath,
+					TLSCertFile:     crtPath,
+					TLSCipherSuites: cipherSuites,
+				},
+				expectedAppServer: Server{
+					SecureBindAddress: secureAddr,
+					TLS:               tlsConfigCipherSuites,
+				},
+			}),
 			Entry("with metrics HTTP and HTTPS addresses", legacyServersTableInput{
 				legacyServer: LegacyServer{
 					HTTPAddress:          insecureAddr,
diff --git a/pkg/http/server_test.go b/pkg/http/server_test.go
index 3c48cabe..6944c491 100644
--- a/pkg/http/server_test.go
+++ b/pkg/http/server_test.go
@@ -261,6 +261,40 @@ var _ = Describe("Server", func() {
 				expectHTTPListener: false,
 				expectTLSListener:  true,
 			}),
+			Entry("with an ipv4 valid https bind address, and valid TLS config with CipherSuites", &newServerTableInput{
+				opts: Opts{
+					Handler:           handler,
+					SecureBindAddress: "127.0.0.1:0",
+					TLS: &options.TLS{
+						Key:  &ipv4KeyDataSource,
+						Cert: &ipv4CertDataSource,
+						CipherSuites: []string{
+							"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
+							"TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",
+						},
+					},
+				},
+				expectedErr:        nil,
+				expectHTTPListener: false,
+				expectTLSListener:  true,
+			}),
+			Entry("with an ipv4 valid https bind address, and invalid TLS config with unknown CipherSuites", &newServerTableInput{
+				opts: Opts{
+					Handler:           handler,
+					SecureBindAddress: "127.0.0.1:0",
+					TLS: &options.TLS{
+						Key:  &ipv4KeyDataSource,
+						Cert: &ipv4CertDataSource,
+						CipherSuites: []string{
+							"TLS_RSA_WITH_RC4_64_SHA",
+							"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
+						},
+					},
+				},
+				expectedErr:        errors.New("error setting up TLS listener: could not parse cipher suites: unknown TLS cipher suite name specified \"TLS_RSA_WITH_RC4_64_SHA\""),
+				expectHTTPListener: false,
+				expectTLSListener:  true,
+			}),
 			Entry("with an ipv6 valid http bind address", &newServerTableInput{
 				opts: Opts{
 					Handler:     handler,
@@ -454,6 +488,40 @@ var _ = Describe("Server", func() {
 				expectHTTPListener: false,
 				expectTLSListener:  true,
 			}),
+			Entry("with an ipv6 valid https bind address, and valid TLS config with CipherSuites", &newServerTableInput{
+				opts: Opts{
+					Handler:           handler,
+					SecureBindAddress: "[::1]:0",
+					TLS: &options.TLS{
+						Key:  &ipv4KeyDataSource,
+						Cert: &ipv4CertDataSource,
+						CipherSuites: []string{
+							"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
+							"TLS_ECDHE_RSA_WITH_AES_256_CBC_SHA",
+						},
+					},
+				},
+				expectedErr:        nil,
+				expectHTTPListener: false,
+				expectTLSListener:  true,
+			}),
+			Entry("with an ipv6 valid https bind address, and invalid TLS config with unknown CipherSuites", &newServerTableInput{
+				opts: Opts{
+					Handler:           handler,
+					SecureBindAddress: "[::1]:0",
+					TLS: &options.TLS{
+						Key:  &ipv4KeyDataSource,
+						Cert: &ipv4CertDataSource,
+						CipherSuites: []string{
+							"TLS_RSA_WITH_RC4_64_SHA",
+							"TLS_ECDHE_RSA_WITH_AES_128_CBC_SHA",
+						},
+					},
+				},
+				expectedErr:        errors.New("error setting up TLS listener: could not parse cipher suites: unknown TLS cipher suite name specified \"TLS_RSA_WITH_RC4_64_SHA\""),
+				expectHTTPListener: false,
+				expectTLSListener:  true,
+			}),
 		)
 	})