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

https: throw error if required params missing #3064

Closed
wants to merge 1 commit into from

Conversation

kulkarniankita
Copy link

throw Error when required parameters for options in https.createServer are missing

The required parameters of https.createServer is key and cert. When these parameters are not provided by the user then a new Error is thrown for the missing parameter.
A subsequent test is also added to verify the same.

This was discussed in Issue: #3024

cc: @jasnell @mhdawson

@thefourtheye thefourtheye added the https Issues or PRs related to the https subsystem. label Sep 25, 2015
https.createServer(options, function (req, res) {
res.send(200);
}).listen(common.PORT);
}, Error);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test only tests cert case. What about key missing case? It would be better if the error messages are checked explicitly.

Copy link
Author

Choose a reason for hiding this comment

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

Good point. Adding a test case for it as well

@jasnell jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 25, 2015
};

assert.throws(function() {
https.createServer(options, function (req, res) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For this usecase, the callback passed should not be called. So, its better to use assert.fail there.

Copy link
Author

Choose a reason for hiding this comment

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

I am curious why the callback passed should not be called?

Copy link
Contributor

Choose a reason for hiding this comment

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

@kulkarniankita Because the point of the test is to throw on invalid cert option.

Copy link
Member

Choose a reason for hiding this comment

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

Style: function(req, res) {

@jasnell
Copy link
Member

jasnell commented Sep 25, 2015

Marked as semver-major due to the introduction of the new throw.

@thefourtheye
Copy link
Contributor

@kulkarniankita As this is your first contribution to this project, you might want to go through https://github.com/nodejs/node/blob/master/CONTRIBUTING.md and fix the commit logs :-)

@kulkarniankita
Copy link
Author

Because of my issue change, the test-https-pfx.js broke since its asserting on DEPTH_ZERO_SELF_SIGNED_CERT. Should that be fixed to not assert on it anymore and assert on my fix?

@srl295
Copy link
Member

srl295 commented Sep 25, 2015

@kulkarniankita could test-https-pfx.js just set the cert and key parameters to something valid?

@kulkarniankita
Copy link
Author

Hi @srl295 yeah adding something valid fixed the test although I am not sure of behaviour change in test-tls-no-cert-required.js as that is failing with the msg that I just added, cert is required there and not provided. There is a comment above that confuses me.

// Omitting the cert or pfx option to tls.createServer() should not throw. // AECDH-NULL-SHA is a no-authentication/no-encryption cipher and hence // doesn't need a certificate. tls.createServer({ ciphers: 'AECDH-NULL-SHA' }).listen(common.PORT, function()

Now this comment means that whenever there is a no-authentication/no-encryption cipher then I should not be asking for a certificate or key. This makes sense so my code should have additional checking for this type of certificate but want to confirm this. (cc: @jasnell )

@Fishrock123
Copy link
Contributor

cc @nodejs/crypto

@indutny
Copy link
Member

indutny commented Sep 26, 2015

What about ciphers without auth? like aNULL or something like this. They don't require cert at all.

@jasnell
Copy link
Member

jasnell commented Sep 26, 2015

@indutny... Is there a concise list of known ciphers that do/don't require
cert/key?
On Sep 25, 2015 6:04 PM, "Fedor Indutny" notifications@github.com wrote:

What about ciphers without auth? like aNULL or something like this. They
don't require cert at all.


Reply to this email directly or view it on GitHub
#3064 (comment).

@indutny
Copy link
Member

indutny commented Sep 26, 2015

@jasnell of course!

$ ./out/Release/openssl-cli ciphers -v aNULL
AECDH-AES256-SHA        SSLv3 Kx=ECDH     Au=None Enc=AES(256)  Mac=SHA1
ADH-AES256-GCM-SHA384   TLSv1.2 Kx=DH       Au=None Enc=AESGCM(256) Mac=AEAD
ADH-AES256-SHA256       TLSv1.2 Kx=DH       Au=None Enc=AES(256)  Mac=SHA256
ADH-AES256-SHA          SSLv3 Kx=DH       Au=None Enc=AES(256)  Mac=SHA1
ADH-CAMELLIA256-SHA     SSLv3 Kx=DH       Au=None Enc=Camellia(256) Mac=SHA1
AECDH-AES128-SHA        SSLv3 Kx=ECDH     Au=None Enc=AES(128)  Mac=SHA1
ADH-AES128-GCM-SHA256   TLSv1.2 Kx=DH       Au=None Enc=AESGCM(128) Mac=AEAD
ADH-AES128-SHA256       TLSv1.2 Kx=DH       Au=None Enc=AES(128)  Mac=SHA256
ADH-AES128-SHA          SSLv3 Kx=DH       Au=None Enc=AES(128)  Mac=SHA1
ADH-SEED-SHA            SSLv3 Kx=DH       Au=None Enc=SEED(128) Mac=SHA1
ADH-CAMELLIA128-SHA     SSLv3 Kx=DH       Au=None Enc=Camellia(128) Mac=SHA1
AECDH-RC4-SHA           SSLv3 Kx=ECDH     Au=None Enc=RC4(128)  Mac=SHA1
ADH-RC4-MD5             SSLv3 Kx=DH       Au=None Enc=RC4(128)  Mac=MD5
AECDH-DES-CBC3-SHA      SSLv3 Kx=ECDH     Au=None Enc=3DES(168) Mac=SHA1
ADH-DES-CBC3-SHA        SSLv3 Kx=DH       Au=None Enc=3DES(168) Mac=SHA1
ADH-DES-CBC-SHA         SSLv3 Kx=DH       Au=None Enc=DES(56)   Mac=SHA1
EXP-ADH-DES-CBC-SHA     SSLv3 Kx=DH(512)  Au=None Enc=DES(40)   Mac=SHA1 export
EXP-ADH-RC4-MD5         SSLv3 Kx=DH(512)  Au=None Enc=RC4(40)   Mac=MD5  export
AECDH-NULL-SHA          SSLv3 Kx=ECDH     Au=None Enc=None      Mac=SHA1

@indutny
Copy link
Member

indutny commented Sep 26, 2015

@jasnell I hope you don't suggest hard-coding this?

@jasnell
Copy link
Member

jasnell commented Sep 26, 2015

No, I'm just trying to determine if there's some reasonable algorithmic way
to determine if the cert or key are required or not. If not, then we may
just have to punt on doing the check this way
On Sep 25, 2015 6:30 PM, "Fedor Indutny" notifications@github.com wrote:

@jasnell https://github.com/jasnell I hope you don't suggest
hard-coding this?


Reply to this email directly or view it on GitHub
#3064 (comment).

@thefourtheye
Copy link
Contributor

So this behavior is actually the expected behavior. @indutny What about the certificate check? I think that can stay. Right?

@indutny
Copy link
Member

indutny commented Sep 26, 2015

@thefourtheye not really aNULL does not really need neither key nor cert.

@kulkarniankita
Copy link
Author

I notice there is a getCiphers method in crypto module. Wondering if anywhere in the existing code, it checks for aNull type of check.

@thefourtheye
Copy link
Contributor

@indutny oh, I wonder how the connection is encrypted and validations happen then.

@indutny
Copy link
Member

indutny commented Sep 26, 2015

@thefourtheye it is using either ephemeral Diffie-Hellman, or not encrypting it at all.

@indutny
Copy link
Member

indutny commented Sep 26, 2015

@kulkarniankita I think we can try iterating through ciphers and checking:

The string returned by SSL_CIPHER_description() in case of success consists of cleartext information separated by one or more blanks in the following sequence:

       <ciphername>
           Textual representation of the cipher name.

       <protocol version>
           Protocol version: SSLv2, SSLv3, TLSv1.2. The TLSv1.0 ciphers are flagged with SSLv3. No new ciphers were added by TLSv1.1.

       Kx=<key exchange>
           Key exchange method: RSA (for export ciphers as RSA(512) or RSA(1024)), DH (for export ciphers as DH(512) or DH(1024)), DH/RSA, DH/DSS, Fortezza.

       Au=<authentication>
           Authentication method: RSA, DSS, DH, None. None is the representation of anonymous ciphers.

       Enc=<symmetric encryption method>
           Encryption method with number of secret bits: DES(40), DES(56), 3DES(168), RC4(40), RC4(56), RC4(64), RC4(128), RC2(40), RC2(56), RC2(128), IDEA(128), Fortezza, None.

       Mac=<message authentication code>
           Message digest: MD5, SHA1.

       <export flag>
           If the cipher is flagged exportable with respect to old US crypto regulations, the word "export" is printed.

If Au=None - cert is not required.

Or, as a simpler fix! We can check if used cipher list is a default one, and throw an error like you do in this PR. This will protect users from most common errors, while keeping the Au=None ciphers work just fine for those who need them.

lib/_tls_wrap.js Outdated
@@ -846,10 +846,23 @@ Server.prototype.setOptions = function(options) {
this.rejectUnauthorized = false;
}


Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary newline.

@thefourtheye
Copy link
Contributor

@indutny Oh... I guess if server and client are not able to come to an agreement about the algorithm to be used, then they ll fall back to any of the null mechanisms. Is that correct?

Apart from that, this patch is not necessary as it is trying to fix something which is actually the expected behavior right? I suppose we can close this then.

@kulkarniankita thanks for the contribution. I am sorry that this patch didnt make it in. Please feel free to discuss further if needed.

@thefourtheye
Copy link
Contributor

Oops sorry. Missed @indutny's suggestions. Reopening now.

@thefourtheye thefourtheye reopened this Sep 26, 2015
@jasnell
Copy link
Member

jasnell commented Oct 26, 2015

Looks good on initial read but I'd like to get @indutny and @shigeki to take a final look.

@shigeki
Copy link
Contributor

shigeki commented Oct 27, 2015

This is bad when option.pfx is used. The PKCS#12 file contains cert and key data so that option.cert and option.key are not needed. That's why the pfx test does not have them. The check should be skipped if it has option.pfx.

Throw an error when required parameters are missing. Handles ciphers that requires no auth.
Does not throw error If pfx option is provided.
Additional tests added for the same.

Fixes: nodejs#3024
PR-URL: nodejs#3064
} else {
this.cert = options.cert;
}

if (options.ca) this.ca = options.ca;

Choose a reason for hiding this comment

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

What happens if I want to set the certificate authority for reading only but not the cert/private key.
Example: http://docs.aws.amazon.com/apigateway/latest/developerguide/getting-started-client-side-ssl-authentication.html

Copy link
Author

Choose a reason for hiding this comment

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

@cbshakumar you mean if cert authority is provided then skip this whole cert/key error handling right?

Choose a reason for hiding this comment

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

@kulkarniankita It has been a while since I've looked at this. Should it be possible to set the certificate authority and not set a cert?

@jasnell
Copy link
Member

jasnell commented Feb 28, 2017

Given the lack of forward progress and the uncertainty of what the actual behavior should be, I'm closing. This can be reopened if necessary

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
https Issues or PRs related to the https subsystem. semver-major PRs that contain breaking changes and should be released in the next major version. stalled Issues and PRs that are stalled. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.