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

Changed the web-server from HTTP1 to HTTP2.. #609

Open
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

not-meet
Copy link

@not-meet not-meet commented Oct 23, 2024

Linked Issue(s)

#598

Acceptance Criteria fulfillment

  • Enable HTTP/2 in development mode.
  • Enable HTTP/2 in production mode .
  • Generate automated self-signed certificates.
  • Automate certificate refresh.

reference taken from -> https://medium.com/@greg.farrow1/nextjs-https-for-a-local-dev-server-98bb441eabd7

Further comments

Screenshot from 2024-10-23 18-23-34
Screenshot from 2024-10-23 18-23-53
Screenshot from 2024-10-23 18-24-14

Comment

This pull request includes several changes to improve the security and modularity of the web server setup, along with some other minor updates. The most important changes include switching to HTTPS for local development, converting the configuration file to use ES modules, and adding a script to generate SSL certificates.

Security improvements:

Modularity and configuration updates:

Automated SSL certificate generation:

@CLAassistant
Copy link

CLAassistant commented Oct 23, 2024

CLA assistant check
All committers have signed the CLA.

createServer(httpsOptions, (req, res) => {
const parsedUrl = parse(req.url, true);
handle(req, res, parsedUrl);
}).listen(3333, (err) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't use the PORT environment variable.

handle(req, res, parsedUrl);
}).listen(3333, (err) => {
if (err) throw err;
console.log('> Server started on https://localhost:3333');
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above.

@not-meet
Copy link
Author

@jayantbh i have made the changes as asked.. kindly review the changes.. i am up for anymore changes and to learn.. :)

@not-meet not-meet requested a review from jayantbh October 24, 2024 13:32
@jayantbh
Copy link
Contributor

Hey sorry for the delay. We'll check it out in a day or so.

@not-meet
Copy link
Author

hey @jayantbh no issues with the delay :-) i waited patiently i saw somehow the code i pushed was having linting error so i fixed it with pre-commit run and it made that yarn lock changes and to re assure i also ran the container and from my side everything is working fine, do check the pr and if possible lets merge this one!

@@ -123,7 +123,7 @@
"eslint-config-next": "13.5.6",
"eslint-plugin-import": "^2.29.0",
"eslint-plugin-prettier": "^5.0.1",
"eslint-plugin-react": "^7.29.4",
"eslint-plugin-react": "^7.37.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change actually needed?

If not, you might be able to reduce the size of your PR significantly.

Copy link
Contributor

Choose a reason for hiding this comment

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

@not-meet reminder ☝🏽

Copy link
Author

Choose a reason for hiding this comment

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

@jayantbh hi really sorry for the delay ill fix this asap, actually this happened back when the pr was not able to merge because of linting then i used pre-commit to fix the issue and it gave me an error and a command to fix the error and after that when i ran pre-commit this issue happened... ill fix this as soon as possible.. :-)

Copy link
Author

Choose a reason for hiding this comment

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

@jayantbh hi.. i made the eslint-plugin- react changes as they were also ran pre-commit run all files to make sure the linting problem dose not happen again...i hope this will solve all the issues while merging the pr :-)

@not-meet not-meet requested a review from jayantbh October 29, 2024 19:08
@jayantbh
Copy link
Contributor

I checked out the branch to run it locally ./dev.sh and it failed.
Basically the experience should be that you should not need to run anything other than ./dev.sh to get started.
Related: Running ./local-setup.sh is optional, so anything that is not skippable should not go there.
Since certificates will be a requirement, it should go in ./dev.sh.

Nonetheless, I keep getting this error and wasn't able to get it to work even after running the certificates script. Mainly because the script creates certificates w.r.t. the current directory, so running this script anywhere other than the web-server directory will break it.

If you hook the certificates generation behaviour to ./dev.sh, you might have some new ways to deal with it.

web yarn run v1.22.22
web $ [ -e ../.env ] && set -a && . ../.env; NEXT_MANUAL_SIG_HANDLE=true node server.j
web s
web (node:230) [MODULE_TYPELESS_PACKAGE_JSON] Warning: Module type of file:///app/web-
web server/server.js is not specified and it doesn't parse as CommonJS.
web Reparsing as ES module because module syntax was detected. This incurs a performan
web ce overhead.
web To eliminate this warning, add "type": "module" to /app/web-server/package.json.
web (Use `node --trace-warnings ...` to show where the warning was created)
web node:fs:561
web   return binding.open(
web                  ^
web Error: ENOENT: no such file or directory, open './certificates/localhost.key'
web     at Object.openSync (node:fs:561:18)
web     at Object.readFileSync (node:fs:445:35)
web     at file:///app/web-server/server.js:12:11
web     at ModuleJob.run (node:internal/modules/esm/module_job:262:25)
web     at async onImport.tracePromise.__proto__ (node:internal/modules/esm/loader:483
web :26) {
web   errno: -2,
web   code: 'ENOENT',
web   syscall: 'open',
web   path: './certificates/localhost.key'
web }
web Node.js v22.9.0
web error Command failed with exit code 1.
web info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this comman
web d.

@not-meet
Copy link
Author

@jayantbh alright gotcha! so i have to do something that can generate the certificates inside the desired folder where it should be.. when a client runs ./dev.sh ...i'll do it asap and will make sure this time there won't be any issues regarding all this.... btw very happy diwali @jayantbh <3

@jayantbh
Copy link
Contributor

Happy Diwali to you too, @not-meet! ❤️

@not-meet
Copy link
Author

hey @jayantbh i have made changes as you asked but i have one doubt in this... after doing the changes
Screenshot from 2024-10-30 22-24-01
so as you can see in this the certificates are generating on there own but after that it also says module type warning to convert type to module... when i did that... the server stopped running and started showing import errors such as

Screenshot from 2024-10-30 21-36-49

though without changing the type to module everything is working fine on my side :-)

