chore(networking)!: Move all TLS support over to openssl #1986

bruceg · 2020-03-04T22:42:38Z

This is incomplete, as several error paths remain incomplete.

There is one possible user-visible configuration change. At this point, the verify_hostname setting is non-functional due to a difference (at least in my understanding) of how openssl handles verification as compared to native_tls. I am still investigating and may need to port some code over to accommodate the differences.

Closes #1402
Closes #1929

Signed-off-by: Bruce Guenter <bruce@timber.io>

LucioFranco · 2020-03-05T15:30:02Z

@bruceg for the verify stuff it looks like SslConnectorBuilder::set_verify and https://docs.rs/openssl/0.10.28/openssl/ssl/struct.SslVerifyMode.html setting this to NONE should be the same as setting the dangerous setting for native tls. In the end native-tls maps to openssl so we should be able to find in the code what it does and port that.

Otherwise, this is looking really good, once you have the error paths in I can give this the final look over. Great work!

bruceg · 2020-03-05T15:40:40Z

I've found the separate settings for verifying the hostname vs the certificate, but it will require a little bit of type reworking (the functions are in separate types). However, I've been hamstrung by a separate verification bug -- even with using SslVerifyMode::NONE, the send_lines_tls function in test_utils.rs is failing due to "unable to get local issuer certificate", which shouldn't be happening if verification is turned off.

bruceg · 2020-03-05T16:37:33Z

The issue lies in the acceptor side's certificate verification setting, which actually points to a bit of a problem with our configuration defaults. We default verify_certificate to true, which can actually have two different meanings for the acceptor side that native-tls didn't expose:

If the connector presents a certificate, verify it, but succeed otherwise. This is the existing behavior.
Require a certificate from the connector, fail if it isn't present, and verify it.

If we have a single verify_certificate setting, I think 2 makes more sense for the acceptor, since you can't verify what isn't presented and clients don't normally present a certificate. However, that becomes a breaking change to our defaults. I could add another setting for that, but it becomes a little redundant.

It was requiring certificate verification, which has changed behavior now. Signed-off-by: Bruce Guenter <bruce@timber.io>

lukesteensen

The bits I'm familiar with look reasonable 👍

LucioFranco

LGTM, just one non blocking question.

src/tls.rs

This ends up *disabling* `verify_hostname` for all of the sinks that directly or indirectly use the `hyper_openssl::HttpsConnector::with_connector` method. Signed-off-by: Bruce Guenter <bruce@timber.io>

binarylogic · 2020-03-05T21:50:12Z

If this is a breaking change we should add a ! to the title.

Signed-off-by: Bruce Guenter <bruce@timber.io>

This restores the verification behavior prior to this change series. This effectively reverts e165c9a "Fix test `sources::socket::test::tcp_with_tls`" Signed-off-by: Bruce Guenter <bruce@timber.io>

bruceg · 2020-03-05T23:35:51Z

Ok, I think the churn is done. I'd appreciate a re-review of the late commits, as there are some behavioural changes.

LucioFranco · 2020-03-06T15:43:49Z

src/tls.rs

