Certificate Compression (RFC8879) by tmshort · Pull Request #18186 · openssl/openssl

tmshort · 2022-04-26T15:29:10Z

Replacement of #9155 (github doesn't allow change of source repositories)

Add support for brotli compression
Add support for Zstandardi compression
Add support for RFC8879
Add support for callback API, which allows the user to handle either compression expansion or pre-compressed server certificates.

Checklist

documentation is added or updated
tests are added or updated

openssl-machine · 2022-05-27T00:07:23Z

This PR is in a state where it requires action by @openssl/otc but the last update was 30 days ago

tmshort · 2022-06-02T16:56:36Z

Not sure why address_ub_santitizer is failing. I tried running it locally (same options, etc), and it succeeds. The error seems to be in a file this PR does not touch?

mattcaswell

Nice work!

I reviewed this commit by commit, so some of my comments may have been made irrelevant by later changes

mattcaswell · 2022-06-03T10:54:35Z

Should we run some tests with this code and asan?

I'll look into that.

tmshort · 2022-06-09T19:09:40Z

Thank you @mattcaswell! I will be updating this shortly.

FdaSilvaYY

A few more nits ...

FdaSilvaYY · 2022-06-12T12:34:51Z

I don't see zstd options description

I had meant to remove brotli since it was not longer considered "special" (like zlib). It's now just a regular option to be used with enable- or no-.

FdaSilvaYY · 2022-06-12T13:16:18Z

This allocation can failed.

Yes, and it's checked in tls13_process_compressed_certificate(); which is called by both the client and server code. So, I put the check in there, rather than having two checks before the call of the function.

tmshort · 2022-06-28T14:13:05Z

Ping @mattcaswell ?

mattcaswell · 2022-07-14T09:34:24Z

Ping @mattcaswell ?

Sorry - been away on vacation for the last 2 weeks. Will try and pick this up when I am caught up.

tmshort · 2022-08-01T20:14:40Z

I removed BUF_MEM from the APIs.

tmshort · 2022-08-03T17:09:31Z

Should we run some tests with this code and asan?

Done.

tmshort · 2022-08-03T18:43:01Z

ping @mattcaswell ?

tmshort · 2022-10-17T13:52:43Z

👍 I'm happy to resolve the conflicts when merging.

I just did it locally, but I'm also a committer.

openssl-machine · 2022-10-18T14:00:43Z

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

tmshort · 2022-10-18T14:10:45Z

Just comments... starting merge.

tmshort · 2022-10-18T15:45:00Z

Merged to master. Thanks @mattcaswell @hlandau.

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

Use the normal OPENSSL_NO_ prefix to enable/disable ZLIB Make `BIO_f_zlib()` always available. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

* Compressed Certificate extension (server/client) * Server certificates (send/receive) * Client certificate (send/receive) Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

size_t-ify the COMP_METHOD structure and functions. Get rid of the non-functional COMP_METHODS and return NULL instead. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

slontis · 2022-10-19T03:06:50Z

Coverity has picked up a few things/..

New defect(s) Reported-by: Coverity Scan
Showing 3 of 3 defect(s)

** CID 1516095: (DEADCODE)
/test/cert_comp_test.c: 163 in test_ssl_cert_comp()
/test/cert_comp_test.c: 206 in test_ssl_cert_comp()

*** CID 1516095: (DEADCODE)
/test/cert_comp_test.c: 163 in test_ssl_cert_comp()
157 if (!TEST_true(create_ssl_ctx_pair(NULL, TLS_server_method(),
158 TLS_client_method(),
159 TLS1_3_VERSION, 0,
160 &sctx, &cctx, cert, privkey)))
161 goto end;
162 if (test == 3) {

CID 1516095:    (DEADCODE)
Execution cannot reach this statement: "server_pref[0] = expected_s...".

163 server_pref[0] = expected_server;
164 server_pref[1] = expected_client;
165 if (!TEST_true(SSL_CTX_set1_cert_comp_preference(sctx, server_pref, 2)))
166 goto end;
167 client_pref[0] = expected_client;
168 if (!TEST_true(SSL_CTX_set1_cert_comp_preference(cctx, client_pref, 1)))
/test/cert_comp_test.c: 206 in test_ssl_cert_comp()
200 goto end;
201 }
202
203 if (!TEST_true(create_ssl_connection(serverssl, clientssl, SSL_ERROR_NONE)))
204 goto end;
205 if (test == 3) {

CID 1516095:    (DEADCODE)
Execution cannot reach the expression "sc" inside this statement: "sc = ((serverssl == NULL) ?...".

206 SSL_CONNECTION sc = SSL_CONNECTION_FROM_SSL(serverssl);
207
208 / expect that the pre-compressed cert won't be used */
209 if (!TEST_int_eq(sc->cert->key->cert_comp_used, 0))
210 goto end;
211

** CID 1516094: Memory - illegal accesses (UNINIT)

*** CID 1516094: Memory - illegal accesses (UNINIT)
/ssl/statem/statem_clnt.c: 3735 in tls_construct_client_compressed_certificate()
3729 ret = 1;
3730 goto out;
3731
3732 err:
3733 SSLfatal(sc, SSL_AD_INTERNAL_ERROR, ERR_R_INTERNAL_ERROR);
3734 out:

CID 1516094:  Memory - illegal accesses  (UNINIT)
Using uninitialized value "tmppkt.subs" when calling "WPACKET_cleanup".

3735 WPACKET_cleanup(&tmppkt);
3736 BUF_MEM_free(buf);
3737 COMP_CTX_free(comp);
3738 return ret;
3739 }
3740 #endif

** CID 1516093: (TAINTED_SCALAR)
/ssl/t1_trce.c: 1337 in ssl_print_compressed_certificates()
/ssl/t1_trce.c: 1345 in ssl_print_compressed_certificates()

*** CID 1516093: (TAINTED_SCALAR)
/ssl/t1_trce.c: 1337 in ssl_print_compressed_certificates()
1331 msg += 8;
1332 BIO_indent(bio, indent, 80);
1333 BIO_printf(bio, "Compression type=%s (0x%04x)\n", ssl_trace_str(alg, ssl_comp_cert_tbl), alg);
1334 BIO_indent(bio, indent, 80);
1335 BIO_printf(bio, "Uncompressed length=%d\n", (int)uclen);
1336 BIO_indent(bio, indent, 80);

CID 1516093:    (TAINTED_SCALAR)
Using tainted expression "clen" as the divisor in "(float)uclen / (float)clen".

1337 BIO_printf(bio, "Compressed length=%d, Ratio=%f:1\n", (int)clen, (float)uclen / (float)clen);
1338
1339 BIO_dump_indent(bio, (const char *)msg, clen, indent);
1340
1341 #ifndef OPENSSL_NO_COMP_ALG
1342 if (!ossl_comp_has_alg(alg))
/ssl/t1_trce.c: 1345 in ssl_print_compressed_certificates()
1339 BIO_dump_indent(bio, (const char *)msg, clen, indent);
1340
1341 #ifndef OPENSSL_NO_COMP_ALG
1342 if (!ossl_comp_has_alg(alg))
1343 return 0;
1344

CID 1516093:    (TAINTED_SCALAR)
Passing tainted expression "uclen" to "CRYPTO_malloc", which uses it as an allocation size. [Note: The source code implementation of the function has been overridden by a builtin model.]

