Use the same option source for SSL (certs and keys) in Preview and Dev #1662

JBusillo · 2021-06-10T04:09:21Z

This PR was clobbered due to my error. It has been replaced by PR #2027.

The dev server passes the vite options (config.kit.vite) when creating the http(s) server. This is functioning correctly.
The preview server passes the kit options (config.kit) when creating the http(s) server. This is not functioning correctly.

In preview mode, when the user specifies a cert/key pair, the cert/key pair is ignored, since the logic is looking for the option in the wrong place. Instead, Kit will generate its own cert/key pair. You can observe this by examining the certificate details within the browser.

This change aligns where the dev and preview modes look for a user-specified cert/key pair.

For reference, here is an example of a user-specified cert/key pair in svelte.config.js

/** @type {import('@sveltejs/kit').Config} */
import fs from "fs";
const config = {
  kit: {
    // hydrate the <div id="svelte"> element in src/app.html
    target: "#svelte",
    vite: {
      server: {
        https: {
          key: fs.readFileSync("example.key"),
          cert: fs.readFileSync("example.crt"),
        },
      },
    },
  },
};
export default config;

Before submitting the PR, please make sure you do the following

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

…ue-1659

JBusillo · 2021-06-10T04:23:26Z

We might want to add some documentation in the FAQ about this, instead of having to dig around in the Vite options to accommodate this. Please advise if you'd like me to add to the documentation.

JBusillo · 2021-06-10T17:59:12Z

I'd also like to propose moving the cert and key options from Vite's config to Kit's config. Kit is responsible for setting up the server when in middlewares mode, so it's actually Kit that self-signs certificates and passes certificates to http(s).createServer.

If this were to be implemented, I'd suggest the following changes:

Prevent user from using config.kit.vite.server.https.key and .cert -- perhaps a check in kit/src/cli.js -- which could be removed once kit leaves beta (or not).
Add an "ssl" option to kit as follows:

const config = {
  kit: {
    target: "#svelte",
    ssl: {
      key: fs.readFileSync("example.key"),
      cert: fs.readFileSync("example.crt"),
    }, 
  },
};

Make the supporting changes to kit/src/server/index.js, kit/src/dev/index.js and kit/src/start/index.js
Update Kit config documentation accordingly

Pros:

Clearer line for separation of duties. Kit creates the server, and already uses command line options such as --host, --https and --port. Using the Vite option implies that Vite controls server creation. No other options are used for server creation.
The user doesn't need to dig through Vite's documentation to implement their own self-signed certificates.

Cons:

More code changes, testing, coordination, risk
"Bang for the buck" -- Do many users use their own self-signed certificates?

I'm happy with any decision. I was hesitant on proposing this change, but I thought it might be worth a look.

benmccann · 2021-06-10T21:59:52Z

packages/kit/src/core/start/index.js

@@ -48,7 +48,7 @@ export async function start({ port, host, config, https: use_https = false, cwd
 		read: (file) => fs.readFileSync(join(config.kit.files.assets, file))
 	});

