1470034 - Sync shouldn't log x-client-state or hawk auth headers

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Mark Hammond [:markh] [:mhammond]]

Even at trace level, these values should never be logged as it may expose our users to harm if the logs are made public.

Comment 1

[Ryan Kelly [:rfkelly]]

We should also suppress:

* The "key" value in the JSON response from https://token.services.mozilla.com/1.0/sync/1.5
* The "pushPublicKey" and "pushAuthKey" values in the JSON response from https://api.accounts.firefox.com/v1/account/devices

Comment 2

[Thom Chiovoloni [:tcsc] (ex-moco)]

Do we log these? I thought you had to set `services.common.hawk.log.sensitive` and `identity.fxaccounts.log.sensitive` to log them

Comment 3

[Pavan Veginati]

Attachment: Suggested fix v1

Hi,
I'm interested in taking this up as my first contribution.

I've attached what could be the fix for suppressing headers part.
Can you please let me know if this is the right way to approach this.

Comment 4

[Mark Hammond [:markh] [:mhammond]]

(In reply to Pavan Veginati from comment #3)
> Created attachment 8988548 [details] [diff] [review]
> Suggested fix v1
> 
> Hi,
> I'm interested in taking this up as my first contribution.
> 
> I've attached what could be the fix for suppressing headers part.
> Can you please let me know if this is the right way to approach this.

Yeah, I think that looks great, thanks.

Addressing comment 1 is going to be trickier, but I think it would be more robust to avoid logging response bodies from rest.js completely - ie, I think that if you move:

https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/services/common/rest.js#445-448

to line 439 (ie, so we only log the response body on a failed response), and add a comment at line 445 along the lines of "// Note that for privacy/security reasons we don't log this response body", I think you've covered all the cases.

Thanks!

Comment 5

[Pavan Veginati]

Attachment: Bug 1470034 - Remove x-client-state and hawk auth header trace logs.

Review commit: https://reviewboard.mozilla.org/r/254010/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/254010/

Comment 6

[Pavan Veginati]

Hi, I submitted a review request based on suggested changes. 

Both x-client-state and hawk auth headers including JSON response from https://token.services.mozilla.com/1.0/sync/1.5 are not logged anymore.

However if we set 'services.common.hawk.log.sensitive', "pushPublicKey" and "pushAuthKey" will still be logged from here:

https://searchfox.org/mozilla-central/source/services/common/hawkclient.js#231

Comment 7

[Mark Hammond [:markh] [:mhammond]]

(In reply to Pavan Veginati from comment #6)
> Hi, I submitted a review request based on suggested changes. 

Awesome, thanks! For future reference, if you add "r?reviewer" (eg, "r?markh" in this case), then your chosen reviewer will get notified of the review request (and if you choose wrong, the person you chose can just pass it on to someone else). No bug deal though - I just pushed the change to the "try server" and if it passes (which I suspect it will), I'll r+ it and land it.

> However if we set 'services.common.hawk.log.sensitive', "pushPublicKey" and
> "pushAuthKey" will still be logged from here:

I think that's fine - it seems unlikely someone will accidentally set such a pref and there are some other sensitive things already logged when that pref is set - if we decide to do something about that, I think we should do it in a different bug.

Thanks!

Comment 8

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8988852 [details]
Bug 1470034 - Remove x-client-state and hawk auth header trace logs.

https://reviewboard.mozilla.org/r/254010/#review260910

Looks great, thanks!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/67d9f01cbbec
Remove x-client-state and hawk auth header trace logs. r=markh

Comment 10

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/67d9f01cbbec