Fix permissions issues while reading keys in PKCS#1 format #3289

cwperks · 2023-09-01T19:00:01Z

Description

Netty has logic to use the BouncyCastlePemReader if BouncyCastle is located on the class path. The BouncyCastle provider loaded properly in netty, but was failing to read the private key with permissions issues that failed silently. With netty, if one PemReader fails they will fall back to the next which is only capable of reading keys in the PKCS#8 format.

The regression in PKCS#1 keys happened when bouncycastle was upgraded from jdk15on to jdk15to18.

This PR adds permissions to ensure that netty can read the PKCS#1 keys.

This PR also cleans up the policy file to have a single entry for permission java.util.PropertyPermission "*","read,write"; because the other entries are redundant.

Open Questions:

There is a test in SSLTest to ensure PKCS#1 keys can be read. Why did that test not catch this?

Category (Enhancement, New feature, Bug fix, Test fix, Refactoring, Maintenance, Documentation)

Bug fix

Issues Resolved

#3281

Testing

Used the same certs from the SSLTest for PKCS#1 keys. Before the change the 2.9.0 cluster could not be brought up, after the change the cluster starts successfully.

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.

…n PKCS#1 format Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2023-09-01T19:04:12Z

Reverting back to jdk15on for the artifact resolves the PKCS issue, but jdk15on stopped publishing after 1.70 and the artifact has CVEs associated with it which is why the security repo switched from jdk15on to jdk15to18: #2901

codecov · 2023-09-01T19:06:06Z

Codecov Report

Merging #3289 (6b42451) into main (cd45e78) will decrease coverage by 0.02%.
The diff coverage is 37.50%.

@@             Coverage Diff              @@
##               main    #3289      +/-   ##
============================================
- Coverage     63.23%   63.22%   -0.02%     
+ Complexity     3450     3449       -1     
============================================
  Files           263      263              
  Lines         20040    20053      +13     
  Branches       3344     3348       +4     
============================================
+ Hits          12673    12678       +5     
- Misses         5740     5747       +7     
- Partials       1627     1628       +1

Files Changed	Coverage Δ
...ensearch/security/ssl/DefaultSecurityKeyStore.java	`72.29% <37.50%> (-1.70%)`	⬇️

... and 2 files with indirect coverage changes

cwperks · 2023-09-01T19:10:40Z

Trying again with jdk15to18. It may not be possible to upgrade bouncycastle in the security plugin alone. A couple core modules also use bouncycastle.

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2023-09-01T19:15:09Z

plugin-security.policy

