HPCC-32968 Add ElasticSearch server security configuration values #19310

kenrowland · 2024-11-21T21:27:57Z

Added new configuration values for ElasticSearch server security

Signed-Off-By: Kenneth Rowland [email protected]

Type of change:

This change is a bug fix (non-breaking change which fixes an issue).
This change is a new feature (non-breaking change which adds functionality).
This change improves the code (refactor or other change that does not change the functionality)
This change fixes warnings (the fix does not alter the functionality or the generated code)
This change is a breaking change (fix or feature that will cause existing behavior to change).
This change alters the query API (existing queries will have to be recompiled)

Checklist:

Smoketest:

Send notifications about my Pull Request position in Smoketest queue.
Test my draft Pull Request.

Testing:

github-actions · 2024-11-21T21:30:58Z

Jira Issue: https://hpccsystems.atlassian.net//browse/HPCC-32698

Jirabot Action Result:
Changing assignee from: [email protected] to: [email protected]
Workflow Transition To: Merge Pending
Additional PR: #19310

rpastrana

@kenrowland

rpastrana · 2024-11-22T16:11:33Z

helm/examples/metrics/README.md

+
+**Authentication**
+
+A child of the host configuration where authentication settings are defined. The settings are:


it's not clear if the authentication section is required or optional. It seems the type is optional, but if authentication is provided, the type seems necessary.

Added wording to indicate it's optional

rpastrana · 2024-11-22T16:12:48Z

helm/examples/metrics/README.md

+A child of the host configuration where authentication settings are defined. The settings are:
+
+* type - Authentication type used to connect to the ElasticSearch server. Value defines the remaining settings.
+The allowed values are (optional)


based on above comment, type should be required if authentication is specified. otherwise if authentication is required, there should be a none option here

Changed wording to indicate that type is required

rpastrana · 2024-12-02T16:36:56Z

system/metrics/sinks/elastic/elasticSink.cpp

+
+    // Get authentication settings, if present
+    Owned<IPropertyTree> pAuthConfigTree = pSettingsTree->getPropTree("authentication");
+    if (pAuthConfigTree)


if !pAuthConfigTree could be handled the same way pAuthConfigTree handles it in L153

If the tree is null, then no processing is needed. It seemed easier to read the positive case where the tree exists

in terms of readability, returning false if !pAuthConfigTree is easier to read. as it stands, we don't know how that case is handled until the end of the if block

I changed and pushed a new commit. However, if the auth config tree is missing, true is returned, not false since it's not an error.

system/metrics/sinks/elastic/elasticSink.cpp

rpastrana · 2024-12-02T16:49:43Z

system/metrics/sinks/elastic/elasticSink.cpp

