Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Sep 5, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
28cd976
Upgrade to bc jdk18on and fix permissions issues while reading keys i…
cwperks Sep 1, 2023
b881c81
Switch back to jdk15to18
cwperks Sep 1, 2023
2914474
Adjust to main
cwperks Sep 1, 2023
090d6e1
Check if SSLException
cwperks Sep 1, 2023
6b42451
Keep length check
cwperks Sep 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions plugin-security.policy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,6 @@ grant {
permission java.util.PropertyPermission "*","read,write";
Copy link
Member Author

@cwperks cwperks Sep 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


//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
67 changes: 46 additions & 21 deletions src/main/java/org/opensearch/security/ssl/DefaultSecurityKeyStore.java
View file
Open in desktop
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 {
Copy link
Member

Choose a reason for hiding this comment

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

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));

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