test: generate and test ECDSA certificates in ssl_integration_test.#5096
test: generate and test ECDSA certificates in ssl_integration_test.#5096htuch merged 6 commits intoenvoyproxy:masterfrom
Conversation
Also, some bonus cleanups to improve DRY and maintainability of SSL related test code encountered while doing this. Risk Level: Low Testing: bazel test //test/integration/..., new tests for client cipher suites and ECDSA server certs added to ssl_integration_test. Signed-off-by: Harvey Tuch <htuch@google.com>
| @@ -0,0 +1,40 @@ | |||
| [req] | |||
There was a problem hiding this comment.
Could you somehow re-use servercert.cfg here (e.g. by passing config-name as another parameter)? Otherwise, we're introducing unnecessary duplication.
| EXPECT_EQ(1U, counter->value()); | ||
| counter->reset(); | ||
| } | ||
|
|
There was a problem hiding this comment.
You're missing the case with RSA server and ECDSA client.
There was a problem hiding this comment.
This is currently implied, but sure, I can make an explicit variant.
| TestEnvironment::runfilesPath("/test/config/integration/certs/servercert.pem")); | ||
| tls_certificate->mutable_private_key()->set_filename( | ||
| TestEnvironment::runfilesPath("/test/config/integration/certs/serverkey.pem")); | ||
| if (ecdsa_cert) { |
There was a problem hiding this comment.
Does this have to be either or? Once rest of your changes land, we're going to have ability to set both.
Perhaps pass rsa_cert and ecdsa_cert, and guard that it's either or in this commit?
There was a problem hiding this comment.
Yeah, eventually it will be both, but we will also want additional flexibilty with multi-cert, e.g. to have multiple ECDSA certs in testing to verify we only pick the first. So, I think we can leave it like this for this PR and then add this in later.
Signed-off-by: Harvey Tuch <htuch@google.com>
Signed-off-by: Harvey Tuch <htuch@google.com>
| @@ -0,0 +1,4 @@ | |||
| // NOLINT(namespace-envoy) | |||
| #define TEST_SERVER_ECDSA_CERT_HASH \ | |||
| "F7:97:78:71:62:1F:48:EA:F1:76:A3:AA:11:71:6E:30:99:E7:0A:0F:A2:51:DB:B5:80:42:89:13:78:3A:80:" \ | |||
There was a problem hiding this comment.
constexpr char[] or constexpr absl::string_view?
PiotrSikora
left a comment
There was a problem hiding this comment.
LGTM, other than @lizan's nit (constexpr). Thanks!
Signed-off-by: Harvey Tuch <htuch@google.com>
…nvoyproxy#5096) Also, some bonus cleanups to improve DRY and maintainability of SSL related test code encountered while doing this. Part of envoyproxy#1319. Risk Level: Low Testing: bazel test //test/integration/..., new tests for client cipher suites and ECDSA server certs added to ssl_integration_test. Signed-off-by: Harvey Tuch <htuch@google.com> Signed-off-by: Fred Douglas <fredlas@google.com>
Also, some bonus cleanups to improve DRY and maintainability of SSL related test code encountered
while doing this.
Part of #1319.
Risk Level: Low
Testing: bazel test //test/integration/..., new tests for client cipher suites and ECDSA server
certs added to ssl_integration_test.
Signed-off-by: Harvey Tuch htuch@google.com