-	const server = await get_server(use_https, config.kit, (req, res) => {
+	const server = await get_server(use_https, config.kit.vite, (req, res) => {


the parameter name in get_server and in dev where it's being called (since #1517) is user_config. It sounds like perhaps it should be renamed to vite_config. also the type is any which is allowing it to be different between the different places. it'd be nice if that could be updated to avoid a regression here

I agree with the renaming and type usage. There is a problem with Vite's type definitions for their config, but there will be a correction in the next patch release. Also, a lot of changes already have been made to dev/index.js in #1572.

I think we need to hold off at least until the next Vite patch release and the next SvelteKit patch release.

I'm leaving for the US on 6/18, returning on 7/14, and should have a computer available.

If you want to just do the renaming now and then merge this that'd be fine and we can add the Vite types later

benmccann · 2021-06-10T22:01:33Z

I think another con is would be that Vite's option would then be unused or there'd be two places to specify it. I'd probably just leave it as is because of that

changeset-bot · 2021-06-15T22:48:18Z

🦋 Changeset detected

Latest commit: 1076db3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@sveltejs/kit	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

benmccann · 2021-06-16T22:00:12Z

@JBusillo it looks like the lint check is failing

JBusillo · 2021-06-16T22:06:47Z

Ben -- please hold off the review. It seems like #1572 is preventing sending key/cert options to Vite. It's preventing me from testing:

jbusillo@jbusillo-desktop:~/Projects/test-1659$ npm run dev

> test-1659@0.0.1 dev
> svelte-kit dev

  The value for kit.vite.server.https.key.toString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit.
  The value for kit.vite.server.https.key.toLocaleString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit.
  The value for kit.vite.server.https.cert.toString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit.
  The value for kit.vite.server.https.cert.toLocaleString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit.
> The "options.cert" property must be of type string or an instance of Buffer, TypedArray, or DataView. Received an instance of Object

@johnnysprinkles -- could you verify if your changes are preventing the user from specifying vite.server.https.cert and vite.server.https.key options? You can refer to my test project.

I committed this just so johnnysprinkles has something to work with. I'll fix the lint error when I do my (hopefully) final commit.

johnnysprinkles · 2021-06-16T22:09:54Z

I can’t take a look at the moment but I might have caused a regression, I suggested in the PR to only consider plain objects where constructor is Object but didn’t get feedback on that. I can look tonight.

…

On Wed, Jun 16, 2021 at 3:06 PM Joe Busillo ***@***.***> wrote: Ben -- please hold off the review. It seems like #1572 <#1572> is preventing sending key/cert options to Vite. It's preventing me from testing: ***@***.***:~/Projects/test-1659$ npm run dev > ***@***.*** dev > svelte-kit dev The value for kit.vite.server.https.key.toString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit. The value for kit.vite.server.https.key.toLocaleString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit. The value for kit.vite.server.https.cert.toString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit. The value for kit.vite.server.https.cert.toLocaleString specified in svelte.config.js has been ignored. This option is controlled by SvelteKit. > The "options.cert" property must be of type string or an instance of Buffer, TypedArray, or DataView. Received an instance of Object @johnnysprinkles <https://github.com/johnnysprinkles> -- could you verify if your changes are preventing the user from specifying vite.server.https.cert and vite.server.https.key options? You can refer to my test project <https://github.com/JBusillo/test-1659>. I committed this just so johnnysprinkles has something to work with. I'll fix the lint error when I do my (hopefully) final commit. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1662 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHYF4WWQTUKXF4BPMPTHZ3DTTEOADANCNFSM46NITMGQ> .

packages/kit/src/core/server/index.js

johnnysprinkles · 2021-06-17T05:37:07Z

packages/kit/src/core/server/index.js

-			https_options.key = user_config.server.https.key.toString();
-			https_options.cert = user_config.server.https.cert.toString();
+		if (key && cert) {
+			https_options.key = key;


Am I reading this right? It looks like a no-op to me, since you've destructured key and cert out of https_options, then you're setting those again on the same object you destructured out of.

benmccann · 2021-06-17T12:26:27Z

I'm concerned that this PR might be accidentally merged -- I haven't been able to perform my own testing to this PR.
Is there any way to denote this in the PR?

You can mark it as a draft and take it out of draft mode when you're ready for review

JBusillo · 2021-06-17T15:27:31Z

@benmccann - I don't think I have the required access to change the PR to a draft -- This link says "Any user with write access to the repository can convert a pull request to a draft pull request.".
Could you change the draft status for me? Thanks.

benmccann · 2021-06-17T15:32:26Z

You should have access. There's a link that says "Still in progress? Convert to draft" under the Reviewers heading on the right hand side of the page

JBusillo · 2021-06-17T16:05:21Z

I don't get that option. But, dummdidumm did it for me. Here's what I see on the left side of my PR: Screenshot

benmccann · 2021-06-17T16:48:03Z

Well that option is no longer there since it's already been clicked 😄 Now instead there's a "Ready for review" button between the comments and CI checks

JBusillo · 2021-06-24T03:48:01Z

I forgot to commit my local repository before I left on my trip, so I'll have to wait until my return on July 14. Since this isn't a milestone issue, I don't see too much urgency. I'll have to merge johnnysprinkles changes into it, retest, commit and request a review.

benmccann · 2021-07-26T23:30:05Z

@JBusillo are you ready to revisit this PR?

JBusillo · 2021-07-27T02:20:00Z

@benmccann Yes, my return was delayed by a week, and I'm getting settled back in. I have some upstream merging to do, along with some coordination with Johnny Sprinkles, which I'll initiate via a separate message.

JBusillo · 2021-07-28T00:26:44Z

@benmccann -- I messed up my repository big-time. There were so many other changes to the modules I was working on, that I thought I could start fresh. I have notes and had saved all code changes for this PR, but everything is out of sync. I'm closing this, and will re-open it -- If I don't have access to re-open, I'll need some help/advice. Here goes.....

JBusillo · 2021-07-28T00:29:12Z

@benmccann -- I can't re-open it. I had deleted the fork, since I wanted to "start over" with a fresh copy of master. Please advise. Thx.

benmccann · 2021-07-28T02:35:09Z

I can't reopen either since the fork is deleted. You will just have to copy/paste any code from here to your new branch. To avoid this in the future, make sure you are developing code on a branch. Then you can sync your master against the SvelteKit master (git add remote upstream git@github.com:sveltejs/kit.git; git fetch upstream; git merge upstream/master) and then rebase your branch (git rebase master)

JBusillo · 2021-07-28T04:34:25Z

Got it. I opened up a new PR. The old PR references the new PR, and vice versa. The new PR is set up as a draft for the time being. I just re-applied my code changes, and will do testing tomorrow. There's been a lot of code restructuring in the past 3-4 weeks!
Thanks for your help and advice.

benmccann · 2021-07-28T04:49:42Z

just a heads up - I don't see any code on the new PR

JBusillo · 2021-07-28T05:39:54Z

I plan to commit the code when I'm done with local testing. I wanted to create the PR immediately to document the closed and open PR's. But, thanks for the heads up.

JBusillo added 2 commits June 9, 2021 22:41

Use vite options for Preview SSL cert/key

e8a7281

Merge branch 'issue-1659' of https://github.com/JBusillo/kit into iss…

c908321

…ue-1659

benmccann reviewed Jun 10, 2021

View reviewed changes

Merge branch 'sveltejs:master' into issue-1659

a8cf21d

Rename user_config to vite_config, use types

d2658d8

johnnysprinkles reviewed Jun 17, 2021

View reviewed changes

packages/kit/src/core/server/index.js Outdated Show resolved Hide resolved

JBusillo added 2 commits June 16, 2021 22:33

fix lint error, add typeof

80d43a6

fix start config reference

0f21dd1

johnnysprinkles mentioned this pull request Jun 17, 2021

fix: deep_merge should stop recursing at class instances #1698

Merged

5 tasks

johnnysprinkles reviewed Jun 17, 2021

View reviewed changes

Merge branch 'sveltejs:master' into issue-1659

ff40bf8

dummdidumm marked this pull request as draft June 17, 2021 15:31

benmccann added the awaiting submitter label Jul 11, 2021

benmccann force-pushed the master branch from 3e3e6ec to d0f2afd Compare July 25, 2021 14:15

Commit Staged Changes

1076db3

JBusillo closed this Jul 28, 2021

JBusillo mentioned this pull request Jul 28, 2021

Read file for SSL (certs and keys) and add pfx support #2027

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the same option source for SSL (certs and keys) in Preview and Dev #1662

Use the same option source for SSL (certs and keys) in Preview and Dev #1662

JBusillo commented Jun 10, 2021 •

edited

Loading

JBusillo commented Jun 10, 2021

JBusillo commented Jun 10, 2021

benmccann Jun 10, 2021

JBusillo Jun 15, 2021

benmccann Jun 16, 2021

benmccann commented Jun 10, 2021

changeset-bot bot commented Jun 15, 2021 •

edited

Loading

benmccann commented Jun 16, 2021

JBusillo commented Jun 16, 2021

johnnysprinkles commented Jun 16, 2021 via email

johnnysprinkles Jun 17, 2021 •

edited

Loading

benmccann commented Jun 17, 2021

JBusillo commented Jun 17, 2021

benmccann commented Jun 17, 2021

JBusillo commented Jun 17, 2021

benmccann commented Jun 17, 2021

JBusillo commented Jun 24, 2021

benmccann commented Jul 26, 2021

JBusillo commented Jul 27, 2021

JBusillo commented Jul 28, 2021

JBusillo commented Jul 28, 2021

benmccann commented Jul 28, 2021

JBusillo commented Jul 28, 2021

benmccann commented Jul 28, 2021

JBusillo commented Jul 28, 2021

Use the same option source for SSL (certs and keys) in Preview and Dev #1662

Use the same option source for SSL (certs and keys) in Preview and Dev #1662

Conversation

JBusillo commented Jun 10, 2021 • edited Loading

Before submitting the PR, please make sure you do the following

Tests

Changesets

JBusillo commented Jun 10, 2021

JBusillo commented Jun 10, 2021

benmccann Jun 10, 2021

Choose a reason for hiding this comment

JBusillo Jun 15, 2021

Choose a reason for hiding this comment

benmccann Jun 16, 2021

Choose a reason for hiding this comment

benmccann commented Jun 10, 2021

changeset-bot bot commented Jun 15, 2021 • edited Loading

🦋 Changeset detected

benmccann commented Jun 16, 2021

JBusillo commented Jun 16, 2021

johnnysprinkles commented Jun 16, 2021 via email

johnnysprinkles Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

benmccann commented Jun 17, 2021

JBusillo commented Jun 17, 2021

benmccann commented Jun 17, 2021

JBusillo commented Jun 17, 2021

benmccann commented Jun 17, 2021

JBusillo commented Jun 24, 2021

benmccann commented Jul 26, 2021

JBusillo commented Jul 27, 2021

JBusillo commented Jul 28, 2021

JBusillo commented Jul 28, 2021

benmccann commented Jul 28, 2021

JBusillo commented Jul 28, 2021

benmccann commented Jul 28, 2021

JBusillo commented Jul 28, 2021

JBusillo commented Jun 10, 2021 •

edited

Loading

changeset-bot bot commented Jun 15, 2021 •

edited

Loading

johnnysprinkles Jun 17, 2021 •

edited

Loading