[PLUGIN-1960] Add SMBv2/v3 support to WindowsShareCopy with legacy SMBv1 fallback#1997
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request replaces the legacy jcifs library with smbj in the WindowsShareCopy plugin to support SMBv2 and SMBv3 protocols. This involves updating dependencies in pom.xml and refactoring WindowsShareCopy.java to use SMBClient, Connection, Session, and DiskShare for file operations. The review feedback highlights a resource leak where SMBClient and Session are not properly closed, and suggests using try-with-resources to ensure proper cleanup.
| SMBClient client = new SMBClient(); | ||
| try (Connection connection = client.connect(config.netBiosHostname)) { | ||
| AuthenticationContext authContext = new AuthenticationContext( | ||
| config.netBiosUsername, | ||
| config.netBiosPassword.toCharArray(), | ||
| config.netBiosDomainName == null ? "" : config.netBiosDomainName | ||
| ); | ||
|
|
||
| Session session = connection.authenticate(authContext); | ||
|
|
||
| try (DiskShare share = (DiskShare) session.connectShare(config.netBiosSharename)) { |
There was a problem hiding this comment.
The SMBClient and Session instances are not closed, which can lead to thread leaks (since SMBClient manages its own executor service) and orphaned sessions on the SMB server. Both classes implement AutoCloseable and should be managed using try-with-resources to ensure they are properly closed.
try (SMBClient client = new SMBClient();
Connection connection = client.connect(config.netBiosHostname)) {
AuthenticationContext authContext = new AuthenticationContext(
config.netBiosUsername,
config.netBiosPassword.toCharArray(),
config.netBiosDomainName == null ? "" : config.netBiosDomainName
);
try (Session session = connection.authenticate(authContext);
DiskShare share = (DiskShare) session.connectShare(config.netBiosSharename)) {
vikasrathee-cs
changed the title
vishwasvaidya-cloudsufi
changed the title
| private void executeSMBv2(final Path hdfsDir, final FileSystem hdfs) throws Exception { | ||
| String normalizedSourcePath = config.sourceDirectory; | ||
| if (normalizedSourcePath.startsWith("/") || normalizedSourcePath.startsWith("\\")) { | ||
| normalizedSourcePath = normalizedSourcePath.substring(1); |
There was a problem hiding this comment.
in case of \, it will remove just one slash and move the normalization logic into config.getSourceDirectory() method
| </dependency> | ||
| <dependency> | ||
| <groupId>org.bouncycastle</groupId> | ||
| <artifactId>bcprov-jdk15to18</artifactId> |
There was a problem hiding this comment.
we are having jdk 8, will there be any concern while using this?
There was a problem hiding this comment.
Bouncy Castle uses a specific naming convention where the suffix indicates the supported Java versions. jdk15to18 explicitly means the artifact is compiled for and fully supports Java 1.5 through Java 1.8. Since we are on JDK 8, this is 100% compatible and exactly the intended artifact for our environment.
| <dependency> | ||
| <groupId>net.engio</groupId> | ||
| <artifactId>mbassador</artifactId> | ||
| <version>1.3.0</version> |
There was a problem hiding this comment.
make sure these libs doesn't have any vulnerabilities
2 participants
Kept the legacy jcifs (SMBv1) library to maintain backward compatibility. The plugin dynamically routes to either SMBv1 or SMBv2/v3, defaulting to the newer protocol for all new pipelines.
Key Changes:
Backward Compatibility / UI: Added an SMB Protocol Version dropdown to the UI. New pipelines default to SMBv2/v3. Crucially, if the configuration receives a null value (which happens when existing pipelines are upgraded), the plugin safely falls back to SMBv1 to prevent pipeline breakage.
Dependency Management: Retained jcifs and added smbj along with its explicitly required transitive dependencies (mbassador, asn-one, bcprov) to the pom.xml to ensure CDAP classloader compatibility.
Modern SMB Architecture: Implemented a secure, hierarchical connection lifecycle (Client -> Session -> Share) for the new smbj execution path.
Path Normalization: Added logic to convert UNIX-style forward slashes (/) to Windows-native backslashes () for the smbj share traversal.
Directory Traversal: Implemented explicit directory vs. file traversal logic for SMBv2/v3, including filtering for Windows system pointer directories (. and ..).
Safe File Operations: Applied AccessMask.GENERIC_READ during smbj file operations to prevent accidental file locking on the destination Windows server.