Server support ALPN with OpenSSL #2102

leaf-potato · 2023-01-29T15:43:25Z

What problem does this PR solve?

Issue Number: #1991

Problem Summary: 当前bRPC中进行SSL/TLS握手阶段没有支持ALPN扩展协议。

What is changed and the side effects?

Changed: 在ServerSSLOptions中扩展ALPN配置项。

Side effects:

Performance effects(性能影响): 当用户在ServerSSLOptions中设置ALPN选项时，server启动时会将ALPN选项序列化为标准格式并设置好回调函数。在SSL/TLS握手阶段会调用回调函数选择匹配的协议并返回给客户端。
Breaking backward compatibility(向后兼容性): 是

Check List:

Please make sure your changes are compilable(请确保你的更改可以通过编译).
When providing us with a new feature, it is best to add related tests(如果你向我们增加一个新的功能, 请添加相关测试).
Please follow Contributor Covenant Code of Conduct.(请遵循贡献者准则).

leaf-potato · 2023-01-29T16:24:32Z

Client端的实现还有点问题，暂时先提交Server端的代码。还请大佬们给Review下~

wwbmmm · 2023-01-31T03:27:08Z

src/brpc/adaptive_protocol_type.h

@@ -44,6 +44,7 @@ class AdaptiveProtocolType {
 public:
    explicit AdaptiveProtocolType() : _type(PROTOCOL_UNKNOWN) {}
    explicit AdaptiveProtocolType(ProtocolType type) : _type(type) {}
+    explicit AdaptiveProtocolType(butil::StringPiece name) { AssignWithString(name); }


这里可以直接调用operator=(name)，就不用改那么多了

好的，这个我改下

wwbmmm · 2023-01-31T03:36:49Z

src/brpc/details/ssl_helper.cpp

+        name = name.substr(0, CHAR_MAX); 
+    }
+
+    char lenght = static_cast<char>(name.size());


lenght -> length

wwbmmm · 2023-01-31T03:45:28Z

src/brpc/details/ssl_helper.cpp

@@ -521,6 +558,10 @@ SSL_CTX* CreateServerSSLContext(const std::string& certificate,

 #endif  // OPENSSL_NO_DH

+    // Set ALPN callback to choose application protocol.
+    if (SetServerALPNCallback(ssl_ctx.get(), alpns) != 0) {


如果alpns为空串，则不需要SetServerALPNCallback

wwbmmm · 2023-01-31T03:50:06Z

src/brpc/server.h

@@ -686,6 +688,10 @@ friend class Controller;
    ServerOptions _options;
    butil::EndPoint _listen_addr;

+    // ALPN extention protocol-list format. Server initialize this with alpns options.
+    // OpenSSL API use this variable to avoid conversion at each handshake.
+    std::string _row_alpns;


row在这里是什么意思？

和options.alpns做区分，这儿为格式转化后TLS使用的原始数据

row -> raw?

是的，手误~

cdjingit · 2023-02-01T03:24:57Z

src/brpc/server.cpp

@@ -642,6 +642,33 @@ int Server::InitializeOnce() {
    return 0;
 }

+int Server::InitALPNOptions(ServerSSLOptions* options) {


这里options不会被修改

int Server::InitALPNOptions(const ServerSSLOptions* options);

cdjingit · 2023-02-01T03:30:48Z

src/brpc/server.cpp

+    butil::StringSplitter split(alpns.data(), alpns.data() + alpns.size(), ',');
+
+    std::string row_protocol;
+    for (; split; split++) {


for (butil::StringSplitter split(alpns.data(), ','); split; ++split) { }

写法参考的src/brpc/details/ssl_helper.cpp文件中ParseSSLProtocols函数里的实现
可能的1个改动点：split++ => ++split

split在for 循环外并没有用到，应该包在for 里面。

cdjingit · 2023-02-01T03:33:17Z

src/brpc/server.cpp

@@ -1913,7 +1946,7 @@ int Server::AddCertificate(const CertInfo& cert) {
    ssl_ctx.filters = cert.sni_filters;
    ssl_ctx.ctx = std::make_shared<SocketSSLContext>();
    SSL_CTX* raw_ctx = CreateServerSSLContext(cert.certificate, cert.private_key,
-                                              _options.ssl_options(), &ssl_ctx.filters);
+            _options.ssl_options(), &_row_alpns, &ssl_ctx.filters);


代码缩进

SSL_CTX* raw_ctx = CreateServerSSLContext( cert.certificate, cert.private_key, _options.ssl_options(), &_row_alpns, &ssl_ctx.filters);

src/brpc/details/ssl_helper.cpp

cdjingit · 2023-02-01T03:37:24Z

src/brpc/details/ssl_helper.cpp

@@ -441,6 +441,42 @@ static int SetSSLOptions(SSL_CTX* ctx, const std::string& ciphers,
    return 0;
 }

+static int ServerALPNCallback(SSL* ssl,


代码缩进

static int ServerALPNCallback( SSL* ssl, const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg)

好像第1个参数都没单独换行。是说第2、3行没有和第1个参数缩进对齐？

嗯

static int ServerALPNCallback(SSL* ssl, const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg); 或者这样 static int ServerALPNCallback( SSL* ssl, const unsigned char** out, unsigned char* outlen, const unsigned char* in, unsigned int inlen, void* arg)

cdjingit · 2023-02-01T03:39:55Z

src/brpc/details/ssl_helper.cpp

+    if (select_result == OPENSSL_NPN_NEGOTIATED) {
+        return SSL_TLSEXT_ERR_OK;
+    }
+    return SSL_TLSEXT_ERR_NOACK; 


return select_result == OPENSSL_NPN_NEGOTIATED ? SSL_TLSEXT_ERR_OK : SSL_TLSEXT_ERR_NOACK;

src/brpc/details/ssl_helper.cpp

cdjingit · 2023-02-01T03:45:47Z

src/brpc/details/ssl_helper.cpp

@@ -833,6 +874,23 @@ void Print(std::ostream& os, X509* cert, const char* sep) {
    os << butil::StringPiece(bufp, len);
 }

+std::string ALPNProtocolToString(const AdaptiveProtocolType& protocol) {
+    std::string name = protocol.name();


用butil::StringPiece

butil::StringPiece name(protocol.name()); if (name.starts_with("http")) { name.set("http/1.1"); } ....

chenzhangyi · 2023-02-02T09:21:06Z

能加一下文档说明下这个实现逻辑，以及补充一下UT么？牵涉到协议实现，希望尽量能严谨和完整一点

leaf-potato · 2023-02-05T15:41:36Z

能加一下文档说明下这个实现逻辑，以及补充一下UT么？牵涉到协议实现，希望尽量能严谨和完整一点

@chenzhangyi 可以的，文档&UT后续我都补充下。Client端还没实现完，UT可能需要使用OpenSSL命令来验证协商是否正确

wwbmmm · 2023-02-09T08:33:01Z

UT编译失败了：
brpc_ssl_unittest.cpp:342:66: error: too few arguments to function call, expected 5, have 4
brpc::SSLOptions(), NULL);

old-bear · 2023-02-22T14:56:46Z

src/brpc/details/ssl_helper.cpp

+            const_cast<unsigned char**>(out), outlen, 
+            reinterpret_cast<const unsigned char*>(alpns->data()), alpns->size(),
+            in, inlen);
+    return (select_result == OPENSSL_NPN_NEGOTIATED) 


如果没选择到，返回SSL_TLSEXT_ERR_NOACK会直接断开SSL链接吗？
和返回SSL_TLSEXT_ERR_ALERT_FATAL区别是啥？

old-bear · 2023-02-22T14:58:12Z

src/brpc/server.cpp

@@ -642,6 +642,31 @@ int Server::InitializeOnce() {
    return 0;
 }

+int Server::InitALPNOptions(const ServerSSLOptions* options) {
+    if (options == nullptr) {


如果openssl版本不支持，这里就直接报错？

并不是这儿是判断ServerSSLOptions是否为nullptr

leaf-potato · 2023-07-16T07:03:08Z

UT编译失败了：
brpc_ssl_unittest.cpp:342:66: error: too few arguments to function call, expected 5, have 4
brpc::SSLOptions(), NULL);

@wwbmmm 编译失败已解决，同时补充了本次功能的UT和文档

wwbmmm · 2023-07-18T02:18:11Z

docs/cn/server.md

+
+  ```c++
+  ServerSSLOptions ssl_options;
+  ssl_options->alpns = "http, h2, baidu_std";


应该是ssl_options.alpns吧？

wwbmmm · 2023-07-18T02:18:45Z

docs/cn/server.md

-              const ::example::EchoRequest* request,
-              ::example::EchoResponse* response,
-              ::google::protobuf::Closure* done) {
+    void Echo(::google::protobuf::RpcController* cntl_base,


无关的改动，有点多

leaf-potato · 2023-07-27T11:21:47Z

@wwbmmm 请问下PR还有需要修改的地儿嘛

wwbmmm · 2023-08-02T06:31:31Z

LGTM

leaf-potato · 2023-08-29T01:35:05Z

@wwbmmm 大佬求问考虑合入了嘛~

wwbmmm reviewed Jan 31, 2023

View reviewed changes

cdjingit reviewed Feb 1, 2023

View reviewed changes

src/brpc/details/ssl_helper.cpp Show resolved Hide resolved

cdjingit reviewed Feb 1, 2023

View reviewed changes

src/brpc/details/ssl_helper.cpp Show resolved Hide resolved

cdjingit reviewed Feb 1, 2023

View reviewed changes

leaf-potato force-pushed the alpn branch 2 times, most recently from 6f3f1e3 to bfc4fa5 Compare February 5, 2023 15:02

leaf-potato force-pushed the alpn branch from bfc4fa5 to 8948d9d Compare February 6, 2023 15:57

Server support ALPN with OpenSSL

13a4231

leaf-potato force-pushed the alpn branch from 8948d9d to 13a4231 Compare February 7, 2023 14:51

old-bear reviewed Feb 22, 2023

View reviewed changes

Menci mentioned this pull request May 16, 2023

Add client ALPN support #2251

Merged

Fix SSL unittest compile error

9ea4f47

leaf-potato force-pushed the alpn branch 2 times, most recently from c82a471 to 1ecfa2c Compare July 16, 2023 06:54

Add ALPN protocol unittest

c4fe7fa

leaf-potato force-pushed the alpn branch from 1ecfa2c to b30dcbf Compare July 16, 2023 06:56

leaf-potato force-pushed the alpn branch from b30dcbf to 36a531e Compare July 16, 2023 08:03

wwbmmm reviewed Jul 18, 2023

View reviewed changes

Add ALPN protocol doc

0852d60

leaf-potato force-pushed the alpn branch from 36a531e to 0852d60 Compare July 19, 2023 01:24

wwbmmm merged commit 09acd32 into apache:master Aug 30, 2023
16 checks passed

Server support ALPN with OpenSSL #2102

Server support ALPN with OpenSSL #2102

Conversation

leaf-potato commented Jan 29, 2023

What problem does this PR solve?

What is changed and the side effects?

Check List:

leaf-potato commented Jan 29, 2023

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdjingit Feb 1, 2023 • edited Loading

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenzhangyi commented Feb 2, 2023

leaf-potato commented Feb 5, 2023

wwbmmm commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leaf-potato commented Jul 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leaf-potato commented Jul 27, 2023

wwbmmm commented Aug 2, 2023

leaf-potato commented Aug 29, 2023

cdjingit Feb 1, 2023 •

edited

Loading