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

Add TLS support for trunk serve #532

Closed
wants to merge 3 commits into from
Closed

Conversation

lukaselmer
Copy link
Contributor

Adds TLS support, closes #277

  • Updated CHANGELOG.md describing pertinent changes.
  • Updated README.md with pertinent info (may not always apply).
  • Updated site content with pertinent info (may not always apply).
  • Squash down commits to one or two logical commits which clearly describe the work you've done. If you don't, then Dodd will 🤓.

@lukaselmer lukaselmer changed the title Add TLS support for trunk serve Add TLS support for trunk serve Apr 16, 2023
Copy link
Member

@thedodd thedodd left a comment

Choose a reason for hiding this comment

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

Thanks for taking the time to open this PR! I think this is one of the better shots at adding TLS to the serve stack.

Just a few review items. Also, do you have any thoughts on enabling TLS for Websockets as well? Perhaps it just works out of the box 🤞.

src/config/rt.rs Outdated Show resolved Hide resolved
src/config/rt.rs Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Trunk.toml Outdated Show resolved Hide resolved
examples/yew-tls/Trunk.toml Outdated Show resolved Hide resolved
@lukaselmer
Copy link
Contributor Author

Thanks for taking the time to open this PR! I think this is one of the better shots at adding TLS to the serve stack.

Just a few review items.

Cool, thx! I agree with your feedback and can implement the requested changes in the next couple of days.

Also, do you have any thoughts on enabling TLS for Websockets as well? Perhaps it just works out of the box 🤞.

Good question. When searching for TLS, I noticed https://github.com/thedodd/trunk/pull/492/files, but decided it's not related to what I wanted to do. From what I understand from this PR it's currently not working, and I wouldn't expect it to work with this MR, but it's worth a shot for sure :)

Now that I think of it, ws:// won't work when using https:// - so it's probably a good idea to get wss:// to work.

Hmm. I checked out a couple of issues / PRs, and after reading the changelog of tokio-tungstenite https://github.com/snapview/tokio-tungstenite/blob/master/CHANGELOG.md#0140 I think that one of the tls features for tokio-tungstenite (e.g. rustls-tls-native-roots, which is suggested in https://github.com/thedodd/trunk/pull/492/files) could be added to support wss:// urls. I think we would need a proxy to validate this 🤔

@lukaselmer lukaselmer force-pushed the master branch 2 times, most recently from 4339597 to 5c06dd2 Compare April 19, 2023 01:35
@lukaselmer
Copy link
Contributor Author

@thedodd It's done. I think that the code now is better than it was before. Let me know if other parts should be improved

Copy link
Contributor Author

@lukaselmer lukaselmer left a comment

Choose a reason for hiding this comment

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

Done

@lukaselmer
Copy link
Contributor Author

@thedodd this PR is ready to merge IMO. Is there something that should be improved?

@lukaselmer lukaselmer requested a review from thedodd May 10, 2023 18:04
Use clippy, improve error handling

Improve tls example

Rename TLS options, improve path resolution

Move Rustls config parsing into rt.rs
@lukaselmer
Copy link
Contributor Author

@thedodd it's still done, and I just rebased it to the latest upstream branch, so the merge conflicts should be gone. Is there something that should be improved?

@lpotthast
Copy link
Contributor

lpotthast commented Aug 8, 2023

@thedodd The changes of this branch work flawlessly.

It would be great if this PR could be merged and released. This allows to develop with HTTPS locally, mimicing production environments more closely. For certain things, running with a certificate is even a requirement!

I think that #458 should be closed / rejected in favour of this PR.

@ctron
Copy link
Collaborator

ctron commented Sep 4, 2023

I think the PR needs none more change, the provided certificate in the example expired already: July 31, 2022.

I think it makes sense to use a rather long expiration period for such an example certificate (10+ years).

@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Oct 20, 2023
@lukaselmer
Copy link
Contributor Author

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

This is not stale. Still waiting to be merged... Or feedback what to fix 🙄

@ctron
Copy link
Collaborator

ctron commented Oct 20, 2023

@lukaselmer I already took a look if I could merge it into trunk-ng. The PR was a bit too big for getting that done quickly.

I would encourage to raise a PR for https://github.com/ctron/trunk (trunk-ng branch).

@github-actions github-actions bot removed the Stale label Oct 21, 2023
@lpotthast lpotthast mentioned this pull request Nov 16, 2023
@lpotthast
Copy link
Contributor

This is now merged into https://github.com/ctron/trunk (trunk-ng branch).

@ctron
Copy link
Collaborator

ctron commented Nov 20, 2023

it is also released with version 0.17.13 of trunk-ng.

@ctron
Copy link
Collaborator

ctron commented Dec 12, 2023

This was brought back to trunk with PR #623 and it should be part of the next release of trunk too.

@ctron
Copy link
Collaborator

ctron commented Dec 13, 2023

This should be released with trunk 0.18.0

@ctron ctron closed this Dec 13, 2023
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.

Support serving over TLS
4 participants