[sssd PR#585][opened] tcurl: do not log the payload

[fidencio]

URL: https://github.com/SSSD/sssd/pull/585
Author: fidencio
 Title: #585: tcurl: do not log the payload
Action: opened

PR body:
"""
We should never ever log the payload. In order to avoid doing this,
let's parse the data and replace the payload content for "...".

One example of the log before the patch is:
(Wed May 30 21:43:49 2018) [sssd[kcm]] [tcurl_write_data] (0x2000): ---> begin libcurl data
(Wed May 30 21:43:49 2018) [sssd[kcm]] [tcurl_write_data] (0x2000): {
    "version": 1,
    "kdc_offset": 0,
    "principal": {
        "type": 0,
        "realm": "DOMAIN.TEST",
        "components": [
            "admin"
        ]
    },
    "creds": [
            "uuid": "f87d2975-fc14-4cb8-8275-f0e474f82403",
            "payload": "AAAAAAAAAAEAAAAPS05PV05IT1NUUy5URVNUAAAABWFkbWluAAAAAAAAAAMAAAAMWC1DQUNIRUNPTkY6AAAAFWtyYjVfY2NhY2hlX2NvbmZfZGF0YQAAAApmYXN0X2F2YWlsAAAAJmtyYnRndC9LTk9XTkhPU1RTLlRFU1RAS05PV05IT1NUUy5URVNUAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAADeWVzAAAAAA=="
        }
    ]
}
(Wed May 30 21:43:49 2018) [sssd[kcm]] [tcurl_write_data] (0x2000): <--- end libcurl data

And after the patch:
(Wed May 30 21:45:19 2018) [sssd[kcm]] [tcurl_print_sanitized_data] (0x2000): ---> begin libcurl data
(Wed May 30 21:45:19 2018) [sssd[kcm]] [tcurl_print_sanitized_data] (0x2000): {
    "version": 1,
    "kdc_offset": 0,
    "principal": {
        "type": 0,
        "realm": "DOMAIN.TEST",
        "components": [
            "admin"
        ]
    },
    "creds": [
        {
            "uuid": "727b7a15-eba7-4d44-aebe-dc6d1ecaf5f4",
            "payload": "..."
        }
    ]
}
(Wed May 30 21:45:19 2018) [sssd[kcm]] [tcurl_print_sanitized_data] (0x2000): <--- end libcurl data

Resolves:
https://pagure.io/SSSD/sssd/issue/3674

Signed-off-by: Fabiano Fidêncio 
"""

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/585/head:pr585
git checkout pr585

[sssd PR#585][comment] tcurl: do not log the payload

[fidencio]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

fidencio commented:
"""
CI: http://vm-031.$%7Babc%7Dlogs/job/89/61/summary.html
"""

See the full comment at https://github.com/SSSD/sssd/pull/585#issuecomment-393429297

[sssd PR#585][comment] tcurl: do not log the payload

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

jhrozek commented:
"""
Thank you for working on the issue. The patch works, but I have some suggestions how to improve the code.

First, the code is quite expensive. At the very least, it should only ever be called if a high debug level is set.

But primarily I'm thinking that the tcurl module might not even be the right place to sanitize the logs, because the tcurl module knows nothing about what goes through it. It might even happen that some generic HTTP code would go through which would contain "payload" and that would mess up the debugging.

So I'm actually wondering if the logging should happen primarily in the secrets KCM back end where we could dump the JSON, but replace some items in the JSON.

About the logging in the tcurl module, I'm thinking it is still valuable for developers, but we might only allow it if SSSD is compiled with some #define or some environment variable is set.

What do you think?
"""

See the full comment at https://github.com/SSSD/sssd/pull/585#issuecomment-393864147

[sssd PR#585][comment] tcurl: do not log the payload

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

jhrozek commented:
"""
btw since we are trying to get a release out of the door, maybe a stopgap fix could be to just put some #ifdefs around the tcurl print function and then see if we can log the sanitized JSON?
"""

See the full comment at https://github.com/SSSD/sssd/pull/585#issuecomment-393864401

[sssd PR#585][+Changes requested] tcurl: do not log the payload

[fidencio]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

Label: +Changes requested

[sssd PR#585][synchronized] tcurl: do not log the payload

[fidencio]

URL: https://github.com/SSSD/sssd/pull/585
Author: fidencio
 Title: #585: tcurl: do not log the payload
Action: synchronized

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/585/head:pr585
git checkout pr585

[sssd PR#585][comment] tcurl: do not log the payload

[fidencio]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

fidencio commented:
"""
I have updated the patch with a stopgap fix.
"""

See the full comment at https://github.com/SSSD/sssd/pull/585#issuecomment-394688792

[sssd PR#585][-Changes requested] tcurl: do not log the payload

[fidencio]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

Label: -Changes requested

[sssd PR#585][comment] tcurl: do not log the payload

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

jhrozek commented:
"""
Ack. If someone can set an environment variable in SSSD there are more harmful things they can do than logging a TGT value into a file only readable by root.
"""

See the full comment at https://github.com/SSSD/sssd/pull/585#issuecomment-394835399

[sssd PR#585][+Accepted] tcurl: do not log the payload

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

Label: +Accepted

[sssd PR#585][comment] tcurl: do not log the payload

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

jhrozek commented:
"""
* master: f8025ae01699b5221079a4ee2c6111c514642ce4
"""

See the full comment at https://github.com/SSSD/sssd/pull/585#issuecomment-395765273

[sssd PR#585][closed] tcurl: do not log the payload

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/585
Author: fidencio
 Title: #585: tcurl: do not log the payload
Action: closed

To pull the PR as Git branch:
git remote add ghsssd https://github.com/SSSD/sssd
git fetch ghsssd pull/585/head:pr585
git checkout pr585

[sssd PR#585][+Pushed] tcurl: do not log the payload

[jhrozek]

URL: https://github.com/SSSD/sssd/pull/585
Title: #585: tcurl: do not log the payload

Label: +Pushed