-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BB2-3486: Mask mbi in logs #1252
BB2-3486: Mask mbi in logs #1252
Conversation
d6512b2
to
1358f3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tests look great, code looks great! Just would want some more local validation before approving, I can do that early next week, or if somebody else can take that over, we should be good to go. Just some validation that this works correctly when the server is run and an MBI-match is passed in. Some steps for that in the description would be great too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some nitch cases:
lower case char in MBI, even though the CMS doc says:
refer to: https://www.cms.gov/medicare/new-medicare-card/understanding-the-mbi-with-format.pdf
"
What kinds of characters are in the MBI? MBIs are numbers and upper-case letters. We use numbers 1-9 and all letters from A to Z, except for S, L, O, I, B, and Z. If you use lowercase letters, our system will convert them to uppercase letters."
considering the mbi literals could come from various sources : human type in, client code sent, etc....
might consider also detect MBI word with lower case...
Did some manual tests - works as expected, e.g.
`>>> result=re.sub(MBI_PATTERN, 'MBI', "1EG4-TE5-MK74 XXX 1EG4TE5MK74", flags=re.VERBOSE)
print(result)
MBI XXX MBI
result=re.sub(MBI_PATTERN, 'MBI', "1EG4-TE5-MK74 XXX 1EG4tE5Mk74", flags=re.VERBOSE)
print(result)
MBI XXX 1EG4tE5Mk74`
9f872ca
to
a54a97b
Compare
Moving to draft while testing steps get updated and additional code changes are pending. |
a54a97b
to
6b2e122
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind explaining some of these changes? I'm not too familiar with how some of this is set up, but curious for instance about why the django
section needed to be added. I'm doing some reading on this to learn too, but if you can provide some starting context, that might help!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you check these logs
web-1 | 2024-10-17 14:59:44,183 WARNING [35] django.request line:241 Unauthorized: /v2/fhir/Patient/identifier=***MBI***
web-1 | 2024-10-17 14:59:44,187 WARNING [35] django.server line:212 "GET //v2/fhir/Patient/identifier=***MBI*** HTTP/1.1"
Here these logs are coming from
django.request
and django.server
Adding django to list of loggers will make sure we are capturing all loggers in filter to hide sensitive information.
If we remove this, then some of logs can still log MBI without going via filter. In this case
web-1 | 2024-10-17 14:59:44,187 WARNING [35] django.server line:212 "GET //v2/fhir/Patient/identifier=***MBI*** HTTP/1.1"
MBI will be exposed if we don't have that entry in list of loggers
a67bc7d
to
a52bf70
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks mostly good, just two more minor comments but hopefully we can resolve this soon.
dd35063
to
a11834c
Compare
JIRA Ticket:
BB2-3486
What Does This PR Do?
Here we have created sensitive data filter for logging which is going to mask all data across all logs
What Should Reviewers Watch For?
If you're reviewing this PR, please check for these things in particular:
Validation
Make GET request
1EG0TE5MK74 -- This fake mbi
By making below request mbi is masked with
***MBI***
What Security Implications Does This PR Have?
Please indicate if this PR does any of the following:
security engineer's approval.
Any Migrations?
etc)