DRIVERS-1653 Add observeSensitiveCommands property to the client entity type

[prashantmital]

An explicit flag to override a unified test runner's default behavior of ignoring commands with sensitive information.

[kmahar]

From a technical perspective this seems fine to me, but I do have a more process-related concern, which is that requiring a schema version of 1.5 may block various drivers from actually being able to run these tests if they haven't made the test runner changes in previous schema versions for e.g. Atlas planned maintenance. I think being able to run these and validate compliance is quite high priority given the .NET vulnerability that led us to add these in the first place. That said, maybe such drivers could do some special casing to run this file anyway, or write prose versions of the tests? This is a general shortcoming of the semver approach to the runner which we discussed back in the design phase and iirc couldn't really figure out a nice way around.

[benjirewis]

LGTM mod @kmahar's comment about schema version. Tried the new observeSensitiveCommands in the Go driver and it seems to work as intended.

Both the Go driver and CXX driver only support schema version 1.3, so it is a big bump just to run these tests. Not sure there's much to do about that though.

[jmikola]

If/when this gets merged, please revise the description in DRIVERS-1653 after merging to both update the commit reference for syncing tests and mention the new syntax and schema version.

[jmikola]

For context, the original text of "the command(s) will be ignored in addition to configureFailPoint and any commands containing sensitive information (per the Command Monitoring spec)" was the result of my misunderstanding of that spec. Since libmongoc has historically never emitted events for internal auth commands, I never realized that the correct behavior was to actually emit events and just redact the command/reply documents.

Furthermore, I was oblivious to the issue described in CDRIVER-3797 where an event might be emitted for the createUser helper, since PHP doesn't use those helpers at all nor does it provide its own.

[jmikola]

In regard to @kmahar and @benjirewis's concerns about this version bump, I think it's quite possible that drivers could add support for this new option (1.5) without implementing everything in previous schema versions. This requires some manual tracking on their end, but it's the same approach I took when prototyping Atlas testing (1.2) before versioned API was implemented (1.1). I bumped our internal test runner's supported schema version to 1.2 and left a comment that features in 1.1 were not supported.

I concur that it's a crappy work-around, but I don't see another way given our versioning. @alcaeus has floated the idea of instead categorizing test runner features with tags and having tests enumerate which features they depend on, but I think that has the possibility of getting quite messy as well (especially if the same "named" feature were to have multiple variations/versions over time).

[jmikola]

I do have a question related to something @ShaneHarvey raised in Slack:

If your driver test runner doesn’t ignore auth commands by default, how does the expectEvents matching code know to ignore the auth commands when the test suite runs against an auth-enabled cluster?

If the unified test runner previously always ignored auth commands, and a new observeSensitiveCommands option is going to change that behavior, wouldn't this conflict with any tests using observeSensitiveCommands and running against an auth-enabled cluster?

Assuming a new MongoClient is constructed for each test, and never used until the first operation within a test (after command monitoring and event collection is already enabled), the observed events are always going to be preceded with auth-related commands. And this is something the test itself cannot anticipate if it has no control over whether it's running on a cluster with or without auth.

I think we'd need one of the following:

• Some mechanism to prohibit using observeSensitiveCommands with auth-enabled clusters, such that it's really only useful for tests such as this where runCommand is manually executing auth commands.
• Require tests to ensure the MongoClient's connections are already initialized (e.g. issue a ping command) before we start observing events and executing operations. This would get the initial auth commands out of the way.

[prashantmital]

I don't think we need to special for when we are connected to an auth enabled cluster. If auth commands are not germane to the test (but some other security sensitive commands like createUser are), the test spec can simply set observeSensitiveCommands=true and then add the auth commands to be ignored to ignoreCommandMonitoringEvents. The way I see it, observeSensitiveCommands just disables the default behavior of ignoring sensitive commands but doesn't preclude the test spec from explicitly ignoring some or all of those commands using ignoreCommandMonitoringEvents anyway. Would you agree @jmikola ?

[jmikola]

If auth commands are not germane to the test (but some other security sensitive commands like createUser are), the test spec can simply set observeSensitiveCommands=true and then add the auth commands to be ignored to ignoreCommandMonitoringEvents. The way I see it, observeSensitiveCommands just disables the default behavior of ignoring sensitive commands but doesn't preclude the test spec from explicitly ignoring some or all of those commands using ignoreCommandMonitoringEvents anyway.

Let's consider the test being modified in this PR. It uses runCommand to issue a fabricated authenticate command, which at this point is likely only ever used for X509 auth (since MONGODB-CR is removed). If this test happens to run on a cluster using X509 auth, I don't see how we could rely on ignoreCommandMonitoringEvents to filter out only the X509 authentication required by the environment but allow the command issued by the runCommand operation. Presumably, we'd have the same problem if the spec test decided to use something like saslStart or saslContinue.

[benjirewis]

LGTM. Seems easiest to just not run the tests with observeSensitiveCommands against auth-enabled clusters.

[jmikola]

tl;dr: I suggest removing "Test runners MUST fail if observeSensitiveCommands is true and authentication is enabled".

I don't think this needs to be a requirement since it's adding a bit of complexity to the implementation just to protect users from occasional test failures (e.g. the example where authenticate was being tested for redaction and might conflict with a cluster using X509 auth).

If I understand correctly, this doesn't actually care about auth specifically -- since it's a runtime check for what the cluster actually is. I suppose this could trigger if either auth was true or unspecified (and the cluster happened to be using auth).

This is asking a test runner to inspect the environment in an entirely different place than when runOnRequirements might typically be resolved, so my preference would be to just get rid of this and if someone writes a bad test that unexpectedly fails we either catch that during review or after the fact when it inevitably fails on an auth environment.

I was originally going to suggest adding a "valid-fail" for this but I realize we probably can't do that since there's no guarantee the test would run against an auth-enabled cluster in the first place. Even if such a "valid-fail" test used auth: true, it's likely to just be skipped if the runOnRequirement(s) is not met (which would actually be bad because then we wouldn't get a failure).