Fix Mongo Query Sanitizing #152
Conversation
realark
force-pushed
the
ark/mongo_quickfix
branch
from
e6a9dce to
1e0d1e2
Compare
There was a problem hiding this comment.
I'd like some extra eyes on this section in particular. I ported over the behavior the instrumentation was attempting to apply, but I'm not 100% sure if the span tags are correct.
There was a problem hiding this comment.
Actually we need to set the service name somewhere:
span.setTag(DDTags.SERVICE_NAME, "mongo");
There was a problem hiding this comment.
I think we want to use mongo.query as the operation name.
There was a problem hiding this comment.
The method call to errorLogs can be replaced with:
span.log(Collections.singletonMap("error.object", throwable));
realark
force-pushed
the
ark/mongo_quickfix
branch
from
9d36973 to
c5c426a
Compare
There was a problem hiding this comment.
This kind of scares me a little... unbounded cache that relies on the behavior below for removal. What do you think @realark?
There was a problem hiding this comment.
I'm okay with it because the monog CommandListener api guarantees the started event will generate a failed/success event.
There was a problem hiding this comment.
In other words, the mongo java driver itself would have to be buggy for the map to leak. We're not relying on good user code.
There was a problem hiding this comment.
I believe this method is no longer used and can be deleted.
tylerbenson
left a comment
tylerbenson
left a comment
There was a problem hiding this comment.
Once that unused method is deleted, I'm ok with this being merged.
palazzem
left a comment
palazzem
left a comment
There was a problem hiding this comment.
I think we should address Tyler's comments, especially unbounded cache issue
realark
force-pushed
the
ark/mongo_quickfix
branch
from
c5c426a to
b85b9b0
Compare
|
@palazzem I think the cache is ok for now since it was already being done that way with the existing mongo instrumentation. Certainly something we can look at in the future though. |
This fixes the bug which was capturing raw mongo queries with sensitive information.
The root cause was the byteman instrumentation was attempting to use a patched version of the TracingCommandListener. However, it was unable to do the bytecode patching because that class is not on the runtime classpath (because it is a dependency).
The fix is to create a new class, DDTracingCommandListener, inside of MongoHelper with the desired behavior. This is mostly the code from opentracing's TracingCommandListener. I will leave comments in sections where this PR will cause change.
In addition to the new unit tests, manual testing was done with the dropwizard-mongo sample app.