You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by GitBox <gi...@apache.org> on 2022/01/17 07:31:26 UTC
[GitHub] [trafficserver] maskit opened a new pull request #8610: Make TLS Early Data available with BoringSSL
maskit opened a new pull request #8610:
URL: https://github.com/apache/trafficserver/pull/8610
Added code to support TLS Early Data with BoringSSL.
https://commondatastorage.googleapis.com/chromium-boringssl-docs/ssl.h.html#Early-data
I reused code for OpenSSL as much as possible, and there should be no logic change on code for OpenSSL. I tested the code for BoringSSL with the existing autest, and also ran the test commands (openssl s_client) by hand and checked outputs on the both ATS side and the client side. All the outputs seemed fine to me, though we can't check those on our CI because we don't have BoringSSL build job.
This closes #7503.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #8610: Make TLS Early Data available with BoringSSL
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #8610:
URL: https://github.com/apache/trafficserver/pull/8610#issuecomment-1020649485
Looks good to me, let's fix @bneradt 's comments though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit commented on pull request #8610: Make TLS Early Data available with BoringSSL
Posted by GitBox <gi...@apache.org>.
maskit commented on pull request #8610:
URL: https://github.com/apache/trafficserver/pull/8610#issuecomment-1023383048
@bneradt Thanks for your suggestions. I added comments.
As for CI jobs, although we need consensus from the community, I'm fine with using Fedora job for BoringSSL. Fedora 36 and Ubuntu 22.04 LTS will have OpenSSL 3.0, so perhaps we can let Ubuntu job run with OpenSSL 3.0 in the future.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] maskit merged pull request #8610: Make TLS Early Data available with BoringSSL
Posted by GitBox <gi...@apache.org>.
maskit merged pull request #8610:
URL: https://github.com/apache/trafficserver/pull/8610
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on pull request #8610: Make TLS Early Data available with BoringSSL
Posted by GitBox <gi...@apache.org>.
bneradt commented on pull request #8610:
URL: https://github.com/apache/trafficserver/pull/8610#issuecomment-1018788927
> Looks good. Just a few suggestions for comments we can add.
>
> Maybe we can set up BoringSSL to run for one of the PR jobs in CI? Perhaps we can use the Fedora run, for instance, to run with BoringSSL.
For the record, this seems to do the trick:
```
docker pull controller.trafficserver.org/ats/fedora:33
docker run -it controller.trafficserver.org/ats/fedora:33 /bin/bash
# Now in the docker container, running as root.
yum install -y cmake go
cd /opt
git clone https://boringssl.googlesource.com/boringssl
cd boringssl
mkdir build
cd build
cmake -DCMAKE_BUILD_TYPE=Release ..
make -j $(nproc)
cd ../
mkdir lib
cd lib
ln -s ../build/ssl/libssl.a
ln -s ../build/crypto/libcrypto.a
```
We can add that to the controller.trafficserver.org/ats/fedora:33, making it a part of its image.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] bneradt commented on a change in pull request #8610: Make TLS Early Data available with BoringSSL
Posted by GitBox <gi...@apache.org>.
bneradt commented on a change in pull request #8610:
URL: https://github.com/apache/trafficserver/pull/8610#discussion_r789901835
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -2014,15 +2021,61 @@ SSLNetVConnection::_ssl_accept()
#if TS_HAS_TLS_EARLY_DATA
if (SSLConfigParams::server_max_early_data > 0 && !this->_early_data_finish) {
- size_t nread;
+#if HAVE_SSL_READ_EARLY_DATA
+ size_t nread = 0;
+#else
+ ssize_t nread = 0;
+#endif
while (true) {
- IOBufferBlock *block = new_IOBufferBlock();
+ bool had_error_on_reading_early_data = false;
+ bool finished_reading_early_data = false;
+ IOBufferBlock *block = new_IOBufferBlock();
block->alloc(BUFFER_SIZE_INDEX_16K);
- ret = SSL_read_early_data(ssl, block->buf(), index_to_buffer_size(BUFFER_SIZE_INDEX_16K), &nread);
+#if HAVE_SSL_READ_EARLY_DATA
+ ret = SSL_read_early_data(ssl, block->buf(), index_to_buffer_size(BUFFER_SIZE_INDEX_16K), &nread);
if (ret == SSL_READ_EARLY_DATA_ERROR) {
- Debug("ssl_early_data", "SSL_READ_EARLY_DATA_ERROR");
+ had_error_on_reading_early_data = true;
+ } else if (ret == SSL_READ_EARLY_DATA_FINISH) {
+ finished_reading_early_data = true;
+ }
+#else
+ ret = SSL_accept(ssl);
Review comment:
Let's comment the `#else` block indicating that it is the BoringSSL implementation. (You've done that elsewhere in this patch, I think it would be good here as well.)
##########
File path: iocore/net/SSLNetVConnection.cc
##########
@@ -2205,15 +2262,45 @@ SSLNetVConnection::_ssl_read_buffer(void *buf, int64_t nbytes, int64_t &nread)
}
if (SSLConfigParams::server_max_early_data > 0 && !this->_early_data_finish) {
+ bool had_error_on_reading_early_data = false;
+ bool finished_reading_early_data = false;
Debug("ssl_early_data", "More early data to read.");
ssl_error_t ssl_error = SSL_ERROR_NONE;
- size_t read_bytes = 0;
-
- int ret = SSL_read_early_data(ssl, buf, static_cast<size_t>(nbytes), &read_bytes);
+ int ret;
+#if HAVE_SSL_READ_EARLY_DATA
+ size_t read_bytes = 0;
+#else
+ ssize_t read_bytes = 0;
+#endif
+#ifdef HAVE_SSL_READ_EARLY_DATA
+ ret = SSL_read_early_data(ssl, buf, static_cast<size_t>(nbytes), &read_bytes);
if (ret == SSL_READ_EARLY_DATA_ERROR) {
- Debug("ssl_early_data", "SSL_READ_EARLY_DATA_ERROR");
- ssl_error = SSL_get_error(ssl, ret);
+ had_error_on_reading_early_data = true;
+ ssl_error = SSL_get_error(ssl, ret);
+ } else if (ret == SSL_READ_EARLY_DATA_FINISH) {
+ finished_reading_early_data = true;
+ }
+#else
+ if (SSL_in_early_data(ssl)) {
Review comment:
Let's also comment this as the BoringSSL version.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [trafficserver] duke8253 commented on pull request #8610: Make TLS Early Data available with BoringSSL
Posted by GitBox <gi...@apache.org>.
duke8253 commented on pull request #8610:
URL: https://github.com/apache/trafficserver/pull/8610#issuecomment-1020649485
Looks good to me, let's fix @bneradt 's comments though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org