Paths

Table of Contentst

[services] Tunnelbroker - Remove all logs connected to `userID`, `deviceID` or `sessionID`
ClosedPublic
Actions

Authored by • max on Aug 7 2022, 12:00 PM.

Details

Reviewers

• karol
tomek
• jon
ashoat

Commits

rCOMM0b2320642c5f: [services] Tunnelbroker - Remove all logs connected to `userID`, `deviceID` or…

Summary

Logging on every new message can flood output and log files and only have a sense for tracing purposes.
We should remove this log message. Removing user-sensitive logging which includes userID, deviceID or sessionID as well.

Related Linear task: ENG-1491

Test Plan

No test plan, this is just a log message removing.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

• max created this revision.Aug 7 2022, 12:00 PM

Herald added subscribers: • abosh, • karol, atul and 3 others. · View Herald TranscriptAug 7 2022, 12:00 PM

• max published this revision for review.Aug 7 2022, 12:01 PM

• max edited the summary of this revision. (Show Details)

• max added reviewers: • karol, tomek, • jon.

• jon added inline comments.Aug 7 2022, 12:12 PM

services/tunnelbroker/src/Amqp/AmqpManager.cpp
71 ↗	(On Diff #15415)	Feel like this should just be changed to DEBUG

Harbormaster completed remote builds in B11206: Diff 15415.Aug 7 2022, 12:13 PM

• max marked an inline comment as done.Aug 7 2022, 12:24 PM

• max added inline comments.

services/tunnelbroker/src/Amqp/AmqpManager.cpp
71 ↗	(On Diff #15415)	Feel like this should just be changed to DEBUG I was thinking about this too, but to use debug we should rebuild the server from production mode. Our CI tests will run debug version and this message will flood the output in CI tests. I think, in case we need to verbose trace something like this it can be added locally when/if the problem occurs.

I was thinking about this too, but to use debug we should rebuild the server from production mode.

Our CI tests will run debug version and this message will flood the output in CI tests.

For CI gates, I don't see a problem, I would rather filter through too many logs than not enough. And 99% of the time we will just want the last ~20 lines before failure anyway.

But the line in question is finishing an "effect", and I think logging it in some manner is the right thing to do.

• karol accepted this revision.Aug 8 2022, 3:53 AM

• jon accepted this revision.Aug 8 2022, 9:00 AM

• jon added inline comments.

services/tunnelbroker/src/Amqp/AmqpManager.cpp
71 ↗	(On Diff #15415)	Oh, look slike glog doesn't even support a DEBUG log level... odd https://rpg.ifi.uzh.ch/docs/glog.html

phab says something isn't submitted...

• max marked 2 inline comments as done.Aug 8 2022, 9:25 AM

• max added inline comments.

services/tunnelbroker/src/Amqp/AmqpManager.cpp
71 ↗	(On Diff #15415)	Oh, look slike glog doesn't even support a DEBUG log level... odd https://rpg.ifi.uzh.ch/docs/glog.html Yes, we can use VERBOSE instead. But, not sure we need it here.

• jon added inline comments.Aug 8 2022, 9:53 AM

services/tunnelbroker/src/Amqp/AmqpManager.cpp
71 ↗	(On Diff #15415)	in rust I would do something like `debug!(...)` but this isn't rust. If you don't think this log line provides much value, then removing is fine. It can always be re-added if amqp behavior needs to be explored.

We also should not be printing anything like deviceID or userID to the log on production. @max, can you think of any other case where this is happening?

I was thinking about this too, but to use debug we should rebuild the server from production mode.

It would be great if we could change the log level while the server is running, but that has some disadvantages. But having to rebuild in order to change log level sounds like a mistake and we should be able to do that - can we start supporting it?

This revision is now accepted and ready to land.Aug 10 2022, 5:52 AM

Any time we are printing something like deviceID, we should be EXTREMELY careful not to enable that in production. I'd be more comfortable if we simply did not have log statements that print things like deviceID and userID in checked-in code.

Separately, pinging my question above, as it's been two days and I still haven't seen a response:

We also should not be printing anything like deviceID or userID to the log on production. @max, can you think of any other case where this is happening?

This revision now requires changes to proceed.Aug 10 2022, 6:58 AM

In D4769#138306, @tomek wrote:

I was thinking about this too, but to use debug we should rebuild the server from production mode.

It would be great if we could change the log level while the server is running, but that has some disadvantages. But having to rebuild in order to change log level sounds like a mistake and we should be able to do that - can we start supporting it?

We can use VERBOSE levels to change the logging levels without rebuilding.

In D4769#138340, @ashoat wrote:

Any time we are printing something like deviceID, we should be EXTREMELY careful not to enable that in production. I'd be more comfortable if we simply did not have log statements that print things like deviceID and userID in checked-in code.

I'm sure that we don't need this sort of tracing information in a production build, that's the main reason to remove such logs from services.

Separately, pinging my question above, as it's been two days and I still haven't seen a response:

We also should not be printing anything like deviceID or userID to the log on production. @max, can you think of any other case where this is happening?

Sorry for the late answer! I've checked all of the LOG(...) invocations and there is no such "tracings" left. This is one that should be definitely removed.
We have a logs containing the deviceID when client validations failed. Like this one.
I'm not sure is it some sort of security flaw in that case and it should be removed as well. Such sort of errors will be thrown only if something is wrong with the validations.

Yes, remove all of those please. All places with deviceID, userID... anything that could be connected to a user. Those logs should all be removed.

• max planned changes to this revision.Aug 10 2022, 7:51 AM

• max retitled this revision from [services] Tunnelbroker - Remove log on every new message to [services] Tunnelbroker - Remove all logs connected to `userID` or `deviceID`.

• max edited the summary of this revision. (Show Details)

Remove all logs with the userID, deviceID or sessionID.

• max retitled this revision from [services] Tunnelbroker - Remove all logs connected to `userID` or `deviceID` to [services] Tunnelbroker - Remove all logs connected to `userID`, `deviceID` or `sessionID`.Aug 10 2022, 4:45 PM

• max edited the summary of this revision. (Show Details)

Harbormaster completed remote builds in B11289: Diff 15534.Aug 10 2022, 4:54 PM

ashoat accepted this revision.Aug 10 2022, 7:21 PM

This revision is now accepted and ready to land.Aug 10 2022, 7:21 PM

Closed by commit rCOMM0b2320642c5f: [services] Tunnelbroker - Remove all logs connected to `userID`, `deviceID` or… (authored by • max). · Explain WhyAug 11 2022, 12:30 AM

This revision was automatically updated to reflect the committed changes.

• max added a commit: rCOMM0b2320642c5f: [services] Tunnelbroker - Remove all logs connected to `userID`, `deviceID` or….