Block updates to log level for org.apache.http if less specific than INFO #105020

mosche · 2024-02-01T14:27:52Z

The apache http client supports logging detailed information about requests, including authorization information, if logging
at DEBUG, TRACE, ... . This PR prevents setting the log level to anything less specific than INFO for a number of restricted loggers (currently org.apache.http only).

There's a couple of ways to do so:

Setting the level directly, e.g. for org.apache.http or org.apache.http.client: This might trigger a validation error when attempting to update the setting.
Setting the level indirectly through an ancestor, e.g. org.apache or simply the root logger. If necessary the restriction will be enforced by explicitly setting restricted loggers to INFO level.
And finally, if setting the default level to anything less specific than INFO. This case is similar to above.

…INFO.

elasticsearchmachine · 2024-02-01T14:28:17Z

Pinging @elastic/es-core-infra (Team:Core/Infra)

...aRestTest/java/org/elasticsearch/repositories/blobstore/testkit/S3SnapshotRepoTestKitIT.java

ldematte

Looks good, but it seems you need to adjust some tests/assertions/mocks (e.g. OpenIdConnectAuthenticatorTests)

…org.apache.http logger

rjernst

This looks pretty good. I left a few nits.

server/src/test/java/org/elasticsearch/common/logging/LoggersTests.java

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

test/framework/src/main/java/org/elasticsearch/common/logging/TestLoggers.java

server/src/main/java/org/elasticsearch/common/logging/Loggers.java

DaveCTurner

See this comment: if org.elasticsearch.transport.NetworkTraceFlag#TRACE_ENABLED is set (i.e. -Des.insecure_network_trace_enabled=true) then we should still permit this level of detailed logging.

Also this definitely needs to be mentioned in the docs, it's going to cause surprises otherwise.

Also should we block the AWS SDK request logging mentioned here (unless -Des.insecure_network_trace_enabled=true)?

elasticsearch/docs/reference/snapshot-restore/repository-s3.asciidoc

Lines 565 to 567 in 552d2f5

    
           from the logs emitted by the storage system, configure {es} to log every 
        
           request it makes to the S3 API by <<configuring-logging-levels,setting the 
        
           logging level>> of the `com.amazonaws.request` logger to `DEBUG`:

Also we link to some AWS docs that specifically say to enable wire tracing here, so should also mention that this requires -Des.insecure_network_trace_enabled=true:

elasticsearch/docs/reference/snapshot-restore/repository-s3.asciidoc

Lines 583 to 586 in 552d2f5

    
           problem. See the 
        
           https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/java-dg-logging.html[AWS Java SDK] 
        
           documentation for further information, including details about other loggers 
        
           that can be used to obtain even more verbose logs. When you have finished

… is set

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

mosche · 2024-02-05T13:45:08Z

Also this definitely needs to be mentioned in the docs, it's going to cause surprises otherwise.

@DaveCTurner WDYT, are the changes to the docs sufficient or would you like me to be more specific. Can you think of any other place where this should be mentioned?

DaveCTurner

Docs changes look good; however I think we can't simply start rejecting previously-valid settings for bwc reasons.

server/src/main/java/org/elasticsearch/common/logging/Loggers.java

server/src/test/java/org/elasticsearch/common/logging/LoggersTests.java

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java

DaveCTurner · 2024-02-05T14:28:23Z

I think it's ok to reject attempts to configure these loggers via the API, or to start a node with an invalid (static) config, and also to ignore any invalid (dynamic) config, we just can't reject the config as invalid if it's already there in the cluster state.

rjernst · 2024-02-05T15:33:57Z

we just can't reject the config as invalid if it's already there in the cluster state.

We could ignore the cluster state in this case, converting the typical error to a warning log message?

mosche · 2024-02-06T12:24:27Z

we just can't reject the config as invalid if it's already there in the cluster state.

We could ignore the cluster state in this case, converting the typical error to a warning log message?

Just verifying I understood this right, so in case the cluster state already contains a persistent setting violating some logger restrictions (e.g. org.apache.http at DEBUG) we log a warning that the logger configuration is ignored.
If, however, a user wants to set org.apache.http to DEBUG using the cluster settings API or start a node with that setting, it fails.

