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

General improvements to HTTPS API (Second Edition) #1435

Closed
wants to merge 9 commits into from
Closed

General improvements to HTTPS API (Second Edition) #1435

wants to merge 9 commits into from

Conversation

Giotino
Copy link
Collaborator

@Giotino Giotino commented Sep 3, 2020

Following the previous "General improvements to HTTPS API" (#1255) this PR is going to add the remaining HTTPS options described in #1306 (or, at least, most of them).

This PR is still WIP.

Implemented options to date (completed on "code, test and docs"):

  • ciphers: code, test
  • honorCipherOrder: code, test
  • minVersion: code, test
  • maxVersion: code, test

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates in both the README and the types.

(Partially) Fixes #1306

Fixes #1299

@Giotino Giotino marked this pull request as draft September 3, 2020 10:38
@@ -858,6 +858,20 @@ export interface HTTPSOptions {
The passphrase to decrypt the `options.https.key` (if different keys have different passphrases refer to `options.https.key` documentation).
*/
passphrase?: SecureContextOptions['passphrase'];

// TODO add comment
ciphers?: SecureContextOptions['ciphers'];
Copy link
Owner

Choose a reason for hiding this comment

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

Can we make this support an array of ciphers instead of string with :?


const createPrivateKey = pify(pem.createPrivateKey);
const createCSR = pify(pem.createCSR);
const createCertificate = pify(pem.createCertificate);
const createPkcs12 = pify(pem.createPkcs12);

test('https request without ca', withHttpsServer(), async (t, server, got) => {
const ciphers = tls.getCiphers().map(cipher => cipher.toUpperCase());
Copy link
Owner

Choose a reason for hiding this comment

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

Would be more readable if you used destructuring here.

@sindresorhus
Copy link
Owner

Can you fix the merge conflicts?

Base automatically changed from master to main January 23, 2021 11:37
Copy link
Collaborator

@szmarczak szmarczak left a comment

Choose a reason for hiding this comment

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

I just noticed hat I had this review pending for months. OMG.

@@ -858,6 +858,20 @@ export interface HTTPSOptions {
The passphrase to decrypt the `options.https.key` (if different keys have different passphrases refer to `options.https.key` documentation).
*/
passphrase?: SecureContextOptions['passphrase'];

// TODO add comment
Copy link
Collaborator

Choose a reason for hiding this comment

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

// TODO: example

@szmarczak
Copy link
Collaborator

szmarczak commented Apr 11, 2021

@Giotino Can you rebase this PR please? #1667 has been merged, I'm more than happy to answer all your questions if you encounter anything :)

In order to be safe and not lose everything feel free to create a new PR and close this one. I think it will be easier to start from master again.

@Giotino
Copy link
Collaborator Author

Giotino commented Apr 12, 2021

In order to be safe and not lose everything feel free to create a new PR and close this one. I think it will be easier to start from master again.

Yes, I think it's better to close this PR and open a new one, but recently I haven't been able to work on this repo.

@Giotino Giotino closed this Apr 12, 2021
@szmarczak
Copy link
Collaborator

recently I haven't been able to work on this repo

No worries!

@szmarczak szmarczak reopened this Apr 27, 2021
@szmarczak szmarczak closed this Apr 27, 2021
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.

Better HTTP(S) Request options documentation Set Min/MAX TLS version and ciphers for requests
3 participants