Add ClientBuilder helper to make setting up TLS connections easy. #197

mordak · 2021-05-06T00:32:49Z

Addresses #190

Also add support for rustls in addition to native-tls. I think using rustls should be easier (and could even be the default?), so I just made it first class like native-tls. I have tested both the native_tls() and rustls() paths.

This just adds the ClientBuilder. If we're happy with the API here and the way it works, then I can take care of:

Fixing up the examples (doc and /examples) to use it where appropriate.
If we want to get rid of any code that is superseded by this?

I have not tested the STARTTLS code, since I do not have a STARTTLS enabled server to work with. How do we want to handle automating the tests to make sure it all works?

I have not added it, but I can see an argument for adding some knobs to control which CA bundle is added, or adding private CAs or something. The alternative it to just leave the existing API as is, which allows users to make their own TLS connection using whatever configuration that they want.

This change is

Add support for rustls in addition to native-tls.

codecov · 2021-05-06T00:36:15Z

Codecov Report

Merging #197 (fb07d97) into master (c443a3a) will decrease coverage by 3.72%.
The diff coverage is 21.21%.

Impacted Files	Coverage Δ
src/error.rs	`2.73% <0.00%> (-0.21%)`	⬇️
src/extensions/idle.rs	`0.00% <0.00%> (ø)`
src/types/deleted.rs	`98.61% <ø> (ø)`
src/client.rs	`85.55% <25.00%> (+0.60%)`	⬆️
src/client_builder.rs	`27.27% <27.27%> (ø)`
src/types/unsolicited_response.rs	`40.00% <0.00%> (-2.86%)`	⬇️

jonhoo

I like the approach! I left a suggestion in there that I think can deal with the more customized setup routines users may have. As for testing STARTTLS, I thought our integration test already did that? If not, we should add one.

src/client.rs

src/client_builder.rs

Co-authored-by: Jon Gjengset <[email protected]>

Use it for native and rustls initialization.

mordak · 2021-05-06T23:20:23Z

As for testing STARTTLS, I thought our integration test already did that? If not, we should add one.

We have one, but it is marked ignore. Is there another one?

jonhoo

Ugh, you're right, I forgot that Greenmail doesn't support STARTTLS. That's awkward. I suppose we'll just have to leave that one for users to test then 😢

src/client_builder.rs

azure-pipelines.yml

And make sure it works.

jonhoo · 2021-05-08T23:18:57Z

Ugh, separately, we really should add a codecov configuration file that sets less onerous restrictions for PRs Currently they basically always fail. Something like this one (docs here).

mordak · 2021-05-09T00:02:00Z

Ugh, separately, we really should add a codecov configuration file that sets less onerous restrictions for PRs Currently they basically always fail. Something like this one (docs here).

Happily added. Having the codecov always fail is a bit annoying. I copied the one you linked and tweaked it for the values that we're seeing in PRs here (ie 70% coverage low line, and don't fail for -2%). No problem if you want to change them to be tighter.

mordak · 2021-05-09T00:11:51Z

Separately - what, if anything, do we want to move to the builder API?

Doc examples?
Examples?
Anything in the lib?

I can see some of the docs and examples being kind of obvious. For the lib, some of the Client setup stuff could be moved the builder API internally while preserving the public API, or we could remove the public API in favour of the builder API, or we could just leave the lib as is and call it a day. Do you have an inclination here?

jonhoo · 2021-05-09T17:11:01Z

I think my preference here is actually to only have the builder API + Client::new for custom streams. What do you think?

Remove these functions. Update documentation to reference the ClientBuilder. Update examples to use the ClientBuilder except for timeout.rs, which now serves as an example of how to use Client::new() with a custom stream.

mordak · 2021-05-09T21:58:12Z

Ugh, you're right, I forgot that Greenmail doesn't support STARTTLS. That's awkward. I suppose we'll just have to leave that one for users to test then 😢

I put together a local test for this and confirmed the STARTTLS implementation in the ClientBuilder works.

