Skip to content

DRIVERS-1653 Add observeSensitiveCommands property to the client entity type #1015

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Jun 15, 2021
Merged

DRIVERS-1653 Add observeSensitiveCommands property to the client entity type #1015

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
40033ed
DRIVERS-1653 Add observeSensitiveCommands property to the client enti…
prashantmital Jun 9, 2021
16b489c
disable with auth
prashantmital Jun 15, 2021
e940f26
update test
prashantmital Jun 15, 2021
17acc9c
add period
prashantmital Jun 15, 2021
f33a6d5
review changes
prashantmital Jun 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions source/command-monitoring/tests/unified/redacted-commands.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
{
"description": "redacted-commands",
"schemaVersion": "1.0",
"schemaVersion": "1.5",
"runOnRequirements": [
{
"minServerVersion": "5.0"
"minServerVersion": "5.0",
"auth": false
}
],
"createEntities": [
Expand All @@ -12,7 +13,8 @@
"id": "client",
"observeEvents": [
"commandStartedEvent"
]
],
"observeSensitiveCommands": true
}
},
{
Expand Down
4 changes: 3 additions & 1 deletion source/command-monitoring/tests/unified/redacted-commands.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,17 @@
description: "redacted-commands"

schemaVersion: "1.0"
schemaVersion: "1.5"
Copy link
Member

@jmikola jmikola Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


runOnRequirements:
- minServerVersion: "5.0"
auth: false

createEntities:
- client:
id: &client client
observeEvents:
- commandStartedEvent
observeSensitiveCommands: true
- database:
id: &database database
client: *client
Expand Down
35 changes: 33 additions & 2 deletions source/unified-test-format/unified-test-format.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ Unified Test Format
===================

:Spec Title: Unified Test Format
:Spec Version: 1.4.1
:Spec Version: 1.5.0
:Author: Jeremy Mikola
:Advisors: Prashant Mital, Isabel Atkinson, Thomas Reggi
:Status: Accepted
Expand Down Expand Up @@ -414,6 +414,8 @@ The structure of this object is as follows:
treat the comparison as not equal and skip the test. This includes errors that
occur when fetching a single parameter using ``getParameter``.

.. _runOnRequirement_auth:

- ``auth``: Optional boolean. If true, the tests MUST only run if authentication
is enabled. If false, tests MUST only run if authentication is not enabled.
If this field is omitted, there is no authentication requirement.
Expand Down Expand Up @@ -543,11 +545,21 @@ The structure of this object is as follows:
``configureFailPoint`` and any commands containing sensitive information
(per the
`Command Monitoring <../command-monitoring/command-monitoring.rst#security>`__
spec).
spec) unless ``observeSensitiveCommands`` is true.
Copy link
Member

@jmikola jmikola Jun 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Test files SHOULD NOT use this option unless one or more command monitoring
events are specified in `observeEvents <entity_client_observeEvents_>`_.

- ``observeSensitiveCommands``: Optional boolean. If true, events associated
with sensitive commands (per the
`Command Monitoring <../command-monitoring/command-monitoring.rst#security>`__
spec) will be observed for this client. If false or not specified, events for
commands containing sensitive information MUST be ignored.
Authentication SHOULD be disabled when this property is true, i.e.
`auth <runOnRequirement_auth_>`_ should be false for each ``runOnRequirement``.
Test runners MUST fail if ``observeSensitiveCommands`` is true and
authentication is enabled. See `rationale_observeSensitiveCommands`_.
Copy link
Member

@jmikola jmikola Jun 15, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).


.. _entity_client_storeEventsAsEntities:

- ``storeEventsAsEntities``: Optional array of one or more
Expand Down Expand Up @@ -3101,6 +3113,22 @@ The specification does prefer "MUST" in other contexts, such as discussing parts
of the test file format that *are* enforceable by the JSON schema or the test
runner implementation.

.. _rationale_observeSensitiveCommands:

Why can't ``observeSensitiveCommands`` be true when authentication is enabled?
------------------------------------------------------------------------------

When running against authentication-enabled clusters, the events observed by a
client will always begin with auth-related commands (e.g. ``authenticate``,
``saslStart``, ``saslContinue``) because the MongoClient will need to
authenticate a connection before issuing the first command in the test
specification. Since the exact form of the authentication command event will
depend on whether authentication is enabled, as well as, the auth mechanism in
use, it is not possible to anticipate the command monitoring output and perform
the appropriate assertions. Consequently, we have restricted use of this property
to situations where authentication is disabled on the server. This allows
tests to explicitly test sensitive commands via the ``runCommand`` helper.


Breaking Changes
================
Expand Down Expand Up @@ -3212,6 +3240,9 @@ spec changes developed in parallel or during the same release cycle.
Change Log
==========

:2021-06-09: Added an ``observeSensitiveCommands`` property to the ``client``
entity type.

:2021-05-17: Ensure old JSON schema files remain in place

:2021-04-19: Introduce ``serverless`` `runOnRequirement`_.
Expand Down