+            if (secretTree == nullptr)
+            {
+                WARNLOG("ElasticMetricSink: Unable to load secret tree '%s', from vault '%s'", credentialsSecretKey.str(),
+                        !credentialsVaultId.isEmpty() ? credentialsVaultId.str() : "n/a");


the n/a seems fine, but it seems to differ from other similar cases where and empty string is provided in the output.

I felt that an empty string was not sufficient because it required knowledge that absence of a value implies that no vault ID was provided. It could be confusing. Using "n/a" tells the user that the vault ID is not applicable.

IF the error is that the vault id is empty, the single quoted empty string is most appropriate.
Update:
Unless, you're saying the vaultID doesn't matter in this scenario

Absence of a vault ID is not an error. Absence of username and/or password is the error. The vault ID is informational to tell the user if attempting to retrieve from a vault (and which vault of course)

ok, that seems more confusing to me. I wish we had a usability team to weigh in on these types of issues developers are typically not the best to judge.

The API to retrieve the secret has an optional vault parameter. If present, the secret is retrieved from the vault. If absent, the secret is treated as a regular Kubernetes secret. Two different APIs would have been better. So, the vault is included in the log message to indicate if the retrieval was from a vault or not, 'n/a' in the latter case.

system/metrics/sinks/elastic/elasticSink.cpp

kenrowland · 2024-12-05T14:44:21Z

@rpastrana I have amended the last commit with changes as discussed.

rpastrana

seems ok

kenrowland · 2024-12-05T16:05:56Z

@ghalliday Please merge

ghalliday

A couple of trivial comments. Main question is whether passwords provided in the configuration file should be supported at all. There is a danger of promoting bad practices.

ghalliday · 2024-12-09T16:24:27Z

helm/examples/metrics/README.md

 <Environment>
    <Software>
        <metrics name="mymetricsconfig">
            <sinks name="myelasticsink" type="elastic">
                <settings period="30" ignoreZeroMetrics="1">
-                    <host name="<hostname>" port="<port>" protocol="http|htps"/>
+                    <host domain="<domainname>" port="<port>" protocol="http|htps">


ghalliday · 2024-12-09T16:26:14Z

helm/examples/metrics/elasticsearch_metrics.yaml

+#   settings.host.authentication          - authentication settings if authentication is enabled (optional)
+#   settings.host.authentication.type     - authentication type (determines remaining settings) (only 'basic' is supported)
+#   settings.host.authentication.username - username for basic authentication (if not stored in a secret)
+#   settings.host.authentication.password - password for basic authentication (if not stored in a secret)


Why is this supported? The password would be accessible in plain test.

The code treats the password as encrypted using the same mechanism as when read from the environment.xml. If you feel the containerized version should always have the value in a secret then I can remove it. Should it be decided it can be provided directly, I updated the comment to reflect that it is encrypted.

Please advise if it should be removed in favor of always being a secret (see comment below on always loading as a secret)

ghalliday · 2024-12-09T16:30:31Z

helm/examples/metrics/elasticsearch_metrics.yaml

+#   settings.host.authentication.username - username for basic authentication (if not stored in a secret)
+#   settings.host.authentication.password - password for basic authentication (if not stored in a secret)
+#   settings.host.authentication.credentialsSecret  - name of secret containing username and password for basic authentication
+#   settings.host.authentication.credentialsVaultId - vault id for secret containing username password for basic authentication


Added optional

ghalliday · 2024-12-09T16:32:33Z

system/metrics/sinks/elastic/elasticSink.cpp

    }
    else
    {
-        WARNLOG("ElasticMetricSink: Host configuration missing or invalid");
+        // if basic auth, username and password are stored directly in the configuration


should require configuration via a secret

If a secret is required for the username and password, how would that work in a bare metal environment? Is it possible to load secrets for bare metal?

Yes secrets can be supplied in bare-metal environments, either by placing a file in the correct directory, or by using hashicorp vault.

ghalliday

Please squash. I will merge as is, but in general we should not bee supporting passwords in configuration.

Added new configuration values for ElasticSearch server security Signed-Off-By: Kenneth Rowland [email protected]

kenrowland · 2024-12-11T15:13:12Z

@ghalliday squashed. Please merge

kenrowland force-pushed the HPCC-32968 branch from b145e1b to 20b4ea5 Compare November 21, 2024 22:20

kenrowland changed the title ~~HPCC-32698 Add ElasticSearch server security configuration values~~ HPCC-32968 Add ElasticSearch server security configuration values Nov 21, 2024

kenrowland force-pushed the HPCC-32968 branch from 20b4ea5 to 625b88f Compare November 21, 2024 22:44

kenrowland requested a review from rpastrana November 22, 2024 14:59

rpastrana requested changes Dec 2, 2024

View reviewed changes

kenrowland requested a review from rpastrana December 2, 2024 19:40

kenrowland force-pushed the HPCC-32968 branch from d80deec to b4abfb1 Compare December 5, 2024 14:43

rpastrana approved these changes Dec 5, 2024

View reviewed changes

kenrowland force-pushed the HPCC-32968 branch from b4abfb1 to 6566d44 Compare December 9, 2024 16:01

ghalliday reviewed Dec 9, 2024

View reviewed changes

kenrowland requested a review from ghalliday December 9, 2024 19:17

ghalliday approved these changes Dec 11, 2024

View reviewed changes

HPCC-32968 Add ElasticSearch server security configuration values

400d590

Added new configuration values for ElasticSearch server security Signed-Off-By: Kenneth Rowland [email protected]

kenrowland force-pushed the HPCC-32968 branch from a90db54 to 400d590 Compare December 11, 2024 15:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HPCC-32968 Add ElasticSearch server security configuration values #19310

HPCC-32968 Add ElasticSearch server security configuration values #19310

kenrowland commented Nov 21, 2024 •

edited

Loading

github-actions bot commented Nov 21, 2024

rpastrana left a comment

rpastrana Nov 22, 2024

kenrowland Dec 2, 2024

rpastrana Nov 22, 2024

kenrowland Dec 2, 2024

rpastrana Dec 2, 2024

kenrowland Dec 2, 2024

rpastrana Dec 2, 2024

kenrowland Dec 2, 2024

rpastrana Dec 2, 2024

kenrowland Dec 2, 2024

rpastrana Dec 2, 2024 •

edited

Loading

kenrowland Dec 2, 2024

rpastrana Dec 2, 2024

kenrowland Dec 3, 2024

kenrowland commented Dec 5, 2024

rpastrana left a comment

kenrowland commented Dec 5, 2024

ghalliday left a comment

ghalliday Dec 9, 2024

kenrowland Dec 9, 2024

ghalliday Dec 9, 2024

kenrowland Dec 9, 2024

ghalliday Dec 9, 2024

kenrowland Dec 9, 2024

ghalliday Dec 9, 2024

kenrowland Dec 9, 2024

ghalliday Dec 11, 2024

ghalliday left a comment

kenrowland commented Dec 11, 2024


		Authentication

		A child of the host configuration where authentication settings are defined. The settings are:

HPCC-32968 Add ElasticSearch server security configuration values #19310

Are you sure you want to change the base?

HPCC-32968 Add ElasticSearch server security configuration values #19310

Conversation

kenrowland commented Nov 21, 2024 • edited Loading

Type of change:

Checklist:

Smoketest:

Testing:

github-actions bot commented Nov 21, 2024

rpastrana left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rpastrana Dec 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenrowland commented Dec 5, 2024

rpastrana left a comment

Choose a reason for hiding this comment

kenrowland commented Dec 5, 2024

ghalliday left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghalliday left a comment

Choose a reason for hiding this comment

kenrowland commented Dec 11, 2024

kenrowland commented Nov 21, 2024 •

edited

Loading

rpastrana Dec 2, 2024 •

edited

Loading