Support certs with separate Extended Key Usage #493

lavacat · 2020-06-04T19:24:38Z

Problem: Right now certs that specify extended key usage must set
both clientAuth and serverAuth.

Solution: Add settings to path additional server cert and use it when
creating ssl server

*Issue #474 *

Description of changes:

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

vrozov · 2020-06-05T16:25:19Z

@lavacat Please check why CI build failed

debjanibnrj · 2020-06-05T16:36:27Z

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

@@ -17,6 +17,7 @@



Currently the changes made are only for PEM certs. Since we support jks certs as well, we will probably need to extend these changes to include JKS certs (link)

debjanibnrj · 2020-06-05T16:38:16Z

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

+                SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED_DEFAULT);
+            // certificate allows clientAuth and serverAuth, use same cert
+            SSLCertificateProps sslServerCertProps = sslCertProps;
+            if (!mixedKeyUsageEnabled) {


Slight confused by mixedKeyUsageEnabled. Does that mean the cert has both clientAuth and serverAuth?

debjanibnrj · 2020-06-05T16:41:37Z

src/test/java/com/amazon/opendistroforelasticsearch/security/test/AbstractSecurityUnitTest.java

@@ -258,7 +258,7 @@ protected NodeSettingsSupplier minimumSecuritySettingsSslOnly(Settings other) {
        return new NodeSettingsSupplier() {
            @Override
            public Settings get(int i) {
-                return minimumSecuritySettingsBuilder(i, true, false).put(other).build();
+                return minimumSecuritySettingsBuilder(i, true, true).put(other).build();


Why update this from true to false? It is set to false by default so that custom transport settings are used.

src/main/java/com/amazon/opendistroforelasticsearch/security/ssl/util/SSLConfigConstants.java

debjanibnrj · 2020-06-05T17:14:02Z

...ain/java/com/amazon/opendistroforelasticsearch/security/ssl/OpenDistroSecuritySSLPlugin.java

@@ -329,6 +329,12 @@ public Object run() {
        settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_PEMKEY_PASSWORD, Property.NodeScope, Property.Filtered));
        settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_PEMTRUSTEDCAS_FILEPATH, Property.NodeScope, Property.Filtered));

+        settings.add(Setting.boolSetting(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED, SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED_DEFAULT, Property.NodeScope, Property.Filtered));


it may be good to disable these settings if mixedKeyUsage is false. Eg -

Suggested change

settings.add(Setting.boolSetting(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED, SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED_DEFAULT, Property.NodeScope, Property.Filtered));

if (!isMixedKeyUsageAllowed(settings)) {

settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMCERT_FILEPATH, Property.NodeScope, Property.Filtered));

settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMKEY_FILEPATH, Property.NodeScope, Property.Filtered));

settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMKEY_PASSWORD, Property.NodeScope, Property.Filtered));

settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMTRUSTEDCAS_FILEPATH, Property.NodeScope, Property.Filtered));

}

You can create a helper function like -

private static boolean isMixedKeyUsageAllowed(final Settings settings) { return settings.getAsBoolean(SSLConfigConstants. OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED, false); }

lavacat · 2020-08-31T19:17:27Z

@debjanibnrj Thanks for comments. I'll resume working on this.

will add JKS cert support
mixedKeyUsageEnabled means both clientAuth and serverAuth, if it's confusing, happy to use some other name
agree with having .server. and .client. and defaulting to old way. Will refactor

codecov · 2020-10-12T18:27:06Z

Codecov Report

Merging #493 (0c1aefb) into master (8419ab5) will increase coverage by 0.36%.
The diff coverage is 90.43%.

@@             Coverage Diff              @@
##             master     #493      +/-   ##
============================================
+ Coverage     63.91%   64.27%   +0.36%     
- Complexity     3169     3210      +41     
============================================
  Files           239      244       +5     
  Lines         16882    17040     +158     
  Branches       3036     3034       -2     
============================================
+ Hits          10790    10953     +163     
+ Misses         4543     4541       -2     
+ Partials       1549     1546       -3

