Added basic support for TLS 1.3 using openssl min/max proto version functions #13

Geo25rey · 2020-10-22T23:35:07Z

Closes: #8

Required for: libp2p/go-libp2p-tls#71, ipfs/kubo#7150

Geo25rey · 2020-10-24T13:36:02Z

@Stebalien Can you help me get this merged so I can more easily test libp2p/go-libp2p-tls#71?

marten-seemann · 2020-10-26T05:56:07Z

conn.go

@@ -383,6 +405,16 @@ func (c *Conn) shutdown() func() error {
 	defer c.mtx.Unlock()
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
+	timed_out := false
+	time.AfterFunc(300*time.Millisecond, func() {


Where does this value come from?

@marten-seemann It's a local variable meant to prevent an infinite loop.

First, I don't understand where an infinite loop would come from (I don't understand the whole handshake_started / handshake_finished logic in this PR.

Second, starting a timer here is definitely not the solution. 300 ms seem to be an arbitrary value, and it will just cause problems later on. Furthermore, this code is racy anyway (you're writing to and reading from timed_out from separate go routines).

@marten-seemann The point of the timeout loop is that it prevents a connection being closed while a handshake is in progress, which occurs in the C portion of the OpenSSL library. I consistently run into that error in my libp2p-tls PR. I can remove this from this PR for now, but it will still be necessary I think. Also, I can't test how well this PR helps with libp2p integration until this is merged.

Assuming that Go booleans are equivalent to C booleans once compiled, they are atomic (modified and read in 1 instruction) and thus don't actually have any race conditions.

marten-seemann · 2020-10-26T05:56:34Z

ctx.go

@@ -95,7 +95,7 @@ func NewCtxWithVersion(version SSLVersion) (*Ctx, error) {
 	case TLSv1_2:
 		method = C.X_TLSv1_2_method()
 	case AnyVersion:
-		method = C.X_SSLv23_method()
+		method = C.X_TLS_method()


Why is here no case for TLSv1_3?

Using the method parameter is deprecated in openssl now, so there is no TLSv1_3. That's why I added the min and max proto functions.

I think we'll need to build an abstraction around that then, i.e. we shouldn't have a NewCtxWithVersion and a Set{Min,Max}ProtoVersion at the same time.
I'd be ok with either:

removing NewCtxWithVersion, and forcing users to use Set{Min,Max}ProtoVersion, or

not introducing Set{Min,Max}ProtoVersion as a public function, but making it private and then calling it from NewCtxWithVersion

The first option probably allows for more flexibility (as you can set min and max separately), whereas NewCtxWithVersion forces you to use min = max, right?

What do you think?

I think the real question is what is the policy for deprecated functions in OpenSSL?

I've seen in some other Go wrapper libraries that by default they don't include deprecated functions but have a tag to enable them at compile time. I can see implementing something before the next version, but that's a PR in itself.

As a general rule, I'd try to avoid deprecated functions, if possible. After all, they're deprecated for a reason.

My point in my previous comment was that we should abstract away that complexity from users of go-openssl. That means, we should either offer NewCtxWithVersion or Set{Min,Max}ProtoVersion.

marten-seemann · 2020-10-26T05:57:30Z

ctx.go

+
+func (c *Ctx) SetMinProtoVersion(version Version) bool {
+	return C.X_SSL_CTX_set_min_proto_version(
+		c.ctx, C.int(version)) == 1


Under which circumstances can this fail? I wouldn't expect a return value on a setter function.

Not sure how it could fail. I'm just passing along the value from the openssl C library.

marten-seemann · 2020-10-26T05:58:03Z

key_test.go

+	//_, err = key.MarshalPKCS1PrivateKeyPEM()
+	//if err != nil {
+	//	t.Fatal(err)
+	//}


This should probably be fixed.

This is unrelated to TLS 1.3 so I'm just suppressing the error for now. I think the function is also deprecated. I'm sure if you the tests without this commit you'll see the function fails there. It doesn't seem to affect overall functionality of the go-openssl library.

I don't see any deprecation in the interface:

go-openssl/key.go

Lines 97 to 110 in 6f65c2c

type PrivateKey interface {

PublicKey

// Signs the data using PKCS1.15

SignPKCS1v15(Method, []byte) ([]byte, error)

// MarshalPKCS1PrivateKeyPEM converts the private key to PEM-encoded PKCS1

// format

MarshalPKCS1PrivateKeyPEM() (pem_block []byte, err error)

// MarshalPKCS1PrivateKeyDER converts the private key to DER-encoded PKCS1

// format

MarshalPKCS1PrivateKeyDER() (der_block []byte, err error)

}

I can confirm that the test is failing on my local machine though, and I think this should be fixed in a separate PR (with a proper fix, not a FIXME).

I'll remove the FIXMEs for now.

marten-seemann · 2020-10-26T05:58:37Z

conn.go

@@ -303,11 +310,26 @@ func (c *Conn) handshake() func() error {
 // Handshake performs an SSL handshake. If a handshake is not manually
 // triggered, it will run before the first I/O on the encrypted stream.
 func (c *Conn) Handshake() error {
+	c.mtx.Lock()


Not sure I understand these changes. Why do you need those state bools?

In the go-libp2p-tls test, the handshake tests seem to fail because the tls connection is still being initialize when it is requested to be closed. This should hopefully fix it. But the only way to test if it works is adding it to the go-openssl repo. If the test results aren't promising then we can remove these.

marten-seemann · 2020-11-14T03:21:18Z

conn.go

@@ -383,6 +405,16 @@ func (c *Conn) shutdown() func() error {
 	defer c.mtx.Unlock()
 	runtime.LockOSThread()
 	defer runtime.UnlockOSThread()
+	timed_out := false
+	time.AfterFunc(300*time.Millisecond, func() {


First, I don't understand where an infinite loop would come from (I don't understand the whole handshake_started / handshake_finished logic in this PR.

Second, starting a timer here is definitely not the solution. 300 ms seem to be an arbitrary value, and it will just cause problems later on. Furthermore, this code is racy anyway (you're writing to and reading from timed_out from separate go routines).

marten-seemann · 2020-11-14T03:39:09Z

ctx.go

+		c.ctx, C.int(version)) == 1
+}
+
+func (c *Ctx) GetMinProtoVersion() Version {


What do we need these getter functions for? This value was set by the user before, so it seems like there's no need for it?

Added basic support for TLS 1.3

396f700

Geo25rey mentioned this pull request Oct 23, 2020

WIP: Add Openssl with TLS 1.3 libp2p/go-libp2p-tls#71

Draft

Potentially fixed handshake/close connection race condition

c97a591

marten-seemann reviewed Oct 26, 2020

View reviewed changes

Geo25rey mentioned this pull request Oct 29, 2020

[BOUNTY] Explore OpenSSL support in the TLS security transport ipfs/kubo#7150

Open

marten-seemann suggested changes Nov 14, 2020

View reviewed changes

marten-seemann reviewed Nov 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added basic support for TLS 1.3 using openssl min/max proto version functions #13

Added basic support for TLS 1.3 using openssl min/max proto version functions #13

Geo25rey commented Oct 22, 2020 •

edited

Loading

Geo25rey commented Oct 24, 2020

marten-seemann Oct 26, 2020

Geo25rey Oct 26, 2020

marten-seemann Nov 14, 2020

Geo25rey Nov 14, 2020

marten-seemann Oct 26, 2020

Geo25rey Oct 26, 2020

marten-seemann Nov 14, 2020

Geo25rey Nov 14, 2020

marten-seemann Nov 17, 2020

marten-seemann Oct 26, 2020

Geo25rey Oct 26, 2020

marten-seemann Oct 26, 2020

Geo25rey Oct 26, 2020

marten-seemann Nov 14, 2020

Geo25rey Nov 14, 2020

marten-seemann Oct 26, 2020

Geo25rey Oct 26, 2020

marten-seemann Nov 14, 2020

marten-seemann Nov 14, 2020

	type PrivateKey interface {
	PublicKey

	// Signs the data using PKCS1.15
	SignPKCS1v15(Method, []byte) ([]byte, error)

	// MarshalPKCS1PrivateKeyPEM converts the private key to PEM-encoded PKCS1
	// format
	MarshalPKCS1PrivateKeyPEM() (pem_block []byte, err error)

	// MarshalPKCS1PrivateKeyDER converts the private key to DER-encoded PKCS1
	// format
	MarshalPKCS1PrivateKeyDER() (der_block []byte, err error)
	}

Added basic support for TLS 1.3 using openssl min/max proto version functions #13

Are you sure you want to change the base?

Added basic support for TLS 1.3 using openssl min/max proto version functions #13

Conversation

Geo25rey commented Oct 22, 2020 • edited Loading

Geo25rey commented Oct 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Geo25rey commented Oct 22, 2020 •

edited

Loading