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

Ensure clean http url #538

Merged
merged 13 commits into from
Jun 11, 2021
Merged

Conversation

ryokather
Copy link
Contributor

@ryokather ryokather commented Jun 7, 2021

Description

This PR fixes issue #367 in which a new specification was established such that http.url, a span attribute, must not contain username or password information for security purposes. This would ensure that URLs passed in the form of https://username:password@www.example.com/ would result in a http span attribute value of https://www.example.com/.

This issue was fixed by implementing an additional util function in util/open-telemetry-http called remove_url_credentials(url: str) -> str. This function utilizes the urllib.parse module to cleanly strip a username and password from a given url if present. This fix was applied to the following instrumentations: aiohttp-client, asgi, requests, tornado, urllib, and wsgi. Note that the current implementation of the urllib3 instrumentation would not process the username and password of a url so this logic was not needed.

Type of change

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

An additional test called test_credential_removal was provided in every instrumentation that sets the HTTP_URL attribute. This includes the instrumentations aiohttp-client, asgi, requests, tornado, urllib, urllib3, and wsgi. Tests were run using tox.

Does This PR Require a Core Repo Change?

  • Yes. - Link to PR:
  • No.

Checklist:

See contributing.md for styleguide, changelog guidelines, and more.

  • Followed the style guidelines of this project
  • Changelogs have been updated
  • Unit tests have been added
  • Documentation has been updated

Ryo Kather added 9 commits June 7, 2021 12:53
Restored aiohttp test due to overwritten test

Readded aiohttp url credential test

Addressed yarl not found in tornado client in some tests
Readded removed dependency

Added dependency for docker test

Fixed linting errors

Placed yarl dependency and moved common function into utils
Rearranged location of required install

Fixed dependency location and linting
resolve pylint

Fixed linting and changed return type to str
@ryokather ryokather requested review from a team, aabmass and ocelotl and removed request for a team June 7, 2021 20:38
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Jun 7, 2021

CLA Signed

The committers are authorized under a signed CLA.

opentelemetry-instrumentation/setup.cfg Outdated Show resolved Hide resolved
@ryokather ryokather requested a review from ocelotl June 8, 2021 20:14
@@ -60,3 +61,33 @@ def unwrap(obj, attr: str):
func = getattr(obj, attr, None)
if func and isinstance(func, ObjectProxy) and hasattr(func, "__wrapped__"):
setattr(obj, attr, func.__wrapped__)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this logic be better off here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only thought I had was that it is most similar to the other utility function http_status_to_status_code in the same file and the library you linked is only used by web server instrumentations. However, it might make more logical sense to separate it from the opentelemetry-instrumentation itself. What would you suggest? @lzchen

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe the library would be used for all http related instrumentations, it just so happens the logic contained is only used in server instrumentations.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good 👍 Just updated with the last remaining suggestions.

@lzchen
Copy link
Contributor

lzchen commented Jun 10, 2021

@ryokather
Please rebase and then we can merge this!

@ryokather
Copy link
Contributor Author

Should be done!

@lzchen lzchen merged commit 865837f into open-telemetry:main Jun 11, 2021
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.

3 participants