You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@trafficserver.apache.org by "cmcfarlen (via GitHub)" <gi...@apache.org> on 2023/06/28 01:13:43 UTC
[GitHub] [trafficserver] cmcfarlen opened a new pull request, #9924: Check openssl version and add appropriate defines
cmcfarlen opened a new pull request, #9924:
URL: https://github.com/apache/trafficserver/pull/9924
(no comment)
--
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] cmcfarlen merged pull request #9924: Check openssl version and add appropriate defines
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen merged PR #9924:
URL: https://github.com/apache/trafficserver/pull/9924
--
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] cmcfarlen commented on a diff in pull request #9924: Check openssl version and add appropriate defines
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9924:
URL: https://github.com/apache/trafficserver/pull/9924#discussion_r1245406505
##########
src/tscore/CMakeLists.txt:
##########
@@ -103,6 +102,14 @@ add_library(tscore SHARED
)
add_library(ts::tscore ALIAS tscore)
+if(OPENSSL_IS_BORINGSSL)
+ target_sources(tscore PUBLIC HKDF_boringssl.cc)
+elseif(OPENSSL_IS_OPENSSL3)
+ target_sources(tscore PUBLIC HKDF_openssl3.cc)
+else()
+ target_sources(tscore PUBLIC HKDF_openssl.cc)
Review Comment:
I couldn't figure out which was the default for add_library so just punted. I think we need to audit these usage requirements in general.
--
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] cmcfarlen commented on a diff in pull request #9924: Check openssl version and add appropriate defines
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9924:
URL: https://github.com/apache/trafficserver/pull/9924#discussion_r1245408810
##########
CMakeLists.txt:
##########
@@ -167,7 +167,14 @@ endif()
find_package(PCRE)
include(FindOpenSSL)
+include(CheckOpenSSLIsBoringSSL)
find_package(OpenSSL)
+check_openssl_is_boringssl(OPENSSL_IS_BORINGSSL "${OPENSSL_INCLUDE_DIR}")
+
+if(OPENSSL_VERSION VERSION_GREATER_EQUAL "3.0.0")
+ set(OPENSSL_IS_OPENSSL3 ON)
+ add_compile_definitions(OPENSSL_API_COMPAT=10002 OPENSSL_IS_OPENSSL3)
Review Comment:
I will remove the definition in certifier plugin. I don't think it needs to be in both. If its a problem, perhaps there is something else going on.
--
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] cmcfarlen commented on pull request #9924: Check openssl version and add appropriate defines
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on PR #9924:
URL: https://github.com/apache/trafficserver/pull/9924#issuecomment-1611735031
> I like these changes, but there are some minor points that should be addressed. How is boringssl versioned? Is it possible that finding boringssl could still end up with `OPENSSL_IS_OPENSSL3` getting set?
borinssl defines `OPENSSL_IS_BORINGSSL` itself, so they will both be set.
--
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] JosiahWI commented on a diff in pull request #9924: Check openssl version and add appropriate defines
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9924:
URL: https://github.com/apache/trafficserver/pull/9924#discussion_r1245413731
##########
src/tscore/CMakeLists.txt:
##########
@@ -103,6 +102,14 @@ add_library(tscore SHARED
)
add_library(ts::tscore ALIAS tscore)
+if(OPENSSL_IS_BORINGSSL)
+ target_sources(tscore PUBLIC HKDF_boringssl.cc)
+elseif(OPENSSL_IS_OPENSSL3)
+ target_sources(tscore PUBLIC HKDF_openssl3.cc)
+else()
+ target_sources(tscore PUBLIC HKDF_openssl.cc)
Review Comment:
We do. I've been fixing a few here and there, but most of it will probably have to be part of the general cleanup. Regardless, we should make sure that new ones are correct.
Part of the general problem there is that the cyclic dependencies result in usage requirements being weird.
--
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] JosiahWI commented on a diff in pull request #9924: Check openssl version and add appropriate defines
Posted by "JosiahWI (via GitHub)" <gi...@apache.org>.
JosiahWI commented on code in PR #9924:
URL: https://github.com/apache/trafficserver/pull/9924#discussion_r1245085969
##########
CMakeLists.txt:
##########
@@ -167,7 +167,14 @@ endif()
find_package(PCRE)
include(FindOpenSSL)
Review Comment:
Since you're cleaning up the OpenSSL checks, can we remove this direct inclusion of `FindOpenSSL`? `find_package` should find the right module to include for us.
##########
CMakeLists.txt:
##########
@@ -167,7 +167,14 @@ endif()
find_package(PCRE)
include(FindOpenSSL)
+include(CheckOpenSSLIsBoringSSL)
find_package(OpenSSL)
+check_openssl_is_boringssl(OPENSSL_IS_BORINGSSL "${OPENSSL_INCLUDE_DIR}")
+
+if(OPENSSL_VERSION VERSION_GREATER_EQUAL "3.0.0")
+ set(OPENSSL_IS_OPENSSL3 ON)
Review Comment:
Could we use `TRUE` for internal checks? I think it helps distinguish between options that can be toggled, and deterministic checks.
##########
CMakeLists.txt:
##########
@@ -167,7 +167,14 @@ endif()
find_package(PCRE)
include(FindOpenSSL)
+include(CheckOpenSSLIsBoringSSL)
find_package(OpenSSL)
+check_openssl_is_boringssl(OPENSSL_IS_BORINGSSL "${OPENSSL_INCLUDE_DIR}")
+
+if(OPENSSL_VERSION VERSION_GREATER_EQUAL "3.0.0")
+ set(OPENSSL_IS_OPENSSL3 ON)
+ add_compile_definitions(OPENSSL_API_COMPAT=10002 OPENSSL_IS_OPENSSL3)
Review Comment:
`./plugins/certifier/CMakeLists.txt:add_definitions(-DOPENSSL_API_COMPAT=10002)`
Interesting, this is duplicated in the certifier. That's not necessarily bad because it helps decouple, but I'm wondering whether the one in certifier should also be guarded by a conditional.
##########
src/tscore/CMakeLists.txt:
##########
@@ -103,6 +102,14 @@ add_library(tscore SHARED
)
add_library(ts::tscore ALIAS tscore)
+if(OPENSSL_IS_BORINGSSL)
+ target_sources(tscore PUBLIC HKDF_boringssl.cc)
+elseif(OPENSSL_IS_OPENSSL3)
+ target_sources(tscore PUBLIC HKDF_openssl3.cc)
+else()
+ target_sources(tscore PUBLIC HKDF_openssl.cc)
Review Comment:
Seeing a `PUBLIC` usage requirement for target sources is like seeing Santa Clause driving his sleigh in December without his coat on. Are we doing something fancy, or should these be `PRIVATE`?
--
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] cmcfarlen commented on a diff in pull request #9924: Check openssl version and add appropriate defines
Posted by "cmcfarlen (via GitHub)" <gi...@apache.org>.
cmcfarlen commented on code in PR #9924:
URL: https://github.com/apache/trafficserver/pull/9924#discussion_r1245395142
##########
CMakeLists.txt:
##########
@@ -167,7 +167,14 @@ endif()
find_package(PCRE)
include(FindOpenSSL)
+include(CheckOpenSSLIsBoringSSL)
find_package(OpenSSL)
+check_openssl_is_boringssl(OPENSSL_IS_BORINGSSL "${OPENSSL_INCLUDE_DIR}")
+
+if(OPENSSL_VERSION VERSION_GREATER_EQUAL "3.0.0")
+ set(OPENSSL_IS_OPENSSL3 ON)
+ add_compile_definitions(OPENSSL_API_COMPAT=10002 OPENSSL_IS_OPENSSL3)
Review Comment:
This is how auto tools is setup when openssl3 is found. I'm not sure if its necessary.
--
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