Add separate option for verifying incoming HTTPS traffic #2974
Conversation
kyhavlov
force-pushed
the
verify-incoming-https
branch
from
663f2fd
to
a301873
Compare
|
To keep backward compatibility with the existing sense of This is still a little tricky, but I think it's simpler than some kind of "skip" flag for HTTPS. We can document that |
There was a problem hiding this comment.
The api/api_test.go could use a test that disabling the VerifyIncomingHTTPS allows a TLS connection without a matching certificate. This is a good case for the t.Run("...", func(t *testing.T) {...}) subtests since you can re-use the clients in the TestClientTLSOptions test.
testutil/server.go
Outdated
| CertFile string `json:"cert_file,omitempty"` | ||
| KeyFile string `json:"key_file,omitempty"` | ||
| VerifyIncoming bool `json:"verify_incoming,omitempty"` | ||
| VerifyIncomingHTTPS bool `json:"verify_incoming_https,omitempty"` |
There was a problem hiding this comment.
Don't you need a VerifyIncomingRPC as well here?
There was a problem hiding this comment.
yeah, it should be here for completeness
| * <a name="verify_incoming_rpc"></a><a href="#verify_incoming_rpc">`verify_incoming_rpc`</a> - If | ||
| set to true, Consul requires that all incoming RPC | ||
| connections make use of TLS and that the client provides a certificate signed | ||
| by the Certificate Authority from the [`ca_file`](#ca_file). By default, this is false, and |
There was a problem hiding this comment.
Doesn't that need updating with the recent CAPath change you were working on?
|
|
||
| * <a name="verify_incoming_https"></a><a href="#verify_incoming_https">`verify_incoming_https`</a> - If | ||
| set to true, Consul requires that all incoming HTTPS | ||
| connections make use of TLS and that the client provides a certificate signed | ||
| by the Certificate Authority from the [`ca_file`](#ca_file). By default, this is false, and |
api/api_test.go
Outdated
| if err != nil { | ||
| t.Fatal(err) | ||
| } | ||
| // Start a server without VerifyIncomingHTTPS |
There was a problem hiding this comment.
Move this to the top and give it a different name, e.g. srvNoVerify
api/api_test.go
Outdated
| @@ -256,6 +256,7 @@ func TestSetupTLSConfig(t *testing.T) { | |||
|
|
|||
| func TestClientTLSOptions(t *testing.T) { | |||
| t.Parallel() | |||
| // Start a server that verifies incoming HTTPS connections | |||
| _, s := makeClientWithConfig(t, nil, func(conf *testutil.TestServerConfig) { | |||
There was a problem hiding this comment.
s/s/srvVerify/
Don't you have to set conf.VerifyIncomingHTTPS = true ?
There was a problem hiding this comment.
it is set to true, right below this
|
LGTM |
|
Please squash the change |
Resolves #2969