Skip to content
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

Use whitelists rather then blacklists for header and query annotations #64

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

drpacman
Copy link
Contributor

Hi Lev,
It is hard to know upfront all possible security related headers (e.g. containing authentication tokens/user info etc) which risks leaking sensitive info into Zipkin traces.

My previous implementation took a blacklisting approach. Based on the above security concerns, I now realise it is more sensible to have use a whitelist for inclusion of query params and header values then a blacklist.

… good practise - a blacklist as originally implemented requires infinite knowledge so things slip through...
@levkhomich levkhomich force-pushed the master branch 4 times, most recently from fc8ad34 to fb4a6a1 Compare March 16, 2016 22:06
@drpacman
Copy link
Contributor Author

drpacman commented Apr 6, 2016

Hi Lev,
Good to see recent activity again on this repo. I have remerged from your updated master and resolved merge conflicts to make specific headers included. We have seen in our exploratory usage of akka-tracing that this feature is necessary as we have security credentials in headers (which vary across products) which must not be logged

@coveralls
Copy link

Coverage Status

Coverage increased (+0.3%) to 88.18% when pulling 4c6ff8e on drpacman:master into fb4a6a1 on levkhomich:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 88.112% when pulling 4c6ff8e on drpacman:master into fb4a6a1 on levkhomich:master.

@levkhomich levkhomich force-pushed the master branch 5 times, most recently from 7929214 to e9a090e Compare August 14, 2017 22:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants