Allow TLS cipher suites to be set for the OPA server #6537
Conversation
cmd/run.go
Outdated
| @@ -221,6 +225,7 @@ Current behaviors enabled by this flag include: | |||
| runCommand.Flags().IntVar(&cmdParams.rt.GracefulShutdownPeriod, "shutdown-grace-period", 10, "set the time (in seconds) that the server will wait to gracefully shut down") | |||
| runCommand.Flags().IntVar(&cmdParams.rt.ShutdownWaitPeriod, "shutdown-wait-period", 0, "set the time (in seconds) that the server will wait before initiating shutdown") | |||
| runCommand.Flags().BoolVar(&cmdParams.skipKnownSchemaCheck, "skip-known-schema-check", false, "disables type checking on known input schemas") | |||
| runCommand.Flags().StringSliceVar(&cmdParams.cipherSuites, "tls-cipher-suites", []string{}, "set list of enabled TLS 1.0–1.2 cipher suites") | |||
There was a problem hiding this comment.
Is it worth noting for which requests these will be used for? aka, not http.send?
There was a problem hiding this comment.
Since it's a flag on opa run and the other tls-related flags don't specify anything in particular, I think we should be fine. We can always update if users have questions.
cmd/run.go
Outdated
| } | ||
|
|
||
| for _, c := range tls.InsecureCipherSuites() { | ||
| cipherSuitesMap[c.Name] = c.ID |
There was a problem hiding this comment.
Should we take the opportunity here to warn about the user specifying an insecure suit?
There was a problem hiding this comment.
I was thinking about it but we have no logger initialized at this point and adding a print did not seem that important. We could move this in the runtime if necessary but doesn't feel like.
| @@ -400,6 +401,12 @@ func (s *Server) WithNDBCacheEnabled(ndbCacheEnabled bool) *Server { | |||
| return s | |||
| } | |||
|
|
|||
| // WithCipherSuites sets the list of enabled TLS 1.0–1.2 cipher suites. | |||
There was a problem hiding this comment.
Some of the supported suites accepted by verifyCipherSuites() are compatible with TLS 1.3: TLS_AES_128_GCM_SHA256, TLS_AES_256_GCM_SHA384, TLS_CHACHA20_POLY1305_SHA256. Is this description wrong, or is something else in play here that disqualifies 1.3?
If the former, maybe we should not be so specific in the description.
If the latter, then we should probably make sure the user can't select these suites in the first place.
There was a problem hiding this comment.
Ok, It's the latter. See my comment below 👇 .
This change adds a new flag to `opa run` to allow users to specify a list of enabled TLS 1.0–1.2 cipher suites. This allows users to control the cipher suites the OPA server supports during a TLS handshake. Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar
force-pushed
the
add-cipher-suites
branch
from
64e4115
to
bb6b1a7
Compare
This change adds a new flag to
opa runto allow users to specify a list of enabled TLS 1.0–1.2 cipher suites. This allows users to control the cipher suites the OPA server supports during a TLS handshake.