Identify extension Transport requests and permit handshake and extension registration actions #2599
Conversation
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
Codecov Report
📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more @@ Coverage Diff @@
## main #2599 +/- ##
============================================
+ Coverage 61.46% 61.55% +0.09%
- Complexity 3391 3403 +12
============================================
Files 272 272
Lines 18740 18750 +10
Branches 3284 3288 +4
============================================
+ Hits 11518 11542 +24
+ Misses 5618 5609 -9
+ Partials 1604 1599 -5
|
src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java
Show resolved
Hide resolved
Signed-off-by: Craig Perkins <[email protected]>
|
There's a call to |
Signed-off-by: Craig Perkins <[email protected]>
cwperks
requested review from
cliu123,
DarshitChanpura,
davidlago,
peternied,
RyanL1997 and
stephen-crawford
as code owners
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
| @@ -326,6 +327,11 @@ protected void addAdditionalContextValues(final String action, final TransportRe | |||
| } | |||
| } | |||
|
|
|||
| String extensionUniqueId = getThreadContext().getHeader("extension_unique_id"); | |||
| if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS) && extensionUniqueId != null) { | |||
There was a problem hiding this comment.
Can this check be stronger?
Absolutely, this check is basic at the moment and only checks for the presence of the header. A simple improvement on this would be to check not only the existence of the header, but also that it is a value contained in extensions/extensions.yml.
There should also be a check similar to nodes_dn that verifies that the principal extracted from the certificate of the extension matches a list of known extensions_dns that would be placed in the appropriate area of the extensions/extensions.yml configuration for the extension.
There was a problem hiding this comment.
I modified this to check for the ID in the extensions registry.
There was a problem hiding this comment.
Is there a way we could remove the FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS) check? It seems redundant as the extension_unique_id value won't be set and the ExtensionsManager won't find any matching extensions
There was a problem hiding this comment.
+1 to removing redundant check. Also, we should add automated tests to verify this behavior
There was a problem hiding this comment.
@DarshitChanpura sure I will remove the check. I do think the check should remain, but I'll remove it since it if it seems redundant.
I'll look into how a test can be written to verify this behavior.
There was a problem hiding this comment.
@cwperks Seems like there is a principal you'd like to see in how some features work that is at odds with our other practices. If you were to ask me what I think your principal is I'd state it as "Functionality for extensions should be behind a feature flag(s) to prevent issues on clusters that do not have extensions enabled". Does this seem correct, did you want to expand on this?
This is a counterpoint to "Reduce coupling whenever possible". When we make a call to a static function call to determine global state the responsibility of the code has been broadly increased, making maintenance more difficult.
I think this dialog represents different principals we'd like to see prioritized in the code, it might documenting and discussing as a tenant, what do you think?
There was a problem hiding this comment.
@peternied Yes that's correct. In this particular case its new functionality in the security plugin that would directly let an extension handshake with an opensearch node and also submit transport requests to initialize the extension like registering its REST handlers with OpenSearch - all of this is specific to extensions and is not permitted for anything other than an extension. In this case, I can't see any abstraction to add here because it is functionality for extensions and since other extension functionality is gated with a feature flag I think its appropriate to include here too.
I don't see this as coupling necessarily except that there are 3 places that now rely on the same threadcontext header and I'm not sure how to do that otherwise.
Cross cluster scenarios are simpler because its the security plugin that populates the threadcontext header so the security plugin is the only codebase that needs to be aware of what that header is. I can't see a better way here currently.
There was a problem hiding this comment.
@DarshitChanpura @peternied I removed the feature flag check. There currently is no great way to test this change in an automated test. I think the best way to automate a check for this change will be an integration test that verifies that an extension can be initialized when connecting with an opensearch node with the security plugin installed. There is an open issue on the sdk repo for integration tests that will eventually validate this change.
Signed-off-by: Craig Perkins <[email protected]>
|
@opensearch-project/security - The related PRs in core and the SDK have been merged. Can I get some reviews of this PR now? |
| @@ -326,6 +327,11 @@ protected void addAdditionalContextValues(final String action, final TransportRe | |||
| } | |||
| } | |||
|
|
|||
| String extensionUniqueId = getThreadContext().getHeader("extension_unique_id"); | |||
| if (FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS) && extensionUniqueId != null) { | |||
There was a problem hiding this comment.
Is there a way we could remove the FeatureFlags.isEnabled(FeatureFlags.EXTENSIONS) check? It seems redundant as the extension_unique_id value won't be set and the ExtensionsManager won't find any matching extensions
| @@ -326,6 +329,14 @@ protected void addAdditionalContextValues(final String action, final TransportRe | |||
| } | |||
| } | |||
|
|
|||
| String extensionUniqueId = getThreadContext().getHeader("extension_unique_id"); | |||
There was a problem hiding this comment.
For follow up PR, can we put this in a constant in core and use that constant here?
There was a problem hiding this comment.
@peternied yes that's a good idea to import the constant from core. After this PR, I plan to expand on the TLS logic to introduce extension_dns similar to node_dns that make this check stronger by verifying that the principal extracted from the extension certificate is present in a list of known principals and will include the change to make this a constant.
For the FeatureFlags, I do think it makes sense to include a reference to the feature flag since this feature is directly related to extensions though its not necessarily needed.
Signed-off-by: Craig Perkins <[email protected]>
Signed-off-by: Craig Perkins <[email protected]>
There was a problem hiding this comment.
Thanks @cwperks for addressing PR comments. Feel free to merge this PR.
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-2599-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1ff823f07256e6bbeaef558910a643fec0cc0037
# Push it to GitHub
git push --set-upstream origin backport/backport-2599-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.xThen, create a pull request where the |
…ion registration actions (opensearch-project#2599) * Identify extension Transport requests and permit handshake and extension registration actions Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit 1ff823f)
…ion registration actions (#2599) (#2734) * Identify extension Transport requests and permit handshake and extension registration actions Signed-off-by: Craig Perkins <[email protected]> (cherry picked from commit 1ff823f) Signed-off-by: Craig Perkins <[email protected]>
…ion registration actions (opensearch-project#2599) * Identify extension Transport requests and permit handshake and extension registration actions Signed-off-by: Craig Perkins <[email protected]>
…ion registration actions (opensearch-project#2599) * Identify extension Transport requests and permit handshake and extension registration actions Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Maciej Mierzwa <[email protected]>
…ion registration actions (opensearch-project#2599) * Identify extension Transport requests and permit handshake and extension registration actions Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Maciej Mierzwa <[email protected]>
…ion registration actions (opensearch-project#2599) * Identify extension Transport requests and permit handshake and extension registration actions Signed-off-by: Craig Perkins <[email protected]> Signed-off-by: Sam <[email protected]>
Description
Companion PR in core: opensearch-project/OpenSearch#6866
Compare PR in sdk repo: opensearch-project/opensearch-sdk-java#619
I am opening up a draft PR for comments. When running the HelloWorld extension with the Security plugin installed, I ran into an issue where the extension and the cluster could not communicate. When implementing TLS for extensions in the SDK I ran into an issue where during the handshake process of the extension connecting with the OS node it would always hit this block of the Security plugin: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/transport/SecurityRequestHandler.java#L260-L268.
To get beyond this, I took inspiration from cross cluster search where nodes in the cluster identify which cluster they are a member of using the ThreadContext header:
_opendistro_security_remotecn. This will identify a transport request coming from an extension using the headerextension_unique_idwhich is populated in a method from core that the extension can use when handshaking with the opensearch node. I'm not confident that I fully understand how theplugins.security.nodes_dnsetting works to identify nodes in the cluster and used to identify intra cluster and cross cluster transport requests so I am looking into it in more detail now.New feature
Testing
Tested by running the HelloWorld extension with transport ssl enabled and verified that the handshake request was permitted and the extension registration process completed successfully. I will add a document outlining how to setup the HelloWorld extension with TLS and show it working with the security plugin.
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.