getsentry / sentry

Developer-first error tracking and performance monitoring
https://sentry.io
Other
39.07k stars 4.19k forks source link

SAML tls certificate trust can be bypassed by modifying a valid saml server configuration with an invalid one #32758

Open ABVitali opened 2 years ago

ABVitali commented 2 years ago

Environment

self-hosted (https://develop.sentry.dev/self-hosted/)

Version

22.3.0.dev0

Steps to Reproduce

1) Configure a self signed certificate for your SAML server (In my case I created a certificate valid only for the IP of my SAML server) 2) Try to configure sentry to use that SAML server, you obtain an error because sentry can't trust the SAML server certificate.

3) add the certificate to the list of trusted certificates in the Sentry instance 4) configure again Sentry to use the SAML server. This time this will be possible because of step 3

5) Modify the auth conf IN sentry changing for example the IP with a Domain name, notice that the certificate is now again invalid for this new "name" of the SAML server.

6) modify the conf of the SAML server to respond to the requests using the new domain name.

Expected Result

I expect that at point 5 it is not allowed to change the Sentry auth config without verifying again the certificate validity. The correct flow in my opnion would be to force the removal of the SAML login, the creation of a new one passing by the certificate validity checks done in point 2 of my previous list.

Actual Result

I can modify the SAML auth configuration and simply use my insecure SAML server. Of course in my case this is fine since I'm just testing everything. My point is more about the "consistency" of the certificate validation strategy.

I really hope that what I wrote is clear, let me know if you need any integration and/or screenshot. Thank you in advance!

getsentry-release commented 2 years ago

Routing to @getsentry/enterprise for triage. ⏲️

maxiuyuan commented 2 years ago

Thank you! i was able to reproduce this and will make a ticket in our backlog