Impacted Files	Coverage Δ	Complexity Δ
...icsearch/security/ssl/util/SSLConfigConstants.java	`77.77% <ø> (ø)`	`7.00 <0.00> (ø)`
...istroforelasticsearch/security/ssl/util/Utils.java	`63.63% <0.00%> (-6.37%)`	`5.00 <1.00> (+1.00)`	⬇️
...elasticsearch/security/ssl/util/KeystoreProps.java	`72.72% <72.72%> (ø)`	`2.00 <2.00> (?)`
...sticsearch/security/ssl/util/CertFromKeystore.java	`73.91% <73.91%> (ø)`	`8.00 <8.00> (?)`
...icsearch/security/ssl/util/CertFromTruststore.java	`90.90% <90.90%> (ø)`	`8.00 <8.00> (?)`
...ecurity/ssl/DefaultOpenDistroSecurityKeyStore.java	`69.10% <94.38%> (+3.20%)`	`71.00 <10.00> (+3.00)`
...arch/security/ssl/OpenDistroSecuritySSLPlugin.java	`84.44% <100.00%> (+1.83%)`	`25.00 <0.00> (+1.00)`
...elasticsearch/security/ssl/util/CertFileProps.java	`100.00% <100.00%> (ø)`	`5.00 <5.00> (?)`
...relasticsearch/security/ssl/util/CertFromFile.java	`100.00% <100.00%> (ø)`	`13.00 <13.00> (?)`
...relasticsearch/security/auditlog/NullAuditLog.java	`33.33% <0.00%> (-4.77%)`	`7.00% <0.00%> (-1.00%)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8419ab5...3e54769. Read the comment docs.

nicot

Looks great to me! Thanks for making these changes.

nicot · 2020-10-12T23:04:07Z

src/test/resources/ssl/extended_key_usage/README.txt

@@ -0,0 +1,22 @@
+# Commands to generate certs in this folder


Thanks for adding this!

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

...ain/java/com/amazon/opendistroforelasticsearch/security/ssl/OpenDistroSecuritySSLPlugin.java

debjanibnrj · 2020-10-22T17:29:48Z

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java


+        } else if (rawPemCertFilePath != null || extendedKeyUsageEnabled) {


Why add || extendedKeyUsageEnabled here?

debjanibnrj · 2020-10-22T17:36:34Z

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

+            KeystoreProps truststoreProps = new KeystoreProps(
+                truststoreFilePath, truststoreType, truststorePassword);
+            try {
+                CertFromKeystore certFromKeystore;


Thanks for refactoring these into classes. Much cleaner approach. 👍

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

...ain/java/com/amazon/opendistroforelasticsearch/security/ssl/OpenDistroSecuritySSLPlugin.java

src/main/java/com/amazon/opendistroforelasticsearch/security/ssl/util/CertFromKeystore.java

lavacat · 2020-11-20T22:20:33Z

@nicot @debjanibnrj

added keystore, truststore files and integration test in SSLTest.java
added unit tests for all new CertFrom* classes
cleaned up imports

@debjanibnrj based on your question "Why add || extendedKeyUsageEnabled here?"
did a small refactor and added cleaner useKeyStore and useRawFiles variables

Nothing else changed

debjanibnrj

Thanks for your contribution. Added a minor comment but overall looks great!

debjanibnrj · 2020-11-21T00:22:29Z

@hardik-k-shah can you have a look as well

lavacat · 2020-11-23T17:50:07Z

Thanks for your contribution. Added a minor comment but overall looks great!

@debjanibnrj FYI, don't see the new comments. I think I've addressed all older ones.

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

hardik-k-shah

Thanks @lavacat and @nicot for your contribution.
Overall it looks good. Few minor comments.

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

hardik-k-shah · 2020-11-24T02:06:20Z

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java

+                            null);
+                    final String keyPassword = settings.get(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_KEYSTORE_KEYPASSWORD,
+                            keystorePassword);
+


should we also check truststoreAlias and keystoreAlias for non-null and throw exception if anyone of it was set to null?

If the alias isn't provided SSLCertificateHelper.exportServerCertChain will take the first one. I've made it required for extended key usage because server/client alias should be different at least for keystore.
Made a comment and added a test

src/test/java/com/amazon/opendistroforelasticsearch/security/ssl/util/CertFromFileTests.java

lavacat · 2020-11-25T06:44:40Z

@debjanibnrj @hardik-k-shah thanks for comments and noticing those mistakes, made all changes as you've suggested.

Problem: Right now certs that specify extended key usage must set both clientAuth and serverAuth. Solution: Add settings to path additional server cert and use it when creating ssl server

Problem: Right now certs that specify extended key usage must set both clientAuth and serverAuth. Solution: Add settings to path additional server cert and use it when creating ssl server Co-authored-by: Bogdan Kanivets <bkanivets@twitter.com>

lavacat requested a review from a team as a code owner June 4, 2020 19:24

lavacat changed the title ~~Support certs with separate Extended Key Usage~~ WIP: Support certs with separate Extended Key Usage Jun 4, 2020

debjanibnrj suggested changes Jun 5, 2020

View reviewed changes

lavacat force-pushed the master branch from a5acada to 37c0ccb Compare October 2, 2020 00:12

lavacat force-pushed the master branch from 37c0ccb to 03c005a Compare October 12, 2020 17:49

lavacat requested a review from debjanibnrj October 12, 2020 19:24

nicot previously approved these changes Oct 12, 2020

View reviewed changes

lavacat changed the title ~~WIP: Support certs with separate Extended Key Usage~~ Support certs with separate Extended Key Usage Oct 21, 2020

debjanibnrj reviewed Oct 22, 2020

View reviewed changes

lavacat dismissed nicot’s stale review via f9e490b November 20, 2020 21:54

lavacat force-pushed the master branch from 03c005a to f9e490b Compare November 20, 2020 21:54

debjanibnrj previously approved these changes Nov 20, 2020

View reviewed changes

nicot previously approved these changes Nov 20, 2020

View reviewed changes

debjanibnrj reviewed Nov 23, 2020

View reviewed changes

...va/com/amazon/opendistroforelasticsearch/security/ssl/DefaultOpenDistroSecurityKeyStore.java Outdated Show resolved Hide resolved

hardik-k-shah reviewed Nov 24, 2020

View reviewed changes

lavacat dismissed stale reviews from nicot and debjanibnrj via 3ab1449 November 25, 2020 06:42

lavacat force-pushed the master branch from f9e490b to 3ab1449 Compare November 25, 2020 06:42

Support certs with separate Extended Key Usage

3e54769

Problem: Right now certs that specify extended key usage must set both clientAuth and serverAuth. Solution: Add settings to path additional server cert and use it when creating ssl server

lavacat force-pushed the master branch from 3ab1449 to 3e54769 Compare November 25, 2020 16:21

debjanibnrj approved these changes Nov 28, 2020

View reviewed changes

hardik-k-shah approved these changes Dec 1, 2020

View reviewed changes

hardik-k-shah merged commit 721e7e2 into opensearch-project:master Dec 1, 2020

vrozov added the enhancement New feature or request label Dec 1, 2020

hardik-k-shah mentioned this pull request Mar 9, 2022

[DOC] Missing documentation for security features opensearch-project/documentation-website#433

Closed

gaobinlong pushed a commit to gaobinlong/security that referenced this pull request Aug 30, 2023

Add THIRD PARTY file (opensearch-project#493)

b121a0f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support certs with separate Extended Key Usage #493

Support certs with separate Extended Key Usage #493

lavacat commented Jun 4, 2020

vrozov commented Jun 5, 2020

debjanibnrj Jun 5, 2020

debjanibnrj Jun 5, 2020

debjanibnrj Jun 5, 2020

debjanibnrj Jun 5, 2020

lavacat commented Aug 31, 2020

codecov bot commented Oct 12, 2020 •

edited

Loading

nicot left a comment

nicot Oct 12, 2020

debjanibnrj Oct 22, 2020

debjanibnrj Oct 22, 2020

lavacat commented Nov 20, 2020

debjanibnrj left a comment

debjanibnrj commented Nov 21, 2020

lavacat commented Nov 23, 2020

hardik-k-shah left a comment

hardik-k-shah Nov 24, 2020

lavacat Nov 25, 2020

lavacat commented Nov 25, 2020

-        settings.add(Setting.boolSetting(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED, SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_MIXED_KEY_USAGE_ENABLED_DEFAULT, Property.NodeScope, Property.Filtered));
+    if (!isMixedKeyUsageAllowed(settings)) {
+          settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMCERT_FILEPATH, Property.NodeScope, Property.Filtered));
+          settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMKEY_FILEPATH, Property.NodeScope, Property.Filtered));
+          settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMKEY_PASSWORD, Property.NodeScope, Property.Filtered));
+          settings.add(Setting.simpleString(SSLConfigConstants.OPENDISTRO_SECURITY_SSL_TRANSPORT_SERVER_PEMTRUSTEDCAS_FILEPATH, Property.NodeScope, Property.Filtered));
+        }


		} else if (rawPemCertFilePath != null \|\| extendedKeyUsageEnabled) {

Support certs with separate Extended Key Usage #493

Support certs with separate Extended Key Usage #493

Conversation

lavacat commented Jun 4, 2020

vrozov commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavacat commented Aug 31, 2020

codecov bot commented Oct 12, 2020 • edited Loading

Codecov Report

nicot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavacat commented Nov 20, 2020

debjanibnrj left a comment

Choose a reason for hiding this comment

debjanibnrj commented Nov 21, 2020

lavacat commented Nov 23, 2020

hardik-k-shah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavacat commented Nov 25, 2020

codecov bot commented Oct 12, 2020 •

edited

Loading