tls: replace NPN with ALPN for client connections#10822
tls: replace NPN with ALPN for client connections#10822edsiper merged 1 commit intofluent:masterfrom
Conversation
zzbamboo
requested review from
edsiper,
fujimotos and
koleini
as code owners
WalkthroughClient-side ALPN handling in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client App
participant Ctx as TLS Context
participant SSL as OpenSSL (SSL_CTX)
participant Srv as Server (Handshake)
rect rgba(220,240,255,0.4)
note over App,Ctx: Configuration Phase (Client)
App->>Ctx: tls_context_alpn_set(alpn)
alt Prior implementation
Ctx->>SSL: SSL_CTX_set_next_proto_select_cb(client_alpn_select)
note right of SSL: registers callback for handshake-time selection
else New implementation
Ctx->>SSL: SSL_CTX_set_alpn_protos(alpn_bytes)
SSL-->>Ctx: return (0=ok, !=0=error)
end
end
rect rgba(235,255,235,0.4)
note over Ctx,Srv: Handshake Phase
App->>Srv: ClientHello (ALPN ext)
alt Prior implementation
SSL->>SSL: invoke client_alpn_select during handshake
else New implementation
SSL->>SSL: consult configured ALPN list directly
end
Srv-->>App: ServerHello (selected ALPN)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
src/tls/openssl.c (7)
264-276: Guard ALPN calls for OpenSSL builds without ALPNIf older OpenSSL (<1.0.2) or OPENSSL_NO_ALPN is in use, this will not compile. Add a compile-time guard or confirm minimum supported OpenSSL version and document it.
else { - if (SSL_CTX_set_alpn_protos(ctx->ctx, - (const unsigned char *) &ctx->alpn[1], - (unsigned int)ctx->alpn[0]) != 0) { - result = -1; - } +#if defined(OPENSSL_NO_ALPN) || (OPENSSL_VERSION_NUMBER < 0x1000200fL) + flb_error("[tls] ALPN not supported by this OpenSSL build/version"); + result = -1; +#else + if (ctx->alpn != NULL) { + unsigned int alpn_len = (unsigned char) ctx->alpn[0]; + if (SSL_CTX_set_alpn_protos(ctx->ctx, + (const unsigned char *) &ctx->alpn[1], + alpn_len) != 0) { + result = -1; + } + } +#endif }
239-246: Validate token length (1..255) and avoid strcpy; use memcpyPrevents malformed ALPN vectors and truncation. Also avoids copying a trailing NUL into the buffer.
- wire_format_alpn[wire_format_alpn_index] = \ - (char) strlen(alpn_token); - - strcpy(&wire_format_alpn[wire_format_alpn_index + 1], - alpn_token); - - wire_format_alpn_index += strlen(alpn_token) + 1; + size_t token_len = strlen(alpn_token); + if (token_len == 0 || token_len > 255) { + flb_free(wire_format_alpn); + free(alpn_working_copy); + return -1; + } + wire_format_alpn[wire_format_alpn_index] = (char) token_len; + memcpy(&wire_format_alpn[wire_format_alpn_index + 1], + alpn_token, token_len); + wire_format_alpn_index += token_len + 1;
252-255: Free previous ctx->alpn to avoid leaks on reconfigurationIf tls_context_alpn_set is called more than once, the previous buffer leaks.
- wire_format_alpn[0] = (char) wire_format_alpn_index - 1; - ctx->alpn = wire_format_alpn; + wire_format_alpn[0] = (char) wire_format_alpn_index - 1; + if (ctx->alpn != NULL) { + flb_free(ctx->alpn); + } + ctx->alpn = wire_format_alpn;
183-188: Avoid length sign-extension in server ALPN selection tooMirror the unsigned-char cast where length is passed.
- (unsigned int) ctx->alpn[0], + (unsigned int) (unsigned char) ctx->alpn[0],
1093-1103: Avoid double-setting ALPN (context-level + per-session)After setting via SSL_CTX_set_alpn_protos, this per-session SSL_set_alpn_protos is redundant. Pick one (prefer context-level for defaults) to reduce code paths and potential drift.
- if (ctx->alpn != NULL) { - ret = SSL_set_alpn_protos(session->ssl, - (const unsigned char *) &ctx->alpn[1], - (unsigned int) ctx->alpn[0]); - - if (ret != 0) { - flb_error("[tls] error: alpn setup failed : %d", ret); - pthread_mutex_unlock(&ctx->mutex); - return -1; - } - }
649-652: Fix log context: prints crt_file when reporting key_file errorTiny but confusing during debugging.
- flb_error("[tls] key_file '%s' %lu: %s", - crt_file, ERR_get_error(), err_buf); + flb_error("[tls] key_file '%s' %lu: %s", + key_file, ERR_get_error(), err_buf);
1067-1069: Typo in error message“vailidation” → “validation”.
- flb_error("[tls] error: hostname parameter vailidation is failed : %s", + flb_error("[tls] error: hostname parameter validation failed: %s",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/tls/openssl.c(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/tls/openssl.c (1)
271-279: Don’t hard-fail when ALPN is unset; guard NULL and fix length castRequiring ALPN (returning -1 if ctx->alpn is NULL) breaks clients that don’t configure ALPN. Also, casting a possibly-signed char directly to unsigned int risks sign-extension. Guard for NULL and coerce length via unsigned char; optionally log on failure.
- if (ctx->alpn == NULL) { - return -1; - } - if (SSL_CTX_set_alpn_protos( - ctx->ctx, - (const unsigned char *) &ctx->alpn[1], - (unsigned int) ctx->alpn[0]) != 0) { - return -1; - } + if (ctx->alpn != NULL) { + unsigned int alpn_len = (unsigned char) ctx->alpn[0]; + if (SSL_CTX_set_alpn_protos(ctx->ctx, + (const unsigned char *) &ctx->alpn[1], + alpn_len) != 0) { + flb_error("[tls] error: failed to set ALPN on SSL context"); + return -1; + } + }
🧹 Nitpick comments (2)
src/tls/openssl.c (2)
1097-1107: Avoid duplicate ALPN configuration (ctx and per-session)You already set ALPN on SSL_CTX; setting again on each SSL with SSL_set_alpn_protos is redundant. Prefer one source of truth to reduce complexity and ambiguity. If per-session override isn’t needed, remove this block.
- if (ctx->alpn != NULL) { - ret = SSL_set_alpn_protos(session->ssl, - (const unsigned char *) &ctx->alpn[1], - (unsigned int) ctx->alpn[0]); - - if (ret != 0) { - flb_error("[tls] error: alpn setup failed : %d", ret); - pthread_mutex_unlock(&ctx->mutex); - return -1; - } - } + /* ALPN configured at SSL_CTX level; per-session override not required */
239-246: Build ALPN vector without transient NULs and enforce per-protocol length ≤ 255Use memcpy with explicit lengths and validate each token’s length fits a single-octet prefix. This avoids relying on strcpy side effects and makes the wire format explicit.
- wire_format_alpn[wire_format_alpn_index] = \ - (char) strlen(alpn_token); - - strcpy(&wire_format_alpn[wire_format_alpn_index + 1], - alpn_token); - - wire_format_alpn_index += strlen(alpn_token) + 1; + size_t tlen = strlen(alpn_token); + if (tlen == 0 || tlen > 255) { + flb_free(wire_format_alpn); + free(alpn_working_copy); + return -1; + } + wire_format_alpn[wire_format_alpn_index++] = (unsigned char) tlen; + memcpy(&wire_format_alpn[wire_format_alpn_index], alpn_token, tlen); + wire_format_alpn_index += tlen; @@ - wire_format_alpn[0] = (char) wire_format_alpn_index - 1; + wire_format_alpn[0] = (unsigned char) (wire_format_alpn_index - 1);Also applies to: 252-255
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/tls/openssl.c(1 hunks)
cosmo0920
left a comment
cosmo0920
left a comment
There was a problem hiding this comment.
Yes. It is considered as appropriate patch for using ALPN protocol on HELLO in TLS negotiations.
Signed-off-by: Biao Zhu <zhumouren0623@qq.com>
zzbamboo
force-pushed
the
tls-client-npn-to-alpn
branch
from
0dc3e49 to
1d8ae53
Compare
|
#10822 |
3 participants
Replace SSL_CTX_set_next_proto_select_cb with SSL_CTX_set_alpn_protos
for client-side protocol negotiation. This modernizes the TLS
implementation by using the standardized ALPN (RFC 7301) instead
of the deprecated NPN protocol.
Enter
[N/A]in the box, if an item is not applicable to your change.Testing
Before we can approve your change; please submit the following in a comment:
If this is a change to packaging of containers or native binaries then please confirm it works for all targets.
ok-package-testlabel to test for all targets (requires maintainer to do).Documentation
Backporting
Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.
Summary by CodeRabbit