nzw921rx commented on PR #12297: URL: https://github.com/apache/seatunnel/pull/12297#issuecomment-5771666816
### The SMB dependency graph is bundled but not isolated The license metadata has now been added, but passing the dependency-license check does not verify runtime dependency isolation. `connector-file-smb` inherits `maven-shade-plugin`, so `smbj` and its transitive dependencies are bundled into the connector JAR. However, the `connector-file` parent only relocates Avro, ORC, and Parquet packages. There is no relocation for the dependencies introduced by SMBJ: - `com.hierynomus:smbj` - `com.hierynomus:asn-one` - `net.engio:mbassador` - `org.bouncycastle:bcprov-jdk18on` As a result, their classes remain under the original `com.hierynomus.*`, `net.engio.mbassy.*`, and `org.bouncycastle.*` namespaces. This creates an uber JAR, but does not provide dependency isolation. This is particularly risky for Bouncy Castle. The SeaTunnel distribution already records `bcprov-lts8on:2.73.9`, while SMBJ introduces `bcprov-jdk18on:1.75`. Both expose classes under `org.bouncycastle.*`. SeaTunnel uses child-first plugin classloaders, and multiple source/transform connector JARs may share the same classloader. If another loaded connector contains a different Bouncy Castle version, classpath order may determine which implementation is used, potentially causing: - `NoSuchMethodError` - linkage or classloading failures - unexpected security-provider behavior JCA provider registration may also have JVM-global effects. Could you please define an explicit isolation strategy for the dependencies introduced by this connector? Suggested approach: 1. Relocate connector-private packages such as `com.hierynomus.*` and `net.engio.mbassy.*` into a connector-specific namespace. 2. Do not relocate `org.bouncycastle.*` blindly. Prefer excluding SMBJ's `bcprov-jdk18on:1.75` and using a project-managed Bouncy Castle version if it is compatible. 3. If the existing version is not compatible, use a dedicated shaded dependency or another explicitly isolated solution, and verify provider/service loading after relocation. 4. Add a classloading or integration regression that loads the SMB connector alongside the Bouncy Castle version already present in the distribution and exercises SMB authentication, signing, or encryption. The existing `LICENSE` and `NOTICE` entries must remain regardless of whether these dependencies are relocated, because relocation does not change their licensing obligations. I consider this a merge blocker until the Bouncy Castle version and classloader interaction are either isolated or demonstrated to be safe. -- 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]
