Add tlsNoVerifyHosts to disable TLS verification for certain hosts #3057

tumile · 2020-09-11T21:41:15Z

Motivation:
Provide a way to turn off TLS verification for specific hosts (like tlsNoVerify but on specific hosts only) in private development environments. See comment.

Modification:

Add ClientFactoryBuilder#tlsNoVerifyHosts(String... insecureHosts).
Add IgnoreHostsTrustManager implementation. Refer to this commit from okhttp.

Result:
Close #2722

CLAassistant · 2020-09-11T21:41:20Z

All committers have signed the CLA.

codecov · 2020-09-11T22:01:19Z

Codecov Report

Merging #3057 into master will increase coverage by 0.01%.
The diff coverage is 91.89%.

@@             Coverage Diff              @@
##             master    #3057      +/-   ##
============================================
+ Coverage     73.40%   73.42%   +0.01%     
- Complexity    12306    12325      +19     
============================================
  Files          1066     1067       +1     
  Lines         47623    47659      +36     
  Branches       6009     6013       +4     
============================================
+ Hits          34959    34994      +35     
+ Misses         9611     9610       -1     
- Partials       3053     3055       +2

Impacted Files	Coverage Δ	Complexity Δ
...necorp/armeria/client/IgnoreHostsTrustManager.java	`88.88% <88.88%> (ø)`	`11.00 <11.00> (?)`
.../linecorp/armeria/client/ClientFactoryBuilder.java	`73.68% <100.00%> (+1.30%)`	`57.00 <5.00> (+6.00)`
.../armeria/client/endpoint/dns/DnsEndpointGroup.java	`78.12% <0.00%> (-3.13%)`	`13.00% <0.00%> (-2.00%)`
...com/linecorp/armeria/server/saml/SamlEndpoint.java	`62.50% <0.00%> (-2.50%)`	`10.00% <0.00%> (-1.00%)`
.../linecorp/armeria/client/Http2ResponseDecoder.java	`70.76% <0.00%> (-2.31%)`	`40.00% <0.00%> (ø%)`
.../linecorp/armeria/client/DefaultClientFactory.java	`64.51% <0.00%> (-2.16%)`	`27.00% <0.00%> (-1.00%)`
...p/armeria/common/stream/AbstractStreamMessage.java	`82.85% <0.00%> (-1.91%)`	`16.00% <0.00%> (ø%)`
...rp/armeria/common/stream/DefaultStreamMessage.java	`85.71% <0.00%> (-1.03%)`	`67.00% <0.00%> (ø%)`
...a/common/grpc/protocol/ArmeriaMessageDeframer.java	`74.42% <0.00%> (-0.46%)`	`53.00% <0.00%> (-1.00%)`
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9cbd2cc...c3abb87. Read the comment docs.

ikhoon

Mostly LGTM!

core/src/test/java/com/linecorp/armeria/client/IgnoreHostsTrustManagerTest.java

core/src/test/java/com/linecorp/armeria/client/HttpClientSniTest.java

tumile · 2020-09-18T17:18:58Z

Thanks @ikhoon! I've updated to address your reviews.

minwoox · 2020-09-19T13:57:57Z

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java

