Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

proposal: crypto/tls: custom TLS extensions support #51497

Closed
kosmas-valianos opened this issue Mar 5, 2022 · 5 comments
Closed

proposal: crypto/tls: custom TLS extensions support #51497

kosmas-valianos opened this issue Mar 5, 2022 · 5 comments
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Milestone

Comments

@kosmas-valianos
Copy link

A similar issue had been opened at #25807 but it was retracted without any explanation so I would like to bring this into light again as it is a pain for VPN solutions written in Go.

TLS Hello messages are allowed to have custom extensions therefore an application can ask the underlying TLS library to add arbitrary data. As we really need this we have ended up patching Go so I can actually present a working solution to give a general idea of what I mean and how it can play out:

  1. A new type in crypto/tls/common.go
type HelloExtension struct {
	Type uint16
	Data []uint8
}
  1. Add HelloExtensions []HelloExtension in the Config struct and copy in the Clone() func
  2. Add []HelloExtension to clientHelloMsg (client case)
  3. Add to the marshal
			if len(m.helloExtensions) > 0 {
				for _, e := range m.helloExtensions {
					b.AddUint16(e.Type)
					b.AddUint16LengthPrefixed(func(b *cryptobyte.Builder) {
						b.AddBytes(e.Data)
					})
				}
			}
  1. Applications can then create extensions with simply tls.HelloExtension{ Type: XXX, Data: YYY} and append them in the tls.Config.HelloExtensions
@gopherbot gopherbot added this to the Proposal milestone Mar 5, 2022
@seankhliao seankhliao added the Proposal-Crypto Proposal related to crypto packages or other security issues label Mar 5, 2022
@seankhliao
Copy link
Member

cc @golang/security

@FiloSottile
Copy link
Contributor

Extensions can modify any expected behavior of the TLS stack, which this (or any) API doesn't enable. This API lets applications advertise support for things we don't support, which is too much of a footgun to implement.

The only safe use of this API would be to set a "extra potentially unencrypted data" extension. Maybe that's worth proposing to the TLS WG as something to standardize.

@kosmas-valianos
Copy link
Author

Basically sounds it requires more work than simply adding the extensions support which in itself is trivial (our patch is like 20 lines). Falls into the scope of making Go's crypto/tls a "full fledged" TLS library like OpenSSL. Besides the "hack" patch, using OpenSSL can be another workaround I guess until Go's TLS library becomes mature enough to replace it

@FiloSottile
Copy link
Contributor

Having as much flexibility and complexity as OpenSSL is explicitly a non-goal of crypto/tls. This means the library won't work for some edge cases, but has major advantages in terms of safety, security track record, and maintenance cost.

(Anyway, I don't see how even OpenSSL can allow arbitrary extensions, since an extension can change how any part of the TLS stack behaves, and not even OpenSSL can provide hooks to modify behavior all over the place.)

@kosmas-valianos
Copy link
Author

Okay looks crypto/tls is never going to have this so I will go ahead and close the issue. If someone really wants to use custom TLS extensions has to pick his poison with either patching Go, as we did, or using some other SSL library that does this https://www.openssl.org/docs/man1.1.1/man3/SSL_CTX_add_server_custom_ext.html

@golang golang locked and limited conversation to collaborators Mar 9, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge Proposal Proposal-Crypto Proposal related to crypto packages or other security issues
Projects
None yet
Development

No branches or pull requests

4 participants