@jayantbh
Copy link
Contributor

I think you'd have to change the next.config.js to use import/export instead of require/module.exports and it should work just fine.

@not-meet
Copy link
Author

did changes as asked! also did some renames of eslint and next.config to align with type :"module".. also one more thing while running the server it showed one warning of unrecognized file extension

Screenshot from 2024-10-31 01-54-52

i tried experimenting by removing it but after removing it the website crashed so i kept it like it is !
up for anymore changes or tweaks @jayantbh :-D

@jayantbh
Copy link
Contributor

jayantbh commented Nov 1, 2024

Yeah that's okay, that warning has been there since ages but it's never really caused any real issues... or any issues as far as we could tell.

@not-meet
Copy link
Author

not-meet commented Nov 4, 2024

hey @jayantbh so as i fixed the linting issue all the checks have passed and i believe the pr is somewhere close to get merge too !! so if there are any more tweaks you want in the pr i'll be more than happy to do them... :-)

@jayantbh
Copy link
Contributor

jayantbh commented Nov 4, 2024

Hey @not-meet, I see that it works ok for dev mode, except that the certificate's expiry date is the time when it was generated, except that the certificate isn't signed (even self-signed) which might be throwing the insecure warning.

Also, in prod mode there is no mechanism to generate the certificate.

I think if you make the certificate generation be part of the server.js script, so it creates one right on server boot, it might work.

But I also see that there's no mechanism to refresh the certificate either.

@jayantbh
Copy link
Contributor

jayantbh commented Nov 4, 2024

I recommend making a checklist of functionality that this PR needs to have so you can ensure nothing gets missed.

@not-meet
Copy link
Author

not-meet commented Nov 5, 2024

Hey @not-meet, I see that it works ok for dev mode, except that the certificate's expiry date is the time when it was generated, except that the certificate isn't signed (even self-signed) which might be throwing the insecure warning.

Also, in prod mode there is no mechanism to generate the certificate.

I think if you make the certificate generation be part of the server.js script, so it creates one right on server boot, it might work.

But I also see that there's no mechanism to refresh the certificate either.

@jayantbh apologies for that... ill fix all the issues as fast as i can... also will make a checklist of functionality that PR need as well <3

@not-meet
Copy link
Author

not-meet commented Nov 9, 2024

Hi @jayantbh ,

First of all, apologies for being inactive for the past 2-3 days, as I was unwell. I’ve made some progress with the requirements and researched how to best implement them.

Currently, on the dev server, server.js calls the generate certificate script itself. I also made the following changes to the script:

The script now generates a private key and a self-signed root certificate.
It creates a server certificate with a SAN (Subject Alternative Name) configuration to support multiple domains.
The certificates are also self signed
I followed these blog posts for handling the "Not Secure" issue:

https://surma.dev/things/h2setup/
https://medium.com/@tbusser/creating-a-browser-trusted-self-signed-ssl-certificate-2709ce43fd15

I discovered that the "Not Secure" warning in dev appears because the certificates aren’t added to the local Certificate Authority. I found a solution using "mkcert" to create a CA, but it requires installation on the local machine. I assume some adjustments in Docker to initialize the CA at runtime,may help resolve this issue.

also,
Since the certificates are removed when the container stops, is it essential to set up certificate renewal in the dev environment?

For the production setup:
I wasn’t able to get a complete picture of how the production server is configured.
I understand that certificate renewal could potentially cause downtime or require a restart, but with the implementation of NGINX or Apache, this could be mitigated. If you could provide some insight into the production setup, it would be a big help!

I know this PR is taking longer than expected, and I’m aware that some of my rookie mistakes have caused delays. I’ll make sure to wrap this up as soon as possible, with just a bit of guidance. Thank you for your patience! <3

@not-meet not-meet force-pushed the http2 branch 2 times, most recently from ab5b221 to 0d577b8 Compare November 10, 2024 08:25
@jayantbh
Copy link
Contributor

I think you're making good progress!
You can refer to https://github.com/middlewarehq/middleware?tab=readme-ov-file#-quick-start to see how to install the production version of Middleware.

not-meet and others added 7 commits November 15, 2024 16:51
* fix: updated Next to 14.2.16

* fix: updated Next to 15.0.1

* fix: pre-commit hook update for eslint-config-next

* fix: pre-commit hook update for eslint-config-next

* fix: pre-commit hook update for eslint-plugin-react, eslint-plugin-unused-imports
Resolve conflicts and apply lint fixes
@not-meet not-meet force-pushed the http2 branch 3 times, most recently from 605749d to 40a7221 Compare November 15, 2024 11:40
@not-meet
Copy link
Author

Hi @jayantbh !! so i did the changes in the dev server as asked now the certificates are generated when the docker files runs plus a CA is also estabilished for the ssl certificates..

What i did ?
for the prod server i have also added http2 support by creating another file named prod-server.js and adding certbot for certificate renewal and generation in the yaml file..

i tested it by generating certificates locally and for me it worked....!

Note- currently i am facing a problem with the dev server which is.. that the browser still shows not secure even after adding a CA and verifying the certificates..!
as far as i have researched the browser will show the error till the certificates are not added in the CA of the local machine ...

@jayantbh
Copy link
Contributor

I think that's only because you can't verifiably sign certificates, even self-signed with the localhost domain. Maybe signing it as www.localhost will work? Of course, you'll need to then load the webserver on www.localhost, which should be trivial. Give that a shot?

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