+    @Override
+    public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
+            throws CertificateException {
+        throw new UnsupportedOperationException();


Shouldn't we also implement this method?

It does not give access to the hostname so I wasn't sure if it should be implemented.
It's just me if you think otherwise I'll add it 😃

It's probably fine not to implement this method and the method that accepts Socket because we always use an SSLEngine for TLS communication. If we have to implement this, we could retrieve the host name from x509Certificates[0].

Thanks for chiming in. I'm not entirely familiar with TLS so advice is much appreciated here. But it looks to me that we don't need this right now, so maybe it can be added down the road if needed?

trustin

Thanks a lot for your first contribution, @tumile! It will be a nice addition to Armeria once merged. 👍

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

trustin · 2020-09-20T14:48:32Z

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

+    public ClientFactoryBuilder tlsNoVerifyHosts(String... insecureHosts) {
+        tlsCustomizer(b -> b.trustManager(IgnoreHostsTrustManager.of(insecureHosts)));
+        return this;
+    }


A user might want to do the following:

cfb.tlsNoVerifyHosts("a.com"); cfb.tlsNoVerifyHosts("b.com"); ...

How about keeping the list of the insecure hosts and setting the trust manager when the ClientFactory is built? We will also have to make sure tlsNoVerify() and tlsNoVerifyHosts() are mutually exclusive, throwing an IllegalStateException (or should we make tlsNoVerifyHost() always win?)

I agree with making them exclusive. Makes it less ambiguous about which hosts are being ignored (or all).

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java

trustin · 2020-09-20T14:57:54Z

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java

+    @Override
+    public void checkServerTrusted(X509Certificate[] x509Certificates, String s)
+            throws CertificateException {
+        throw new UnsupportedOperationException();


It's probably fine not to implement this method and the method that accepts Socket because we always use an SSLEngine for TLS communication. If we have to implement this, we could retrieve the host name from x509Certificates[0].

minwoox

Thanks!

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java

core/src/test/java/com/linecorp/armeria/client/IgnoreHostsTrustManagerTest.java

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java

core/src/test/java/com/linecorp/armeria/client/IgnoreHostsTrustManagerTest.java

tumile · 2020-09-21T04:08:35Z

@ikhoon thanks a lot! I was working on it, but this is even better 😄

ikhoon · 2020-09-21T04:15:03Z

@ikhoon thanks a lot! I was working on it, but this is even better 😄

Oops... sorry. We are about to release the next version of Armeria. 😅

By the way, if you forked the code from okhttp,

we usually copy the license header together.

armeria/core/src/main/java/com/linecorp/armeria/common/ClientCookieDecoder.java

Lines 16 to 56 in b8b689f

    
           /* 
        
            * Copyright 2015 The Netty Project 
        
            * 
        
            * The Netty Project licenses this file to you under the Apache License, 
        
            * version 2.0 (the "License"); you may not use this file except in compliance 
        
            * with the License. You may obtain a copy of the License at: 
        
            * 
        
            *   http://www.apache.org/licenses/LICENSE-2.0 
        
            * 
        
            * Unless required by applicable law or agreed to in writing, software 
        
            * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT 
        
            * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the 
        
            * License for the specific language governing permissions and limitations 
        
            * under the License. 
        
            */ 
        
           package com.linecorp.armeria.common; 
        
           import static com.linecorp.armeria.common.CookieUtil.initCookie; 
        
           import java.util.Date; 
        
           import javax.annotation.Nullable; 
        
           import org.slf4j.Logger; 
        
           import org.slf4j.LoggerFactory; 
        
           import io.netty.handler.codec.DateFormatter; 
        
           import io.netty.handler.codec.http.cookie.CookieHeaderNames; 
        
           /** 
        
            * A <a href="http://tools.ietf.org/html/rfc6265">RFC 6265</a> compliant cookie decoder for client side. 
        
            * 
        
            * <p>It will store the way the raw value was wrapped in {@link Cookie#isValueQuoted()} so it can be sent back 
        
            * to the origin server as is.</p> 
        
            * 
        
            * @see ClientCookieEncoder 
        
            */ 
        
           final class ClientCookieDecoder { 
        
               // Forked from netty-4.1.43 
        
               // https://github.com/netty/netty/blob/587afddb279bea3fd0f64d3421de8e69a35cecb9/codec-http/src/main/java/io/netty/handler/codec/http/cookie/ClientCookieDecoder.java

And need to mention the library in the NOTICE.txt

armeria/NOTICE.txt

Lines 63 to 69 in a51a482

    
           Modified redistributions 
        
           ======================== 
        
           This product contains a modified part of Brave, distributed by Zipkin.io: 
        
             * License: licenses/LICENSE.brave.al20.txt (Apache License v2.0) 
        
             * Homepage: https://github.com/openzipkin/brave

…osts

…tls-no-verify-hosts

ikhoon

Thanks! @tumile I’m looking forward to your next contribution. 🚀

core/src/main/java/com/linecorp/armeria/client/ClientFactoryBuilder.java

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java

core/src/test/java/com/linecorp/armeria/client/HttpClientSniTest.java

minwoox · 2020-09-22T03:11:37Z

Thanks @tumile! 🚀🚀🚀

tumile · 2020-09-22T03:14:53Z

Thanks everyone 😄

Add IgnoreHostsTrustManager

6415a17

tumile requested review from ikhoon, minwoox and trustin as code owners September 11, 2020 21:41

trustin added the new feature label Sep 12, 2020

trustin added this to the 1.0.1 milestone Sep 12, 2020

Tuan Le added 4 commits September 17, 2020 01:44

Test

60c34c1

Use a mock SSLEngine

5f5e8f7

Fix

2b511b9

Fix

12713c3

ikhoon reviewed Sep 18, 2020

View reviewed changes

minwoox mentioned this pull request Sep 18, 2020

Add release note for 1.1.0 #3070

Merged

Use ServerExtension

93d324a

minwoox reviewed Sep 19, 2020

View reviewed changes

trustin reviewed Sep 20, 2020

View reviewed changes

Review

3bd2e8d

minwoox approved these changes Sep 21, 2020

View reviewed changes

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java Outdated Show resolved Hide resolved

core/src/main/java/com/linecorp/armeria/client/IgnoreHostsTrustManager.java Show resolved Hide resolved

ikhoon reviewed Sep 21, 2020

View reviewed changes

Clean up nits

beb97dd

Tuan Le added 3 commits September 21, 2020 00:18

Merge branch 'master' of github.com:line/armeria into tls-no-verify-h…

2de30e7

…osts

Merge branch 'tls-no-verify-hosts' of github.com:tumile/armeria into …

62c89ed

…tls-no-verify-hosts

Add license

84c55ab

ikhoon approved these changes Sep 21, 2020

View reviewed changes

trustin reviewed Sep 22, 2020

View reviewed changes

Remove invalid reference

17d5e52

Address the comments by @trustin

c3abb87

minwoox merged commit 3da7376 into line:master Sep 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add tlsNoVerifyHosts to disable TLS verification for certain hosts #3057

Add tlsNoVerifyHosts to disable TLS verification for certain hosts #3057

tumile commented Sep 11, 2020

CLAassistant commented Sep 11, 2020 •

edited

codecov bot commented Sep 11, 2020 •

edited

ikhoon left a comment

tumile commented Sep 18, 2020

minwoox Sep 19, 2020

tumile Sep 19, 2020 •

edited

trustin Sep 20, 2020

tumile Sep 20, 2020

minwoox Sep 21, 2020 •

edited

trustin left a comment

trustin Sep 20, 2020

tumile Sep 20, 2020 •

edited

trustin Sep 20, 2020

minwoox left a comment

tumile commented Sep 21, 2020

ikhoon commented Sep 21, 2020

ikhoon left a comment

minwoox commented Sep 22, 2020

tumile commented Sep 22, 2020

Add tlsNoVerifyHosts to disable TLS verification for certain hosts #3057

Add tlsNoVerifyHosts to disable TLS verification for certain hosts #3057

Conversation

tumile commented Sep 11, 2020

CLAassistant commented Sep 11, 2020 • edited

codecov bot commented Sep 11, 2020 • edited

Codecov Report

ikhoon left a comment

Choose a reason for hiding this comment

tumile commented Sep 18, 2020

minwoox Sep 19, 2020

Choose a reason for hiding this comment

tumile Sep 19, 2020 • edited

Choose a reason for hiding this comment

trustin Sep 20, 2020

Choose a reason for hiding this comment

tumile Sep 20, 2020

Choose a reason for hiding this comment

minwoox Sep 21, 2020 • edited

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

trustin Sep 20, 2020

Choose a reason for hiding this comment

tumile Sep 20, 2020 • edited

Choose a reason for hiding this comment

trustin Sep 20, 2020

Choose a reason for hiding this comment

minwoox left a comment

Choose a reason for hiding this comment

tumile commented Sep 21, 2020

ikhoon commented Sep 21, 2020

ikhoon left a comment

Choose a reason for hiding this comment

minwoox commented Sep 22, 2020

tumile commented Sep 22, 2020

CLAassistant commented Sep 11, 2020 •

edited

codecov bot commented Sep 11, 2020 •

edited

tumile Sep 19, 2020 •

edited

minwoox Sep 21, 2020 •

edited

tumile Sep 20, 2020 •

edited