fgerlits commented on code in PR #1806:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1806#discussion_r1674182950
##########
encrypt-config/EncryptConfigMain.cpp:
##########
@@ -48,6 +48,15 @@ int main(int argc, char* argv[]) try {
argument_parser.add_argument("--property-value")
.metavar("VALUE")
.help(minifi::utils::string::join_pack("The new value of the sensitive
property (", OPERATION_FLOW_CONFIG, " only)"));
+ argument_parser.add_argument("--parameter-context-id")
+ .metavar("PARAMETER_CONTEXT_ID")
+ .help(minifi::utils::string::join_pack("Parameter context id (",
OPERATION_FLOW_CONFIG, " only)"));
Review Comment:
I think `ID` would be enough as the metavar, otherwise the help text will be
too long:
```
--parameter-context-id PARAMETER_CONTEXT_ID Parameter context id
(flow-config only)
```
vs
```
--parameter-context-id ID Parameter context id (flow-config
only)
```
##########
encrypt-config/FlowConfigEncryptor.cpp:
##########
@@ -82,58 +100,59 @@ std::vector<SensitiveProperty>
listSensitiveProperties(const minifi::core::Proce
gsl_Expects(controller_service_node);
const auto* controller_service =
controller_service_node->getControllerServiceImplementation();
gsl_Expects(controller_service);
+ auto props = controller_service->getProperties();
if (processed_controller_services.contains(controller_service->getUUID()))
{
continue;
}
processed_controller_services.insert(controller_service->getUUID());
- for (const auto& [_, property] : controller_service->getProperties()) {
+ for (const auto& [_, property] : props) {
Review Comment:
why did this change?
##########
encrypt-config/FlowConfigEncryptor.cpp:
##########
@@ -82,58 +100,59 @@ std::vector<SensitiveProperty>
listSensitiveProperties(const minifi::core::Proce
gsl_Expects(controller_service_node);
const auto* controller_service =
controller_service_node->getControllerServiceImplementation();
gsl_Expects(controller_service);
+ auto props = controller_service->getProperties();
if (processed_controller_services.contains(controller_service->getUUID()))
{
continue;
}
processed_controller_services.insert(controller_service->getUUID());
- for (const auto& [_, property] : controller_service->getProperties()) {
+ for (const auto& [_, property] : props) {
if (property.isSensitive()) {
- sensitive_properties.push_back(SensitiveProperty{
+ sensitive_items.push_back(SensitiveItem{
.component_type = ComponentType::ControllerService,
.component_id = controller_service->getUUID(),
.component_name = controller_service->getName(),
- .property_name = property.getName(),
- .property_display_name = property.getDisplayName(),
- .property_value = property.getValue().to_string()});
+ .item_name = property.getName(),
+ .item_display_name = property.getDisplayName(),
+ .item_value = property.getValue().to_string()});
}
}
}
- return sensitive_properties;
+ return sensitive_items;
}
-std::unordered_map<minifi::utils::Identifier, minifi::core::flow::Overrides>
createOverridesInteractively(const std::vector<SensitiveProperty>&
sensitive_properties) {
+std::unordered_map<minifi::utils::Identifier, minifi::core::flow::Overrides>
createOverridesInteractively(const std::vector<SensitiveItem>& sensitive_items)
{
std::unordered_map<minifi::utils::Identifier, minifi::core::flow::Overrides>
overrides;
std::cout << '\n';
- for (const auto& sensitive_property : sensitive_properties) {
- std::cout << magic_enum::enum_name(sensitive_property.component_type) << "
" << sensitive_property.component_name << " (" <<
sensitive_property.component_id.to_string() << ") "
- << "has sensitive property " <<
sensitive_property.property_display_name << "\n enter a new value or press
Enter to keep the current value unchanged: ";
+ for (const auto& sensitive_item : sensitive_items) {
+ std::cout << magic_enum::enum_name(sensitive_item.component_type) << " "
<< sensitive_item.component_name << " (" <<
sensitive_item.component_id.to_string() << ") "
+ << "has sensitive item " << sensitive_item.item_display_name <<
"\n enter a new value or press Enter to keep the current value unchanged: ";
std::cout.flush();
std::string new_value;
std::getline(std::cin, new_value);
if (!new_value.empty()) {
-
overrides[sensitive_property.component_id].add(sensitive_property.property_name,
new_value);
+ overrides[sensitive_item.component_id].add(sensitive_item.item_name,
new_value);
}
}
return overrides;
}
-std::unordered_map<minifi::utils::Identifier, minifi::core::flow::Overrides>
createOverridesForSingleProperty(
- const std::vector<SensitiveProperty>& sensitive_properties, const
std::string& component_id, const std::string& property_name, const std::string&
property_value) {
- const auto sensitive_property_it =
std::ranges::find_if(sensitive_properties, [&](const auto& sensitive_property) {
- return sensitive_property.component_id.to_string().view() == component_id
&& (sensitive_property.property_name == property_name ||
sensitive_property.property_display_name == property_name);
+std::unordered_map<minifi::utils::Identifier, minifi::core::flow::Overrides>
createOverridesForSingleItem(
+ const std::vector<SensitiveItem>& sensitive_items, const std::string&
component_id, const std::string& item_name, const std::string& item_value) {
+ const auto sensitive_item_it = std::ranges::find_if(sensitive_items,
[&](const auto& sensitive_item) {
+ return sensitive_item.component_id.to_string().view() == component_id &&
(sensitive_item.item_name == item_name || sensitive_item.item_display_name ==
item_name);
});
- if (sensitive_property_it == sensitive_properties.end()) {
- std::cout << "No sensitive property found with this component ID and
property name.\n";
+ if (sensitive_item_it == sensitive_items.end()) {
+ std::cout << "No sensitive item found with this component ID and item
name.\n";
Review Comment:
Since we never say "item" in the help text, I would keep this term internal,
and use "property or parameter" in user-facing messages.
```suggestion
std::cout << "No sensitive property or parameter found with this
component ID and name.\n";
```
##########
libminifi/src/core/ParameterTokenParser.cpp:
##########
@@ -85,21 +86,31 @@ std::string
ParameterTokenParser::replaceParameters(ParameterContext* parameter_
if (!parameter_context) {
throw ParameterException("Property references a parameter in its value,
but no parameter context was provided.");
}
-
gsl_Assert(token->getName().has_value());
auto parameter = parameter_context->getParameter(token->getName().value());
if (!parameter.has_value()) {
throw ParameterException("Parameter '" + token->getName().value() + "'
not found");
}
- if (is_sensitive) {
- throw ParameterException("Non-sensitive parameter '" + parameter->name +
"' cannot be referenced in a sensitive property");
- }
- result.append(std::string(token->getAdditionalHashmarks(), '#') +
parameter->value);
+ result.append(std::string(token->getAdditionalHashmarks(), '#') +
getRawParameterValue(*parameter));
last_end = token->getStart() + token->getSize();
}
result.append(input_.substr(last_end));
return result;
}
+std::string NonSensitiveParameterTokenParser::getRawParameterValue(const
Parameter& parameter) const {
+ if (parameter.sensitive) {
+ throw ParameterException("Sensitive parameter '" + parameter.name + "'
cannot be referenced in a non-sensitive property");
+ }
+ return parameter.value;
+}
+
+std::string SensitiveParameterTokenParser::getRawParameterValue(const
Parameter& parameter) const {
+ if (!parameter.sensitive) {
+ throw ParameterException("Non-sensitive parameter '" + parameter.name + "'
cannot be referenced in a sensitive property");
+ }
+ return utils::crypto::property_encryption::decrypt(parameter.value,
sensitive_values_encryptor_);
+}
Review Comment:
Why can't we reference a non-sensitive parameter from a sensitive property?
(Or even the other way round?)
##########
libminifi/include/core/ParameterTokenParser.h:
##########
@@ -131,5 +138,27 @@ class ParameterTokenParser {
std::vector<std::unique_ptr<ParameterToken>> tokens_;
};
+class NonSensitiveParameterTokenParser : public ParameterTokenParser {
+ public:
+ using ParameterTokenParser::ParameterTokenParser;
+
+ protected:
+ std::string getRawParameterValue(const Parameter& parameter) const override;
+};
+
+class SensitiveParameterTokenParser : public ParameterTokenParser {
+ public:
+ SensitiveParameterTokenParser(std::string input,
utils::crypto::EncryptionProvider& sensitive_values_encryptor)
+ : ParameterTokenParser(std::move(input)),
+ sensitive_values_encryptor_(sensitive_values_encryptor) {
+ }
+
+ protected:
+ std::string getRawParameterValue(const Parameter& parameter) const override;
+
+ private:
+ utils::crypto::EncryptionProvider& sensitive_values_encryptor_;
Review Comment:
could this be a `const &` (also in the constructor)?
##########
libminifi/src/core/json/JsonFlowSerializer.cpp:
##########
@@ -68,18 +68,42 @@ void
JsonFlowSerializer::encryptSensitiveProperties(rapidjson::Value& property_j
}
}
-std::string JsonFlowSerializer::serialize(const core::ProcessGroup&
process_group, const core::flow::FlowSchema& schema, const
utils::crypto::EncryptionProvider& encryption_provider,
- const std::unordered_map<utils::Identifier, core::flow::Overrides>&
overrides) const {
- gsl_Expects(schema.root_group.size() == 1 && schema.identifier.size() == 1 &&
- schema.processors.size() == 1 && schema.processor_properties.size() == 1
&&
- schema.controller_services.size() == 1 &&
schema.controller_service_properties.size() == 1);
+void JsonFlowSerializer::encryptSensitiveParameters(rapidjson::Value&
flow_definition_json, rapidjson::Document::AllocatorType& alloc, const
core::flow::FlowSchema& schema,
+ const utils::crypto::EncryptionProvider& encryption_provider, const
std::unordered_map<utils::Identifier, core::flow::Overrides>& overrides) const {
+ if (!flow_definition_json.HasMember(schema.parameter_contexts[0])) {
+ return;
+ }
- rapidjson::Document doc;
- auto alloc = doc.GetAllocator();
- rapidjson::Value flow_definition_json;
- flow_definition_json.CopyFrom(flow_definition_json_, alloc);
- auto& root_group = getMember(flow_definition_json, schema.root_group[0]);
+ auto parameter_contexts = getMember(flow_definition_json,
schema.parameter_contexts[0]).GetArray();
+ for (auto& parameter_context : parameter_contexts) {
+ for (auto& parameter : getMember(parameter_context,
schema.parameters[0]).GetArray()) {
+ bool is_sensitive = getMember(parameter, schema.sensitive[0]).GetBool();
+ if (!is_sensitive) {
+ continue;
+ }
+ const std::string parameter_context_id_str{getMember(parameter_context,
schema.identifier[0]).GetString(), getMember(parameter_context,
schema.identifier[0]).GetStringLength()};
+ const auto parameter_context_id =
utils::Identifier::parse(parameter_context_id_str);
+ if (!parameter_context_id) {
+ logger_->log_warn("Invalid parameter context ID found in the flow
definition: {}", parameter_context_id_str);
+ continue;
+ }
+
+ std::string parameter_value{getMember(parameter,
schema.value[0]).GetString(), getMember(parameter,
schema.value[0]).GetStringLength()};
+ if (overrides.contains(*parameter_context_id)) {
+ const auto& override_values = overrides.at(*parameter_context_id);
+ const auto parameter_name = getMember(parameter,
schema.name[0]).GetString();
Review Comment:
we should use `GetStringLength()` here, too, as elsewhere (eg. at line 91
above)
##########
extensions/standard-processors/tests/unit/FlowJsonTests.cpp:
##########
@@ -789,4 +808,118 @@ TEST_CASE("Dynamic properties can use parameters") {
CHECK(value == "value1");
}
+TEST_CASE("Test sensitive parameters in sensitive properties") {
+ ConfigurationTestController test_controller;
+ auto context = test_controller.getContext();
+ auto encrypted_parameter_value =
minifi::utils::crypto::property_encryption::encrypt("value1",
*context.sensitive_values_encryptor);
+ auto encrypted_sensitive_property_value =
minifi::utils::crypto::property_encryption::encrypt("#{my_value}",
*context.sensitive_values_encryptor);
+ core::flow::AdaptiveConfiguration config(context);
+
+ static const std::string CONFIG_JSON =
+ fmt::format(R"(
+{{
+ "parameterContexts": [
+ {{
+ "identifier": "721e10b7-8e00-3188-9a27-476cca376978",
+ "name": "my-context",
+ "description": "my parameter context",
+ "parameters": [
+ {{
+ "name": "my_value",
+ "description": "",
+ "sensitive": true,
+ "value": "{}"
+ }}
+ ]
+ }}
+ ],
+ "rootGroup": {{
+ "name": "MiNiFi Flow",
+ "processors": [{{
+ "identifier": "00000000-0000-0000-0000-000000000001",
+ "name": "MyProcessor",
+ "type": "org.apache.nifi.processors.DummyFlowJsonProcessor",
+ "schedulingStrategy": "TIMER_DRIVEN",
+ "schedulingPeriod": "3 sec",
+ "properties": {{
+ "Sensitive Property": "{}"
+ }}
+ }}],
+ "parameterContextName": "my-context"
Review Comment:
Is "parameterContextName" something we inherited from NiFi? Since the
parameter context has a unique ID, `"parameterContext":
"721e10b7-8e00-3188-9a27-476cca376978"` seems better, and more in line with our
existing schema.
--
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]
