Add custom verifier to hostname verification by HTTP client #3610
Conversation
core/org.wso2.carbon.utils/src/main/java/org/wso2/carbon/utils/CustomHostNameVerifier.java
Show resolved
Hide resolved
core/org.wso2.carbon.utils/src/main/java/org/wso2/carbon/utils/CustomHostNameVerifier.java
Outdated
Show resolved
Hide resolved
core/org.wso2.carbon.utils/src/main/java/org/wso2/carbon/utils/CustomHostNameVerifier.java
Outdated
Show resolved
Hide resolved
core/org.wso2.carbon.utils/src/main/java/org/wso2/carbon/utils/CustomHostNameVerifier.java
Outdated
Show resolved
Hide resolved
core/org.wso2.carbon.utils/src/main/java/org/wso2/carbon/utils/HTTPClientUtils.java
Show resolved
Hide resolved
| * | ||
| * @return HttpClientBuilder. | ||
| */ | ||
| public static HttpClientBuilder getHTTPClientWithCustomHostNameVerifier() { |
There was a problem hiding this comment.
how about createClientWithCustomVerifier?
LakshiAthapaththu
force-pushed
the
httpclient_with_custom_verifier
branch
from
a170f8d
to
2d903fb
Compare
| if (isValidCommonNames && !ArrayUtils.contains(subjectAlternativeNames, commonNames[0])) { | ||
| subjectAltsWithLocalhosts = ArrayUtils.add(subjectAltsWithLocalhosts, commonNames[0]); | ||
| } | ||
| this.verify(hostname, commonNames, subjectAltsWithLocalhosts, false); |
There was a problem hiding this comment.
shouldn't this be super.verify?
|
|
||
| String[] subjectAltsWithLocalhosts = ArrayUtils.addAll(subjectAlternativeNames, LOCALHOSTS); | ||
|
|
||
| boolean isValidCommonNames = Optional.ofNullable(commonNames) |
There was a problem hiding this comment.
let's rename to hasValidCommonNames
parent/pom.xml
Outdated
| <groupId>org.wso2.orbit.org.apache.commons</groupId> | ||
| <artifactId>commons-lang3</artifactId> | ||
| <version>${commons-lang3.orbit.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
I think commons lang library should already be available. Do we need need lang3 APIs for the ArrayUtils ops? Wouldn't the existing ones be enough?
|
PR builder started |
|
PR builder completed |
There was a problem hiding this comment.
Approving the pull request based on the successful pr build https://github.com/wso2/product-is/actions/runs/5583544562
Purpose
As explained in wso2/product-is#16181
localhostis not required to be added as a SAN in the certificate even in the flows which uselocalhostas the internal hostname ( eg: Self registration flow). But currently whenlocalhostis not available as a SAN, some internal API calls fail. Sometimes back a fix has been implemented to ignorelocalhostverification in wso2/carbon-identity-framework#4262 by introducing a custom verifier to the HTTP client. But in two places this customized HTTP client is not used and therefore self-registration gets failed. As these places are in two different modules, the HTTP client with custom hostname verification has been moved to the kernel as a util method through this PR. Further, we expect to use the same HTTP client for any internal API calls to avoid such issues in the future.Goals
Resolves the issue of self-registration flow does not work when
localhostis removed from the SAN.Approach
From this PR, an HTTP client creation with customized hostname verification (introduced in wso2/carbon-identity-framework#4262) logic is moved to the Kernal.
Further as per the issue wso2/product-is#16182 when hostname and internal hostname are changed according to option 2 in https://is.docs.wso2.com/en/latest/deploy/change-the-hostname/ an error is observed. This is because even when the hostname and CN are equal when we give a list of subject alternative names, the hostname is validated against the SAN. A fix is added to resolve that issue as well.
Documentation
wso2/product-is#16255
Related PRs
wso2/carbon-identity-framework#4794