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

TLS fails reading self-signed certificate on node 4.2.5+ #5100

Closed
dbkup opened this issue Feb 5, 2016 · 11 comments
Closed

TLS fails reading self-signed certificate on node 4.2.5+ #5100

dbkup opened this issue Feb 5, 2016 · 11 comments
Labels
confirmed-bug Issues with confirmed bugs. lts Issues and PRs related to Long Term Support releases. tls Issues and PRs related to the tls subsystem.

Comments

@dbkup
Copy link

dbkup commented Feb 5, 2016

I'm having an issue with ssl certificate validation using the tls module. The server is started with:

tls.createServer({
        pfx: fs.readFileSync(config.certFile),
        passphrase: config.keyPass,
        requestCert: true, 
        rejectUnauthorized: false 
    }, ...);

The client:

tls.connect({
    port: config.port,
    host:config.host,
    pfx: fs.readFileSync(config.cert),
    passphrase: config.pass,
    rejectUnauthorized: false
});

My issue is that I get tlsSocket.authorizationError SELF_SIGNED_CERT_IN_CHAIN on v4.2.5+ but not on older versions. Here's my output on a Windows machine, but the same happens on an Ubuntu server.

>nodist 4.2.5
>node --version
v4.2.5

>node server.js
server started:
auth->SELF_SIGNED_CERT_IN_CHAIN

>nodist 4.2.4
nodev4.2.4

>node server.js
server started:
auth->null

The auth-> line is printed to console with the tlsSocket.authorizationError parameter when a client connects. In the case of a successful connect this field is null.
Tested down to 0.12.9, all versions read the certificate without issues.

@diegossilveira
Copy link

@dbkup, looks like this does not affect node from v5.0.0 to v5.3.0, right? I'm facing similar problem here with versions v5.4.0 and v5.5.0. Maybe, #4165, which was introduced in v5.4.0 and v4.2.5 is causing that.

@dbkup
Copy link
Author

dbkup commented Feb 5, 2016

@diegossilveira I tested with the v4 branch, so quite possible that that is the reason.

@MylesBorins
Copy link
Contributor

there were three commits that landed on 4.2.5 that touched TLS. Perhaps @indutny has some insight

@MylesBorins MylesBorins added tls Issues and PRs related to the tls subsystem. lts Issues and PRs related to Long Term Support releases. v4.x labels Feb 5, 2016
@MylesBorins
Copy link
Contributor

/cc @jasnell

@indutny indutny added the confirmed-bug Issues with confirmed bugs. label Feb 5, 2016
@indutny
Copy link
Member

indutny commented Feb 5, 2016

It is definitely caused by that PFX commit. Reverting it fixes the issue. Will look more deeply into it.

@indutny
Copy link
Member

indutny commented Feb 5, 2016

This is where the issue comes from: a2c1799#diff-801e3948990f4965a8ea4aca4a423864L928 . Going to investigate the best way to fix it right now.

@indutny
Copy link
Member

indutny commented Feb 5, 2016

Ok, so I have several thoughts about this. There are two conflicting things in my opinion:

  • API stability
  • Sanity of the pfx option in tls.createServer/tls.connect

From stability point of view, we should not really break anything unless there is security need for this, and that commit is absolutely breaking change (as we have just figured out).

From sanity point of view, there is no way in PKCS12 (pfx) to distinguish between regular server/client certs and CA certs for validating the other side. It seems to be pretty unsafe to me, but this is the way it has been implemented and documented for a long time.

@dbkup is what pfx currently provides exactly what you need from it? Do you expect it to use the same certs for both client validation and sever authorization?

cc @nodejs/crypto @nodejs/ctc @nodejs/lts

@indutny
Copy link
Member

indutny commented Feb 5, 2016

Should be fixed by #5109

@jasnell
Copy link
Member

jasnell commented Feb 5, 2016

OK, let's get this fixed in the next LTS release after next week's security
release. Should be maybe a week later. I'm definitely thinking we need a RC
cycle tho for all LTS releases moving forward.
On Feb 5, 2016 2:34 PM, "Fedor Indutny" [email protected] wrote:

Should be fixed by #5109 #5109


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

indutny added a commit to indutny/io.js that referenced this issue Feb 6, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: nodejs#5100
@indutny
Copy link
Member

indutny commented Feb 6, 2016

@jasnell do you think we should break this behavior in next major release? I mean should we make pfx non-ca?

@dbkup
Copy link
Author

dbkup commented Feb 6, 2016

@indutny Nice reaction time, thank you :). I've since checked my certs once more. I have a local CA which was used for signing the client/server certificates in question. Seems to me that this behaviour should be supported for use in an internal network, i.e. the certs are used in a controlled environment.