mordak · 2021-05-09T22:00:33Z

I think my preference here is actually to only have the builder API + Client::new for custom streams. What do you think?

Oh that sounds good to me. I prefer having one good way to do it: it is less code to maintain and nicer for users.

I pushed a diff that removes connect() and connect_starttls() and updates the examples, tests and docs to use the ClientBuilder instead. I think the only possible wrinkle is the greenmail tests, which may not be happy with the certificate validation. If that's the case then I can figure it out.

jonhoo

Looks great, thanks!

jonhoo · 2021-05-11T02:38:38Z

Cargo.toml

@@ -15,10 +15,12 @@ categories = ["email", "network-programming"]

 [features]
 tls = ["native-tls"]
+rustls-tls = ["rustls-connector"]


Separately, I wonder if we want to take this opportunity to rename the "other" tls feature native-tls, since it isn't really "more" standard than rustls.

I think this is a good idea, so did a PR up here.

Add ClientBuilder helper to make setting up TLS connections easy.

ce3283d

Add support for rustls in addition to native-tls.

mordak added 2 commits May 5, 2021 21:18

Fix cirrus tests.

d7182b2

clippy.

82bcafd

jonhoo requested changes May 6, 2021

View reviewed changes

src/client.rs Outdated Show resolved Hide resolved

src/client_builder.rs Outdated Show resolved Hide resolved

src/client_builder.rs Show resolved Hide resolved

mordak and others added 3 commits May 6, 2021 16:02

Update src/client.rs

61f4a75

Co-authored-by: Jon Gjengset <[email protected]>

Use correct tls feature name.

0f32fe5

Add custom connect() function to handle TLS initialization.

3f798df

Use it for native and rustls initialization.

jonhoo requested changes May 8, 2021

View reviewed changes

src/client_builder.rs Show resolved Hide resolved

src/client_builder.rs Show resolved Hide resolved

src/client_builder.rs Show resolved Hide resolved

src/client_builder.rs Show resolved Hide resolved

azure-pipelines.yml Show resolved Hide resolved

mordak added 3 commits May 8, 2021 13:53

Merge remote-tracking branch 'origin/master' into tls_builder

75694a6

Make doc tests work with --no-default-features

59dccd9

Add cargo hack to pipeline

40369f7

And make sure it works.

mordak added 2 commits May 8, 2021 19:47

Rejig doc test cfg lines

7e2372f

Add codecov config.

f7cdba4

Replace connect() and connect_starttls() with ClientBuilder.

25efded

Remove these functions. Update documentation to reference the ClientBuilder. Update examples to use the ClientBuilder except for timeout.rs, which now serves as an example of how to use Client::new() with a custom stream.

Fix integration tests.

fb07d97

jonhoo approved these changes May 11, 2021

View reviewed changes

jonhoo merged commit 7204697 into jonhoo:master May 11, 2021

jonhoo mentioned this pull request May 11, 2021

Update changelog for v3 #199

Merged

mordak mentioned this pull request May 23, 2021

Change "tls" feature to "native-tls" #201

Merged

Add ClientBuilder helper to make setting up TLS connections easy. #197

Add ClientBuilder helper to make setting up TLS connections easy. #197

Uh oh!

Conversation

mordak commented May 6, 2021 • edited by jonhoo Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented May 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jonhoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mordak commented May 6, 2021

Uh oh!

jonhoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jonhoo commented May 8, 2021

Uh oh!

mordak commented May 9, 2021

Uh oh!

mordak commented May 9, 2021

Uh oh!

jonhoo commented May 9, 2021

Uh oh!

mordak commented May 9, 2021

Uh oh!

mordak commented May 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonhoo left a comment

Choose a reason for hiding this comment

Uh oh!

jonhoo May 11, 2021

Choose a reason for hiding this comment

Uh oh!

mordak May 23, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mordak commented May 6, 2021 •

edited by jonhoo

Loading

codecov bot commented May 6, 2021 •

edited

Loading

mordak commented May 9, 2021 •

edited

Loading