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

Fixes #670: Added support for printing out the Common Name from the SSL certificate that is being used. #675

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

bramp
Copy link

@bramp bramp commented Jan 31, 2021

Please ensure that your pull request fulfills these requirements:

What is the purpose of this pull request? (bug fix, enhancement, new feature,...)

New Feature

Fixes #670: Added support for printing out the Common Name from the SSL certificate that is being used.

This avoids SSL errors when going to the host (such as a IP address) that doesn't match the name in the certificate.

What changes did you make?

  • Within the bin/http-server the binary now checks the SSL cert is valid and extracts the common name. It then prints that common name as one of the options. A minor refactor of the printing was also made.
  • Added unit tests for starting a SSL server. (Those tests can be kept, even without this new feature)

Provide some example code that this change will affect, if applicable:

N/A

Is there anything you'd like reviewers to focus on?

Please provide testing instructions, if applicable:

$ npm test

# Create cert (use localhost or whatever as a common name)
$ openssl req -newkey rsa:2048 -new -nodes -x509 -days 3650 -keyout key.pem -out cert.pem

# With ssl
$ bin/http-server -c-1 -S
Available on:
  https://localhost:8080
  ...

# Without ssl
$ bin/http-server -c-1 -S
Available on:
  https://127.0.0.1:8080
  ...

…or tests related changes:

* Every httpServer.createServer now has a vow to check for errors. Additionally, the server object is returned, instead of using the callbacks. This is because the server is an EventEmitter, and vows.js can capture the error correctly.

* Documented respondsWithMessage function

* Refactored one case of checking for a 404 with respondsWith(404, ...)
…from the SSL certificate that is being used.

This avoids SSL errors when going to the host (such as a IP address) that doesn't match the name in the certificate.
@thornjad thornjad added the minor version non-breaking, non-trivial change label Oct 12, 2021
@thornjad
Copy link
Member

I think this looks really good! And I'm impressed by the small addition to grab something like that! Unfortunately, there have been some major changes in master, especially to the tests, and there are some pretty big conflicts to solve.

@thornjad thornjad added enhancement patch version Small, non-breaking, bug fix or trivial change and removed minor version non-breaking, non-trivial change labels Oct 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement patch version Small, non-breaking, bug fix or trivial change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When using https display https://common_name:8080/ as one of the valid hostnames
2 participants