Improve trusted CA support#100
Merged
Merged
Conversation
Contributor
Author
|
Tests are failing because modules are out of date. PR to fix: #101 |
Owner
|
Thanks! Can you please merge in the main to let the test run fine. |
For this commit, this always returns nil, but adding this signature to the function allows us to return errors when more complex functionality is added in follow on commits. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
This allows the config to specify an explict CA Bundle to use when trusting the upstream target. No other certificates, other than those signed by one of the CAs in this bundle, will be trusted. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
Signed-off-by: Emily Shepherd <emily@redcoat.dev>
The helm chart had a field called customCA which, when specified, would cause a secret to be mounted in the containers /etc/ssl/certs directory, implictly causing go to pick the CA bundle up and trust it for outbound TLS connections. With the added support to explictly define a CA to trust in the application, this commit updates the helm field's logic to instead use that. This has the advantage that _only_ this CA will be trusted, not it in addition to all other standard CAs bundled with the container image. Signed-off-by: Emily Shepherd <emily@redcoat.dev>
f6386be to
fcd5eac
Compare
Contributor
Author
Rebased on main and actioned your review comment |
Contributor
Author
|
I will add a test to cover the CA loading logic and check that it is configured corrected in fasthttp |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
This PR:
CT_CA_BUNDLE_FILE/auth.egress.tls_config.ca_bundle_file)config.ca_bundle_file.customCAvalue) to use the configuration option rather than mounting the CA as a system trust root for the container.This approach is preferable to the previous (
customCA) behaviour because now when setting a CA it, and only it, will be trusted. Previously the new CA would be trusted but so would all the default trust roots bundled with the container. Explicitly limiting the desired trusted CA bundle is preferable for some security setups.Additionally, the helm chart continues to support both the
customCAmethod and more explicit configuration viaconfig.ca_bundle_fileto support cases where the application wants to use a non-secret based certificate (such as a trust bundle injected in via the runtime, kubelet, serviceaccount CA, etc...)By adding the
tls_configconfiguration block, I hope to submit feature PRs adding support for client TLS certs to upstream, and TLS support for the server itself.As a side effect, this PR changes the return signature of the private
newProcessor()method from(*processor)to(*processor, error). The tests have been updated to match. As TLS functionality is implemented in fasthttp - this PR simply turns it on - no specific test for TLS support has been added.