Skip to content

Commit

Permalink
Fix permissions issues while reading keys in PKCS#1 format (#3289)
Browse files Browse the repository at this point in the history
### 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](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).

---------

Signed-off-by: Craig Perkins <cwperx@amazon.com>
  • Loading branch information
cwperks authored Sep 5, 2023
1 parent 49ebf4a commit 1034cef
Show file tree
Hide file tree
Showing 2 changed files with 48 additions and 23 deletions.
4 changes: 2 additions & 2 deletions plugin-security.policy
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ grant {
permission java.util.PropertyPermission "*","read,write";

//Enable when we switch to UnboundID LDAP SDK
//permission java.util.PropertyPermission "*", "read,write";
//permission java.lang.RuntimePermission "setFactory";
//permission javax.net.ssl.SSLPermission "setHostnameVerifier";

Expand All @@ -60,11 +59,12 @@ grant {
permission java.security.SecurityPermission "putProviderProperty.BC";
permission java.security.SecurityPermission "insertProvider.BC";
permission java.security.SecurityPermission "removeProviderProperty.BC";
permission java.security.SecurityPermission "getProperty.org.bouncycastle.rsa.max_size";
permission java.security.SecurityPermission "getProperty.org.bouncycastle.rsa.max_mr_tests";

permission java.lang.RuntimePermission "accessUserInformation";

permission java.security.SecurityPermission "org.apache.xml.security.register";
permission java.util.PropertyPermission "org.apache.xml.security.ignoreLineBreaks", "write";

permission java.lang.RuntimePermission "createClassLoader";

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -968,18 +968,26 @@ private SslContext buildSSLServerContext(
final ClientAuth authMode
) throws SSLException {

final SslContextBuilder _sslContextBuilder = configureSSLServerContextBuilder(
SslContextBuilder.forServer(_key, _cert),
sslProvider,
ciphers,
authMode
);
try {
final SslContextBuilder _sslContextBuilder = AccessController.doPrivileged(new PrivilegedExceptionAction<SslContextBuilder>() {
@Override
public SslContextBuilder run() throws Exception {
return configureSSLServerContextBuilder(SslContextBuilder.forServer(_key, _cert), sslProvider, ciphers, authMode);
}
});

if (_trustedCerts != null && _trustedCerts.length > 0) {
_sslContextBuilder.trustManager(_trustedCerts);
}
if (_trustedCerts != null && _trustedCerts.length > 0) {
_sslContextBuilder.trustManager(_trustedCerts);
}

return buildSSLContext0(_sslContextBuilder);
return buildSSLContext0(_sslContextBuilder);
} catch (final PrivilegedActionException e) {
if (e.getCause() instanceof SSLException) {
throw (SSLException) e.getCause();
} else {
throw new RuntimeException(e);
}
}
}

private SslContext buildSSLServerContext(
Expand All @@ -991,19 +999,32 @@ private SslContext buildSSLServerContext(
final SslProvider sslProvider,
final ClientAuth authMode
) throws SSLException {
final SecurityManager sm = System.getSecurityManager();

final SslContextBuilder _sslContextBuilder = configureSSLServerContextBuilder(
SslContextBuilder.forServer(_cert, _key, pwd),
sslProvider,
ciphers,
authMode
);

if (_trustedCerts != null) {
_sslContextBuilder.trustManager(_trustedCerts);
if (sm != null) {
sm.checkPermission(new SpecialPermission());
}

return buildSSLContext0(_sslContextBuilder);
try {
final SslContextBuilder _sslContextBuilder = AccessController.doPrivileged(new PrivilegedExceptionAction<SslContextBuilder>() {
@Override
public SslContextBuilder run() throws Exception {
return configureSSLServerContextBuilder(SslContextBuilder.forServer(_cert, _key, pwd), sslProvider, ciphers, authMode);
}
});

if (_trustedCerts != null) {
_sslContextBuilder.trustManager(_trustedCerts);
}

return buildSSLContext0(_sslContextBuilder);
} catch (final PrivilegedActionException e) {
if (e.getCause() instanceof SSLException) {
throw (SSLException) e.getCause();
} else {
throw new RuntimeException(e);
}
}
}

private SslContextBuilder configureSSLServerContextBuilder(
Expand Down Expand Up @@ -1095,7 +1116,11 @@ public SslContext run() throws Exception {
}
});
} catch (final PrivilegedActionException e) {
throw (SSLException) e.getCause();
if (e.getCause() instanceof SSLException) {
throw (SSLException) e.getCause();
} else {
throw new RuntimeException(e);
}
}

return sslContext;
Expand Down

0 comments on commit 1034cef

Please sign in to comment.