tls wild card check supports for port #20875
Conversation
If wild card dns check (both CN, or SAN) does not contain port, it applies to all ports on the server.
|
@bnoordhuis Ben, it is the other way around.. TLS client: https://really.whatever.com:2443 -----> TLS Server (Server has CN or SAN as *.whatever.com) The way TLS handle the wild card check (openssl for an example) is : if CN/SAN does not contain Currently node check is missing the above. And when certificate is issued for server, it typically will not contain Without the above (or something similar) check, the tlsServerCheck is broken for |
|
I could be missing something but |
|
@apapirovski: the proposed change will not modify that - (see the original testcases) Use the same example above, each protocol has it default port (unless admin changes it.. https: 443, ldaps : 636, and so on) TLS client: https://really.whatever.com -----> TLS Server (Server has CN or SAN as *.whatever.com) ^^^ this will be over port 443 (automatically without caller/tls client has to do anything). Node current handles this without the above changes. The current The current checkServerIdentity does accept a hostname with a port (if the tls client sends in a port) - this is how I found out the issue.. |
|
@shiup if we're passing in host instead of hostname somewhere then that needs to be fixed. The function is fine as it is. It accepts hostname which would never have a port number. The test cases you've provided are incorrect since they pass in a host (includes port), not a hostname (doesn't include port). |
|
@apapirovski that would work. I am curious on why it would be the prefer fix, as there is a path to the checkServerIdentity, which will pass in It seems like the bullet proof way is closer to the source. (which this proposed fix is) Just curious, and for my learning. Question : who will be doing that ? Do you want me to create another PR for path fixing (which I can, but I am more in security than others) And I think |
|
@shiup I'm still unsure where we make the mistake of passing the wrong value. Could you link it? Unfortunately most of the places that refer to |
|
@apapirovski : will the included test cases provide a clue on where the path needs a kick ? And I would still partition for doing the check directly in the checkServerIdentity, as there is that |
|
@shiup No, because your newly added tests have a I see one call site in Node.js itself and it passes in |
|
@apapirovski hmm.. checkServerIdentity will have an issue when the TLS server certificate CN/SAN contains a port, will it ? Which unfortunately is also allowed by the spec (let me look up the spec on SAN, it has been a while.. CN is not possible, not sure about the asn1 for SAN). |
|
@shiup I see no evidence that's a valid practice. |
|
@apapirovski I refreshed my memory with the asn1 spec for x509.. I withdrawn my concern. So your preference, fix the path, or the function. |
|
@shiup I guess I still don't get where this is broken. Can you explain? I went to the only call site in the code and it passes in the expected value. |
|
@apaprocki: one of the paths (in the node library), not sure where yet - that I need to dig up, is pass in the host, not And now that you explain how the Thanks ^ |
|
@shiup I would have a look at your code. You might be passing in actual const hostname = options.servername ||
options.host ||
(options.socket && options.socket._host) ||
'localhost';You can always specify |
|
@apapirovski thanks for the offering.. Much appreciated.. And thanks for the above We may not have an option, as we use other libraries. The caller I have is just curl with https://seriously.whatever.com:2443 .. |
|
@shiup can you link the third party library? Would love to check it. |
|
@apapirovski will do.. (assuming it is not my fault first :-) <-- which is a possibility with |
|
@apapirovski Here is what I found.. Using curl library, passed in Here is my proposed fix.. |
|
This can be closed from the above information.. thanks. |
If wild card dns check (both CN, or SAN) does not
contain port, it applies to all ports on the server.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes