Skip to content
This repository has been archived by the owner on Aug 2, 2022. It is now read-only.

Add required mutual auth to gRPC Server/Client #254

Merged

Conversation

sidheart
Copy link
Contributor

Issue #, if available: 253

Description of changes:
Previously we had 1 sided TLS on the server side. Data between the
client and server was send over an encrypted channel, but any client
could make requests to the server.

This commit changes the behavior so that only clients with the matching
certificates can make requests to the server when TLS is enabled. This
commit does NOT add support for installing a trust manager. That must be
added in the future.

Tests:

  • GRPCTest verifies that when https is enabled, an authenticated client is capable of making
    requests, but an unauthenticated client is rejected.

Code coverage percentage for this patch:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov
Copy link

codecov bot commented Jun 23, 2020

Codecov Report

Merging #254 into master will increase coverage by 0.01%.
The diff coverage is 82.85%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #254      +/-   ##
============================================
+ Coverage     66.14%   66.16%   +0.01%     
+ Complexity     1789     1733      -56     
============================================
  Files           270      259      -11     
  Lines         11988    11720     -268     
  Branches        952      929      -23     
============================================
- Hits           7929     7754     -175     
+ Misses         3744     3666      -78     
+ Partials        315      300      -15     
Impacted Files Coverage Δ Complexity Δ
...icsearch/performanceanalyzer/CertificateUtils.java 20.00% <ø> (+20.00%) 3.00 <0.00> (+3.00)
...performanceanalyzer/net/GRPCConnectionManager.java 86.15% <75.00%> (+12.93%) 22.00 <1.00> (+2.00)
...asticsearch/performanceanalyzer/net/NetServer.java 77.14% <85.71%> (+18.93%) 18.00 <2.00> (+4.00)
...rch/performanceanalyzer/config/PluginSettings.java 30.00% <100.00%> (+1.29%) 14.00 <1.00> (+1.00)
...asticsearch/performanceanalyzer/net/NetClient.java 74.13% <0.00%> (-1.73%) 11.00% <0.00%> (ø%)
...ca/framework/api/summaries/HotResourceSummary.java 84.81% <0.00%> (-0.19%) 20.00% <0.00%> (ø%)
...manceanalyzer/rca/store/rca/HotNodeClusterRca.java 81.18% <0.00%> (ø) 24.00% <0.00%> (ø%)
...nceanalyzer/rca/framework/core/GenericContext.java 100.00% <0.00%> (ø) 8.00% <0.00%> (-2.00%)
...nceanalyzer/rca/store/rca/hot_node/HighCpuRca.java 100.00% <0.00%> (ø) 1.00% <0.00%> (ø%)
...analyzer/rca/store/ElasticSearchAnalysisGraph.java 100.00% <0.00%> (ø) 7.00% <0.00%> (ø%)
... and 25 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 669a264...87e18e3. Read the comment docs.

@sidheart sidheart requested review from ktkrg and yojs June 23, 2020 00:21
@sidheart sidheart requested a review from ktkrg July 7, 2020 20:28
Comment on lines 177 to 193
private ManagedChannel buildSecureChannel(final String remoteHost) {
try {
SslContextBuilder sslContextBuilder = GrpcSslContexts.forClient().keyManager(certFile, pkeyFile);
if (trustedCasFile != null) {
sslContextBuilder.trustManager(trustedCasFile);
}
return NettyChannelBuilder.forAddress(remoteHost, this.port)
.sslContext(sslContextBuilder.build())
.build();
} catch (SSLException e) {
LOG.error("Unable to build an SSL gRPC client. Exception: {}", e.getMessage());
e.printStackTrace();

// Wrap the SSL Exception in a generic RTE and re-throw.
throw new RuntimeException(e);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: whitespace off by one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

channel.shutdownNow();
try {
channel.awaitTermination(1, TimeUnit.MINUTES);
} catch (InterruptedException e) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to propagate the interrupted status when catching an InterruptedException by calling Thread.currentThread().interrupt() after channel.shutdownNow();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

try {
server.awaitTermination(1, TimeUnit.MINUTES);
} catch (InterruptedException e) {
server.shutdownNow();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as above. You need to propagate the interrupted status by calling Thread.currentThread().interrupt();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Copy link
Contributor

@ktkrg ktkrg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good. Left some minor nits and one line changes.

Sid Narayan added 3 commits July 14, 2020 13:27
Previously we had 1 sided TLS on the server side. Data between the
client and server was send over an encrypted channel, but any client
could make requests to the server.

This commit changes the behavior so that only clients with the matching
certificates can make requests to the server when TLS is enabled. This
commit does NOT add support for installing a trust manager. That must be
added in the future.
@sidheart sidheart force-pushed the grpc-mutual-auth-fix branch from 4b13b62 to 73d110e Compare July 14, 2020 20:30
@ktkrg ktkrg merged commit b39d252 into opendistro-for-elasticsearch:master Jul 14, 2020
@sidheart sidheart linked an issue Jul 30, 2020 that may be closed by this pull request
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Configurable Mutual Auth to REST endpoints
3 participants