I have also tried generating another set of certs using openssl 1.0.1f, suspecting a compatibility issue of some sorts, getting other errors, like UNABLE_TO_VERIFY_LEAF_SIGNATURE and DEPTH_ZERO_SELF_SIGNED_CERT (on v4.2.4) so I guess I'm not doing something right. I will check your update for my case when it's available.

shigeki pushed a commit to shigeki/node that referenced this issue Feb 8, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: nodejs#5100
@indutny indutny closed this as completed in 23196fe Feb 8, 2016
indutny added a commit to indutny/io.js that referenced this issue Feb 8, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.
Trott pushed a commit to Trott/io.js that referenced this issue Feb 8, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.

PR-URL: nodejs#5144
Reviewed-By: Rich Trott <[email protected]>
rvagg pushed a commit that referenced this issue Feb 9, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
rvagg pushed a commit that referenced this issue Feb 9, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.

PR-URL: #5144
Reviewed-By: Rich Trott <[email protected]>
rvagg pushed a commit that referenced this issue Feb 9, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
rvagg pushed a commit that referenced this issue Feb 9, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.

PR-URL: #5144
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.

PR-URL: #5144
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit that referenced this issue Feb 18, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.

PR-URL: #5144
Reviewed-By: Rich Trott <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 2, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: #5100
PR-URL: #5109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 2, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.

PR-URL: #5144
Reviewed-By: Rich Trott <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
According to documentation all certificates specified in `pfx` option
should be treated as a CA certificates too. While it doesn't seem to be
logically correct to me, we can't afford to break API stability at this
point.

Fix: nodejs#5100
PR-URL: nodejs#5109
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Shigeki Ohtsu <[email protected]>
scovetta pushed a commit to scovetta/node that referenced this issue Apr 2, 2016
This is a follow-up fix for half-broken test in 23196fe, and an attempt
to recover some dignity after breaking CI.

PR-URL: nodejs#5144
Reviewed-By: Rich Trott <[email protected]>
Saeed-Navarik added a commit to Saeed-Navarik/node that referenced this issue Oct 6, 2017
ryzokuken added a commit to ryzokuken/node that referenced this issue Mar 18, 2018
Rename the tests appropriately alongside mentioning the subsystem
Also, make a few basic changes to make sure the tests conform to the standard test structure

1. Renamed test-regress-GH-io-1068 to test-tty-stdin-end
2. Renamed test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror
3. Renamed test-regress-GH-node-9326 to test-kill-segfault-freebsd
4. Renamed test-timers-regress-nodejsGH-9765 to test-timers-setimmediate-infinite-loop
5. Renamed test-tls-pfx-nodejsgh-5100-regr to test-tls-pfx-authorizationerror
6. Renamed test-tls-regr-nodejsgh-5108 to test-tls-tlswrap-segfault

Fixes: nodejs#19105
Refs: nodejs#19105
Refs: https://github.com/nodejs/node/blob/master/doc/guides/writing-tests.md#test-structure
lpinca pushed a commit that referenced this issue Mar 18, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-io-1068 to test-tty-stdin-end
- Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror
- Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd
- Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop
- Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror
- Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault

PR-URL: #19332
Fixes: #19105
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-io-1068 to test-tty-stdin-end
- Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror
- Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd
- Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop
- Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror
- Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault

PR-URL: #19332
Fixes: #19105
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
MylesBorins pushed a commit that referenced this issue Mar 20, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-io-1068 to test-tty-stdin-end
- Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror
- Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd
- Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop
- Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror
- Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault

PR-URL: #19332
Fixes: #19105
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
BethGriggs pushed a commit that referenced this issue Dec 3, 2018
Rename the tests appropriately alongside mentioning the subsystem.
Also, make a few basic changes to make sure the tests conform to the
standard test structure.

- Rename test-regress-GH-io-1068 to test-tty-stdin-end
- Rename test-regress-GH-io-1811 to test-zlib-kmaxlength-rangeerror
- Rename test-regress-GH-node-9326 to test-kill-segfault-freebsd
- Rename test-timers-regress-GH-9765 to test-timers-setimmediate-infinite-loop
- Rename test-tls-pfx-gh-5100-regr to test-tls-pfx-authorizationerror
- Rename test-tls-regr-gh-5108 to test-tls-tlswrap-segfault

PR-URL: #19332
Fixes: #19105
Reviewed-By: Gireesh Punathil <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Weijia Wang <[email protected]>
Reviewed-By: Yuta Hiroto <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Shingo Inoue <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. lts Issues and PRs related to Long Term Support releases. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants