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

Jump to bottom

Remove hardcoded provider #4588

Closed
wants to merge 10 commits into from
Closed

Remove hardcoded provider #4588

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
20f49f0
Remove hardcoded provider reference
terryquigleysas Jul 22, 2024
0f4458d
Spotless
terryquigleysas Jul 22, 2024
5c255ef
Merge branch 'opensearch-project:main' into remove_hardcoded_bc_provider
terryquigleysas Jul 26, 2024
6b1b9a5
Refactored adding of Bouncy Castle provider.
terryquigleysas Jul 26, 2024
0781143
Additional property added to policy for BCFIPS
terryquigleysas Jul 26, 2024
5b4ca68
Additional property added to policy for BCFIPS
terryquigleysas Jul 26, 2024
750f4ef
Merge branch 'remove_hardcoded_bc_provider' of https://github.com/ter…
terryquigleysas Jul 26, 2024
4c0e650
Merge branch 'main' into remove_hardcoded_bc_provider
terryquigleysas Jul 26, 2024
c5a3d45
Reorder policy file
terryquigleysas Jul 26, 2024
a286e29
Refactor
terryquigleysas Jul 26, 2024
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
6 changes: 6 additions & 0 deletions plugin-security.policy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,12 @@ grant {
permission java.security.SecurityPermission "getProperty.org.bouncycastle.rsa.max_size";
permission java.security.SecurityPermission "getProperty.org.bouncycastle.rsa.max_mr_tests";

// Additional BouncyCastle FIPS permissions
permission java.security.SecurityPermission "putProviderProperty.BCFIPS";
permission java.security.SecurityPermission "insertProvider.BCFIPS";
permission java.security.SecurityPermission "removeProviderProperty.BCFIPS";
permission java.security.SecurityPermission "getProperty.org.bouncycastle.disabledAlgorithms";

permission java.lang.RuntimePermission "accessUserInformation";

permission java.security.SecurityPermission "org.apache.xml.security.register";
Expand Down
52 changes: 38 additions & 14 deletions src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
import java.security.AccessController;
import java.security.MessageDigest;
import java.security.PrivilegedAction;
import java.security.Provider;
import java.security.Security;
import java.util.ArrayList;
import java.util.Arrays;
Expand Down Expand Up @@ -63,7 +64,6 @@
import org.apache.logging.log4j.Logger;
import org.apache.lucene.search.QueryCachingPolicy;
import org.apache.lucene.search.Weight;
import org.bouncycastle.jce.provider.BouncyCastleProvider;

import org.opensearch.OpenSearchException;
import org.opensearch.OpenSearchSecurityException;
Expand Down Expand Up @@ -378,26 +378,15 @@
demoCertHashes.add("a2ce3f577a5031398c1b4f58761444d837b031d0aff7614f8b9b5e4a9d59dbd1"); // esnode
demoCertHashes.add("cd708e8dc707ae065f7ad8582979764b497f062e273d478054ab2f49c5469c6"); // root-ca

tryAddSecurityProviders();

// updates correct sha256sum
demoCertHashes.add("a3556d6bb61f7bd63cb19b1c8d0078d30c12739dedb0455c5792ac8627782042"); // kirk
demoCertHashes.add("25e34a9a5d4f1dceed1666eb624397bf3fe5787a7133cd32838ace0381bce1f7"); // kirk-key
demoCertHashes.add("a2ce3f577a5031398c1b4f58761444d837b031d0aff7614f8b9b5e4a9d59dbd1"); // esnode
demoCertHashes.add("ba9c5a61065f7f6115188128ffbdaa18fca34562b78b811f082439e2bef1d282"); // esnode-key
demoCertHashes.add("bcd708e8dc707ae065f7ad8582979764b497f062e273d478054ab2f49c5469c6"); // root-ca

final SecurityManager sm = System.getSecurityManager();

if (sm != null) {
sm.checkPermission(new SpecialPermission());
}
Comment on lines -388 to -392
Copy link
Contributor

Choose a reason for hiding this comment

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

What was this used for? What does the special permission check do and why is it no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@derek-ho I think this boilerplate code setup for doPrivileged calls originates from the Elasticsearch recommendations here https://www.elastic.co/guide/en/elasticsearch/plugins/current/creating-classic-plugins.html#plugin-authors-jsm


AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
if (Security.getProvider("BC") == null) {
Security.addProvider(new BouncyCastleProvider());
}
Comment on lines -395 to -397
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you point me to the code that would be doing the instantiation instead of this block right here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would now be handled by the JDK setup itself which I believe is preferred. I have tested the Blake2b code and the demo certificates against a local JDK 17, the bundled JDK 21 and run the Bulk Integration Tests.

Otherwise an alternative could be to hardcode one or both , e.g like 2.11...terryquigleysas:security:2.11#diff-e7ef66295cba81a49e4349781a2e9678d2b021a570e978bb4feb422b03d5d74aR334

For FIPS we can reconfigure the Java environment, e.g. #3420 (comment)

return null;
});

final String advancedModulesEnabledKey = ConfigConstants.SECURITY_ADVANCED_MODULES_ENABLED;
if (settings.hasValue(advancedModulesEnabledKey)) {
deprecationLogger.deprecate("Setting {} is ignored.", advancedModulesEnabledKey);
Expand Down Expand Up @@ -491,6 +480,41 @@
}
}

@SuppressWarnings("removal")
private void tryAddSecurityProviders() {
final SecurityManager sm = System.getSecurityManager();

if (sm != null) {
sm.checkPermission(new SpecialPermission());

Check warning on line 488 in src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L488 

Added line #L488 was not covered by tests
}

// Add provider if on the classpath. Only add first provider found.
AccessController.doPrivileged((PrivilegedAction<Void>) () -> {
if (Security.getProvider("BC") == null) {
try {
Class<?> providerClass = Class.forName("org.bouncycastle.jce.provider.BouncyCastleProvider");
Provider provider = (Provider) providerClass.getDeclaredConstructor().newInstance();
Security.addProvider(provider);
log.debug("Bouncy Castle Provider added");
return null;
} catch (Exception e) {
log.debug("Bouncy Castle Provider could not be added", e);

Check warning on line 501 in src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L500-L501 

Added lines #L500 - L501 were not covered by tests
}
}
if (Security.getProvider("BCFIPS") == null) {
try {
Class<?> providerClass = Class.forName("org.bouncycastle.jcajce.provider.BouncyCastleFipsProvider");
Provider provider = (Provider) providerClass.getDeclaredConstructor().newInstance();
Security.addProvider(provider);
log.debug("Bouncy Castle FIPS Provider added");

Check warning on line 509 in src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L506-L509 

Added lines #L506 - L509 were not covered by tests
} catch (Exception e) {
log.debug("Bouncy Castle FIPS Provider could not be added", e);
}

Check warning on line 512 in src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java

View check run for this annotation

Codecov / codecov/patch

src/main/java/org/opensearch/security/OpenSearchSecurityPlugin.java#L512 

Added line #L512 was not covered by tests
}
return null;
});
}

private void verifyTLSVersion(final String settings, final List<String> configuredProtocols) {
for (final var tls : configuredProtocols) {
if (tls.equalsIgnoreCase("TLSv1") || tls.equalsIgnoreCase("TLSv1.1")) {
Expand Down
Loading