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

Upgrade rustls and tokio-rustls #175

Merged
merged 2 commits into from
May 8, 2024
Merged

Upgrade rustls and tokio-rustls #175

merged 2 commits into from
May 8, 2024

Conversation

soywod
Copy link
Contributor

@soywod soywod commented May 7, 2024

I bumped rustls@0.23.1 and tokio-rustls@0.26.0.

I also replaced webpki-roots by rustls-native-certs (see why), and put native certs loading in a static Lazy cell.


What is the prefered method to test examples (the server 127.0.0.1:12345)?

@coveralls
Copy link
Collaborator

coveralls commented May 7, 2024

Pull Request Test Coverage Report for Build 8999710278

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 52.67%

Totals Coverage Status
Change from base Build 8992715358: 0.0%
Covered Lines: 730
Relevant Lines: 1386

💛 - Coveralls

@duesee
Copy link
Owner

duesee commented May 7, 2024

Awesome, thank you! Are you able to fix the remaining CI issues? Skimming over the jobs, we need at least to add "BSD-3-Clause" to deny.toml (and check with cargo deny check)? Happy to have this in. We should also consider using rustls-native-certs in the proxy :-)

@duesee
Copy link
Owner

duesee commented May 7, 2024

I think the other CI fail was spurious. I rerun the job, let's see what happens :-) (Happy to have a "local CI" soon :D)

@soywod
Copy link
Contributor Author

soywod commented May 7, 2024

Are you able to fix the remaining CI issues?

Yes, except for the Windows one. I will fix tomorrow morning.

We should also consider using rustls-native-certs in the proxy :-)

Is it not what I tried to achieve there?

@duesee
Copy link
Owner

duesee commented May 7, 2024

Is it not what I tried to achieve there?

Urgh... sorry. I think I confused something. This is even better :-)

@soywod
Copy link
Contributor Author

soywod commented May 7, 2024

What is the prefered method to test examples (the server 127.0.0.1:12345)?

And what about this? How do you usually spawn your server to test locally?

@jakoschiko
Copy link
Collaborator

What is the prefered method to test examples (the server 127.0.0.1:12345)?

And what about this? How do you usually spawn your server to test locally?

The examples can be tested by using netcat. Though it's very time-consuming and I wouldn't recommend it.

Usually we test imap-flow by using the proxy. We set up different dummy mail accounts and connect to them via the proxy. The setup is an one-time effort and then it's really easy to do some real-world testing.

We also spent much time in writing integration tests, just run cargo test in the flow-test folder. Unfortunately the code coverage is not perfect yet and this won't test TLS specific code.

@soywod soywod marked this pull request as ready for review May 8, 2024 06:33
@duesee
Copy link
Owner

duesee commented May 8, 2024

What Jakob said. Also: The idea was that you can run client and server in pairs. Maybe we should put that in CI, too. Would require some well-defined exit condition such as a logout. I'll file an issue.

@soywod
Copy link
Contributor Author

soywod commented May 8, 2024

I try to fix the CI (had an idea based on this comment) but it needs approval every time I push, is there a way to "auto-approve" (at least for this PR)?

@duesee
Copy link
Owner

duesee commented May 8, 2024

I try to fix the CI (had an idea based on this comment) but it needs approval every time I push, is there a way to "auto-approve" (at least for this PR)?

Fixed (hopefully). We don't strictly require any approval for PRs anymore :-)

@soywod
Copy link
Contributor Author

soywod commented May 8, 2024

Awesome, seems to work! The thing I learnt is that rustup is installed by default on GitHub Action machines. By running rustup install it tries to remove the existing installation, and failed on Windows due to permission issues (wrong path?). Better just rustup update to avoid this.

Now there is a new error on Windows, I try to fix!

EDIT: seems to be related to rustls/rustls#1913

@soywod
Copy link
Contributor Author

soywod commented May 8, 2024

Yay, all green! The second error was linked to tokio-rustls. It uses aws-lc as a crypto lib, which needs NASM on Windows.

Ready to merge.

@duesee
Copy link
Owner

duesee commented May 8, 2024

Awesome, thanks! One last thing: Could you cleanup the commit history, i.e., squash all CI changes, and squash all TLS changes? Then this is ready to go :-) Already approving.

@duesee duesee self-requested a review May 8, 2024 08:19
@soywod
Copy link
Contributor Author

soywod commented May 8, 2024 via email

@duesee
Copy link
Owner

duesee commented May 8, 2024

Peeeerfect! 🎉 Thanks!

@duesee duesee merged commit 4af3e1a into duesee:main May 8, 2024
10 checks passed
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.

4 participants