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

Add the client cert and key support to HttpTransport #3258

Conversation

grammy-jiang
Copy link
Contributor

@grammy-jiang grammy-jiang commented Jul 9, 2024

Enable 2-way SSL authentication support in sentry-sdk #3256

The document of self-hosted Sentry strongly recommends using a load balancer in front of Sentry. And when the load balancer enables two-way SSL authentication, this pull request makes sentry-sdk allow to pass the client cert and key file to urllib3.PoolManager.

This pull request adds two new key arguments to sentry_sdk.init:

import sentry_sdk

sentry_sdk.init(
    dsn="https://[email protected]/0",
    ...,
    ca_certs="THE PATH OF ROOT CERTIFICATE",
    # The two new key arguments
    cert_file="THE PATH OF CERTIFICATE",
    key_file="THE PATH OF KEY",
)

General Notes

Thank you for contributing to sentry-python!

Please add tests to validate your changes, and lint your code using tox -e linters.

Running the test suite on your PR might require maintainer approval. Some tests (AWS Lambda) additionally require a maintainer to add a special label to run and will fail if the label is not present.

For maintainers

Sensitive test suites require maintainer review to ensure that tests do not compromise our secrets. This review must be repeated after any code revisions.

Before running sensitive test suites, please carefully check the PR. Then, apply the Trigger: tests using secrets label. The label will be removed after any code changes to enforce our policy requiring maintainers to review all code revisions before running sensitive tests.

@grammy-jiang grammy-jiang marked this pull request as ready for review July 9, 2024 11:51
Copy link
Member

@szokeasaurusrex szokeasaurusrex left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution!

Before we can merge this, you also need to make sure to add the new options here, so they show up in the type hint for sentry_sdk.init.

Also, please add tests for your change

@grammy-jiang
Copy link
Contributor Author

grammy-jiang commented Jul 9, 2024

Thanks, @szokeasaurusrex .

The test case is added. The two new arguments are added to ClientConstructor too.

The test case can pass locally in pytest. Not sure how thing is going on in the pipeline. Let's see.

sentry_sdk/consts.py Outdated Show resolved Hide resolved
Copy link
Member

@sl0thentr0py sl0thentr0py left a comment

Choose a reason for hiding this comment

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

ty!

@sl0thentr0py sl0thentr0py added the Trigger: tests using secrets PR code is safe; run CI label Jul 10, 2024
Copy link

codecov bot commented Jul 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.41%. Comparing base (2cb2dca) to head (6037b5b).
Report is 6 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3258      +/-   ##
==========================================
- Coverage   79.41%   79.41%   -0.01%     
==========================================
  Files         132      132              
  Lines       14263    14265       +2     
  Branches     2992     2992              
==========================================
+ Hits        11327    11328       +1     
  Misses       2091     2091              
- Partials      845      846       +1     
Files Coverage Δ
sentry_sdk/consts.py 91.62% <ø> (ø)
sentry_sdk/transport.py 81.43% <100.00%> (+0.12%) ⬆️

... and 1 file with indirect coverage changes

@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Jul 11, 2024
@sl0thentr0py sl0thentr0py added the Trigger: tests using secrets PR code is safe; run CI label Jul 11, 2024
@github-actions github-actions bot removed the Trigger: tests using secrets PR code is safe; run CI label Jul 11, 2024
@sl0thentr0py
Copy link
Member

sl0thentr0py commented Jul 12, 2024

@szokeasaurusrex needs to approve this too because his request changes status is blocking the merge

@sl0thentr0py sl0thentr0py added the Trigger: tests using secrets PR code is safe; run CI label Jul 12, 2024
@sl0thentr0py sl0thentr0py enabled auto-merge (squash) July 12, 2024 12:35
@sl0thentr0py sl0thentr0py merged commit 4fb51f2 into getsentry:master Jul 12, 2024
123 of 125 checks passed
@grammy-jiang grammy-jiang deleted the Load-Balancer-with-2-way-SSL-authentication-support branch July 12, 2024 13:09
arjennienhuis pushed a commit to arjennienhuis/sentry-python that referenced this pull request Sep 30, 2024
* Add the client cert and key support to HttpTransport

* Add a test case for the two-way ssl support in HttpTransport

* Move cert_file and key_file to the end of arguments in ClientConstructor in consts.py

---------

Co-authored-by: Neel Shah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Trigger: tests using secrets PR code is safe; run CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants