feat: add proxy_ssl_server_name #3084

whitehatboxer · 2020-12-21T03:05:27Z

What this PR does / why we need it:

Pre-submission checklist:

Did you explain what problem does this PR solve? Or what new features have been added?
Have you added corresponding test cases?
Have you modified the corresponding document?
Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

tokers · 2020-12-21T05:37:37Z

conf/config-default.yaml

@@ -174,6 +174,8 @@ nginx_config:                     # config for render the template to generate n
    # lua_shared_dicts:            # add custom shared cache to nginx.conf
    #  ipc_shared_dict: 100m       # custom shared cache, format: `cache-key: cache-size`

+    proxy_ssl_server_name: false   # disable passing of the server name through tls


This should be enabled by default.

I will fix it later.

conf/config-default.yaml

tokers · 2020-12-30T05:15:05Z

conf/config-default.yaml

@@ -174,6 +174,8 @@ nginx_config:                     # config for render the template to generate n
    # lua_shared_dicts:            # add custom shared cache to nginx.conf
    #  ipc_shared_dict: 100m       # custom shared cache, format: `cache-key: cache-size`

+    proxy_ssl_server_name: true   # disable passing of the server name through tls


The comment is not right. Just reference the Nginx doc:

Enables or disables passing of the server name through TLS Server Name Indication extension (SNI, RFC 6066) when establishing a connection with the proxied HTTPS server.

I will fix it later.

Already replace it.

tokers · 2021-01-20T01:38:51Z

@unbeatablekb Should have test cases for it, you can prepare a keypair that the SNI is not matched with the backend server and assert the proxy is aborted.

whitehatboxer · 2021-01-20T04:02:08Z

@tokers Thanks for giving help. I will add test for it after learning nginx test which would be done within this weeks.

spacewander · 2021-01-20T04:12:49Z

@unbeatablekb
You need to add proxy_ssl_server_name in t/APISIX.pm.
You can take a look at

apisix/t/plugin/proxy-rewrite.t

Lines 258 to 308 in b78c87a

    
           === TEST 8: set route(rewrite host + scheme) 
        
           --- config 
        
               location /t { 
        
                   content_by_lua_block { 
        
                       local t = require("lib.test_admin").test 
        
                       local code, body = t('/apisix/admin/routes/1', 
        
                            ngx.HTTP_PUT, 
        
                            [[{ 
        
                                   "methods": ["GET"], 
        
                                   "plugins": { 
        
                                       "proxy-rewrite": { 
        
                                           "uri": "/plugin_proxy_rewrite", 
        
                                           "scheme": "https", 
        
                                           "host": "test.com" 
        
                                       } 
        
                                   }, 
        
                                   "upstream": { 
        
                                       "nodes": { 
        
                                           "127.0.0.1:1983": 1 
        
                                       }, 
        
                                       "type": "roundrobin" 
        
                                   }, 
        
                                   "uri": "/hello" 
        
                           }]] 
        
                           ) 
        
                       if code >= 300 then 
        
                           ngx.status = code 
        
                       end 
        
                       ngx.say(body) 
        
                   } 
        
               } 
        
           --- request 
        
           GET /t 
        
           --- response_body 
        
           passed 
        
           --- no_error_log 
        
           [error] 
        
           === TEST 9: rewrite host + scheme 
        
           --- request 
        
           GET /hello HTTP/1.1 
        
           --- response_body 
        
           uri: /plugin_proxy_rewrite 
        
           host: test.com 
        
           scheme: https 
        
           --- no_error_log 
        
           [error]

and write your own one.

whitehatboxer · 2021-01-21T04:13:43Z

@spacewander Thanks for your advice. I will do it.

whitehatboxer · 2021-01-25T14:05:29Z

@tokers @spacewander
The deadline took effect. I try to learn the test architecture about apisix at last weekends.

Finally I add test cases in t/core. I'm willing to hear your advice and I will modify it in the day.

membphis · 2021-01-25T15:45:35Z

@tokers @spacewander
The deadline took effect. I try to learn the test architecture about apisix at last weekends.

Finally I add test cases in t/core. I'm willing to hear your advice and I will modify it in the day.

if you need any help, please let us know

tokers · 2021-01-26T01:31:36Z

@unbeatablekb CI failed.

spacewander · 2021-01-26T04:36:08Z

