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 memorizing trust manager #70

Closed

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Feb 28, 2020

This is based on #69, so merge that one first.

I would propose to remove the MemorizingTrustManager. It's a nice tool, but it increases maintenance effort. Right now, we have a conflict between the build tools that we use ourselves and the build tools that the library uses. It's fixable of course, but on the other hand, we are accessing purely public information. Do we really need to verify fingerprints of untrusted TLS certificates to access that public information?

What are your thoughts, @rorist?

@rorist
Copy link
Member

rorist commented Feb 28, 2020

I see no problem removing this lib, this was not really needed as there nothing critical done with the data as far as I can see. This was just feeling wrong to disable cert check :)

Also it possibly introduced some memory leak propagating the app context to this 3rd party..

It was added in b8b9ec4

Would you need to disable cert validation though ? something like this 2cecb98#diff-aa8b212ccdffa5693a50f4b15c83ee1fR39

@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 28, 2020

Would you need to disable cert validation though?

Good question. You're right, right now self-signed certs are rejected:

2020-02-28-143717_447x602_scrot

In times where getting a valid certificate is easy (with Let's Encrypt), do we want to allow self-signed certificates, or do we want to require a valid trust anchor?

(Note that plain-HTTP URLs are still possible if someone does not want to set up HTTPS.)

(CC @gidsi @rnestler)

@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 28, 2020

@rorist I'll merge the trust manager removal commit into the "updates" PR (#69), since it prevents the tests from succeeding. We can discuss the trust settings in another issue :)

@dbrgn dbrgn closed this Feb 28, 2020
@rnestler rnestler deleted the remove-memorizing-trust-manager branch April 27, 2020 16:15
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.

None yet

2 participants