Skip to content

Support rustls as an alternative TLS backend#735

Closed
jplatte wants to merge 9 commits into
transact-rs:masterfrom
lumeohq:rustls
Closed

Support rustls as an alternative TLS backend#735
jplatte wants to merge 9 commits into
transact-rs:masterfrom
lumeohq:rustls

Conversation

@jplatte

@jplatte jplatte commented Oct 13, 2020

Copy link
Copy Markdown
Contributor

Resolves #575.

@jplatte

jplatte commented Oct 13, 2020

Copy link
Copy Markdown
Contributor Author

Note to self: CI failure might be due to not activating one of the new _prefixed features correctly.

@jplatte

jplatte commented Oct 14, 2020

Copy link
Copy Markdown
Contributor Author

Was a different error, sqlx-cli was using the old feature. I guess maybe I should also not trigger the "runtime has to be selected" error when one of the old ones is activated, it seems like that doesn't necessarily trigger the first compile_error! that matches the configuration.

@jplatte

jplatte commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

I moved some ground work to #740, now pushed just a very minimal commit that should only update cargo features (it noticably doesn't touch Cargo.lock) and hoping that will succeed in CI. Really no idea why it failed before..

@jplatte

jplatte commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

Hey, I made it work... Somehow?? It does seem a bit like a lockfile update was the cause for the previous breakage, so I'll open a separate PR for just cargo update to see whether that makes tests fail.

@jplatte jplatte mentioned this pull request Oct 20, 2020
@jplatte jplatte marked this pull request as ready for review October 20, 2020 12:01
@jplatte

jplatte commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

Ah, so there is some postgres & mysql specific TLS code. Only tested with sqlite locally.

@jplatte

jplatte commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

Pushed all new preliminary refactorings separately from the rustls commit again to have CI verify them in isolation.

@jplatte

jplatte commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

So it was a dependency thing afterall! I think I should leave the cargo update commit in then (could also revert the async-std update, later realized it wasn't necessary), so nobody else has issues with this in their PR after this is merged.

@jplatte jplatte left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now truly ready for review!

Comment thread sqlx-core/src/net/tls.rs Outdated
@jplatte jplatte mentioned this pull request Oct 20, 2020
@jplatte

jplatte commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

So this added 45 new CI jobs to the existing 49... Should probably revisit testing every possible runtime, tls backend, database (version) 😅

@jplatte

jplatte commented Oct 20, 2020

Copy link
Copy Markdown
Contributor Author

Ah, so some tests rely on certificate validation being optional. I'll wait for feedback before playing around and wasting more CI time..

@BlackHoleFox

BlackHoleFox commented Oct 20, 2020

Copy link
Copy Markdown
Contributor

I'm glad to see someone else was looking at ditching OpenSSL too :D. I see a thread above around the certificate validation requirements. I actually have a branch here where I was working on replacing everything with Rustls and I implemented the different certification validation levels. Feel free to take a look @jplatte.

@jplatte

jplatte commented Oct 22, 2020

Copy link
Copy Markdown
Contributor Author

With @BlackHoleFox'es changes integrated, this now passes all 94 CI jobs 🎉 😅

@abonander Thoughts on the large increase in CI jobs?

@abonander

Copy link
Copy Markdown
Collaborator

For the record, I'm not a huge fan of the combinatorial explosion of jobs here. At least we're not footing the bill for CI, eh?

@jplatte looks like this needs a rebase though.

@mehcode

mehcode commented Oct 30, 2020

Copy link
Copy Markdown
Contributor

Can we perhaps mark a good chunk of the CI jobs here as "non-required" or something and only run them on master ?


I love the capability added here. Great work @jplatte. I really wish we could do this with feature pairs in Cargo e.g., sqlx = { features = [ "runtime-async-std", "tls-native" ] }.

Though I think that might be served with something more suited for mutual exclusion, e.g., sqlx = { options = { "runtime": "async-std", "tls": "native-tls" } }.

Would be an interesting thing to add to Cargo. That's not relevant to this PR though.

@jplatte

jplatte commented Oct 30, 2020

Copy link
Copy Markdown
Contributor Author

Can we perhaps mark a good chunk of the CI jobs here as "non-required" or something and only run them on master ?

Sure. Check runs pretty fast, so that can probably be run always. What about only running one version of each database for PRs?

@pimeys

pimeys commented Oct 30, 2020

Copy link
Copy Markdown

I was reading this with an interest, could it solve the issue of connecting to SQL Server from macOS platforms, where Apple's Security Framework doesn't allow the short keys Microsoft uses in the server. We have trouble doing TLS connections in Tiberius with native-tls on macOS, but it seems there's no TLS for the Microsoft database yet in SQLx...

@jplatte jplatte force-pushed the rustls branch 3 times, most recently from e110bfd to eb0bb6e Compare November 3, 2020 23:42
@jplatte

jplatte commented Nov 3, 2020

Copy link
Copy Markdown
Contributor Author

I've tried but can't figure out how to skip some runtime tests run on pull requests (without duplicating jobs). I guess using one of those weird YAML features to copy stuff around like GitLab encourages for CI jobs IIRC could work, but I haven't seen anybody do that with Actions. Ideas?

@vultix

vultix commented Nov 5, 2020

Copy link
Copy Markdown

@pimeys I'm here hoping the same thing. My team is currently blocked on MacOS because of the use of native-tls

@mehcode

mehcode commented Nov 12, 2020

Copy link
Copy Markdown
Contributor

Thanks for all the work here. We need to revisit CI but let's not keep blocking on that.

@mehcode mehcode closed this Nov 12, 2020
@mehcode mehcode reopened this Nov 12, 2020
@mehcode mehcode closed this Nov 12, 2020
@mehcode

mehcode commented Nov 12, 2020

Copy link
Copy Markdown
Contributor

Merged on the command line: 1ed75ba

@jplatte jplatte deleted the rustls branch November 12, 2020 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add rustls feature

6 participants