-
Notifications
You must be signed in to change notification settings - Fork 918
GODRIVER-2013 Security-sensitive command-monitoring redactions #681
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.
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
Changes from all commits
b52a5c7
99a049d
5629c2a
82d6746
7545fc6
c3be6a5
2f8caa1
c59e576
415a3b9
6177d81
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,252 @@ | ||
| description: "redacted-commands" | ||
|
|
||
| schemaVersion: "1.5" | ||
|
|
||
| runOnRequirements: | ||
| - minServerVersion: "5.0" | ||
| auth: false | ||
|
|
||
| createEntities: | ||
| - client: | ||
| id: &client client | ||
| observeEvents: | ||
| - commandStartedEvent | ||
| observeSensitiveCommands: true | ||
| - database: | ||
| id: &database database | ||
| client: *client | ||
| databaseName: &databaseName command-monitoring-tests | ||
|
|
||
| tests: | ||
| - description: "authenticate" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: authenticate | ||
| command: | ||
| authenticate: "private" | ||
| # Malformed authentication commands will fail against the server, but we | ||
| # just want to assert that security-related commands are redacted | ||
| # in command monitoring. | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: authenticate | ||
| # This only asserts that the command name has been redacted from the published command; | ||
| # however, it's unlikely that a driver would redact this but leave other, sensitive fields. | ||
| # We cannot simply assert that command is an empty document because it's at root-level, so | ||
| # additional fields in the actual document will be permitted | ||
| command: { authenticate: { $$exists: false } } | ||
|
|
||
| - description: "saslStart" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: saslStart | ||
| command: | ||
| saslStart: "private" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: saslStart | ||
| command: { saslStart: { $$exists: false } } | ||
|
|
||
| - description: "saslContinue" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: saslContinue | ||
| command: | ||
| saslContinue: "private" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: saslContinue | ||
| command: { saslContinue: { $$exists: false } } | ||
|
|
||
| - description: "getnonce" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: getnonce | ||
| command: | ||
| getnonce: "private" | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: getnonce | ||
| command: { getnonce: { $$exists: false } } | ||
|
|
||
| - description: "createUser" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: createUser | ||
| command: | ||
| createUser: "private" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: createUser | ||
| command: { createUser: { $$exists: false } } | ||
|
|
||
| - description: "updateUser" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: updateUser | ||
| command: | ||
| updateUser: "private" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: updateUser | ||
| command: { updateUser: { $$exists: false } } | ||
|
|
||
| - description: "copydbgetnonce" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: copydbgetnonce | ||
| command: | ||
| copydbgetnonce: "private" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: copydbgetnonce | ||
| command: { copydbgetnonce: { $$exists: false } } | ||
|
|
||
| - description: "copydbsaslstart" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: copydbsaslstart | ||
| command: | ||
| copydbsaslstart: "private" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: copydbsaslstart | ||
| command: { copydbsaslstart: { $$exists: false } } | ||
|
|
||
| - description: "copydb" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: copydb | ||
| command: | ||
| copydb: "private" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: copydb | ||
| command: { copydb: { $$exists: false } } | ||
|
|
||
| - description: "hello with speculative authenticate" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: hello | ||
| command: | ||
| hello: "private" | ||
| speculativeAuthenticate: "foo" | ||
| expectError: | ||
| isError: true | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: ismaster | ||
| command: | ||
| ismaster: "private" | ||
| speculativeAuthenticate: "foo" | ||
| expectError: | ||
| isError: true | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: isMaster | ||
| command: | ||
| isMaster: "private" | ||
| speculativeAuthenticate: "foo" | ||
| expectError: | ||
| isError: true | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: hello | ||
| command: { hello: { $$exists: false } } | ||
| - commandStartedEvent: | ||
| commandName: ismaster | ||
| command: { ismaster: { $$exists: false } } | ||
| - commandStartedEvent: | ||
| commandName: isMaster | ||
| command: { isMaster: { $$exists: false } } | ||
|
|
||
| - description: "hello without speculative authenticate is not redacted" | ||
| operations: | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: hello | ||
| command: | ||
| hello: "public" | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: ismaster | ||
| command: | ||
| ismaster: "public" | ||
| - name: runCommand | ||
| object: *database | ||
| arguments: | ||
| commandName: isMaster | ||
| command: | ||
| isMaster: "public" | ||
| expectEvents: | ||
| - client: *client | ||
| events: | ||
| - commandStartedEvent: | ||
| commandName: hello | ||
| command: { hello: "public" } | ||
| - commandStartedEvent: | ||
| commandName: ismaster | ||
| command: { ismaster: "public" } | ||
| - commandStartedEvent: | ||
| commandName: isMaster | ||
| command: { isMaster: "public" } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,6 +9,7 @@ package unified | |
| import ( | ||
| "context" | ||
| "fmt" | ||
| "strings" | ||
| "sync/atomic" | ||
| "time" | ||
|
|
||
|
|
@@ -22,18 +23,23 @@ import ( | |
| "go.mongodb.org/mongo-driver/x/bsonx/bsoncore" | ||
| ) | ||
|
|
||
| // Security-sensitive commands that should be ignored in command monitoring by default. | ||
| var securitySensitiveCommands = []string{"authenticate", "saslStart", "saslContinue", "getnonce", | ||
| "createUser", "updateUser", "copydbgetnonce", "copydbsaslstart", "copydb"} | ||
|
|
||
| // clientEntity is a wrapper for a mongo.Client object that also holds additional information required during test | ||
| // execution. | ||
| type clientEntity struct { | ||
| *mongo.Client | ||
|
|
||
| recordEvents atomic.Value | ||
| started []*event.CommandStartedEvent | ||
| succeeded []*event.CommandSucceededEvent | ||
| failed []*event.CommandFailedEvent | ||
| pooled []*event.PoolEvent | ||
| ignoredCommands map[string]struct{} | ||
| numConnsCheckedOut int32 | ||
| recordEvents atomic.Value | ||
| started []*event.CommandStartedEvent | ||
| succeeded []*event.CommandSucceededEvent | ||
| failed []*event.CommandFailedEvent | ||
| pooled []*event.PoolEvent | ||
| ignoredCommands map[string]struct{} | ||
| observeSensitiveCommands *bool | ||
| numConnsCheckedOut int32 | ||
|
|
||
| // These should not be changed after the clientEntity is initialized | ||
| observedEvents map[monitoringEventType]struct{} | ||
|
|
@@ -43,14 +49,23 @@ type clientEntity struct { | |
| } | ||
|
|
||
| func newClientEntity(ctx context.Context, em *EntityMap, entityOptions *entityOptions) (*clientEntity, error) { | ||
| // The "configureFailPoint" command should always be ignored. | ||
| ignoredCommands := map[string]struct{}{ | ||
| "configureFailPoint": {}, | ||
| } | ||
| // If not observing sensitive commands, add security-sensitive commands | ||
| // to ignoredCommands by default. | ||
| if entityOptions.ObserveSensitiveCommands == nil || !*entityOptions.ObserveSensitiveCommands { | ||
benjirewis marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| for _, cmd := range securitySensitiveCommands { | ||
| ignoredCommands[cmd] = struct{}{} | ||
| } | ||
| } | ||
kevinAlbs marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| entity := &clientEntity{ | ||
| // The "configureFailPoint" command should always be ignored. | ||
| ignoredCommands: map[string]struct{}{ | ||
| "configureFailPoint": {}, | ||
| }, | ||
| observedEvents: make(map[monitoringEventType]struct{}), | ||
| storedEvents: make(map[monitoringEventType][]string), | ||
| entityMap: em, | ||
| ignoredCommands: ignoredCommands, | ||
| observedEvents: make(map[monitoringEventType]struct{}), | ||
| storedEvents: make(map[monitoringEventType][]string), | ||
| entityMap: em, | ||
| observeSensitiveCommands: entityOptions.ObserveSensitiveCommands, | ||
| } | ||
| entity.setRecordEvents(true) | ||
|
|
||
|
|
@@ -144,10 +159,30 @@ func (c *clientEntity) stopListeningForEvents() { | |
| c.setRecordEvents(false) | ||
| } | ||
|
|
||
| func (c *clientEntity) isIgnoredEvent(event *event.CommandStartedEvent) bool { | ||
| // Check if command is in ignoredCommands. | ||
| if _, ok := c.ignoredCommands[event.CommandName]; ok { | ||
| return true | ||
| } | ||
|
|
||
| if event.CommandName == "hello" || strings.ToLower(event.CommandName) == "ismaster" { | ||
| _, err := event.Command.LookupErr("speculativeAuthenticate") | ||
| speculativeAuth := err == nil | ||
|
|
||
| // If observeSensitiveCommands is false (or unset) and hello command is with | ||
| // speculative authenticate, command should be ignored. | ||
| if (c.observeSensitiveCommands == nil || !*c.observeSensitiveCommands) && | ||
| speculativeAuth { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } | ||
|
|
||
| func (c *clientEntity) startedEvents() []*event.CommandStartedEvent { | ||
| var events []*event.CommandStartedEvent | ||
| for _, evt := range c.started { | ||
| if _, ok := c.ignoredCommands[evt.CommandName]; !ok { | ||
| if !c.isIgnoredEvent(evt) { | ||
| events = append(events, evt) | ||
| } | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are we using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Cool, thanks for the detailed explanation! |
||
| } | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.