Skip to content

Conversation

@hyde-zhang
Copy link
Contributor

@hyde-zhang hyde-zhang commented Aug 24, 2021

I think the SocketUseSSL config actually makes the code skips the SocketCAFile config when it is set to Y.

Since we have SocketCAFile option to use a supplied CA certificate to verify server identity, we still need the part of the code to add the supplied CA certificate to the CertPool and other functionalities. Skipping client authentication shouldn't skip everything.

@ackleymi
Copy link
Member

ackleymi commented Sep 8, 2021

@hyde-zhang Add a test or two around this in tls_test.go

@ackleymi
Copy link
Member

ackleymi commented Sep 8, 2021

Use go test -v -timeout=15s -count=1 -run TLSTestSuite as a shortcut to only execute tls tests

- Return error if client key cert file pair incomplete
- Add simple test for TLS load with only CA
@hyde-zhang
Copy link
Contributor Author

hyde-zhang commented Sep 8, 2021

@ackleymi Thanks for looking into this.

Since the acceptor and initiator uses the same loadTLSConfig function, updated the patch a bit such that:

  • TLS config will be created if:

    • As long as the settings has either pk or certificate entry set (might be incomplete)
    • Or no key pair setting at all but skip (SSL) client certs is allowed

    • The client certificates of TLS config will only be loaded if key and certificate both valid, but TLS config can still be loaded.
    • Initiator will be able to use TLS without its own pk and certs
    • Acceptor will skip client certs verification during hand shake if SocketUseSSL = Y i.e. when initiator is not providing any thing to verify
    • Acceptor can potentially start without setting its own pk and certs but this will be checked by crypto/tls. Again maybe initiator and acceptor shouldn't be using the same loadTLSConfig function.
  • I found the SocketUseSSL config name a bit confusing as it is really just allowSkipClientCerts.

s.Nil(tlsConfig.RootCAs)
s.Nil(tlsConfig.ClientCAs)
s.Equal(tls.NoClientCert, tlsConfig.ClientAuth)
s.Equal(tls.RequireAndVerifyClientCert, tlsConfig.ClientAuth)
Copy link
Contributor Author

@hyde-zhang hyde-zhang Sep 8, 2021

Choose a reason for hiding this comment

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

Still need to verify the incoming message using host's root CA set i.e. only skip when SocketUseSSL = Y - when initiator is allowed to not providing key pairs for its own identify

@hyde-zhang
Copy link
Contributor Author

@ackleymi Updated previous comments. Found it was a bit confusing. Sorry about that.

@hyde-zhang
Copy link
Contributor Author

Hey @ackleymi, any chance we can get this merged so that when initiator needs to use a custom CA file it can set SocketUseSSL=Y and only supply SocketCAFile (i.e. without setting SocketPrivateKeyFile and SocketCertificateFile)

@ackleymi ackleymi merged commit 94d7217 into quickfixgo:main Nov 29, 2021
@hyde-zhang
Copy link
Contributor Author

@ackleymi thanks

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