Support for HTTP/2 (server-side) #2051

reta · 2022-08-24T14:01:00Z

Signed-off-by: Andriy Redko [email protected]

Description

Implement SSL/TLS support for HTTP/2, see [1]

HTTP/2 (default)

$ curl https://localhost:9200 -kv -u admin:admin

*   Trying 127.0.0.1:9200...
* Connected to localhost (127.0.0.1) port 9200 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* TLSv1.0 (OUT), TLS header, Certificate Status (22):
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS header, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS header, Finished (20):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Request CERT (13):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.2 (OUT), TLS header, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* TLSv1.3 (OUT), TLS handshake, Certificate (11):
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256
* ALPN, server accepted to use h2
* Server certificate:
*  subject: DC=de; L=test; O=node; OU=node; CN=node-0.example.com
*  start date: Apr 22 03:43:47 2018 GMT
*  expire date: Apr 19 03:43:47 2028 GMT
*  issuer: DC=com; DC=example; O=Example Com Inc.; OU=Example Com Inc. Root CA; CN=Example Com Inc. Root CA
*  SSL certificate verify result: unable to get local issuer certificate (20), continuing anyway.
* Using HTTP2, server supports multiplexing
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* Server auth using Basic with user 'admin'
* Using Stream ID: 1 (easy handle 0x5653ef9e5e80)
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
> GET / HTTP/2
> Host: localhost:9200
> authorization: Basic YWRtaW46YWRtaW4=
> user-agent: curl/7.81.0
> accept: */*
> 
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* Connection state changed (MAX_CONCURRENT_STREAMS == 4294967295)!
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
< HTTP/2 200 
< content-type: application/json; charset=UTF-8
< content-length: 579
< 
{
  "name" : "runTask-0",
  "cluster_name" : "opensearch",
  "cluster_uuid" : "nf9IrHKAQb6VJtrLoR-4Xw",
  "version" : {
    "distribution" : "opensearch",
    "number" : "3.0.0-SNAPSHOT",
    "build_type" : "tar",
    "build_hash" : "ae6dbeaaad98c4849eff6c0a7863d9c751040b36",
    "build_date" : "2022-08-25T19:38:47.959594Z",
    "build_snapshot" : true,
    "lucene_version" : "9.4.0",
    "minimum_wire_compatibility_version" : "2.3.0",
    "minimum_index_compatibility_version" : "2.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}

HTTP/1.1

$ curl https://localhost:9200 -kv -u admin:admin --http1.1
*   Trying 127.0.0.1:9200...
* Connected to localhost (127.0.0.1) port 9200 (#0)
* ALPN, offering http/1.1
* TLSv1.0 (OUT), TLS header, Certificate Status (22):
* TLSv1.3 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS header, Certificate Status (22):
* TLSv1.3 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS header, Finished (20):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* TLSv1.3 (IN), TLS handshake, Request CERT (13):
* TLSv1.3 (IN), TLS handshake, Certificate (11):
* TLSv1.3 (IN), TLS handshake, CERT verify (15):
* TLSv1.3 (IN), TLS handshake, Finished (20):
* TLSv1.2 (OUT), TLS header, Finished (20):
* TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* TLSv1.3 (OUT), TLS handshake, Certificate (11):
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
* TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_128_GCM_SHA256
* ALPN, server accepted to use http/1.1
* Server certificate:
*  subject: DC=de; L=test; O=node; OU=node; CN=node-0.example.com
*  start date: Apr 22 03:43:47 2018 GMT
*  expire date: Apr 19 03:43:47 2028 GMT
*  issuer: DC=com; DC=example; O=Example Com Inc.; OU=Example Com Inc. Root CA; CN=Example Com Inc. Root CA
*  SSL certificate verify result: unable to get local issuer certificate (20), continuing anyway.
* Server auth using Basic with user 'admin'
* TLSv1.2 (OUT), TLS header, Supplemental data (23):
> GET / HTTP/1.1
> Host: localhost:9200
> Authorization: Basic YWRtaW46YWRtaW4=
> User-Agent: curl/7.81.0
> Accept: */*
> 
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* TLSv1.2 (IN), TLS header, Supplemental data (23):
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< content-type: application/json; charset=UTF-8
< content-length: 579
< 
{
  "name" : "runTask-0",
  "cluster_name" : "opensearch",
  "cluster_uuid" : "nf9IrHKAQb6VJtrLoR-4Xw",
  "version" : {
    "distribution" : "opensearch",
    "number" : "3.0.0-SNAPSHOT",
    "build_type" : "tar",
    "build_hash" : "ae6dbeaaad98c4849eff6c0a7863d9c751040b36",
    "build_date" : "2022-08-25T19:38:47.959594Z",
    "build_snapshot" : true,
    "lucene_version" : "9.4.0",
    "minimum_wire_compatibility_version" : "2.3.0",
    "minimum_index_compatibility_version" : "2.0.0"
  },
  "tagline" : "The OpenSearch Project: https://opensearch.org/"
}

[1] opensearch-project/OpenSearch#3651

Issues Resolved

Closes [FEATURE] Support for HTTP/2 (server-side) #2049
Resolves Bump the main branch to 3.0 #1953

Testing

TODO: tests for HTTP/2

Check List

New functionality includes testing
New functionality has been documented
Commits are signed per the DCO using --signoff

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.

peternied

Nice! Approach looks solid and straight forward.

You will see the BWC tests fail until opensearch-project/OpenSearch#3615 has been resolved but that shouldn't stop you from making progress in the other GitHub Action issues

reta · 2022-08-25T22:32:13Z

Nice! Approach looks solid and straight forward.

Thanks a lot @peternied !

cwperks · 2022-08-26T14:28:04Z

src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java

+            .applicationProtocolConfig(
+                new ApplicationProtocolConfig(
+                    Protocol.ALPN,
+                    // NO_ADVERTISE is currently the only mode supported by both OpenSsl and JDK providers.


I'm curious about this comment. I see that the same comment is present on the Http2Client example from netty: https://netty.io/4.1/xref/io/netty/example/http2/helloworld/client/Http2Client.html. Is the comment still true? In an effort to understand the default configuration here, would we ever consider using a different values for SelectorFailureBehavior and SelectedListenerFailureBehavior?

I'm curious about this comment. I see that the same comment is present on the Http2Client example from netty: https://netty.io/4.1/xref/io/netty/example/http2/helloworld/client/Http2Client.html.

That is correct, it was picked up from there since we could use JDK and OpenSSL providers.

Is the comment still true?

I suspect it is relevant since it is coming from Netty project.

In an effort to understand the default configuration here, would we ever consider using a different values for SelectorFailureBehavior and SelectedListenerFailureBehavior?

Sure, seems to be a sensitive setting, we could always expose it for configuration if need arrives, thoughts?

@cwperks quick update from the Netty code:

Is the comment still true?

Yes, it is still true

@reta Thank you for checking.

cwperks · 2022-08-26T14:29:13Z

src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java

-            .sessionCacheSize(0).sessionTimeout(0).sslProvider(sslProvider);
+            .sessionCacheSize(0).sessionTimeout(0).sslProvider(sslProvider)
+            .applicationProtocolConfig(
+                new ApplicationProtocolConfig(


Looks like this default configuration is repeated down below. Should we consider making this a static constant?

May be better as a function if we consider future possibility to configure SelectorFailureBehavior and SelectedListenerFailureBehavior?

Sounds reasonable.

Done, thanks @cwperks !

Signed-off-by: Andriy Redko <[email protected]>

peternied

Thanks for the contribution, I am happy to merge these changes assuming the CI checks get fixed, please let us know if you need any help look into the CI failures.

reta · 2022-09-23T18:10:50Z

Thanks @peternied , looking into failures now

DarshitChanpura · 2022-09-23T19:25:14Z

src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java

+                    Http2SecurityUtil.CIPHERS.stream(),
+                    StreamSupport.stream(ciphers.spliterator(), false))
+                .collect(Collectors.toSet()), SupportedCipherSuiteFilter.INSTANCE)
+            .clientAuth(Objects.requireNonNull(authMode)) // https://github.com/netty/netty/issues/4722


Seems like the mentioned netty issue was fixed a while ago but this comment wasn't removed. Should we remove it? I know it maybe out of scope of this PR but wanted to know your thoughts

DarshitChanpura · 2022-09-23T19:35:11Z

src/main/java/org/opensearch/security/ssl/util/SSLRequestHelper.java

+        SslHandler sslhandler = (SslHandler) httpChannel.getNettyChannel().pipeline().get("ssl_http");
+        if(sslhandler == null && httpChannel.inboundPipeline() != null) {
+            sslhandler = (SslHandler) httpChannel.inboundPipeline().get("ssl_http");
+        }


For my curiosity, Can you help me understand the reason behind retrieving sslHandler from an inboundPipeline() and why is this check required?

@DarshitChanpura if you look into previous code, it looked at channel's pipeline to find out the SSL handler. Now, with HTTP/2, the new pipeline is created (hence, the inbound channel pipeline is "lost") and the SSL handler is not accessible anymore. The solution (sort of): keep inbound pipeline reference so the SSL handler could be retrieved.

Signed-off-by: Andriy Redko <[email protected]>

reta · 2022-09-23T19:46:19Z

@peternied could you please help me with this Github action CI / build-artifact-names [1], I don't quite understand what it is doing by trying to locate next major OpenSearch version. Thank you.

[1] https://github.com/opensearch-project/security/actions/runs/3115185145/jobs/5051826328

Signed-off-by: Andriy Redko <[email protected]>

peternied · 2022-09-23T23:22:17Z

RE: CI / build-artifact-names failure

@reta Nice work finding a bug in our version increment test cases, it was attempting to download maven artifacts from OpenSearch v4 which do not existing. I've created #2106 to resolve this

peternied · 2022-09-26T22:41:19Z

Those BWC tests are stalling out this PR from merging, I've created a pull request #2108 to disable them, so this change could update our the main branch to v3.0 as you've already done the hard work.

reta · 2022-09-26T22:47:18Z

Those BWC tests are stalling out this PR from merging, I've created a pull request #2108 to disable them, so this change could update our the main branch to v3.0 as you've already done the hard work.

Thanks a lot @peternied , will try to help with BWC + 3.0 track

Rev'ed version number and fixed compilation issues Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Peter Nied <[email protected]>

peternied · 2022-09-28T01:02:51Z

will try to help with BWC + 3.0 track

Good news, BWC tests have started working with the 2.4.0 build, I've got #2112 which has incorperated the fixes from this change into it. This should rev the build number so your PR can focus around HTTP/2 support only. Its been a long journey, nearly there...

Signed-off-by: Peter Nied <[email protected]>

reta · 2022-09-29T03:20:47Z

will try to help with BWC + 3.0 track

Good news, BWC tests have started working with the 2.4.0 build, I've got #2112 which has incorperated the fixes from this change into it. This should rev the build number so your PR can focus around HTTP/2 support only. Its been a long journey, nearly there...

Thanks a mill @peternied !

Merge from main - will fix all CI checks

codecov-commenter · 2022-09-29T13:20:53Z

Codecov Report

Merging #2051 (2fd9c25) into main (2a00e2b) will decrease coverage by 0.00%.
The diff coverage is 72.91%.

@@             Coverage Diff              @@
##               main    #2051      +/-   ##
============================================
- Coverage     61.04%   61.03%   -0.01%     
- Complexity     3230     3232       +2     
============================================
  Files           256      256              
  Lines         18077    18102      +25     
  Branches       3224     3229       +5     
============================================
+ Hits          11035    11049      +14     
- Misses         5468     5471       +3     
- Partials       1574     1582       +8

Impacted Files	Coverage Δ
...ic/auth/http/kerberos/HTTPSpnegoAuthenticator.java	`0.00% <0.00%> (ø)`
...auth/http/saml/SamlFilesystemMetadataResolver.java	`0.00% <0.00%> (ø)`
...earch/security/auditlog/impl/AbstractAuditLog.java	`73.75% <0.00%> (ø)`
...ecurity/configuration/ConfigurationRepository.java	`74.31% <0.00%> (+2.18%)`	⬆️
...ttp/netty/SecuritySSLNettyHttpServerTransport.java	`79.48% <63.15%> (-15.52%)`	⬇️
...opensearch/security/ssl/util/SSLRequestHelper.java	`63.80% <71.42%> (-0.90%)`	⬇️
.../opensearch/security/OpenSearchSecurityPlugin.java	`79.87% <100.00%> (ø)`
...ensearch/security/ssl/DefaultSecurityKeyStore.java	`67.83% <100.00%> (+0.20%)`	⬆️
.../org/opensearch/security/support/PemKeyReader.java	`73.38% <100.00%> (ø)`
...urity/ssl/transport/SecuritySSLNettyTransport.java	`62.36% <0.00%> (-4.31%)`	⬇️
... and 2 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

reta · 2022-09-29T13:22:33Z

🥳 thanks a lot for help @peternied !

peternied · 2022-09-29T19:07:30Z

@opensearch-project/security Could we get another review on this change?

DarshitChanpura

Ty @reta !

* Support for HTTP/2 (server-side) Signed-off-by: Andriy Redko <[email protected]> * Addressing code review comments Signed-off-by: Andriy Redko <[email protected]> * Fixed ClusterManager compilation issues Signed-off-by: Andriy Redko <[email protected]> * Fixing bwc test version Signed-off-by: Andriy Redko <[email protected]> * Removed outdated comment Signed-off-by: Andriy Redko <[email protected]> * Fixing exception propagation from Http2OrHttpHandler to server transport Signed-off-by: Andriy Redko <[email protected]> * Switch to OpenSearch v3.0 Rev'ed version number and fixed compilation issues Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Peter Nied <[email protected]> Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Peter Nied <[email protected]> Co-authored-by: Peter Nied <[email protected]> Signed-off-by: Stephen Crawford <[email protected]>

reta force-pushed the issue-2049 branch 3 times, most recently from a83b4f7 to 85a490c Compare August 25, 2022 19:55

peternied reviewed Aug 25, 2022

View reviewed changes

reta mentioned this pull request Aug 25, 2022

Support for HTTP/2 (server-side) opensearch-project/OpenSearch#3847

Merged

5 tasks

cwperks reviewed Aug 26, 2022

View reviewed changes

reta force-pushed the issue-2049 branch 3 times, most recently from 0363410 to e9da7b2 Compare September 1, 2022 14:04

reta changed the title ~~[WIP] Support for HTTP/2 (server-side)~~ Support for HTTP/2 (server-side) Sep 1, 2022

reta marked this pull request as ready for review September 1, 2022 14:10

reta requested a review from a team September 1, 2022 14:10

reta force-pushed the issue-2049 branch 2 times, most recently from bbd2eee to f1536e7 Compare September 1, 2022 16:17

reta added 2 commits September 20, 2022 14:40

Support for HTTP/2 (server-side)

2178bb2

Signed-off-by: Andriy Redko <[email protected]>

Addressing code review comments

d0d8804

Signed-off-by: Andriy Redko <[email protected]>

reta force-pushed the issue-2049 branch from f1536e7 to d0d8804 Compare September 20, 2022 18:41

Fixed ClusterManager compilation issues

4c8730d

Signed-off-by: Andriy Redko <[email protected]>

peternied previously approved these changes Sep 23, 2022

View reviewed changes

DarshitChanpura reviewed Sep 23, 2022

View reviewed changes

reta dismissed peternied’s stale review via df4e0f9 September 23, 2022 19:31

reta force-pushed the issue-2049 branch from feab7fa to df4e0f9 Compare September 23, 2022 19:31

DarshitChanpura reviewed Sep 23, 2022

View reviewed changes

Fixing bwc test version

7417a9b

Signed-off-by: Andriy Redko <[email protected]>

reta force-pushed the issue-2049 branch from df4e0f9 to 7417a9b Compare September 23, 2022 19:36

Removed outdated comment

6ed7c42

Signed-off-by: Andriy Redko <[email protected]>

Fixing exception propagation from Http2OrHttpHandler to server transport

6bc350e

Signed-off-by: Andriy Redko <[email protected]>

peternied mentioned this pull request Sep 23, 2022

Keep version upgrade test working past v2 #2106

Merged

3 tasks

Switch to OpenSearch v3.0

26dd5ce

Rev'ed version number and fixed compilation issues Signed-off-by: Andriy Redko <[email protected]> Signed-off-by: Peter Nied <[email protected]>

peternied mentioned this pull request Sep 28, 2022

Switch to OpenSearch v3.0 #2112

Closed

3 tasks

Merge remote-tracking branch 'reta/issue-2049' into v3.0

cd8b13e

Signed-off-by: Peter Nied <[email protected]>

Merge pull request #1 from peternied/os-version-3

2fd9c25

Merge from main - will fix all CI checks

peternied approved these changes Sep 29, 2022

View reviewed changes

DarshitChanpura approved these changes Sep 29, 2022

View reviewed changes

DarshitChanpura merged commit 422f828 into opensearch-project:main Sep 29, 2022

peternied mentioned this pull request Sep 29, 2022

[HTTP/2] Functional Test cases #2125

Closed

DarshitChanpura mentioned this pull request Oct 11, 2022

[BUG] BWC tests are failing after upgrading to OpenSearch 3.0.0 #1918

Closed

RyanL1997 mentioned this pull request Mar 31, 2023

[Backport 2.x] XContent Refactor #2598

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for HTTP/2 (server-side) #2051

Support for HTTP/2 (server-side) #2051

reta commented Aug 24, 2022 •

edited by peternied

Loading

peternied left a comment

reta commented Aug 25, 2022

cwperks Aug 26, 2022

reta Aug 26, 2022

reta Aug 26, 2022 •

edited

Loading

cwperks Aug 26, 2022

cwperks Aug 26, 2022

reta Aug 26, 2022

cwperks Aug 26, 2022

reta Aug 26, 2022

peternied left a comment

reta commented Sep 23, 2022

DarshitChanpura Sep 23, 2022

DarshitChanpura Sep 23, 2022 •

edited

Loading

reta Sep 23, 2022

reta commented Sep 23, 2022

peternied commented Sep 23, 2022 •

edited

Loading

peternied commented Sep 26, 2022 •

edited

Loading

reta commented Sep 26, 2022

peternied commented Sep 28, 2022

reta commented Sep 29, 2022

codecov-commenter commented Sep 29, 2022 •

edited

Loading

reta commented Sep 29, 2022

peternied commented Sep 29, 2022

DarshitChanpura left a comment

Support for HTTP/2 (server-side) #2051

Support for HTTP/2 (server-side) #2051

Conversation

reta commented Aug 24, 2022 • edited by peternied Loading

Description

Issues Resolved

Testing

Check List

peternied left a comment

Choose a reason for hiding this comment

reta commented Aug 25, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta Aug 26, 2022 • 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

peternied left a comment

Choose a reason for hiding this comment

reta commented Sep 23, 2022

Choose a reason for hiding this comment

DarshitChanpura Sep 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reta commented Sep 23, 2022

peternied commented Sep 23, 2022 • edited Loading

peternied commented Sep 26, 2022 • edited Loading

reta commented Sep 26, 2022

peternied commented Sep 28, 2022

reta commented Sep 29, 2022

codecov-commenter commented Sep 29, 2022 • edited Loading

Codecov Report

reta commented Sep 29, 2022

peternied commented Sep 29, 2022

DarshitChanpura left a comment

Choose a reason for hiding this comment

reta commented Aug 24, 2022 •

edited by peternied

Loading

reta Aug 26, 2022 •

edited

Loading

DarshitChanpura Sep 23, 2022 •

edited

Loading

peternied commented Sep 23, 2022 •

edited

Loading

peternied commented Sep 26, 2022 •

edited

Loading

codecov-commenter commented Sep 29, 2022 •

edited

Loading