@@ -128,15 +128,15 @@ pub struct IdentityStore(Vec<u8>, String);
 impl TlsSettings {
    pub fn from_config(
        config: &Option<TlsConfig>,
-        require_ident: bool,
+        for_server: bool,


Does this mean that this setting is for a server? aka if we are a server we set this to true?

Also looks like we are not updating any other rust files where before this boolean meant something else, just want to make sure that is correct?

Correct, setting this to true is for servers. I renamed it because it controls both requiring an identity (certificate + key) in the configuration as well as changing the defaults for certificate verification. The only users of from_config that would set this to true are the server sources, so no changes to them were required.

Just for the future, can we add a comment what these params mean since I feel like people stumbling across this in the future might be confused.

Absolutely, I was already thinking this as you were asking.

Signed-off-by: Bruce Guenter <bruce@timber.io>

LucioFranco

Thanks for getting this done @bruceg! I know working with openssl is never fun but this should really ensure we have some good consistency with all our sinks/sources/transforms.

Bruce Guenter added 4 commits March 4, 2020 10:14

Merge various TlsConnector creators into the tls lib

61ab43b

Signed-off-by: Bruce Guenter <bruce@timber.io>

Migrate from native_tls to openssl directly

f515717

Signed-off-by: Bruce Guenter <bruce@timber.io>

Drop remainder of uses of hyper-tls

7889de5

Signed-off-by: Bruce Guenter <bruce@timber.io>

Tweak sources-tls feature flag usage

60eb6d1

Signed-off-by: Bruce Guenter <bruce@timber.io>

bruceg added type: tech debt A code change that does not add user value. domain: security Anything related to security domain: networking Anything related to Vector's networking labels Mar 4, 2020

bruceg requested a review from LucioFranco March 4, 2020 22:42

Bruce Guenter added 2 commits March 4, 2020 16:51

Resolve error messages in the tls lib

a0f9096

Signed-off-by: Bruce Guenter <bruce@timber.io>

Resolve TLS error handling in sinks http lib

a6eebde

Signed-off-by: Bruce Guenter <bruce@timber.io>

binarylogic assigned LucioFranco Mar 5, 2020

Fix test sources::socket::test::tcp_with_tls

e165c9a

It was requiring certificate verification, which has changed behavior now. Signed-off-by: Bruce Guenter <bruce@timber.io>

bruceg force-pushed the only-openssl branch from 6941412 to e165c9a Compare March 5, 2020 16:44

bruceg marked this pull request as ready for review March 5, 2020 16:44

bruceg requested a review from lukesteensen as a code owner March 5, 2020 16:44

lukesteensen approved these changes Mar 5, 2020

View reviewed changes

LucioFranco approved these changes Mar 5, 2020

View reviewed changes

src/tls.rs Show resolved Hide resolved

Enable the verify_hostname configuration setting

e89f70c

This ends up *disabling* `verify_hostname` for all of the sinks that directly or indirectly use the `hyper_openssl::HttpsConnector::with_connector` method. Signed-off-by: Bruce Guenter <bruce@timber.io>

bruceg requested a review from binarylogic as a code owner March 5, 2020 20:56

bruceg changed the title ~~chore(networking): Move all TLS support over to openssl~~ chore(networking)!: Move all TLS support over to openssl Mar 5, 2020

bruceg mentioned this pull request Mar 5, 2020

Resolve handling of tls.verify_hostname for HTTP sinks #1992

Closed

Bruce Guenter added 3 commits March 5, 2020 16:04

Merge remote-tracking branch 'origin/master' into only-openssl

bffa2d7

Signed-off-by: Bruce Guenter <bruce@timber.io>

Uncomment the acceptor (server) documentation that has now been enabled

7f7d7d9

Signed-off-by: Bruce Guenter <bruce@timber.io>

Revert certificate verification to false for servers

7444863

This restores the verification behavior prior to this change series. This effectively reverts e165c9a "Fix test `sources::socket::test::tcp_with_tls`" Signed-off-by: Bruce Guenter <bruce@timber.io>

LucioFranco reviewed Mar 6, 2020

View reviewed changes

Add a bit of documentation to clarify the TlsSettings creators

98600c7

Signed-off-by: Bruce Guenter <bruce@timber.io>

LucioFranco approved these changes Mar 6, 2020

View reviewed changes

bruceg merged commit 9f793a7 into master Mar 6, 2020

bruceg deleted the only-openssl branch March 6, 2020 16:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(networking)!: Move all TLS support over to openssl #1986

chore(networking)!: Move all TLS support over to openssl #1986

bruceg commented Mar 4, 2020

LucioFranco commented Mar 5, 2020

bruceg commented Mar 5, 2020

bruceg commented Mar 5, 2020

lukesteensen left a comment

LucioFranco left a comment

binarylogic commented Mar 5, 2020

bruceg commented Mar 5, 2020

LucioFranco Mar 6, 2020

bruceg Mar 6, 2020

LucioFranco Mar 6, 2020

bruceg Mar 6, 2020

LucioFranco left a comment

chore(networking)!: Move all TLS support over to openssl #1986

chore(networking)!: Move all TLS support over to openssl #1986

Conversation

bruceg commented Mar 4, 2020

LucioFranco commented Mar 5, 2020

bruceg commented Mar 5, 2020

bruceg commented Mar 5, 2020

lukesteensen left a comment

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment

binarylogic commented Mar 5, 2020

bruceg commented Mar 5, 2020

LucioFranco Mar 6, 2020

Choose a reason for hiding this comment

bruceg Mar 6, 2020

Choose a reason for hiding this comment

LucioFranco Mar 6, 2020

Choose a reason for hiding this comment

bruceg Mar 6, 2020

Choose a reason for hiding this comment

LucioFranco left a comment

Choose a reason for hiding this comment