Skip to content

Conversation

@blutack
Copy link
Contributor

@blutack blutack commented Jan 9, 2019

I realise this has been covered in #311 but I'm providing a pull request, and there is a potential security issue/footgun with the current behaviour.

In my experience, at least two major exchanges (ICE and CME) do not use client certificate authentication for their FIX trade capture endpoints, and therefore quickfixgo cannot be used with them currently. Instead, the server provides a TLS certificate and login is via username/password field in the login message.

There is a very insecure workaround right now - setting SocketInsecureSkipVerify=Y has the side effect of allowing a TLS connection even without client certificates. However, it also does not verify the received certificates which obviously a presents a large security risk (especially given that both ICE and CME provide valid, signed certificates for their respective endpoint addresses). The security issue I see is that developers who need this functionality might just be setting SocketInsecureSkipVerify, rather than going through the friction of forking/pull requests etc and thereby making themselves vulnerable to MITM/DNS takeovers which would allow exchange credential theft.

With that in mind, I have added a "SocketUseSSL" parameter which will make quickfixgo establish a TLS connection even if client certificates are not present whilst allowing server certificate verification. Current behaviour also means that SocketTimeout & SocketMinimumTLSVersion will be ignored in this mode, in line with the existing behaviour when SocketInsecureSkipVerify is used. I'm happy to refactor things more but given that this is crypto code I'd rather get some feedback/thoughts first.

@cbusbey
Copy link
Contributor

cbusbey commented Jan 14, 2019

This issue has come up a few times, so it is probably best to address it in the framework. This PR however fails our automated builds. Can you address the build error and fix this pr before we dig into the logic?

Thanks.

InsecureSkipVerify alone can no longer be use to create a secure session.
Update tests to reflect that.
@cbusbey cbusbey merged commit 0b05761 into quickfixgo:master Feb 4, 2019
level2player pushed a commit to longbridge/quickfix that referenced this pull request Jul 19, 2022
Add SocketUseSSL parameter to allow SSL/TLS without client certs
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.

2 participants