Skip to content

Peer-forwarder hostname verification#1778

Merged
dlvenable merged 1 commit intoopensearch-project:mainfrom
dlvenable:peer-forwarder-host-validation
Sep 17, 2022
Merged

Peer-forwarder hostname verification#1778
dlvenable merged 1 commit intoopensearch-project:mainfrom
dlvenable:peer-forwarder-host-validation

Conversation

@dlvenable
Copy link
Copy Markdown
Member

Description

This PR makes a few changes to support hostname verification:

  • Provide a new property: ssl_insecure_disable_verification. When set to true, this will disable server verification.
  • By default, validate the hostname.
  • Updated the test certificates to use a SAN to help support hostname verification on localhost.
  • Added documentation to this affect.

Issues Resolved

Resolves #1775

Check List

  • New functionality includes testing.
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed with a real name per the DCO

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.

…ility to disable hostname verification. Generate test certificates with a SAN and document how to do it.

Signed-off-by: David Venable <dlv@amazon.com>
@dlvenable dlvenable requested a review from a team as a code owner September 16, 2022 20:36
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Sep 16, 2022

Codecov Report

Merging #1778 (bb5a5ad) into main (ddd3af8) will decrease coverage by 0.10%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main    #1778      +/-   ##
============================================
- Coverage     94.00%   93.89%   -0.11%     
- Complexity     1449     1453       +4     
============================================
  Files           188      188              
  Lines          4252     4261       +9     
  Branches        343      344       +1     
============================================
+ Hits           3997     4001       +4     
- Misses          174      179       +5     
  Partials         81       81              
Impacted Files Coverage Δ
...arch/dataprepper/peerforwarder/PeerClientPool.java 100.00% <100.00%> (ø)
...pper/peerforwarder/PeerForwarderClientFactory.java 94.73% <100.00%> (+0.29%) ⬆️
...pper/peerforwarder/PeerForwarderConfiguration.java 87.43% <100.00%> (+0.35%) ⬆️
.../org/opensearch/dataprepper/pipeline/Pipeline.java 88.40% <0.00%> (-4.35%) ⬇️
...rwarder/discovery/AwsCloudMapPeerListProvider.java 92.59% <0.00%> (-2.47%) ⬇️

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

Copy link
Copy Markdown
Collaborator

@oeyh oeyh left a comment

Choose a reason for hiding this comment

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

Looks good!

@Test
void send_Events_to_an_unknown_server_should_throw() {
final PeerForwarderConfiguration peerForwarderConfiguration = createConfiguration(
true, ForwardingAuthentication.MUTUAL_TLS, ALTERNATE_SSL_CERTIFICATE_FILE, SSL_KEY_FILE, true);
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: Is the key supposed to be ALTERNATE_SSL_KEY_FILE here?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, but I see how this is confusing. I am intentionally using the trusted key. But, I'm configuring the client to trust the alternate key.

@dlvenable dlvenable merged commit 96ed120 into opensearch-project:main Sep 17, 2022
@dlvenable dlvenable deleted the peer-forwarder-host-validation branch September 19, 2022 16:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Validate server certificates on the client

4 participants