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

Implement certificate compression #95

Merged

Conversation

hwh33
Copy link
Contributor

@hwh33 hwh33 commented Nov 16, 2021

Certificate compression is defined in RFC 8879:
https://datatracker.ietf.org/doc/html/rfc8879

This implementation is client-side only, for server certificates.

Some parrots (e.g. HelloChrome_83) advertise certificate compression. This PR allows clients to connect to hosts which implement certificate compression.


require (
github.com/andybalholm/brotli v1.0.4
github.com/klauspost/compress v1.13.6
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These two dependencies are necessary to support Brotli and Zstandard decompression. I added a go.mod file to allow users of this package to more readily import.

FAKE_TLS_DHE_RSA_WITH_AES_256_CBC_SHA = uint16(0x0039)
FAKE_TLS_DHE_RSA_WITH_AES_256_GCM_SHA384 = uint16(0x009f)
FAKE_TLS_RSA_WITH_RC4_128_MD5 = uint16(0x0004)
FAKE_TLS_EMPTY_RENEGOTIATION_INFO_SCSV = uint16(0x00ff)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

gofmt did this

@hwh33
Copy link
Contributor Author

hwh33 commented Nov 16, 2021

Because there is no server-side implementation, it would be complex to write a proper test for this new logic. I did add the new message to existing tests for message marshaling / unmarshaling.

I also tested this against a number of live sites, including a number which support certificate compression and a number which do not. I manually verified that we were parsing and handling the compressed certificate as expected.

If desired, I could add a semi-manual test, like:

var (
	certCompressionTestHost = flag.String("test-cert-compression-host", "", "required to run TestCertCompression")
	certCompressionTestPort = flag.Int("test-cert-compression-port", 443, "optional parameter for TestCertCompression")
)

// TestCertCompression tests this package's implementation of certificate compression (see
// https://datatracker.ietf.org/doc/html/rfc8879). Because certificate compression is only
// implemented client-side, it would be complex to write a local test. This test can be used to hit
// a remote site which implements certificate compression server-side. This test cannot check
// whether the remote actually served a compressed certificate - building a coverage profile can aid
// in this.
func TestCertCompression(t *testing.T) {
	flag.Parse()
	if *certCompressionTestHost == "" {
		t.Skip()
	}

	// HelloChrome_83 advertises certificate compression.
	helloID := HelloChrome_83

	tcpConn, err := net.Dial("tcp", fmt.Sprintf("%s:%d", *certCompressionTestHost, *certCompressionTestPort))
	if err != nil {
		t.Fatalf("failed to dial test host: %v", err)
	}
	uconn := UClient(tcpConn, &Config{ServerName: *certCompressionTestHost}, helloID)
	if err := uconn.Handshake(); err != nil {
		t.Fatalf("handshake failed: %v", err)
	}
}

Copy link
Contributor

@sleeyax sleeyax left a comment

Choose a reason for hiding this comment

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

I've tested your code and I think you forgot to add a line of code.

Without a change, hs.uconn.certCompressionAlgs is never set and always nil resulting in the following error: tls: received unexpected handshake message of type *tls.compressedCertificateMsg when waiting for *tls.certificateMsgTLS13.

See the review.

}

func (e *UtlsCompressCertExtension) writeToUConn(uc *UConn) error {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return nil
uc.certCompressionAlgs = e.Methods
return nil

Certificate compression is defined in RFC 8879:
https://datatracker.ietf.org/doc/html/rfc8879

This implementation is client-side only, for server certificates.
@hwh33
Copy link
Contributor Author

hwh33 commented Feb 9, 2022

You're totally right @sleeyax! I ported this from our fork (which is ahead) and messed up that part of the port. Thanks for the catch! I went through and double-checked everything else and it seems correct. Apologies for the delayed response!

@yan-foto
Copy link

yan-foto commented Jun 27, 2022

I have noticed that the fork does not always manage to decompress retrieved certificates. For example, for www.facebook.com, which supports all three brotli, zstd, and zlib algorithms, decompressing zlib certs fails as I write this comment. For my usecase, I really don't need the actual certificate, but only to know if the server responds to cert_compress extensions or not.

Another feature that I'd like to see with this fork is to have a flag (maybe in TLS 1.3 state?) that denotes if the server really replied with a compressed certificate chain.

@hwh33
Copy link
Contributor Author

hwh33 commented Jul 1, 2022

Thanks for pointing that out @yan-foto, I was not aware. I'll try to figure out what's going on there.

Another feature that I'd like to see with this fork is to have a flag (maybe in TLS 1.3 state?) that denotes if the server really replied with a compressed certificate chain.

Could you expand on the use case for this feature?

Unfortunately it will probably be a few weeks before I have time to really dig into this stuff. But I will get to it!

@yan-foto
Copy link

yan-foto commented Jul 4, 2022

@hwh33 Thanks for the quick reply!

Could you expand on the use case for this feature?

We scan TLS-enabled hosts and want to verify if and which compression algorithms they support. At the moment there is no way to see if certificate chain was really compressed or if the server ignored our request and just returned the certificates uncompressed.

Unfortunately it will probably be a few weeks before I have time to really dig into this stuff. But I will get to it!

Thanks. I'm not in a hurry. I forked your version and hacked my way into it. But it's not something that anyone would want to have in production!

@gaukas gaukas merged commit 7344e34 into refraction-networking:master Jul 20, 2022
@gaukas
Copy link
Contributor

gaukas commented Jul 20, 2022

Approved, tested and merged. Thanks for contributing! 🎉

@gaukas gaukas linked an issue Jul 20, 2022 that may be closed by this pull request
@gaukas
Copy link
Contributor

gaukas commented Jul 20, 2022

For example, for www.facebook.com, which supports all three brotli, zstd, and zlib algorithms, decompressing zlib certs fails as I write this comment.

Nice catch! Definitely something we would like to fix.
However, as long as this PR enables the connectivity to hosts with certificate compression, I had it merged for good.

At the moment there is no way to see if certificate chain was really compressed or if the server ignored our request and just returned the certificates uncompressed.

Interesting observation. Will need more details on that.

@gaukas
Copy link
Contributor

gaukas commented Jul 31, 2023

Hi @hwh33 and @yan-foto, I would like to follow up on the zlib discussion since it is mentioned again in a latest issue. Have any of you already implemented zlib support? A PR would be really appreciated.

@hwh33
Copy link
Contributor Author

hwh33 commented Aug 1, 2023

Fixed here @gaukas!

#206

@gaukas
Copy link
Contributor

gaukas commented Aug 1, 2023

Thanks @hwh33! I will get it merged.

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.

Bug: Handshake may fail when using specific ClientHelloID
4 participants