Any recommendations how to best approach this? It looks like that requires a Settings update consumer that is aware of the current cluster metadata, so make that a ClusterStateListener and hook it up in NodeConstruction? Or is there a simpler, less intrusive way to achieve this?

…eNotEnabled to test restricted loggers

mark-vieira · 2024-02-12T16:24:10Z

server/build.gradle

@@ -151,8 +151,19 @@ if (BuildParams.isSnapshotBuild() == false) {
    }
 }

+tasks.register('testNetworkTraceNotEnabled', Test) {


I'm not a huge fan of introducing another test task here. What exaclty does the es.insecure_network_trace_enabled system property do?

@mark-vieira I'm absolutely on the same page with you, I just don't see much of an alternative. Also, I admit, having test depend on testNetworkTraceNotEnabled is broken. If you run :server:test --tests XYZ the dependency would still run ...

es.insecure_network_trace_enabled sets this flag, which gates the behavior added in this PR. Goal of the PR is to prevent setting dangerous loggers that might leak sensitive information. However, in order to investigate test failures, we are enabling this flag for most tests including :server:test

public class NetworkTraceFlag { private NetworkTraceFlag() { // no instances; } public static final String PROPERTY_NAME = "es.insecure_network_trace_enabled"; public static final boolean TRACE_ENABLED; static { final var propertyValue = System.getProperty(PROPERTY_NAME); if (propertyValue == null) { TRACE_ENABLED = false; } else if ("true".equals(propertyValue)) { TRACE_ENABLED = true; } else { throw new IllegalArgumentException( Strings.format("system property [%s] may only be set to [true], but was [%s]", PROPERTY_NAME, propertyValue) ); } } }

Hmm, is there anyway we can finagle this in a way that would allow updating this property inside specific tests? Initializing statics makes this tricky but perhaps some way to "reinitialize" this flag dynamically?

We definitely don't want test tasks depending on other test tasks. At worst we'd have check depend on this additional task.

Hmm, is there anyway we can finagle this in a way that would allow updating this property inside specific tests? Initializing statics makes this tricky but perhaps some way to "reinitialize" this flag dynamically?

IMHO that would pretty much defeat the purpose of that flag.

An alternative to a separate test task (using a different configuration) might be to extend the test runner to support forking tests into a new JVM. Though, while convenient, something like would probably be quickly adopted all around with a terrible impact on test run performance :/

I wonder if we're overthinking this. AIUI we are trying to run the entire test suite twice, to ensure coverage of both states of this flag. That seems unnecessary to me, I think we only really need to run a few specific tests with tracing on (or off) and the rest of the test suite needn't care either way. We could make a dedicated suite in /qa for the otherwise-untested state. Or we could randomize it in :server:test and have two dedicated suites in /qa just for the tests that need it switched on or off.

Note that we have some other :qa suites that are similarly tightly-focussed, e.g. :qa:unconfigured-node-name and :qa:custom-rest-controller.

AIUI we are trying to run the entire test suite twice, to ensure coverage of both states of this flag. That seems unnecessary to me, I think we only really need to run a few specific tests with tracing on (or off) and the rest of the test suite needn't care either way.

Only the three test cases below are included when running testNetworkTraceNotEnabled. And none of these will run during test as their assumption fails. Though, we're obviously still scanning the entire test classpath twice to find test cases...

filter { // following test require es.insecure_network_trace_enabled to NOT be enabled includeTestsMatching "org.elasticsearch.common.logging.LoggersTests.testCheckRestrictedLoggers" includeTestsMatching "org.elasticsearch.common.logging.LoggersTests.testSetLevelWithRestrictions" includeTestsMatching "org.elasticsearch.action.admin.cluster.settings.ClusterUpdateSettingsRequestTests.testValidateLoggers" }

But I like the idea of adding a focused :qa suite. Thanks for the pointer @DaveCTurner , I'll have a look at that.

+1 to having an explicit QA project for this.

@rjernst I think this is a good compromise: In 7f57feb I've moved the tests to :qa:restricted-loggers. But I am also keeping similar tests in :server that are using an internal API allowing to explicitly pass restricted loggers. This should address your concern to keep tests close by the code. And at the same time we don't have to compromise on the static / final initialization of TRACE_ENABLED / RESTRICTED_LOGGERS.

…d by NetworkTraceFlag.TRACE_ENABLED. Additionally add qa module to test restricted loggers using public APIs.

qa/restricted-loggers/build.gradle

mosche · 2024-02-19T08:40:32Z

@elasticmachine update branch

mosche · 2024-02-21T08:31:07Z

@DaveCTurner any more feedback from your side?

DaveCTurner · 2024-02-21T08:45:40Z

Nothing to add, no. Thanks for the ping, sorry I didn't realise I was still blocking this.

mosche · 2024-02-21T13:34:59Z

@elasticmachine update branch

rjernst

LGTM. I left one very minor nit, just thinking ahead to when we can isolate logging config, I want to keep internals as private as possible to aid in that future work.

rjernst · 2024-02-21T14:17:52Z

.../java/org/elasticsearch/action/admin/cluster/settings/ClusterUpdateSettingsRequestTests.java

+ * Side Public License, v 1.
+ */
+
+package org.elasticsearch.action.admin.cluster.settings;


This test could be in o.e.c.logging, then Loggers.RESTRICTED_LOGGERS could be package private?

With logging restriction (elastic#105020), the networkTrace flag needs to be set for AWS request debug logging. Relates: elastic#101608

With logging restriction (#105020), the networkTrace flag needs to be set for AWS request debug logging. Relates: #101608

Block updates to log level for org.apache.http if less specific than …

2b7b3fb

…INFO.

mosche added >non-issue :Core/Infra/Logging Log management and logging utilities Team:Core/Infra Meta label for core/infra team labels Feb 1, 2024

elasticsearchmachine added the v8.13.0 label Feb 1, 2024

mosche added 5 commits February 1, 2024 16:44

Merge branch 'main' into ES-7617_restricted_loggers

0129fb4

Don't expand public surface of Loggers, move settings update test

d2acdb5

Merge branch 'main' into ES-7617_restricted_loggers

6e95008

fix failure in EvilLoggerConfigurationTests

9de8689

Remove DEBUG level for org.apache.http.wire in S3SnapshotRepoTestKitIT

55d0b0b

mosche commented Feb 2, 2024

View reviewed changes

...aRestTest/java/org/elasticsearch/repositories/blobstore/testkit/S3SnapshotRepoTestKitIT.java Show resolved Hide resolved

ldematte reviewed Feb 2, 2024

View reviewed changes

mosche added 2 commits February 2, 2024 16:58

Use TestLoggers in OpenIdConnectAuthenticatorTests to set restricted …

a95418e

…org.apache.http logger

Fix assert

8ef1aed

rjernst approved these changes Feb 2, 2024

View reviewed changes

DaveCTurner requested changes Feb 4, 2024

View reviewed changes

mosche added 3 commits February 5, 2024 11:29

PR feedback

1beb51f

Restrict Apache http logger / AWS request logger unless TRACE_ENABLED…

69e593d

… is set

Update documentation

7d53005

mosche commented Feb 5, 2024

View reviewed changes

server/src/test/java/org/elasticsearch/common/settings/ScopedSettingsTests.java Outdated Show resolved Hide resolved

DaveCTurner reviewed Feb 5, 2024

View reviewed changes

validate logger settings in REST layer, separate task testNetworkTrac…

ccf5c50

…eNotEnabled to test restricted loggers

mosche requested a review from a team as a code owner February 12, 2024 07:35

mosche added 2 commits February 12, 2024 08:36

fix

b123d48

fix

779fdf1

mosche added 2 commits February 12, 2024 14:55

Merge branch 'main' into ES-7617_restricted_loggers

b20f493

fix

dd0164d

mark-vieira requested changes Feb 12, 2024

View reviewed changes

mosche added 4 commits February 12, 2024 18:31

fix bug

e301d69

More selective test filter

59ab7b0

let 'check' depend on 'testNetworkTraceNotEnabled'

54f8ae7

Test setting restricted loggers using internal API that isn't impacte…

7f57feb

…d by NetworkTraceFlag.TRACE_ENABLED. Additionally add qa module to test restricted loggers using public APIs.

mark-vieira approved these changes Feb 14, 2024

View reviewed changes

qa/restricted-loggers/build.gradle Outdated Show resolved Hide resolved

elasticsearchmachine added v8.14.0 and removed v8.13.0 labels Feb 14, 2024

Remove copy paste comment

cae9cc1

Merge branch 'main' into ES-7617_restricted_loggers

4e8a52e

DaveCTurner approved these changes Feb 21, 2024

View reviewed changes

Merge branch 'main' into ES-7617_restricted_loggers

1079443

rjernst approved these changes Feb 21, 2024

View reviewed changes

mosche added 2 commits February 21, 2024 16:29

package private RESTRICTED_LOGGERS

06f09e2

Merge branch 'main' into ES-7617_restricted_loggers

da5a04c

mosche merged commit 6b50b6d into elastic:main Feb 21, 2024

mosche deleted the ES-7617_restricted_loggers branch February 21, 2024 16:45

pmpailis mentioned this pull request Mar 12, 2024

Fix leaking log-level set on root logger in LoggerTests #106250

Closed

ywangd added a commit to ywangd/elasticsearch that referenced this pull request May 27, 2024

[Test] Re-enable debug logging for AWS request

b4c4d8c

With logging restriction (elastic#105020), the networkTrace flag needs to be set for AWS request debug logging. Relates: elastic#101608

This was referenced May 27, 2024

[Test] Re-enable debug logging for AWS request #109068

Merged

[CI] S3BlobStoreRepositoryTests testMetrics failing #101608

Closed

elasticsearchmachine pushed a commit that referenced this pull request May 28, 2024

[Test] Re-enable debug logging for AWS request (#109068)

eff8d0f

With logging restriction (#105020), the networkTrace flag needs to be set for AWS request debug logging. Relates: #101608

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Block updates to log level for org.apache.http if less specific than INFO #105020

Block updates to log level for org.apache.http if less specific than INFO #105020

mosche commented Feb 1, 2024

elasticsearchmachine commented Feb 1, 2024

ldematte left a comment

rjernst left a comment

DaveCTurner left a comment

mosche commented Feb 5, 2024

DaveCTurner left a comment

DaveCTurner commented Feb 5, 2024 via email •

edited

Loading

rjernst commented Feb 5, 2024

mosche commented Feb 6, 2024

mark-vieira Feb 12, 2024

mosche Feb 12, 2024 •

edited

Loading

mark-vieira Feb 12, 2024

mosche Feb 13, 2024

DaveCTurner Feb 13, 2024

DaveCTurner Feb 13, 2024

mosche Feb 13, 2024

mark-vieira Feb 13, 2024

mosche Feb 14, 2024

mosche commented Feb 19, 2024 •

edited

Loading

mosche commented Feb 21, 2024

DaveCTurner commented Feb 21, 2024

mosche commented Feb 21, 2024

rjernst left a comment

rjernst Feb 21, 2024

mosche Feb 21, 2024

	from the logs emitted by the storage system, configure {es} to log every
	request it makes to the S3 API by <<configuring-logging-levels,setting the
	logging level>> of the `com.amazonaws.request` logger to `DEBUG`:

	problem. See the
	https://docs.aws.amazon.com/sdk-for-java/v1/developer-guide/java-dg-logging.html[AWS Java SDK]
	documentation for further information, including details about other loggers
	that can be used to obtain even more verbose logs. When you have finished

Block updates to log level for org.apache.http if less specific than INFO #105020

Block updates to log level for org.apache.http if less specific than INFO #105020

Conversation

mosche commented Feb 1, 2024

elasticsearchmachine commented Feb 1, 2024

ldematte left a comment

Choose a reason for hiding this comment

rjernst left a comment

Choose a reason for hiding this comment

DaveCTurner left a comment

Choose a reason for hiding this comment

mosche commented Feb 5, 2024

DaveCTurner left a comment

Choose a reason for hiding this comment

DaveCTurner commented Feb 5, 2024 via email • edited Loading

rjernst commented Feb 5, 2024

mosche commented Feb 6, 2024

Choose a reason for hiding this comment

mosche Feb 12, 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

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

mosche commented Feb 19, 2024 • edited Loading

mosche commented Feb 21, 2024

DaveCTurner commented Feb 21, 2024

mosche commented Feb 21, 2024

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DaveCTurner commented Feb 5, 2024 via email •

edited

Loading

mosche Feb 12, 2024 •

edited

Loading

mosche commented Feb 19, 2024 •

edited

Loading