Skip to content
This repository has been archived by the owner on Nov 30, 2022. It is now read-only.

Stop Fidesops masking uvicorn API logs #766

Closed
adriaaaa opened this issue Jun 30, 2022 · 3 comments · Fixed by #831
Closed

Stop Fidesops masking uvicorn API logs #766

adriaaaa opened this issue Jun 30, 2022 · 3 comments · Fixed by #831
Assignees
Labels
enhancement New feature or request

Comments

@adriaaaa
Copy link

adriaaaa commented Jun 30, 2022

Fidesops will mask all logs by default, even API server logs. This is too much masking as it impedes the debugging abilities of users deploying Fidesops.

The nuance here is that we still need to be confident that all PII gets masked in logs if LOG_PII is False. Some ideas:

  • Create a custom logging class that logs PII and is only used in the traversal layer
  • Create a custom log level for PII
  • (from Catherine) Reverse the logic of NotPii class to Pii, so that by default we do not mask logs, unless we annotate with Pii. This will solve the issue of allowing api server logs to show for Momentive

We would have to do an audit of what wouldn't be masked.

@seanpreston seanpreston added the enhancement New feature or request label Jul 6, 2022
@adriaaaa
Copy link
Author

adriaaaa commented Jul 6, 2022

@pattisdr to review and finalize strategy to prepare this ticket for next sprint

@pattisdr
Copy link
Contributor

pattisdr commented Jul 7, 2022

My first recommendation would be to just merge this one-liner #831 to close this ticket. It's a quick fix that specifically excludes just logs whose name starts with "uvicorn" from being masked.


My other recommendation would be to open a second ticket for the backlog that improves how we log. We actually don't mask much because most of our logs use f-string formatting, and our masking strategy doesn't work with these sorts of logs. In other words, most of our logs are actually treated as NotPii by default. We should move to "%" formatting across all of our logs so these can be appropriately masked (it is the args that are masked). There's a side benefit of using "%" formatting in that it is evaluated lazily and only calculated if the message is actually emitted.

Switching how we format logs will cause most things to be masked, so we should move to Catherine's point of changing logic from NotPii to Pii. Then we switch our logic to just mask Pii and label the few items that are truly Pii. Moving to % formatting everywhere will make it more likely that all developers continue to use this format, and the logs are capable of being masked if we need to.

New ticket should in short:

  1. Create a new Pii class and flip the logic to mask if Pii instead of don't mask if NotPii
  2. Locate the handful of existing logs that currently use %s formatting, that are missing the NotPii label and wrap their arguments in Pii. These are the only logs that we've been masking and we want to continue to do so.
  3. Switch remaining logs to use % style formatting instead of f-string (154 info logs, 53 warnings, 17 errors).
  4. Audit current logs to see if there's anything else that should be labeled as Pii (there shouldn't be much. most of our logs don't contain real Pii).
  5. Get rid of the NotPii class.

eastandwestwind pushed a commit that referenced this issue Jul 7, 2022
* Update get_fides_log_record_factory to skip masking of all uvicorn logs.

* Update changelog.
@pattisdr
Copy link
Contributor

pattisdr commented Jul 8, 2022

Follow-up reticketed here: #837

@pattisdr pattisdr self-assigned this Jul 8, 2022
sanders41 pushed a commit that referenced this issue Sep 22, 2022
* Update get_fides_log_record_factory to skip masking of all uvicorn logs.

* Update changelog.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants