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

@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 :-)

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?

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

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 )

cc @nodejs/crypto

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

@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).

@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

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

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).

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

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

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

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

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

@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.

@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.

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

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

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.

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