DRIVERS-1816 Test redaction of replies to security sensitive commands #1026

alcaeus · 2021-06-24T17:13:03Z

This does not cover replies to all commands, since we're only able to make assertions against successful commands. With some authentication commands, getting them to work is a sizeable effort. For that reason, it's up to engineers to ensure they're not just redacting replies to the commands where we test this, but rather all commands. This includes replies that are potentially sensitive, e.g. replies for saslStart and saslContinue.

Note that due to a limitation of the unified test format, we can't make assertions against the contents of CommandFailedEvents. While the spec doesn't explicitly mention this event, it is assumed that drivers redact replies when they are exposed in CommandFailedEvents. That said, most if not all replies will not contain sensitive information, so I believe it's ok to leave this untested for the time being.

benjirewis

Thanks for adding these tests! To be clear, do these changes assert the following?

Most security-sensitive fields are redacted from security-sensitive commands (you've added assertions for payload, mechanism, etc.)
Server replies to hellos and legacy hellos are similarly redacted.

And, it's up to individual drivers to assert all other replies are redacted (including failures)?

source/command-monitoring/tests/unified/redacted-commands.yml

alcaeus · 2021-06-25T06:28:13Z

source/unified-test-format/schema-1.5.json

@@ -0,0 +1,462 @@
+{


While working on the recent changes, I realised that the original PR that added support for the observeSensitiveCommands client entity option didn't create the 1.5 schema version and also didn't add the tests to the unified test format's Makefile.

I definitely missed that while reviewing #1015.

@prashantmital: Any idea what happened there? You updated the test file itself to use 1.5 but the PR never included a schema-1.5.json. Did you actually make one and just forget to git add it?

Actually, I didn't create schema-1.5.json at all. Completely slipped my mind that this was something I needed to do.

alcaeus · 2021-06-25T06:30:10Z

source/unified-test-format/tests/Makefile

+SCHEMA=../schema-1.5.json

-.PHONY: all invalid valid-fail valid-pass versioned-api load-balancers gridfs transactions crud collection-management HAS_AJV
+.PHONY: all invalid valid-fail valid-pass versioned-api load-balancers gridfs transactions crud collection-management command-monitoring HAS_AJV


I've love to find a different option for this at some point in time. Ideally, we'd go through all directories, grab every .yml file, attempt to read a schema version for it (skipping the file if we don't find one) and validating the file against the explicit schema version indicated in the file. @jmikola do you think this is feasible? If so I'd file a ticket so we can eventually return to this.

This solution would fix two issues:

it would no longer require us to manually add new folders with unified test files here, and

it validates files against the schema version they claim to be, which can catch errors when somebody adds functionality from newer schema versions without bumping the schemaVersion in the file.

DRIVERS-1767 already exists to track this. If there's some variation to the behavior (e.g. skipping files that don't have a schemaVersion field) feel free to note that as a suggested approach in the ticket.

I'll check it out, thanks for pointing me in the right direction

benjirewis · 2021-06-25T14:17:49Z

source/command-monitoring/tests/unified/redacted-commands.yml

+                # This assertion will currently always hold true since we're
+                # not expecting successful authentication, in which case this
+                # field is missing anyways.
+                speculativeAuthenticate: { $$exists: false }


Even if I remove the logic for redacting replies, this test (and the corresponding one for legacy hello) still passes in the Go driver. The reply still exists (because I'm no longer redacting), isWritablePrimary is true, and speculativeAuthenticate does not exist because authentication failed.

Yes, that is unfortunately true. I can either remove the reply assertions in this test or leave them in to show intent.

I've tried to create a user beforehand so we can actually test the auth process (and thus get a speculativeAuthenticate in the reply) but wasn't able to get it to pass consistently without spending more time. With the tight timeline on this I opted to take a more pragmatic approach.

I've updated this to expect the redaction of the reply in this scenario as well. I've worked around the limitation in the C driver, so there's no reason for the spec tests to assume the reply shouldn't be redacted.

This allows redacting replies to a hello command with speculative authentication, even when the reply does not contain any sensitive fields.

This changes the test expectation for a hello command with sensitive information. Previously, the test did not require redaction of the reply, as it does not contain sensitive information (due to speculative authentication failing). However, this makes the test useless, as it can no longer test that the redaction logic applies to hello at all. Until we're able to test successful authentication, it's sensible to require redacting the reply to ensure drivers are spec compliant.

source/command-monitoring/tests/unified/redacted-commands.yml

jmikola · 2021-06-29T20:21:31Z

source/command-monitoring/tests/unified/redacted-commands.yml

      - name: runCommand
        object: *database
        arguments:
          commandName: ismaster


Noted that you're using the all-lowercase variant of isMaster here in order to test both forms accepted by the server (and dictated by the spec).

source/command-monitoring/tests/unified/redacted-commands.yml

source/unified-test-format/tests/Makefile

source/unified-test-format/tests/invalid/entity-client-observeSensitiveCommands-type.yml

durran · 2021-07-01T12:13:31Z

Tests pass on Node: mongodb/node-mongodb-native#2872

durran

Just a schema version bump to 1.5 in entity-client-observeSensitiveCommands-type.yml

* upstream/master: DRIVERS-1836 Skip tests with $out stage on serverless (mongodb#1036) DRIVERS-1816 Test redaction of replies to security sensitive commands (mongodb#1026) DRIVERS-1828: Remove insertedCount and improve use of $$unsetOrMatches (mongodb#1034) DRIVERS-1819 Disable writes and other unsupported operations in snapshot reads (mongodb#1033)

…mongodb#1026) * DRIVERS-1816 Test redaction of replies to security sensitive commands * Use strings for minServerVersions * Add 1.5 schema and validate unified APM tests * Allow redaction of non-sensitive replies to hello This allows redacting replies to a hello command with speculative authentication, even when the reply does not contain any sensitive fields. * Require redacting replies to sensitive commands This changes the test expectation for a hello command with sensitive information. Previously, the test did not require redaction of the reply, as it does not contain sensitive information (due to speculative authentication failing). However, this makes the test useless, as it can no longer test that the redaction logic applies to hello at all. Until we're able to test successful authentication, it's sensible to require redacting the reply to ensure drivers are spec compliant. * Use correct schema version in test

alcaeus requested review from behackett, benjirewis and dariakp June 24, 2021 17:13

alcaeus self-assigned this Jun 24, 2021

alcaeus requested a review from a team as a code owner June 24, 2021 17:13

alcaeus requested review from kmahar and removed request for a team June 24, 2021 17:13

benjirewis reviewed Jun 24, 2021

View reviewed changes

alcaeus requested a review from a team as a code owner June 25, 2021 06:15

alcaeus requested review from jmikola and removed request for a team June 25, 2021 06:15

alcaeus commented Jun 25, 2021

View reviewed changes

alcaeus requested review from benjirewis and removed request for behackett June 25, 2021 06:30

benjirewis reviewed Jun 25, 2021

View reviewed changes

benjirewis mentioned this pull request Jun 25, 2021

GODRIVER-2013 Security-sensitive command-monitoring redactions mongodb/mongo-go-driver#681

Merged

alcaeus added 5 commits June 29, 2021 09:43

DRIVERS-1816 Test redaction of replies to security sensitive commands

029aee0

Use strings for minServerVersions

98fd578

Add 1.5 schema and validate unified APM tests

bb121a8

Allow redaction of non-sensitive replies to hello

6224ead

This allows redacting replies to a hello command with speculative authentication, even when the reply does not contain any sensitive fields.

alcaeus force-pushed the drivers-1816 branch from b9eeb04 to 4895409 Compare June 29, 2021 07:45

jmikola reviewed Jun 29, 2021

View reviewed changes

jmikola mentioned this pull request Jun 29, 2021

DRIVERS-1607: Snapshot reads on Secondaries #1022

Closed

Use correct schema version in test

13419bf

comandeo mentioned this pull request Jul 1, 2021

RUBY-2619 Add test for security-sensitive commands mongodb/mongo-ruby-driver#2286

Merged

durran mentioned this pull request Jul 1, 2021

test(NODE-3381): command monitoring redaction tests mongodb/node-mongodb-native#2872

Merged

durran requested changes Jul 1, 2021

View reviewed changes

durran mentioned this pull request Jul 1, 2021

test(NODE-3381): command monitoring redaction tests mongodb/node-mongodb-native#2873

Merged

alcaeus requested review from durran and jmikola and removed request for dariakp and kmahar July 2, 2021 07:46

durran approved these changes Jul 2, 2021

View reviewed changes

jmikola removed their request for review July 2, 2021 13:44

alcaeus merged commit 289f405 into mongodb:master Jul 5, 2021

alcaeus deleted the drivers-1816 branch July 5, 2021 12:58

DRIVERS-1816 Test redaction of replies to security sensitive commands #1026

DRIVERS-1816 Test redaction of replies to security sensitive commands #1026

Uh oh!

Conversation

alcaeus commented Jun 24, 2021

Uh oh!

benjirewis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

durran commented Jul 1, 2021

Uh oh!

durran left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants