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

Update MandrillRequestDispatcher.java -->TLSv1.2 #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

therealnagaka
Copy link

Dealing with Issue #86 ; Will force usage of TLSv1.2 ;

@billoneil
Copy link
Collaborator

Sorry I didn't see this PR just the issue. To be honest I'm not entirely confident how to set up the SSL properly and wouldn't want to set bad defaults for all clients. I think a better solution would be a way to allow passing in / setting the global httpClient. This would allow your fix to be possible as well as any other customizations someone might need in the future.

The con there is that it would be leaking out the internal implementation (apache http client). I don't think that is uncommon for libraries like this though. What would your thoughts be?

@therealnagaka
Copy link
Author

I think that does make more sense than my proposed forced fix. Part of my hesitation with submitting the PR was that this issue is completely non-existent with java1.8 or newer (I don't know how many others would even need the fix); I think leaving the default behavior as is, but also allowing a pass-in/global httpClient would solve both problems. I'm not sure how that implementation would be done though.

@rschreijer
Copy link
Owner

Can't we do this with some flag? The RequestDispatcher is only used from MandrillUtil.query. Maybe you can make a 'RequestDispatcherWithExplicitTLS', and MandrillUtil.query will decide which one to use based on that flag. By default it changes nothing. If flag is set it uses your new RequestDispatcher.

@billoneil
Copy link
Collaborator

@rschreijer my thoughts are eventually someone will ask to be able to customize the http client. Whether its changing the number of connections per host, gzip, logging, any custom filters making the client configurable could solve all of those but leak the implementation a little.

I think either solution could work.

@lc-nyovchev
Copy link
Contributor

@billoneil your approach sounds better, but maybe instead of leaking the impl, we can just have a higher level /get/post interface, which can have (configurable) settings (and, of course, the http client thing should just be default impl).

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.

4 participants