Skip to content

Disable hostname verification when ssl validation is disabled. #798

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

Merged
merged 1 commit into from
Dec 12, 2022

Conversation

emopti-jrufer
Copy link
Contributor

Fixes gh-625.

Prior to Apache Http Client5 org.springframework.cloud.commons.httpclient.DefaultApacheHttpClientConnectionManagerFactory was being used which added the NoopHostnameVerifier. See https://github.com/spring-cloud/spring-cloud-commons/blob/e72fc6c6906cab8dedd1277dccffb5aeca99aec6/spring-cloud-commons/src/main/java/org/springframework/cloud/commons/httpclient/DefaultApacheHttpClientConnectionManagerFactory.java#L69

@emopti-cmeyer
Copy link

@ryanjbaxter @OlgaMaciaszek hoping you can take a look and get this merged before release to avoid unnecessary breakage. This is a 1-line fix for an issue that was introduced by the switch from Apache HttpClient 4 to 5.

@ryanjbaxter committed similar code for OkHttpClient a few weeks ago here: 1ec3c65

All this pull request does is to make sure that when the spring.cloud.openfeign.httpclient.disable-ssl-validation property is set to true, the HttpClient also ignores host name mismatches (e.g. the common name from the certificate doesn't have to match).

As described in the original pull request description, the Apache HttpClient 4 implementation was also using the NoopHostnameVerifier and as mentioned in my comment here w/ link, the OkHttpClient does the same.

@codecov
Copy link

codecov bot commented Dec 12, 2022

Codecov Report

Merging #798 (f5abf98) into main (9354054) will decrease coverage by 0.03%.
The diff coverage is 0.00%.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #798      +/-   ##
============================================
- Coverage     74.51%   74.47%   -0.04%     
  Complexity      569      569              
============================================
  Files            67       67              
  Lines          2201     2202       +1     
  Branches        297      297              
============================================
  Hits           1640     1640              
- Misses          394      395       +1     
  Partials        167      167              
Impacted Files Coverage Δ
...gn/clientconfig/HttpClient5FeignConfiguration.java 66.66% <0.00%> (-1.63%) ⬇️

@OlgaMaciaszek OlgaMaciaszek self-assigned this Dec 12, 2022
@OlgaMaciaszek OlgaMaciaszek added this to the 4.0.0 milestone Dec 12, 2022
Copy link
Collaborator

@OlgaMaciaszek OlgaMaciaszek left a comment

Choose a reason for hiding this comment

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

Thanks @emopti-jrufer . LGTM.

@OlgaMaciaszek OlgaMaciaszek added bug Something isn't working and removed in progress labels Dec 12, 2022
@OlgaMaciaszek OlgaMaciaszek merged commit 8a1f888 into spring-cloud:main Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

For hc5 hostname verification disable-ssl-validation does not turn off hostname verification
4 participants