HTHou commented on PR #17973:
URL: https://github.com/apache/iotdb/pull/17973#issuecomment-4727643042
English version of my review above:
I think this PR should not be merged yet. The main blocker is that the
current fallback/packaging path distributes OpenSSL 1.1.1 code in the C++ SDK
binary/package.
1. **The OpenSSL 1.1.1w fallback is statically linked into the SDK binary.**
`FetchOpenSSL.cmake` builds OpenSSL 1.1.1w with `./config ... no-shared`,
then links it into `iotdb_session`. OpenSSL 1.1.1 is not Apache-2.0 licensed.
OpenSSL states that 3.0+ is Apache-2.0, while pre-3.0 releases use the old
OpenSSL/SSLeay license: https://openssl-library.org/source/license/. This means
the current implementation would distribute pre-3.0 OpenSSL code inside an ASF
convenience binary. ASF third-party licensing policy says incompatible
components must not be distributed in ASF source releases or convenience
binaries: https://www.apache.org/legal/resolved.html. Please avoid statically
linking or redistributing OpenSSL 1.1.1 unless the PMC/legal path explicitly
approves it. Safer options are to rely on user/system-provided OpenSSL as an
optional dependency, or to make the Thrift/OpenSSL path work with OpenSSL 3.x.
2. **Bundling shared OpenSSL libraries has the same distribution concern.**
`InstallOpenSSLRuntime.cmake` copies `libssl`/`libcrypto` into the SDK
`lib/` directory. If those are OpenSSL 1.1.x libraries, the ASF package is
still redistributing pre-3.0 OpenSSL binaries. This cannot be fixed merely by
listing OpenSSL as Apache License 2.0 in `DEPENDENCIES.md`.
3. **The OpenSSL license entry in `DEPENDENCIES.md` is inaccurate.**
The current entry says `OpenSSL | 1.x ... | Apache License 2.0`, but
Apache-2.0 applies to OpenSSL 3.0+, not 1.1.1w. If any OpenSSL 1.1 usage
remains, the metadata and LICENSE/NOTICE handling need to reflect the
OpenSSL/SSLeay license correctly.
4. **The CMake fallback can still mix OpenSSL 3 targets with the 1.1.1
fallback.**
`find_package(OpenSSL QUIET)` runs first. If it finds OpenSSL 3.x, CMake
may already create `OpenSSL::SSL` and `OpenSSL::Crypto` imported targets.
Unsetting cache variables later does not remove or recreate those targets. The
result can be that the fallback builds 1.1.1w, but `target_link_libraries(...
OpenSSL::SSL OpenSSL::Crypto)` still links the previously created 3.x targets.
Please avoid creating the 3.x targets in the first place, or explicitly reset
target properties and normal variables after selecting the fallback.
5. **The Linux package is not fully self-contained without a loader path.**
Placing `libssl`/`libcrypto` next to `libiotdb_session.so` does not
guarantee the dynamic loader will find them. I do not see an `$ORIGIN`/relative
install RPATH on `iotdb_session`. If shared OpenSSL bundling remains, please
set the installed loader path and add a smoke test that runs on a host without
system OpenSSL 1.1 installed.
Also, OpenSSL 1.1.1 reached EOL on 2023-09-11 and no longer receives public
security fixes: https://openssl-corporation.org/post/2023-09-11-eol-111/.
Making it the default SSL runtime/fallback should be an explicit project-level
decision.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
