add support for openssl #67
Conversation
Signed-off-by: Tiger <[email protected]>
|
I'll change the reference once this get merged libp2p/go-openssl#6 |
Signed-off-by: Tiger <[email protected]>
|
Ive updated the go.mod @Stebalien could you check this? |
|
cc @marten-seemann, could I ask you to review this? |
There was a problem hiding this comment.
After adding a number of detailed comments, I noticed that this PR is using openssl.TLSv1_2. Looking at the code, it seems like go-openssl doesn't even support TLS 1.3 yet.
libp2p use of TLS is only specified for TLS 1.3, and negotiating any earlier TLS version is explicitly prohibited, see https://github.com/libp2p/specs/blob/master/tls/tls.md#handshake-protocol.
| @@ -57,6 +46,25 @@ func NewIdentity(privKey ic.PrivKey) (*Identity, error) { | |||
| }, nil | |||
| } | |||
|
|
|||
| func newIdentity(sk ic.PrivKey) (identity, error) { | |||
| return NewIdentity(sk) | |||
| } | |||
There was a problem hiding this comment.
Why is this additional constructor needed?
| return nil | ||
| } | ||
|
|
||
| func unmarshalExtenstionPublicKey(value []byte, certKeyPub []byte) (ic.PubKey, error) { |
There was a problem hiding this comment.
There's a typo in the function name.
| } | ||
|
|
||
| // identity is used to secure connection. | ||
| type identity interface { |
There was a problem hiding this comment.
Logically, this doesn't seem to belong to Identity. Also, it doesn't access any private members of Identity, so one could easily move it to a separate function.
| } | ||
|
|
||
| // Enable two way tls. | ||
| opensslCtx.SetVerifyMode(openssl.VerifyPeer) |
There was a problem hiding this comment.
I'm not familiar with the go-openssl API here. Why VerifyPeer and not VerifyFailIfNoPeerCert? What happens if the client doesn't present a certificate at all? And what is VerifyClientOnce anyway?
There was a problem hiding this comment.
See the openssl man page. The go-openssl verify option bindings have a one-to-one correspondence to the C openssl bindings. I set it to both VerifyPeer and VerifyFailIfNoPeerCert.
| opensslCtx.SetVerifyMode(openssl.VerifyPeer) | ||
|
|
||
| opensslCtx.SetVerifyCallback(func(a bool, store *openssl.CertificateStoreCtx) bool { | ||
| cert := store.GetCurrentCert() |
There was a problem hiding this comment.
GetCurrentCert() seems to return only a single certificate. What the peer presents though is a certificate chain. How do we get access to the chain (or at least make sure the whole chain is verified)?
There was a problem hiding this comment.
The verify callback is run for every certificate in the chain.
Quote from openssl man page:
The certificate chain is checked starting with the deepest nesting level (the root CA certificate) and worked upward to the peer's certificate. At each level signatures and issuer attributes are checked. Whenever a verification error is found, the error number is stored in x509_ctx and verify_callback is called with preverify_ok=0. By applying X509_CTX_store_* functions verify_callback can locate the certificate in question and perform additional steps (see EXAMPLES). If no error is found for a certificate, verify_callback is called with preverify_ok=1 before advancing to the next level.
|
Another thing (you don't have to implement this, @balajijinnah, and it doesn't need to be part of this PR), but before rolling this out into production we probably want to add an integration test that tests various handshake scenarios (both successful and unsuccessful) between an OpenSSL-enabled and an OpenSSL-disabled build. This is not straightforward unfortunately, as the openssl flag has be set at build time, so we'll probably have to build two binaries and run them against each other. What would be nice in this PR though is to split up the Travis build into two, so we can test in CI that at least the two handshake implementations interoperate among themselves. |
|
@balajijinnah Are you still working on this PR? If not, I'd be glad to continue with it. |
|
@Geo25rey, please carry forward. I'm occupied now :( |
Signed-off-by: Tiger [email protected]