DanielLeens commented on PR #12293: URL: https://github.com/apache/seatunnel/pull/12293#issuecomment-5812767483
Thanks for the thorough follow-up, @SEZ9 — I re-checked the two credential-handling points directly against the current head (`af472b8f13cc`) before replying: - **Issue 1 (trim mismatch)**: confirmed. `ADLSConfigValidator.required()` returns `value.trim()` and validates that trimmed value, but `ADLSHadoopConf.buildWithReadOnlyConfig()` (`ADLSHadoopConf.java:64-66`) re-reads the raw, untrimmed `config.get(...)` for `account_name`/`container`/`endpoint_suffix`/`account_key`/`tenant_id`/`client_*`. So a value with leading/trailing whitespace can pass validation and then either blow up with an unmapped `IllegalArgumentException` from `ADLSRuntimeCompatibility.validateDnsLabel`, or, worse, silently carry the whitespace into `account_key`. Worth fixing — have `validate()` hand back the normalized values, or trim at the `config.get` sites in `ADLSHadoopConf`. - **Issue 4 (authority_host scheme not validated)**: also confirmed. `normalizeAuthorityHost()` (`ADLSRuntimeCompatibility.java:166-173`) only strips trailing slashes and checks non-blank; it never enforces `https`. Since this connector already hard-codes `abfss` specifically to keep credentials off plain HTTP (`ADLSHadoopConf.java:33`), the same rule should apply to the OAuth token endpoint — an `authority_host = "http://..."` would let `ClientCredsTokenProvider` POST `client_secret` in cleartext. Of this whole batch, this is the one I'd rate as a real security gap rather than a doc/robustness nit. Issues 2/3/5/6/7 (HNS prerequisite for exactly-once, undocumented validation rules + template placeholder, missing option rows, log-mask scope, wrong `en` defaults for `discovery_mode`/`start_mode`) all check out as accurate doc/consistency gaps against the current `ADLSFileSinkFactory`/`ADLSFileSourceFactory`/`ConfigShadeUtils`/docs. Agreed on all of them. None of these touch the runtime path I already exercised in my approval (filesystem construction, transactional commit, the CI run on this exact head), and they don't change my read that the connector is structurally sound and safe to land as a first cut — the earlier hidden test-mode switch I flagged in an earlier round is also confirmed gone from `ADLSHadoopConf` on this head. That said, Issue 4 in particular is worth closing rather than treating as pure housekeeping. @sudeephazra, happy to take another look once you have a commit addressing it. -- 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]
