Support for HTTP/3 (server side), a few minor fixes#20394
Support for HTTP/3 (server side), a few minor fixes#20394reta merged 1 commit intoopensearch-project:mainfrom
Conversation
Signed-off-by: Andriy Redko <drreta@gmail.com>
📝 WalkthroughWalkthroughThese changes introduce QUIC token handling for HTTP/3 support in OpenSearch's transport layer. A null safety guard is added to SSL engine retrieval in Netty4HttpChannel. A new SecureQuicTokenHandler class is introduced to generate and validate QUIC tokens using HMAC-SHA256, and HTTP/3 configuration is updated to use this token handler. Changes
Sequence DiagramsequenceDiagram
participant Client
participant HTTP3Server as HTTP/3 Server
participant TokenHandler as SecureQuicTokenHandler
Client->>HTTP3Server: Initial QUIC Connection
HTTP3Server->>TokenHandler: writeToken(dcid, address)
TokenHandler->>TokenHandler: Generate HMAC-SHA256<br/>(server_name + dcid + address)
TokenHandler-->>HTTP3Server: Token (server_name + HMAC + payload)
HTTP3Server-->>Client: Token in response
Client->>HTTP3Server: Retry with Token
HTTP3Server->>TokenHandler: validateToken(token, address)
TokenHandler->>TokenHandler: Extract server_name
TokenHandler->>TokenHandler: Reconstruct payload
TokenHandler->>TokenHandler: Recompute HMAC<br/>& secure compare
alt Token Valid
TokenHandler-->>HTTP3Server: Offset after token
else Token Invalid
TokenHandler-->>HTTP3Server: -1 (invalid)
end
HTTP3Server-->>Client: Connection established/rejected
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Suggested Labels
Suggested Reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.java (2)
93-126: Good use of constant-time comparison for HMAC validation.Using
MessageDigest.isEqual()at line 118 is the correct approach to prevent timing attacks when comparing cryptographic tags.Minor optimization opportunity: The byte-by-byte MAC update (lines 110-112) could be replaced with a bulk read into a byte array for better performance, similar to the approach in
writeToken. However, this is not critical.♻️ Optional: Bulk read for MAC update
- for (int i = 0; i < payload.readableBytes(); ++i) { - mac.update(payload.getByte(payload.readerIndex() + i)); - } + final byte[] payloadBytes = new byte[payload.readableBytes()]; + payload.getBytes(payload.readerIndex(), payloadBytes); + mac.update(payloadBytes);
31-136: Consider extracting shared code to avoid duplication.This implementation appears identical to
modules/transport-netty4/src/main/java/org/opensearch/http/netty4/http3/SecureQuicTokenHandler.java. Based on learnings, only one HTTP transport is active at a time, so there's no conflict. However, maintaining two identical implementations could lead to divergence over time.Consider extracting the common implementation to a shared library module that both transports can depend on.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.javaplugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.javaplugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.java
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java:0-0
Timestamp: 2025-12-19T21:26:37.090Z
Learning: In Netty4QuicServerTransport (modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java), the secureHttpTransportSettingsProvider parameter is guaranteed to be non-null because the plugin verifies its presence during instantiation, so no explicit null-check is needed in the constructor.
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:15.318Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpRequest.java:220-226
Timestamp: 2025-12-09T01:57:20.362Z
Learning: In Netty's HTTP/3 codec implementation, the protocol version string returned by `request.protocolVersion()` is "HTTP/3.0" (not "HTTP/3" or the ALPN token "h3"). This matches the pattern used for HTTP/2 where "HTTP/2.0" is used (not "h2").
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:51.234Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
📚 Learning: 2025-12-12T18:40:08.452Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:256-256
Timestamp: 2025-12-12T18:40:08.452Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), URI limit validation has been moved from the protocol layer to the transport layer, making it protocol-agnostic. The random protocol selection in ReactorHttpClient.https(settings) is intentional to ensure all tests validate correct behavior across HTTP/1.1, HTTP/2, and HTTP/3.
Applied to files:
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.javaplugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.javamodules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java
📚 Learning: 2025-12-19T21:26:37.090Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java:0-0
Timestamp: 2025-12-19T21:26:37.090Z
Learning: In Netty4QuicServerTransport (modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4QuicServerTransport.java), the secureHttpTransportSettingsProvider parameter is guaranteed to be non-null because the plugin verifies its presence during instantiation, so no explicit null-check is needed in the constructor.
Applied to files:
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.javaplugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.javamodules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java
📚 Learning: 2025-12-12T13:31:51.234Z
Learnt from: andriyredko
Repo: opensearch-project/OpenSearch PR: 20017
File: plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java:642-646
Timestamp: 2025-12-12T13:31:51.234Z
Learning: In the OpenSearch ReactorNetty4 secure HTTP transport tests (plugins/transport-reactor-netty4/src/test/java/org/opensearch/http/reactor/netty4/ssl/SecureReactorNetty4HttpServerTransportTests.java), the createBuilderWithPort() helper intentionally uses randomBoolean() for the HTTP/3 enabled setting to ensure all tests validate correct behavior with both HTTP/3 enabled and disabled configurations.
Applied to files:
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.javaplugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.java
📚 Learning: 2025-12-13T20:16:15.318Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4Http3ServerTransport.java:101-123
Timestamp: 2025-12-13T20:16:15.318Z
Learning: In OpenSearch, only one HTTP transport implementation can be active and loaded at a time, so duplicate setting definitions (such as h3.max_stream_local_length, h3.max_stream_remote_length, and h3.max_streams) across different transport implementations like Netty4Http3ServerTransport and ReactorNetty4HttpServerTransport will not cause setting registration conflicts.
Applied to files:
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.java
📚 Learning: 2025-12-19T21:16:42.541Z
Learnt from: reta
Repo: opensearch-project/OpenSearch PR: 20017
File: modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java:138-154
Timestamp: 2025-12-19T21:16:42.541Z
Learning: In Netty4HttpChannel.java (modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java), the recursive traversal of parent channels in getAddressFromParent() is safe because Netty enforces a hard limit on the depth of channel parent chains, preventing unbounded recursion and potential StackOverflowError. When reviewing similar code, rely on the library's depth limits rather than adding custom recursion guards. If future changes remove Netty's bound, consider adding an explicit depth limit or converting to an iterative traversal with a guard.
Applied to files:
modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java
🧬 Code graph analysis (1)
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.java (2)
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.java (1)
SecureQuicTokenHandler(31-137)modules/transport-netty4/src/main/java/org/opensearch/http/netty4/http3/SecureQuicTokenHandler.java (1)
SecureQuicTokenHandler(31-111)
🪛 ast-grep (0.40.4)
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.java
[warning] 72-72: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(HMAC_SHA_256)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 107-107: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(HMAC_SHA_256)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 72-72: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(HMAC_SHA_256)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 107-107: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(HMAC_SHA_256)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: gradle-check
- GitHub Check: assemble (21, windows-latest)
- GitHub Check: assemble (25, ubuntu-latest)
- GitHub Check: assemble (25, ubuntu-24.04-arm)
- GitHub Check: assemble (25, windows-latest)
- GitHub Check: assemble (21, ubuntu-latest)
- GitHub Check: assemble (21, ubuntu-24.04-arm)
- GitHub Check: precommit (21, macos-15-intel)
- GitHub Check: precommit (21, windows-2025, true)
- GitHub Check: precommit (21, macos-15)
- GitHub Check: precommit (25, macos-15-intel)
- GitHub Check: precommit (21, ubuntu-latest)
- GitHub Check: precommit (21, windows-latest)
- GitHub Check: precommit (25, ubuntu-24.04-arm)
- GitHub Check: precommit (25, ubuntu-latest)
- GitHub Check: precommit (25, macos-15)
- GitHub Check: precommit (21, ubuntu-24.04-arm)
- GitHub Check: precommit (25, windows-latest)
- GitHub Check: Analyze (java)
- GitHub Check: detect-breaking-change
- GitHub Check: Mend Security Check
🔇 Additional comments (4)
plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/ReactorNetty4HttpServerTransport.java (1)
30-30: LGTM - SecureQuicTokenHandler integration for HTTP/3.The new token handler is correctly integrated into the HTTP/3 settings configuration. Each transport instance will have its own handler with a unique cryptographic key, which is appropriate for QUIC retry token handling.
Also applies to: 357-358
modules/transport-netty4/src/main/java/org/opensearch/http/netty4/Netty4HttpChannel.java (1)
143-153: LGTM - Null guard prevents NPE when accessing SSL engine attribute.This fix correctly addresses the
NullPointerException: keyatDefaultAttributeMap.attr()reported in the PR objectives. The guard ensureschannel.attr(sslEngineKey)is only called whensslEngineKeyis non-null, with proper fallback to parent channel lookup and graceful handling when no SSL engine is available.plugins/transport-reactor-netty4/src/main/java/org/opensearch/http/reactor/netty4/http3/SecureQuicTokenHandler.java (2)
31-51: LGTM - Secure key initialization using OpenSearch's Randomness utility.The class correctly initializes a 32-byte HMAC-SHA256 key using
Randomness.createSecure(), which provides a cryptographically secure random number generator. The key is instance-scoped, meaning each handler has a unique key, which is appropriate for QUIC retry tokens.Note: The static analysis warnings about AES/ECB and 3DES are false positives — this code uses
Mac.getInstance("HmacSHA256")which is an HMAC algorithm, not a symmetric cipher.
65-83: Potential NPE if address is unresolved.
address.getAddress()returnsnullwhen the hostname cannot be resolved, which would cause aNullPointerExceptionon line 66.In practice, incoming QUIC connections should always have resolved addresses since they originate from UDP datagrams with source IP addresses, making this unlikely to occur. However, adding a defensive check would be more robust.
🛡️ Optional defensive check
@Override public boolean writeToken(ByteBuf out, ByteBuf dcid, InetSocketAddress address) { + if (address.getAddress() == null) { + return false; + } final byte[] addr = address.getAddress().getAddress();
|
@cwperks sincere apologies, if you have a sec - missed on |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #20394 +/- ##
============================================
- Coverage 73.30% 73.26% -0.04%
+ Complexity 71861 71822 -39
============================================
Files 5791 5792 +1
Lines 328565 328610 +45
Branches 47308 47313 +5
============================================
- Hits 240845 240772 -73
- Misses 68420 68479 +59
- Partials 19300 19359 +59 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…ct#20394) Signed-off-by: Andriy Redko <drreta@gmail.com>
…ct#20394) Signed-off-by: Andriy Redko <drreta@gmail.com>
Description
Follow up on #20017, fixing NPE in
securityplugin:Using
SecureQuicTokenHandlerin Reactor Netty4 transport for HTTP/3Related Issues
See please https://github.com/opensearch-project/security/actions/runs/20833384001/job/59852932371?pr=5886, part of #4451
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.
Summary by CodeRabbit
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.