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 2 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
35 changes: 25 additions & 10 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 @@ -991,19 +991,34 @@ 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 SslContextBuilder.forServer(_cert, _key, pwd)
.ciphers(ciphers)
.applicationProtocolConfig(ApplicationProtocolConfig.DISABLED)
.clientAuth(Objects.requireNonNull(authMode)) // https://github.com/netty/netty/issues/4722
.sessionCacheSize(0)
.sessionTimeout(0)
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

.sslProvider(sslProvider);
}
});

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

return buildSSLContext0(_sslContextBuilder);
} catch (final PrivilegedActionException e) {
throw (SSLException) e.getCause();
stephen-crawford marked this conversation as resolved.
Show resolved Hide resolved
}
}

private SslContextBuilder configureSSLServerContextBuilder(
Expand Down