Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support rustls as an alternative TLS backend #735

Closed
wants to merge 9 commits into from

Conversation

jplatte
Copy link
Contributor

@jplatte jplatte commented Oct 13, 2020

Resolves #575.

@jplatte
Copy link
Contributor Author

jplatte commented Oct 13, 2020

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

@jplatte
Copy link
Contributor Author

jplatte commented Oct 14, 2020

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
Copy link
Contributor Author

jplatte commented Oct 20, 2020

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
Copy link
Contributor Author

jplatte commented Oct 20, 2020

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
Copy link
Contributor Author

jplatte commented Oct 20, 2020

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

@jplatte
Copy link
Contributor Author

jplatte commented Oct 20, 2020

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

@jplatte
Copy link
Contributor Author

jplatte commented Oct 20, 2020

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.

Copy link
Contributor Author

@jplatte jplatte left a comment

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!

sqlx-core/src/net/tls.rs Outdated Show resolved Hide resolved
@jplatte jplatte mentioned this pull request Oct 20, 2020
@jplatte
Copy link
Contributor Author

jplatte commented Oct 20, 2020

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

@jplatte
Copy link
Contributor Author

jplatte commented Oct 20, 2020

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

@BlackHoleFox
Copy link
Contributor

BlackHoleFox commented Oct 20, 2020

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
Copy link
Contributor Author

jplatte commented Oct 22, 2020

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

@abonander Thoughts on the large increase in CI jobs?

@abonander
Copy link
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
Copy link
Member

mehcode commented Oct 30, 2020

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
Copy link
Contributor Author

jplatte commented Oct 30, 2020

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
Copy link
Contributor

pimeys commented Oct 30, 2020

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
Copy link
Contributor Author

jplatte commented Nov 3, 2020

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
Copy link

vultix commented Nov 5, 2020

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

@mehcode
Copy link
Member

mehcode commented Nov 12, 2020

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
Copy link
Member

mehcode commented Nov 12, 2020

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
Development

Successfully merging this pull request may close these issues.

Add rustls feature
6 participants