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

Added https support for trunk serve. #458

Closed
wants to merge 2 commits into from

Conversation

murosuke
Copy link

@murosuke murosuke commented Nov 20, 2022

This allows users to serve with https. To enable this, add path to the cert files as below in Trunk.toml. If it failed to read cert files, it will start in http as usual.

[serve]
crt = "./server.crt"
key = "./server.key"

Checklist

  • 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 🤓.

@peteringram0
Copy link

Hi, Is there anyway to try this before it gets merged at all?

Cargo.toml Outdated
@@ -1,6 +1,6 @@
[package]
name = "trunk"
version = "0.16.0"
version = "0.17.0"
Copy link
Member

Choose a reason for hiding this comment

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

do not bump the version

Copy link
Author

Choose a reason for hiding this comment

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

I reverted version.


// Block this routine on the server's completion.
tracing::info!("{} server listening at http://{}", SERVER, addr);
let server = Server::bind(&addr)
Copy link
Member

Choose a reason for hiding this comment

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

Should this come before the tracing log?

Copy link
Author

Choose a reason for hiding this comment

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

Changed comment position just before server.await.

src/config/rt.rs Outdated
@@ -218,6 +218,10 @@ pub struct RtcServe {
pub proxies: Option<Vec<ConfigOptsProxy>>,
/// Whether to disable auto-reload of the web page when a build completes.
pub no_autoreload: bool,
/// Path to crt file.
pub crt: PathBuf,
Copy link
Member

Choose a reason for hiding this comment

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

Use Option<PathBuf> or OsStr for these types

Copy link
Author

Choose a reason for hiding this comment

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

Wrapped key/crt member with Option.

@simbleau
Copy link
Member

simbleau commented Jan 4, 2023

Please add an example with HTTPS, so we may test this.

@murosuke
Copy link
Author

@simbleau Thankyou for reviewing. I made a sample here.
I checked in windows.

run sample as https

  • Create key and crt pair with openssl(without encrypt).
    openssl req -x509 -new -keyout server.key -out server.crt -days 365
  • Place server.key and server.crt under the yew folder.
  • Add localhost as another domain in hosts file to prevents browsers from operating in trust mode.(Secure context WebAPI like showOpenFilePicker works with HTTP localhost, when usually should only work with HTTPS.)
    127.0.0.1 server.my
    • windows
      • C:\Windows\System32\drivers\etc\hosts
    • linux
      • /etc/hosts
  • Run yew with merged trunk.
    trunk serve
  • Access https://server.my:8080 with browser.
    If it is operating successfully, opening file picker should work(this won't work when accessing http://server.my:8080 running with trunk before merge).

@murosuke
Copy link
Author

@peteringram0 Sorry for the late reply.
You can clone original trunk, create branch and easly build it.
git fetch origin pull/458/head:PR-458
git checkout PR-458
cargo build
After build, trunk binary will be generated at target/target/debug/trunk.exe(I checked in windows).
Use target/target/debug/trunk.exe with project you are using trunk with.

@lpotthast
Copy link
Contributor

Is it possible to assist here?

@lpotthast
Copy link
Contributor

Note that this development version can also be installed using:

cargo install --git https://github.com/murosuke/trunk --branch develop trunk

@murosuke murosuke requested a review from simbleau March 18, 2023 08:18
@lpotthast
Copy link
Contributor

lpotthast commented Mar 18, 2023

Note: I can not close the dev-server using Ctrl-C. Is the signal correctly handled? I am using an M1 mac, Trunk config contains an active proxy section.

Also worth noting: I generated a self-signed certificate using a custom ca. That ca is part of my machines keychain. Chrome accepts the generated certificate perfectly. But: I had to set insecure = true in Trunk.tomls [[proxy]] section for communicating with my backend service (axum running in https locally using the same cert). Is Trunk using the systems CA store properly? I had not anticipated that this option would become necessary..

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.

One item so far, I'll finish up review on src/serve.rs shortly.

@@ -127,6 +127,10 @@ pub struct ConfigOptsServe {
#[clap(long = "no-autoreload")]
#[serde(default)]
pub no_autoreload: bool,
#[clap(parse(from_os_str))]
pub crt: Option<PathBuf>,
#[clap(parse(from_os_str))]
Copy link
Member

Choose a reason for hiding this comment

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

Let's add these to an arg group to ensure that it is clear to users that both must be declared together.

@lpotthast lpotthast mentioned this pull request Aug 8, 2023
4 tasks
@rogusdev
Copy link
Contributor

rogusdev commented Oct 1, 2023

Also worth noting: I generated a self-signed certificate using a custom ca. That ca is part of my machines keychain. Chrome accepts the generated certificate perfectly. But: I had to set insecure = true in Trunk.tomls [[proxy]] section for communicating with my backend service

Self signed certificates are officially invalid, by definition, and need insecure acceptance, for sure.

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 Nov 16, 2023
Copy link

This PR was closed because it has been stalled for 10 days with no activity.

@ctron
Copy link
Collaborator

ctron commented Dec 12, 2023

I do believe this was part of trunk-ng. With the merge of trunk-ng into trunk as part of PR #623, that should be part of the next trunk release. If that's not the case, please let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants