Re: [PR] Add mutual SSL support for pipe sinks [iotdb]
jt2594838 merged PR #18080: URL: https://github.com/apache/iotdb/pull/18080 -- 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]
Re: [PR] Add mutual SSL support for pipe sinks [iotdb]
Caideyipi commented on code in PR #18080:
URL: https://github.com/apache/iotdb/pull/18080#discussion_r3568158450
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/sink/protocol/IoTDBSslSyncSink.java:
##
@@ -90,16 +97,95 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
IOTDB_THRIFT_CONNECTOR.getPipePluginName())
.toLowerCase();
-validator.validate(
-args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
-String.format(
-"When ssl transport is enabled, %s and %s must be specified",
-SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
-
IOTDB_THRIFT_SSL_CONNECTOR.getPipePluginName().equals(userSpecifiedConnectorName)
-||
IOTDB_THRIFT_SSL_SINK.getPipePluginName().equals(userSpecifiedConnectorName)
-|| parameters.getBooleanOrDefault(SINK_IOTDB_SSL_ENABLE_KEY,
false),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY));
+validator
+.validate(
+args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
+String.format(
+"When ssl transport is enabled, specify a complete trust-store
pair under the "
Review Comment:
Thanks for catching this. Fixed in 1ca8798cad2: the trust-store and
key-store validation messages now come from the locale-specific en/zh
PipeMessages bundles, with matching %s placeholders. The default
node-commons/datanode build and PipeSinkTest (14/14) pass; the zh node-commons
compile also passes. The full zh datanode compile is currently blocked only by
a pre-existing, unrelated missing
FAILED_TO_GET_PIPE_INFO_FROM_CONFIG_NODE_STATUS constant elsewhere in the zh
bundle.
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/sink/protocol/legacy/IoTDBLegacyPipeSink.java:
##
@@ -147,13 +156,90 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
.validate(
args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
String.format(
-"When %s is specified to true, %s and %s must be specified",
+"When %s or %s is true, specify a complete trust-store pair
under the same "
Review Comment:
Thanks for catching this. Fixed in 1ca8798cad2: both legacy trust-store and
key-store validation templates now live in the en/zh DataNodePipeMessages
bundles, and IoTDBLegacyPipeSink formats those constants. Placeholder counts
match across locales (trust-store 8/8, key-store 6/6). The default build and
PipeSinkTest (14/14) pass; the full zh datanode compile reaches only the
pre-existing, unrelated missing FAILED_TO_GET_PIPE_INFO_FROM_CONFIG_NODE_STATUS
constant elsewhere.
--
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]
Re: [PR] Add mutual SSL support for pipe sinks [iotdb]
HTHou commented on code in PR #18080:
URL: https://github.com/apache/iotdb/pull/18080#discussion_r3567891769
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/sink/protocol/IoTDBSslSyncSink.java:
##
@@ -90,16 +97,95 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
IOTDB_THRIFT_CONNECTOR.getPipePluginName())
.toLowerCase();
-validator.validate(
-args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
-String.format(
-"When ssl transport is enabled, %s and %s must be specified",
-SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
-
IOTDB_THRIFT_SSL_CONNECTOR.getPipePluginName().equals(userSpecifiedConnectorName)
-||
IOTDB_THRIFT_SSL_SINK.getPipePluginName().equals(userSpecifiedConnectorName)
-|| parameters.getBooleanOrDefault(SINK_IOTDB_SSL_ENABLE_KEY,
false),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY));
+validator
+.validate(
+args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
+String.format(
+"When ssl transport is enabled, specify a complete trust-store
pair under the "
Review Comment:
[P1] Move these validation messages into the i18n bundle
These newly added validation errors are user-facing strings, but they are
hard-coded in English. Under `-P with-zh-locale`, they will still be emitted in
English, which violates the repository mandatory i18n rule. Please add matching
constants to both the `en` and `zh` `PipeMessages` files, preserving the format
specifiers, and reference those constants here. The key-store validation
message below needs the same treatment.
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/sink/protocol/legacy/IoTDBLegacyPipeSink.java:
##
@@ -147,13 +156,90 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
.validate(
args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
String.format(
-"When %s is specified to true, %s and %s must be specified",
+"When %s or %s is true, specify a complete trust-store pair
under the same "
Review Comment:
[P1] Move these validation messages into the i18n bundle
These new trust-store and key-store validation errors are hard-coded
user-facing English strings. They will bypass the locale-specific sources and
remain English in the zh build. Please define matching constants in both the
`en` and `zh` `DataNodePipeMessages` files, with identical format specifiers,
and use those constants here.
--
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]
Re: [PR] Add mutual SSL support for pipe sinks [iotdb]
Caideyipi commented on code in PR #18080:
URL: https://github.com/apache/iotdb/pull/18080#discussion_r3517854534
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/sink/protocol/IoTDBSslSyncSink.java:
##
@@ -90,16 +97,83 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
IOTDB_THRIFT_CONNECTOR.getPipePluginName())
.toLowerCase();
-validator.validate(
-args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
-String.format(
-"When ssl transport is enabled, %s and %s must be specified",
-SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
-
IOTDB_THRIFT_SSL_CONNECTOR.getPipePluginName().equals(userSpecifiedConnectorName)
-||
IOTDB_THRIFT_SSL_SINK.getPipePluginName().equals(userSpecifiedConnectorName)
-|| parameters.getBooleanOrDefault(SINK_IOTDB_SSL_ENABLE_KEY,
false),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY));
+validator
+.validate(
+args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
+String.format(
+"When ssl transport is enabled, %s and %s must be specified",
+SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
Review Comment:
Applied in ca064a0c8ce: the trust-store validation error now mentions
connector.*, sink.*, and unprefixed aliases.
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/sink/protocol/IoTDBSslSyncSink.java:
##
@@ -90,16 +97,83 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
IOTDB_THRIFT_CONNECTOR.getPipePluginName())
.toLowerCase();
-validator.validate(
-args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
-String.format(
-"When ssl transport is enabled, %s and %s must be specified",
-SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
-
IOTDB_THRIFT_SSL_CONNECTOR.getPipePluginName().equals(userSpecifiedConnectorName)
-||
IOTDB_THRIFT_SSL_SINK.getPipePluginName().equals(userSpecifiedConnectorName)
-|| parameters.getBooleanOrDefault(SINK_IOTDB_SSL_ENABLE_KEY,
false),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY));
+validator
+.validate(
+args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
+String.format(
+"When ssl transport is enabled, %s and %s must be specified",
+SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
+
IOTDB_THRIFT_SSL_CONNECTOR.getPipePluginName().equals(userSpecifiedConnectorName)
+||
IOTDB_THRIFT_SSL_SINK.getPipePluginName().equals(userSpecifiedConnectorName)
+|| parameters.getBooleanOrDefault(
+Arrays.asList(CONNECTOR_IOTDB_SSL_ENABLE_KEY,
SINK_IOTDB_SSL_ENABLE_KEY),
+false),
+hasCompleteAttributePair(
+parameters,
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PATH_KEY,
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PWD_KEY,
+SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
+SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
+hasNoHalfAttributePair(
+parameters,
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PATH_KEY,
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PWD_KEY,
+SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
+SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY))
+.validate(
+args -> (boolean) args[0] == (boolean) args[1],
+String.format(
+"%s and %s must be specified together",
+SINK_IOTDB_SSL_KEY_STORE_PATH_KEY,
SINK_IOTDB_SSL_KEY_STORE_PWD_KEY),
Review Comment:
Applied in ca064a0c8ce: the key-store pairing error now lists the accepted
connector.*, sink.*, and unprefixed aliases.
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/sink/protocol/legacy/IoTDBLegacyPipeSink.java:
##
@@ -151,9 +160,74 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
SINK_IOTDB_SSL_ENABLE_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
-parameters.getBooleanOrDefault(SINK_IOTDB_SSL_ENABLE_KEY, false),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY));
+parameters.getBooleanOrDefault(
+Arrays.asList(CONNE
Re: [PR] Add mutual SSL support for pipe sinks [iotdb]
Copilot commented on code in PR #18080:
URL: https://github.com/apache/iotdb/pull/18080#discussion_r3510069337
##
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/pipe/sink/protocol/legacy/IoTDBLegacyPipeSink.java:
##
@@ -151,9 +160,21 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
SINK_IOTDB_SSL_ENABLE_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
-parameters.getBooleanOrDefault(SINK_IOTDB_SSL_ENABLE_KEY, false),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY));
+parameters.getBooleanOrDefault(
+Arrays.asList(CONNECTOR_IOTDB_SSL_ENABLE_KEY,
SINK_IOTDB_SSL_ENABLE_KEY), false),
+parameters.hasAnyAttributes(
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
+parameters.hasAnyAttributes(
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PWD_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY))
Review Comment:
This validate() method has the same cross-alias issue as IoTDBSslSyncSink:
using hasAnyAttributes(...) allows key-store/trust-store path and password to
be split across connector.* vs sink.* (or unprefixed vs prefixed), which passes
validation but later resolves to a null password when reading via
getStringByKeys(...).
Validation should require a complete trust-store pair under the same alias,
and enforce key-store path/pwd pairing per alias (connector.*, sink.*, or
unprefixed).
##
iotdb-core/node-commons/src/main/java/org/apache/iotdb/commons/pipe/sink/protocol/IoTDBSslSyncSink.java:
##
@@ -90,16 +97,30 @@ public void validate(final PipeParameterValidator
validator) throws Exception {
IOTDB_THRIFT_CONNECTOR.getPipePluginName())
.toLowerCase();
-validator.validate(
-args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
-String.format(
-"When ssl transport is enabled, %s and %s must be specified",
-SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
-
IOTDB_THRIFT_SSL_CONNECTOR.getPipePluginName().equals(userSpecifiedConnectorName)
-||
IOTDB_THRIFT_SSL_SINK.getPipePluginName().equals(userSpecifiedConnectorName)
-|| parameters.getBooleanOrDefault(SINK_IOTDB_SSL_ENABLE_KEY,
false),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
-parameters.hasAttribute(SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY));
+validator
+.validate(
+args -> !((boolean) args[0]) || ((boolean) args[1] && (boolean)
args[2]),
+String.format(
+"When ssl transport is enabled, %s and %s must be specified",
+SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY),
+
IOTDB_THRIFT_SSL_CONNECTOR.getPipePluginName().equals(userSpecifiedConnectorName)
+||
IOTDB_THRIFT_SSL_SINK.getPipePluginName().equals(userSpecifiedConnectorName)
+|| parameters.getBooleanOrDefault(
+Arrays.asList(CONNECTOR_IOTDB_SSL_ENABLE_KEY,
SINK_IOTDB_SSL_ENABLE_KEY),
+false),
+parameters.hasAnyAttributes(
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PATH_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PATH_KEY),
+parameters.hasAnyAttributes(
+CONNECTOR_IOTDB_SSL_TRUST_STORE_PWD_KEY,
SINK_IOTDB_SSL_TRUST_STORE_PWD_KEY))
+.validate(
+args -> (boolean) args[0] == (boolean) args[1],
+String.format(
+"%s and %s must be specified together",
+SINK_IOTDB_SSL_KEY_STORE_PATH_KEY,
SINK_IOTDB_SSL_KEY_STORE_PWD_KEY),
+parameters.hasAnyAttributes(
+CONNECTOR_IOTDB_SSL_KEY_STORE_PATH_KEY,
SINK_IOTDB_SSL_KEY_STORE_PATH_KEY),
+parameters.hasAnyAttributes(
+CONNECTOR_IOTDB_SSL_KEY_STORE_PWD_KEY,
SINK_IOTDB_SSL_KEY_STORE_PWD_KEY));
Review Comment:
Current validation uses hasAnyAttributes(...) across connector.* and sink.*
keys. This can incorrectly pass when the path is provided under one prefix and
the password under the other (or unprefixed vs prefixed). In that case
validate() succeeds but customize() later resolves only one side via
getStringByKeys(...), producing a null password and failing the SSL handshake
at runtime.
To avoid this, require a complete trust-store pair under the same alias
(connector.*, sink.*, or unprefixed), and validate key-store path/pwd pairing
per alias (not “any path” vs “any pwd”).
--
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: rev
