GODRIVER-2013 Security-sensitive command-monitoring redactions #681
Conversation
kevinAlbs
left a comment
kevinAlbs
left a comment
There was a problem hiding this comment.
LGTM with a minor simplification.
|
The new tests might change depending on the outcome of a new PR for DRIVERS-1653. I'll wait on that before merging. |
| supportedSchemaVersions = map[int]string{ | ||
| 1: "1.3", | ||
| // We do not fully support the 1.5 schema, but we need 1.5 to test with observeSensitiveCommands. | ||
| 1: "1.5", |
There was a problem hiding this comment.
Since censoring security-sensitive commands in command monitoring is a high priority, this was the suggested way to run the new 1.5 tests.
| } | ||
| return false | ||
| } | ||
| return true |
There was a problem hiding this comment.
Optional: Invert the ignoredCommands condition to make the function more readable.
E.g.
if _, ok := c.ignoredCommands[event.CommandName]; ok {
return true
}
// Check if command is "hello" or legacy hello.
if event.CommandName == "hello" || strings.ToLower(event.CommandName) == "ismaster" {
//...
}
return falseThere was a problem hiding this comment.
Way simpler! Thanks, inverted.
| if _, ok := c.ignoredCommands[evt.CommandName]; !ok { | ||
| if !c.isIgnoredEvent(evt) { | ||
| events = append(events, evt) | ||
| } |
There was a problem hiding this comment.
Are we using startedEvents() to assert that an expected set of started events is published during a test? If so, why are we duplicating the command redaction logic in the test runner as well as in the driver? It seems more correct to adjust the expected set of started events in the test case, not redact them in the test runner.
There was a problem hiding this comment.
There are two places in our specifications where we redact security-sensitive commands.
In the command monitoring spec, we require that drivers redact security-sensitive commands and replies at the operation level. This prevents security-sensitive information from appearing in logs or test output due to command monitoring.
In the unified test format:
ignoreCommandMonitoringEvents: Optional array of one or more strings. Command names for which the test runner MUST ignore any observed command monitoring events. The command(s) will be ignored in addition to configureFailPoint and any commands containing sensitive information (per the Command Monitoring spec) unless observeSensitiveCommands is true.
Writing tests and making sure that the expected set of events contains the correct auth-related commands can be problematic. Drivers differ on when certain auth-related commands are sent (particularly in setup), so a unified test runner that always monitors security-sensitive commands might produce different monitoring outputs depending on the driver and prevent a unified test.
There was a problem hiding this comment.
Cool, thanks for the detailed explanation!
|
Looks like more changes will be added in DRIVERS-1816 to further test that security-sensitive fields of commands are redacted and start testing that replies from the server are redacted as well. Merging these changes for now, as I imagine that work will be done as part of a separate ticket. |
|
FYI for posterity: When tests are eventually added to test the redaction of replies from the server, a workaround for empty (redacted) |
3 participants
GODRIVER-2013
Adds tests to ensure that we are redacting security sensitive commands from command monitoring. Updates
redactCommandto also redacthellowhenspeculativeAuthenticateis present.