Conversation
This is to add ZSTD support for the compressed certificate RFC [1]. [1]: https://tools.ietf.org/html/draft-ietf-tls-certificate-compression-10
|
Tested with facebook.com with the cli command and it works fine. Facebook servers prefer ZSTD when both Brotli and ZSTD are advertised in ClientHello. |
kazuho
left a comment
kazuho
left a comment
There was a problem hiding this comment.
Thank you for the pull request. It's good to have support for ZSTD added.
Would you mind adding the following?
- code that uses ZSTD for compressing the certificates
- command line option to the cli command for using ZSTD
- test that uses ZSTD to t/e2e.t alongside the one that we have for brotli
As much as I am happy to see support for ZSTD being added, I am afraid we cannot maintain them without tests.
| ADD_DEFINITIONS(-DPTLS_HAVE_ZSTD) | ||
| LIST(APPEND CORE_EXTRA_LIBS ${LibZSTD}) | ||
| TARGET_LINK_LIBRARIES(cli ${LibZSTD}) | ||
| ENDIF () |
There was a problem hiding this comment.
Could you please move the detection logic next to the one that we have for brotli, and align the approach?
Then, we could do something like IF ((BROTLI_DEC_FOUND AND BROTLI_ENC_FOUND) OR ZSTD_FOUND) to link against lib/certificate_compression.c if either of the compression library was found.
There was a problem hiding this comment.
Thanks for the review. I'll be working on the suggestions.
2 participants
This is to add ZSTD support for the compressed certificate RFC 1.