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

Remove HostnameVerifier option #3150

Merged
merged 4 commits into from
Jan 18, 2024
Merged

Conversation

romtsn
Copy link
Member

@romtsn romtsn commented Jan 18, 2024

📜 Description

This was mainly used by self-hosted users who has a self-signed certificate. There will be 3 options for them going forward:

  • Provide a custom ITransportFactory, where they can copy over most parts like HttpConnection and AsyncHttpTransport with necessary modifications
  • Get a certificate for their server through e.g. Let's Encrypt
  • Fork the SDK and add the hostname verifier back

It's a breaking change, but we agreed that the impact is gonna be so small that it's not worth waiting for the next major, given that it produces so many false positives by security tools of some app stores

💡 Motivation and Context

Closes getsentry/sentry-unity#1513

💚 How did you test it?

📝 Checklist

  • I reviewed the submitted code.
  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

@romtsn
Copy link
Member Author

romtsn commented Jan 18, 2024

@bitsandfoxes the problem is gonna resolve itself after merging :P

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Jan 18, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 351.65 ms 416.65 ms 65.00 ms
Size 1.70 MiB 2.27 MiB 583.82 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
b172d4e 413.47 ms 479.02 ms 65.55 ms
bc4be3b 360.40 ms 435.04 ms 74.64 ms
7eccfdb 389.94 ms 461.29 ms 71.35 ms
7eccfdb 366.98 ms 440.27 ms 73.29 ms
4e260b3 388.40 ms 468.98 ms 80.58 ms
8838e01 387.41 ms 467.00 ms 79.59 ms
c7e2fbc 398.35 ms 468.52 ms 70.17 ms
d6d2b2e 463.14 ms 545.56 ms 82.42 ms
93a76ca 397.30 ms 455.16 ms 57.87 ms
0bd723b 412.52 ms 496.65 ms 84.13 ms

App size

Revision Plain With Sentry Diff
b172d4e 1.72 MiB 2.29 MiB 578.09 KiB
bc4be3b 1.72 MiB 2.29 MiB 576.53 KiB
7eccfdb 1.72 MiB 2.27 MiB 556.93 KiB
7eccfdb 1.72 MiB 2.27 MiB 556.93 KiB
4e260b3 1.72 MiB 2.27 MiB 554.95 KiB
8838e01 1.72 MiB 2.29 MiB 578.15 KiB
c7e2fbc 1.72 MiB 2.29 MiB 576.40 KiB
d6d2b2e 1.72 MiB 2.27 MiB 555.05 KiB
93a76ca 1.72 MiB 2.29 MiB 576.75 KiB
0bd723b 1.72 MiB 2.29 MiB 578.09 KiB

Previous results on branch: rz/chore/remove-hostname-verifier

Startup times

Revision Plain With Sentry Diff
2451d5e 402.38 ms 484.73 ms 82.35 ms
9e5a522 426.67 ms 574.59 ms 147.93 ms

App size

Revision Plain With Sentry Diff
2451d5e 1.70 MiB 2.27 MiB 583.82 KiB
9e5a522 1.70 MiB 2.27 MiB 583.82 KiB

CHANGELOG.md Outdated Show resolved Hide resolved
@romtsn romtsn enabled auto-merge (squash) January 18, 2024 10:04
@romtsn romtsn merged commit 2465853 into main Jan 18, 2024
20 checks passed
@romtsn romtsn deleted the rz/chore/remove-hostname-verifier branch January 18, 2024 10:16
@nienienienie
Copy link

which exactly?

it's flagged by security tools of some app stores

https://github.com/getsentry/sentry-java/releases/tag/7.3.0

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.

Expose hostnameVerifier in the Unity SDK settings
4 participants