@@ -37,7 +37,6 @@ grant {
  permission java.util.PropertyPermission "*","read,write";


This permission covers read and write for all properties so anything else is redundant

Can we investigate these other policy lines separately to minimize the surface area of this change?

The only PropertyPermission this code change removes is permission java.util.PropertyPermission "org.apache.xml.security.ignoreLineBreaks", "write";, but this explicit permission isn't required since there's already an item for permission java.util.PropertyPermission "*","read,write"; near the top of this file. I removed another permission that was commented out as well.

stephen-crawford · 2023-09-01T19:51:57Z

Are the CI failures from something else or are you still working on this?

cwperks · 2023-09-01T19:55:10Z

@scrawfor99 I think they be from something else. I am looking into it now.

Caused by:
java.lang.IllegalStateException: HTTP/2 expected for HTTPS communication but HTTP/1.1 was used
at org.opensearch.security.test.helper.rest.RestHelper.executeRequest(RestHelper.java:286)

peternied

Used the same certs from the SSLTest for PKCS#1 keys. Before the change the 2.9.0 cluster could not be brought up, after the change the cluster starts successfully.

So you can reproduce it, but our test behaves differently? Is it running with the security manager disabled?

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

peternied · 2023-09-01T19:54:50Z

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

+                        .sessionCacheSize(0)
+                        .sessionTimeout(0)


Where did these values come from, how can we confirm these are correct?

I just noticed that this is different on 2.x and main. I was testing with changes on the 2.9 branch where the error was seen, but then stashed my changes and applied them on main. I updated this to accommodate for the differences on main now. These values come from here: https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java#L1009-L1035

peternied · 2023-09-01T19:55:55Z

plugin-security.policy

@@ -37,7 +37,6 @@ grant {
  permission java.util.PropertyPermission "*","read,write";


Can we investigate these other policy lines separately to minimize the surface area of this change?

Signed-off-by: Craig Perkins <[email protected]>

cwperks · 2023-09-01T20:11:06Z

So you can reproduce it, but our test behaves differently? Is it running with the security manager disabled?

That's my hunch as well, but I don't see where/if its disabled for SSLTest

Signed-off-by: Craig Perkins <[email protected]>

peternied · 2023-09-01T23:11:36Z

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

-            ciphers,
-            authMode
-        );
+        try {


Refactor 'doPrivileged(...) with try catch into a function so you can rewrite this block as the following?

final SslContextBuilder _sslContextBuilder = this.doPrivilegedSslAction(() -> configureSSLServerContextBuilder(SslContextBuilder.forServer(_key, _cert), sslProvider, ciphers, authMode));

willyborankin · 2023-09-04T16:34:06Z

I think this problem related to this one as well: #3213

stephen-crawford

This change seems good @cwperks, but how do we know it works? It seems like changes the permission behavior by it is not clear where that gets tested...

I am going to approve but open a fast follow issue and link this.

stephen-crawford · 2023-09-05T19:01:34Z

Follow-up issue: #3318

opensearch-trigger-bot · 2023-09-05T19:02:10Z

The backport to 2.x failed:

The process '/usr/bin/git' failed with exit code 128

To backport manually, run these commands in your terminal:

# Navigate to the root of your repository
cd $(git rev-parse --show-toplevel)
# Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add ../.worktrees/security/backport-2.x 2.x
# Navigate to the new working tree
pushd ../.worktrees/security/backport-2.x
# Create a new branch
git switch --create backport/backport-3289-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 1034cef92eaa20a360c4863106f96b0ae06ab1af
# Push it to GitHub
git push --set-upstream origin backport/backport-3289-to-2.x
# Go back to the original working tree
popd
# Delete the working tree
git worktree remove ../.worktrees/security/backport-2.x

Then, create a pull request where the base branch is 2.x and the compare/head branch is backport/backport-3289-to-2.x.

cwperks · 2023-09-06T13:10:10Z

I will open a manual backport

…rmat (#3322) Backport #3289 to 2.x Signed-off-by: Craig Perkins <[email protected]>

…rmat Backport to 1.x from opensearch-project#3289 Signed-off-by: Andrey Pleskach <[email protected]>

…rmat (#3406) Backport to 1.x from #3289 We did not do it, while 1.x version has the same problem as 2.x branch. Moved only permissions. Signed-off-by: Andrey Pleskach <[email protected]>

Upgrade to bc jdk18on and fix permissions issues while reading keys i…

28cd976

…n PKCS#1 format Signed-off-by: Craig Perkins <[email protected]>

cwperks requested review from cliu123, DarshitChanpura, davidlago, peternied, RyanL1997, stephen-crawford, reta and willyborankin as code owners September 1, 2023 19:00

Switch back to jdk15to18

b881c81

Signed-off-by: Craig Perkins <[email protected]>

cwperks changed the title ~~Upgrade to bc jdk18on and fix permissions issues while reading keys in PKCS#1 format~~ Fix permissions issues while reading keys in PKCS#1 format Sep 1, 2023

cwperks commented Sep 1, 2023

View reviewed changes

peternied reviewed Sep 1, 2023

View reviewed changes

cwperks added 2 commits September 1, 2023 16:05

Adjust to main

2914474

Signed-off-by: Craig Perkins <[email protected]>

Check if SSLException

090d6e1

Signed-off-by: Craig Perkins <[email protected]>

Keep length check

6b42451

Signed-off-by: Craig Perkins <[email protected]>

peternied reviewed Sep 1, 2023

View reviewed changes

willyborankin approved these changes Sep 4, 2023

View reviewed changes

willyborankin mentioned this pull request Sep 5, 2023

Security Plugin cannot startup due to AccessControlException: access denied #3317

Closed

3 tasks

stephen-crawford approved these changes Sep 5, 2023

View reviewed changes

stephen-crawford mentioned this pull request Sep 5, 2023

Add testing and documentation for PKCS#1 #3318

Open

stephen-crawford added the backport 2.x backport to 2.x branch label Sep 5, 2023

stephen-crawford merged commit 1034cef into opensearch-project:main Sep 5, 2023
56 of 58 checks passed

opensearch-trigger-bot bot added the backport-failed label Sep 5, 2023

cwperks mentioned this pull request Sep 6, 2023

[Backport 2.x] Fix permissions issues while reading keys in PKCS#1 format #3322

Merged

cwperks added a commit that referenced this pull request Sep 6, 2023

[Backport 2.x] Fix permissions issues while reading keys in PKCS#1 fo…

552de1b

…rmat (#3322) Backport #3289 to 2.x Signed-off-by: Craig Perkins <[email protected]>

jackiehanyang mentioned this pull request Sep 6, 2023

[AUTOCUT] Integration Test failed for anomaly-detection: 2.10.0 tar distribution opensearch-project/anomaly-detection#1002

Closed

stephen-crawford mentioned this pull request Sep 18, 2023

[BUG] Somehow security plugin rewrites permissions for other plugins which use Bouncy Castle #3213

Closed

willyborankin added a commit to willyborankin/security that referenced this pull request Sep 25, 2023

[Backport 1.x] Fix permissions issues while reading keys in PKCS#1 fo…

6aae3b5

…rmat Backport to 1.x from opensearch-project#3289 Signed-off-by: Andrey Pleskach <[email protected]>

willyborankin mentioned this pull request Sep 25, 2023

[Backport 1.x] Fix permissions issues while reading keys in PKCS#1 format #3406

Merged

DarshitChanpura removed the backport-failed label Jul 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix permissions issues while reading keys in PKCS#1 format #3289

Fix permissions issues while reading keys in PKCS#1 format #3289

cwperks commented Sep 1, 2023

cwperks commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 1, 2023 •

edited

Loading

cwperks commented Sep 1, 2023

cwperks Sep 1, 2023 •

edited

Loading

peternied Sep 1, 2023

cwperks Sep 1, 2023

stephen-crawford commented Sep 1, 2023

cwperks commented Sep 1, 2023

peternied left a comment

peternied Sep 1, 2023

cwperks Sep 1, 2023

peternied Sep 1, 2023

cwperks commented Sep 1, 2023

peternied Sep 1, 2023

willyborankin commented Sep 4, 2023

stephen-crawford left a comment

stephen-crawford commented Sep 5, 2023

opensearch-trigger-bot bot commented Sep 5, 2023

cwperks commented Sep 6, 2023

		@@ -37,7 +37,6 @@ grant {
		permission java.util.PropertyPermission "*","read,write";

Fix permissions issues while reading keys in PKCS#1 format #3289

Fix permissions issues while reading keys in PKCS#1 format #3289

Conversation

cwperks commented Sep 1, 2023

Description

Issues Resolved

Testing

Check List

cwperks commented Sep 1, 2023 • edited Loading

codecov bot commented Sep 1, 2023 • edited Loading

Codecov Report

cwperks commented Sep 1, 2023

cwperks Sep 1, 2023 • edited Loading

Choose a reason for hiding this comment

peternied Sep 1, 2023

Choose a reason for hiding this comment

cwperks Sep 1, 2023

Choose a reason for hiding this comment

stephen-crawford commented Sep 1, 2023

cwperks commented Sep 1, 2023

peternied left a comment

Choose a reason for hiding this comment

peternied Sep 1, 2023

Choose a reason for hiding this comment

cwperks Sep 1, 2023

Choose a reason for hiding this comment

peternied Sep 1, 2023

Choose a reason for hiding this comment

cwperks commented Sep 1, 2023

peternied Sep 1, 2023

Choose a reason for hiding this comment

willyborankin commented Sep 4, 2023

stephen-crawford left a comment

Choose a reason for hiding this comment

stephen-crawford commented Sep 5, 2023

opensearch-trigger-bot bot commented Sep 5, 2023

cwperks commented Sep 6, 2023

cwperks commented Sep 1, 2023 •

edited

Loading

codecov bot commented Sep 1, 2023 •

edited

Loading

cwperks Sep 1, 2023 •

edited

Loading