t/APISIX.pm

@@ -384,6 +384,24 @@ _EOC_
        }
    }

+    server {
+        listen 1985 ssl;


We don't need this. We already have a HTTPS backend:

apisix/t/APISIX.pm

Line 444 in 3db8ebe

listen 1983 ssl;

You just need to pass a wrong host header.

Thanks. I used to create a new server block because I want to add server_name to new block. Now I think this make no sense after thinking about it.

But you said I could write a new one in APISIX.pm. What does that means?

whitehatboxer · 2021-01-26T05:30:06Z

@tokers @spacewander
I need your help. I run my test locally but failed.

The TEST2's output all failed. But I don't know what's wrong wit it.

I already pushed the code and details of the output of running test are show blow.

tokers · 2021-01-26T05:37:21Z

t/core/proxy_ssl.t

+GET /hello
+--- error_code: 502
+--- error_log
+ssl


I don't know how you assert the 502 is due to SSL handshaking failure.

spacewander · 2021-01-26T05:45:59Z

Only enable proxy_ssl_server_name doesn't pass the correct SNI to the backend.
See #2988 (comment).

I have submitted a new one to surpass this PR: #3420

@unbeatablekb
Your contribution is still counted.

whitehatboxer changed the title ~~feature: add proxy_ssl_server_name to support carry SNI while doing SSL handshaking with upstream~~ feature: add proxy_ssl_server_name Dec 21, 2020

juzhiyuan changed the title ~~feature: add proxy_ssl_server_name~~ feat: add proxy_ssl_server_name Dec 21, 2020

tokers reviewed Dec 21, 2020

View reviewed changes

membphis reviewed Dec 31, 2020

View reviewed changes

conf/config-default.yaml Outdated Show resolved Hide resolved

membphis approved these changes Jan 11, 2021

View reviewed changes

membphis requested a review from tokers January 11, 2021 10:16

tokers reviewed Jan 11, 2021

View reviewed changes

spacewander requested a review from tokers January 19, 2021 07:42

whitehatboxer closed this Jan 21, 2021

whitehatboxer reopened this Jan 21, 2021

spacewander requested changes Jan 26, 2021

View reviewed changes

unbeatablekb added 2 commits January 26, 2021 13:20

feature: add proxy_ssl_server_name

a6d2c9f

feature: add test case for proxy_ssl_server_name

c4078fd

tokers reviewed Jan 26, 2021

View reviewed changes

spacewander closed this Jan 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add proxy_ssl_server_name #3084

feat: add proxy_ssl_server_name #3084

whitehatboxer commented Dec 21, 2020 •

edited

Loading

tokers Dec 21, 2020

whitehatboxer Dec 30, 2020

whitehatboxer Dec 30, 2020

tokers Dec 30, 2020

whitehatboxer Jan 14, 2021

whitehatboxer Jan 14, 2021

tokers commented Jan 20, 2021

whitehatboxer commented Jan 20, 2021

spacewander commented Jan 20, 2021

whitehatboxer commented Jan 21, 2021

whitehatboxer commented Jan 25, 2021

membphis commented Jan 25, 2021

tokers commented Jan 26, 2021

spacewander Jan 26, 2021

whitehatboxer Jan 26, 2021 •

edited

Loading

whitehatboxer commented Jan 26, 2021 •

edited

Loading

tokers Jan 26, 2021

whitehatboxer Jan 26, 2021

spacewander commented Jan 26, 2021

feat: add proxy_ssl_server_name #3084

feat: add proxy_ssl_server_name #3084

Conversation

whitehatboxer commented Dec 21, 2020 • edited Loading

What this PR does / why we need it:

Pre-submission checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tokers commented Jan 20, 2021

whitehatboxer commented Jan 20, 2021

spacewander commented Jan 20, 2021

whitehatboxer commented Jan 21, 2021

whitehatboxer commented Jan 25, 2021

membphis commented Jan 25, 2021

tokers commented Jan 26, 2021

Choose a reason for hiding this comment

whitehatboxer Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

whitehatboxer commented Jan 26, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spacewander commented Jan 26, 2021

whitehatboxer commented Dec 21, 2020 •

edited

Loading

whitehatboxer Jan 26, 2021 •

edited

Loading

whitehatboxer commented Jan 26, 2021 •

edited

Loading