Refactored FIPS Checks into a Single Class (For Issue #34772) #36677

LuisAlvelaMendes · 2018-12-15T20:15:20Z

I have refactored all FIPS140 checks indicated in this issue: #34772

Into a single FIPSChecks class, I also tried to implement a custom FIPSContext and interface that operate much like Bootstrap contexts, except without worrying about sometimes not being enforced (as FIPS is always enforced) and without requiring any metadata.

I also modified the unit tests to fit the changes and tried to call this class and the check method (that runs all checks) in the appropriate place, as discussed:

A fitting place to call the checks is in createComponents of the Security plugin

Created a class FIPS Checks with a FIPS interface that allows you to perform a fips check per method, all checks are called at once,

elasticmachine · 2018-12-17T02:52:14Z

Pinging @elastic/es-security

jkakavas

Hi @LuisAlvelaMendes !

Thanks once again for taking this up :) I did a first pass of your PR and I've left several comments and suggestions about how I think this could be further refactored. Let me know what you think.

Please also make sure that you have configured your git installation correctly to set your name in the commits ( https://help.github.com/articles/setting-your-username-in-git/ )

Finally also make sure that you read through or contribution guide and follow all the steps mentioned there. At the absolute minimum, ensure that precommit check is passing.

Let me know if you have any questions

.gitignore

jkakavas · 2018-12-17T10:08:53Z

server/src/main/java/org/elasticsearch/bootstrap/FIPSContext.java

+/**
+ * Context that is passed to every bootstrap check to make decisions on.
+ */
+public class FIPSContext {


I don't think we actually need a FIPSContext class since we only encapsulate the settings here ( as opposed to i.e. a BootstrapContext that encapsulates settings and metadata. )

jkakavas · 2018-12-17T10:15:38Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPSInterface.java

+/**
+ * Encapsulates a FIPS check.
+ */
+public interface FIPSInterface {


I'm not entirely sure it is required to have the FIPS checks implement an interface, but if we go down this path ,I think that conceptually it would make more sense to have a FIPScheck interface that all the relevant checks ( settings, password hash etc. ) would then implement.

The purpose would be to return an adequate FIPSCheckResult while also maintaining some consistency with how bootstrap checks were written .. I'm not quite sure what you mean by having the checks themselves implement it.

I was thinking of an interface named FIPSCheck that then the checks like FipsLicencingCheck or FipsPasswordHashingAlgoCheck would implement. But I do think we don't necessarily need this either, there is no mandate to simulate bootstrap checks implementation. Same with FIPSCheckResult, it feels like an unnecessary abstraction. WDYT ?

jkakavas · 2018-12-17T10:19:40Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPSChecks.java

+
+    public FIPSCheckResult keystoreCheck(FIPSContext context) {
+
+        if (XPackSettings.FIPS_MODE_ENABLED.get(context.settings)) {


Depending on other changes/suggestions, we should probably conditionally apply the FipsChecks if FIPS_MODE_ENABLED already in Security.java so that we don't have to check the settings value each time.

Do you mean add an if clause like
if (XPackSettings.FIPS_MODE_ENABLED.get(context.settings)) {} surrounding it? I'm not sure what impact that would have

I meant that we can have the if clause once in Security.java and call check() on all the checks (or the one check() if we decide to implement FipsChecks in a simpler manner ) IF fips_mode is set

jkakavas · 2018-12-17T10:23:16Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPSChecks.java

+			return FIPSCheckResult.success();
+		}
+
+		else {


we should probably consolidate the error messages from the results so that we don't only present the first (from a seemingly arbitrary check order) error that was encountered to the user

How do you suggest we do it then?

I was thinking something similar to how we use addValidationError

jkakavas · 2018-12-17T10:45:41Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/FIPSInterface.java

@@ -0,0 +1,76 @@
+/*
+ * Licensed to Elasticsearch under one or more contributor


Should be under Elastic License

jkakavas · 2018-12-17T10:46:07Z

...curity/src/main/java/org/elasticsearch/xpack/security/org.elasticsearch.xpack.security_.ajsv

@@ -0,0 +1,16 @@
+<?xml version="1.0" encoding="UTF-8" standalone="no"?>


please remove unrelated files from the PR

jkakavas · 2018-12-17T10:48:19Z

...lugin/security/src/test/java/org/elasticsearch/xpack/security/JKSKeystoreFIPSCheckTests.java

 import org.elasticsearch.common.settings.Settings;
 import org.elasticsearch.test.ESTestCase;

-public class FIPS140JKSKeystoreBootstrapCheckTests extends ESTestCase {
+public class JKSKeystoreFIPSCheckTests extends ESTestCase {


If we have all checks in a single FipsChecks class, I think it would make more sense to refactor all the classes that test the old Bootstrap tests into a FipsChecksTests class and have methods that test all checks there

jkakavas · 2018-12-17T10:52:09Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

-                    FIPS140LicenseBootstrapCheck.ALLOWED_LICENSE_OPERATION_MODES.contains(license.operationMode()) == false) {
-                    throw new IllegalStateException("FIPS mode cannot be used with a [" + license.operationMode() +
+
+                License.OperationMode licenseOperationMode = FIPSChecksInstance.get().getLicenseOperationMode();


we should still get the operation mode from LicenseService.getLicense(state.metaData())

How, exactly? I thought we had established the license operations should be independant from any metadata.

Apologies, I'm not sure when this was established. It can be that my comment in #34772 (comment) was misleading in that regard. I think we can safely use License license = LicenseService.getLicense(state.metaData()); here, do you see any issue with this ?

jkakavas · 2018-12-17T10:53:15Z

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

@@ -499,6 +498,10 @@ protected Clock getClock() {
        securityActionFilter.set(new SecurityActionFilter(authcService.get(), authzService, getLicenseState(),
                requestInterceptors, threadPool, securityContext.get(), destructiveOperations));

+        FIPSContext context = new FIPSContext(settings);
+        FIPSChecksInstance.set(new FIPSChecks(getLicenseState().getOperationMode()));


Why do we need a FIPSChecksInstance ?

So we can get that instance with those values from the instruction FIPSChecksInstance.get().check(context, env); and call the checks

The question was about why is it necessary to wrap the FIPSChecks in a SetOnce and not do

FIPSContext context = new FIPSContext(settings); FIPSChecks checks = new FIPSChecks(getLicenseState().getOperationMode()) checks.check(context, env);

check Bootstraps check in the commented thing refactored tests and also calling FIPSChecks on Bootstrap instead BUILD Sucessfull, still have to correct metadata and tests Fips Checks Refactored Created a class FIPS Checks with a FIPS interface that allows you to perform a fips check per method, all checks are called at once, re-added deleted file

jkakavas · 2018-12-20T17:59:51Z

@LuisAlvelaMendes Thanks for the updates and apologies, I just noticed the new commits. I will take another look tomorrow, please feel free to ping me directly here every time there is a pending action on my side as otherwise your updates might get lost in the sea of Github notifications :)

jkakavas · 2019-01-08T11:47:53Z

Hi @LuisAlvelaMendes

I thought I'd ping you and see how this is going and if you're still interested in seeing this PR through. If there's anything you'd want to discuss, feel free to add a comment here.

e124f8a made me think that you had addressed some of the points we discussed above, hence my comment in #36677 (comment), but I then saw that this is the same as 7a4e6ff that you had committed earlier.

LuisAlvelaMendes · 2019-01-10T19:37:32Z

@jkakavas Sorry, but due to some personal things I will not be able conclude this pull request.

jkakavas · 2019-10-03T11:02:23Z

Closing this in favor of #47499

LuisAlvelaMendes added 6 commits December 2, 2018 18:52

Refactoring FIPS checks

b824b97

check Bootstraps check in the commented thing

0f7dc1a

refactored tests and also calling FIPSChecks on Bootstrap instead

5cc55d3

BUILD Sucessfull, still have to correct metadata and tests

51a126d

Fips Checks Refactored

7a4e6ff

Created a class FIPS Checks with a FIPS interface that allows you to perform a fips check per method, all checks are called at once,

re-added deleted file

aab85e4

jaymode requested a review from jkakavas December 17, 2018 02:51

jaymode added v7.0.0 >refactoring :Security/Security Security issues without another label v6.7.0 labels Dec 17, 2018

jkakavas requested changes Dec 17, 2018

View reviewed changes

LuisAlvelaMendes added 5 commits December 17, 2018 13:05

Merge remote-tracking branch 'elastic/master'

aa2d09c

Merge 'elastic/master', squashed commit

f8004cf

Merge remote-tracking branch 'origin/master'

2a29fea

removed some unecessary files, added license headers

a071e40

jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

danielmitterdorfer added v7.2.0 and removed v6.7.0 labels Feb 7, 2019

jakelandis added v7.3.0 and removed v7.2.0 labels Jun 17, 2019

jpountz added v7.4.0 and removed v7.3.0 labels Jul 5, 2019

colings86 added the v7.5.0 label Aug 30, 2019

colings86 removed the v7.4.0 label Aug 30, 2019

jkakavas mentioned this pull request Oct 3, 2019

Refactor FIPS BootstrapChecks to simple checks #47499

Merged

jkakavas closed this Oct 3, 2019

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored FIPS Checks into a Single Class (For Issue #34772) #36677

Refactored FIPS Checks into a Single Class (For Issue #34772) #36677

LuisAlvelaMendes commented Dec 15, 2018 •

edited

Loading

elasticmachine commented Dec 17, 2018

jkakavas left a comment

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

LuisAlvelaMendes Dec 17, 2018 •

edited

Loading

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

LuisAlvelaMendes Dec 17, 2018 •

edited

Loading

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

LuisAlvelaMendes Dec 17, 2018

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

LuisAlvelaMendes Dec 17, 2018

jkakavas Dec 17, 2018

jkakavas Dec 17, 2018

LuisAlvelaMendes Dec 17, 2018

jkakavas Dec 21, 2018

jkakavas commented Dec 20, 2018

jkakavas commented Jan 8, 2019

LuisAlvelaMendes commented Jan 10, 2019 •

edited

Loading

jkakavas commented Oct 3, 2019


		public FIPSCheckResult keystoreCheck(FIPSContext context) {

		if (XPackSettings.FIPS_MODE_ENABLED.get(context.settings)) {

		@@ -0,0 +1,76 @@
		/*
		* Licensed to Elasticsearch under one or more contributor

		@@ -0,0 +1,16 @@
		<?xml version="1.0" encoding="UTF-8" standalone="no"?>

Refactored FIPS Checks into a Single Class (For Issue #34772) #36677

Refactored FIPS Checks into a Single Class (For Issue #34772) #36677

Conversation

LuisAlvelaMendes commented Dec 15, 2018 • edited Loading

elasticmachine commented Dec 17, 2018

jkakavas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuisAlvelaMendes Dec 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LuisAlvelaMendes Dec 17, 2018 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkakavas commented Dec 20, 2018

jkakavas commented Jan 8, 2019

LuisAlvelaMendes commented Jan 10, 2019 • edited Loading

jkakavas commented Oct 3, 2019

LuisAlvelaMendes commented Dec 15, 2018 •

edited

Loading

LuisAlvelaMendes Dec 17, 2018 •

edited

Loading

LuisAlvelaMendes Dec 17, 2018 •

edited

Loading

LuisAlvelaMendes commented Jan 10, 2019 •

edited

Loading