tests: rework vendored certificates/keys #80
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was originally landed in the `tokio-tls` repo to support the `tokio-native-tls` crate's smoke tests. It was never used by the Rustls code in that repo, but was carried over anyway when we extracted that code into this repo. Let's remove it. We can come up with a better solution for the vendored test certificates we are using.
This keeps the tests dir tidy and will make it easier to add an update script that isn't itself an integration test.
Let the callers put the configs into an `Arc`. This will allow re-using the setup logic from `utils::make_configs()` in contexts where customization of the client or server config is required.
There's still some improvements left to be made, but this reduces a great deal of duplication in the test code.
* Remove the `CHAIN` const and tuple from `TEST_SERVER` - this is now encapsulated in the `ClientConfig` that's returned from `make_configs()` and no tests are constructing a config from scratch. Similarly the domain name is always `"foobar.com"` (this is baked into the vendored end-entity certificate). Let's just use a const for that. * Remove `start_server()` - it's too small to be of much utility. Let's just ref the `lazy_static!` directly.
Prefer `rustls_pemfile::private_key()` to `rsa_private_keys()`. The former is more general, and also doesn't require the `next()` dance that `rsa_private_keys()` does if you're only interested in one private key.
The existing unit tests used vendored cert/key data in a strange way. The `end.cert` and `end.chain` files were the same, and neither was a chain. In both cases the certificate was self-signed, and that same certificate was also configured as a trust anchor in the client configurations. No code/script was included to regenerate the cert (and it was set to expire in Aug). This commit replaces the test files to better simulate a real-world deployment with a trust anchor configured OOB and an intermediate and end-entity chain served by the TLS server. The test certificates are switched to use ECDSA (the rcgen default) for private keys instead of RSA. RSA is for the 90s and ECDSA will be faster :) No tests presently require the root or intermediate private keys, or a serialization of just the end entity cert without the intermediate, so we don't persist this data. This could be added in the future as req'd. All of the key/cert generation is bundled into an ignored integration test `tests/certs/main.rs` using a new dev-only dep on `rcgen`. This felt like the best option on balance, but we could also create a second crate, or look at the unstable nightly Cargo script feature.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I think there's more cleanup opportunity in these tests but here's a first pass:
scripts/generate-certificate.sh- the script and its output were never used by tokio-rustls. As best I can tell it was copied into this repo by mistake.tests/certs/main.rsintegration test that's ignored by default. When run, it usesrcgento recreate the vendored certificate/key data used by unit tests.