1345 if ((ucdata = OPENSSL_malloc(uclen)) == NULL)
1346 return 0;
1347
1348 switch (alg) {
1349 case TLSEXT_comp_cert_zlib:
1350 method = COMP_zlib();

tmshort · 2022-10-19T13:51:38Z

Thanks @slontis, I'll take a look at these.

tmshort · 2022-10-19T13:56:30Z

** CID 1516095: (DEADCODE)
/test/cert_comp_test.c: 163 in test_ssl_cert_comp()
/test/cert_comp_test.c: 206 in test_ssl_cert_comp()

These will look like dead code if less than two compression algorithms are configured. If there are two compression algorithms configured, then this won't be dead code. This is due to a check on line 152 which skips the tests when trying this configuration, the expected values are initialized and set based on the configuration compression algorithms.
EDIT: How does one ignore these errors? @slontis

tmshort · 2022-10-19T14:48:00Z

See #19440

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

Use the normal OPENSSL_NO_ prefix to enable/disable ZLIB Make `BIO_f_zlib()` always available. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

* Compressed Certificate extension (server/client) * Server certificates (send/receive) * Client certificate (send/receive) Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

size_t-ify the COMP_METHOD structure and functions. Get rid of the non-functional COMP_METHODS and return NULL instead. Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

tmshort added branch: master Applies to master branch approval: review pending This pull request needs review by a committer approval: otc review pending triaged: feature The issue/pr requests/adds a feature labels Apr 26, 2022

tmshort mentioned this pull request Apr 26, 2022

Certificate Compression (RFC8879) #9155

Closed

2 tasks

tmshort force-pushed the master-certcomp branch 2 times, most recently from 351cfbb to 3d2f81d Compare June 2, 2022 15:42

tmshort requested review from mattcaswell and t8m June 2, 2022 15:43

mattcaswell requested changes Jun 3, 2022

View reviewed changes

tmshort force-pushed the master-certcomp branch 3 times, most recently from 42bc295 to 32fc27b Compare June 11, 2022 16:27

FdaSilvaYY reviewed Jun 12, 2022

View reviewed changes

tmshort force-pushed the master-certcomp branch from 32fc27b to 369459c Compare June 13, 2022 14:52

tmshort requested a review from mattcaswell June 15, 2022 13:59

tmshort force-pushed the master-certcomp branch from 369459c to 021c166 Compare July 6, 2022 14:56

tmshort force-pushed the master-certcomp branch from 021c166 to b81cc52 Compare August 1, 2022 20:08

tmshort force-pushed the master-certcomp branch 2 times, most recently from 144a3a1 to bf94cb8 Compare August 3, 2022 17:08

tmshort force-pushed the master-certcomp branch from bf94cb8 to 12e8c1e Compare August 3, 2022 18:04

tmshort self-assigned this Oct 17, 2022

tmshort added approval: ready to merge The 24 hour grace period has passed, ready to merge and removed approval: done This pull request has the required number of approvals labels Oct 18, 2022

tmshort closed this Oct 18, 2022

tmshort deleted the master-certcomp branch October 18, 2022 15:45

openssl-machine pushed a commit that referenced this pull request Oct 18, 2022

Add brotli compression support (RFC7924)

12e96a2

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

openssl-machine pushed a commit that referenced this pull request Oct 18, 2022

Add ZSTD compression support (RFC8478bis)

caf9317

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

openssl-machine pushed a commit that referenced this pull request Oct 18, 2022

Add CI to build with brotli and zstd

b540aae

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

openssl-machine pushed a commit that referenced this pull request Oct 18, 2022

Add for_comp flag when retrieving certs for compression

72620ac

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from #18186)

t184256 mentioned this pull request Oct 27, 2022

Support RFC 8879 on TLS Certificate Compression #13597

Closed

tmshort mentioned this pull request Nov 11, 2022

Add RFC8701 GREASE Support #19646

Closed

2 tasks

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022

Add brotli compression support (RFC7924)

45c55f1

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022

Add ZSTD compression support (RFC8478bis)

0c10fd9

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022

Add CI to build with brotli and zstd

bd80e9e

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

beldmit pushed a commit to beldmit/openssl that referenced this pull request Dec 26, 2022

Add for_comp flag when retrieving certs for compression

9fcc0c3

Reviewed-by: Matt Caswell <[email protected]> Reviewed-by: Hugo Landau <[email protected]> (Merged from openssl#18186)

mattcaswell mentioned this pull request Jun 27, 2023

DTLS 1.3 #13900

Open

Uh oh!

Uh oh!

Conversation

tmshort commented Apr 26, 2022

Checklist

Uh oh!

openssl-machine commented May 27, 2022

Uh oh!

tmshort commented Jun 2, 2022

Uh oh!

mattcaswell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mattcaswell Jun 3, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Jun 10, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Aug 8, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort commented Jun 9, 2022

Uh oh!

FdaSilvaYY left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

FdaSilvaYY Jun 12, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Jun 13, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

FdaSilvaYY Jun 12, 2022

Choose a reason for hiding this comment

Uh oh!

tmshort Jun 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmshort commented Jun 28, 2022

Uh oh!

mattcaswell commented Jul 14, 2022

Uh oh!

tmshort commented Aug 1, 2022

Uh oh!

tmshort commented Aug 3, 2022

Uh oh!

tmshort commented Aug 3, 2022

Uh oh!

tmshort commented Oct 17, 2022

Uh oh!

openssl-machine commented Oct 18, 2022

Uh oh!

tmshort commented Oct 18, 2022

Uh oh!

tmshort commented Oct 18, 2022

Uh oh!

slontis commented Oct 19, 2022

Uh oh!

tmshort commented Oct 19, 2022

Uh oh!

FdaSilvaYY left a comment •

edited

Loading

tmshort Jun 13, 2022 •

edited

Loading

tmshort commented Oct 19, 2022 •

edited

Loading