Enabling Full FTR, Integration, and Unit tests to the FIPS Test Pipeline #192632

kc13greiner · 2024-09-11T19:20:36Z

Summary

Closes #192233

Just in time for Thanksgiving - a full buffet of FIPS testing fixes

Usage of non-compliant algorithms manifest as runtime errors, so it is imperative that we attempt to run all tests possible with Kibana in FIPS mode. However, several overrides are needed to run Kibana in FIPS mode, resulting in setup that make it impossible to run.

In this PR

Enable Unit tests for FIPS pipeline
Enable Integration Tests for FIPS pipeline
Enable Full FTR suite for FIPS pipeline (smoke test had originally run a subset)
Skip tests that break with overrides
Fix/change tests to work in FIPS mode to maximize coverage
Examine necessity of MD5 when installing from source (TBD based Ops PR feed back, see self review below)
Remove md5 from es_file_client options

Latest Successful FIPS Test Run

https://buildkite.com/elastic/kibana-fips/builds/268

.buildkite/pipelines/fips.yml

Co-authored-by: Brad White <Ikuni17@users.noreply.github.com>

kc13greiner · 2024-10-02T12:02:48Z

Green CI: https://buildkite.com/elastic/kibana-fips/builds/194

jbudz · 2024-10-02T12:36:07Z

.buildkite/pipelines/fips.yml

    depends_on: build
    timeout_in_minutes: 10
    env:
      FTR_CONFIGS_SCRIPT: '.buildkite/scripts/steps/test/ftr_configs.sh'
      FTR_EXTRA_ARGS: '$FTR_EXTRA_ARGS'
-      LIMIT_CONFIG_TYPE: 'functional'
+      JEST_UNIT_SCRIPT: '.buildkite/scripts/steps/test/jest.sh'


It doesn't look like these will run with FIPS enabled.

kibana/.buildkite/scripts/common/env.sh

Lines 143 to 146 in 833a267

if [[ -f "$KIBANA_DIR/config/node.options" ]]; then

echo -e '\n--enable-fips' >>"$KIBANA_DIR/config/node.options"

echo "--openssl-config=$HOME/nodejs.cnf" >>"$KIBANA_DIR/config/node.options"

fi

is configuring distributions

NODE_OPTIONS for jest at

kibana/.buildkite/scripts/steps/test/jest_parallel.sh

Line 63 in 833a267

cmd="NODE_OPTIONS=\"--max-old-space-size=12288 --trace-warnings\" node ./scripts/jest --config=\"$config\" $parallelism --coverage=false --passWithNoTests"

Ok I think I have it setup properly now!

CI in progress: https://buildkite.com/elastic/kibana-fips/builds/197#01924db0-7734-405c-bc8c-c26facdcbb60

nreese

kibana-presentation changes LGTM
code review only

elasticmachine · 2024-10-03T14:48:50Z

Pinging @elastic/fleet (Team:Fleet)

azasypkin

LGTM from the security perspecitive, just a few notes and suggestions!

.buildkite/scripts/steps/test/jest_parallel.sh

azasypkin · 2024-11-12T11:14:49Z

packages/core/security/core-security-server-internal/src/security_service.test.ts

-  let coreContext: ReturnType<typeof mockCoreContext.create>;
-  let service: SecurityService;
+describe('SecurityService', function () {
+  if (getFips() !== 1) {


question: can we add a code comment here explaining why we're doing this? Do all these tests fail in FIPS? Even #registerSecurityDelegate?

Ah you are correct, I meant to modify this test and I will add the config for both cases, these should pass regardless of FIPS (except the FIPS check itself)

azasypkin · 2024-11-12T11:18:15Z

packages/core/test-helpers/core-test-helpers-kbn-server/src/create_root.ts

@@ -75,6 +77,13 @@ export function createRootWithSettings(
    pkg.version = customKibanaVersion;
  }

+  let oss = true;


Tip
Also a comment here would be helpful.

azasypkin · 2024-11-12T11:25:55Z

packages/core/test-helpers/core-test-helpers-kbn-server/src/create_root.ts

+  if (getFips() === 1) {
+    // Set license to 'trial' if Node is running in FIPS mode
+    license = 'trial';
+  }


Warning
I think it’s fine to default to trial instead of basic when we’re in FIPS mode and the license isn’t explicitly specified in settings. However, I’m a bit concerned about cases where some test suites request a specific license to verify behavior unique to that license, but we override it with trial.

Do we have many test suites that specify a license different from trial? Would it be an option to toggle the correct license at the point where it’s specified? That way, if certain tests don’t make sense to run with trial, we can explicitly skip them there.

Similar to other responses, I think it is important to run as many tests as possible for maximum lines run in FIPS. I only skip if the overrides would break the tests. If a basic license test "runs" successfully with a trial license, I believe we should run it.

I tend to agree with Oleg's concern that with FIPs mode on there is a possibility that we may subtly change test coverage by overriding explicit settings that could potentially surprise test authors:

Test authors may specify basic but actually it is gonna run both basic and trial. Given it's both today I tend to think this is OK (we are not losing coverage), but it could be surprising.

One alternative, instead of implicitly overriding settings in FIPs we can overtly error out with something like:

// If explicitly set, we throw something like this: if (getFips() === 1 && (cliArgs.oss === true || settings.es?.license !== 'trial')) { throw new Error(` explicitly setting "cliArgs.oss: true" or "settings.es.license" to something other than "trial" is not compatible in FIPs mode. Ensure that this test is skipped in FIPs mode in the following way... `) }

Doing a search for license: 'basic' or oss: true in integration_tests does not turn up many results so perhaps skipping this set of tests in FIPs for now is an OK trade-off?

Weakly held opinion, WDY both T?

I think it is important to run as many tests as possible for maximum lines run in FIPS

I agree!

If a basic license test "runs" successfully with a trial license, I believe we should run it.

The use case I had in mind was something like this: "test that this functionality is available and works correctly when the license is basic, potentially deviating from behavior in trial, so that if I mess up the license checks in my code, breaking my feature for basic, this test will catch it". If we silently switch the license to trial, it won’t test what the test author intended. Admittedly and as JL mentioned, it’s not a huge concern since, even though the FIPS test might mistakenly pass, the original non-FIPS test would still fail.

Doing a search for license: 'basic' or oss: true in integration_tests does not turn up many results so perhaps skipping this set of tests in FIPs for now is an OK trade-off?

If it doesn’t involve a lot of changes, I think it’s a reasonable trade-off 👍 Otherwise, feel free to ignore my concern, as I’m more in favor of running potentially misleading FIPS-related tests than not running them.

++ Ok I can get on board with something like theat. There will be many more FIPS related PRs (not for this initial release, but for FRH and when we have to target 140-3). I will definitely take these recommendations into consideration as well as trying to expand the configuration options for Integration Tests to make allow for just single plugins to be enabled (vs. OSS=false)

azasypkin · 2024-11-12T11:32:52Z

packages/core/test-helpers/core-test-helpers-kbn-server/src/create_root.ts

@@ -292,6 +307,7 @@ export function createTestServers({
          hosts: es.getHostUrls(),
          username: kibanaServerTestUser.username,
          password: kibanaServerTestUser.password,
+          ...(getFips() ? kbnSettings.elasticsearch : {}),


question: why do we need this?

in is_scripting_enabled.test.ts the es settings were not being copied over since I had overridden the license to be trial, so I added this just for FIPS mode so we could have the es settings propagated and the trial license

Got it, thanks for clarifying, I think it'd worth having a comment then (my favorite comment is about comments 😆)

azasypkin · 2024-11-12T12:05:27Z

src/core/server/integration_tests/config/check_dynamic_config.test.ts

-      },
+import { getFips } from 'crypto';
+
+if (getFips() === 0) {


question: Here and in all similar places: is this a temporary measure? If so, are we tracking all the tests that were disabled somewhere to ensure we remember to re-enable them at some point? If it’s not temporary, could we add a comment explaining why we’re doing this and what areas are affected?

I cant say it is temporary; I had to disable oss so xpack plugins would register and allow xpack.security.... configs to be allowed.

I think we would have to change the jest integration tests to allow me to add less broad-sweeping overrides.

I am not keeping track per se, but the search to find them is pretty straightforward.

the test in the else (FIPS portion) has the explanation (below), but Im happy to add a comment if you think that is clearer:
'is running in FIPS mode, skipping tests since they fail due to FIPS overrides'

I see, I was just wondering if there was a clear, describable reason why certain tests fail in FIPS, like, for example, “dynamic config relies on MD5 for … and is hence not compatible with FIPS”. But if that’s not the case, or if it’s too much work to pinpoint and describe the reason for every such exception, feel free to keep things as they are, it's not worth it.

azasypkin · 2024-11-12T12:07:36Z

src/core/server/integration_tests/node/migrator.test.ts

@@ -42,7 +44,7 @@ describe('migrator-only node', () => {
          '--no-optimizer',
          '--no-base-path',
          '--no-watch',
-          '--oss',
+          isFipsEnabled ? '--xpack.security.experimental.fipsMode.enabled=true' : '--oss',


I’m surprised we’re still testing OSS stuff (maybe we could save some CI costs… just saying) 🙈

🤷‍♂️ They were running OSS when I found them!

Yeah, it was more of a question for the Core Team 🙂

As I understand it, the --oss flag actually helps these integration tests start up a bit faster since we are excluding loading any xpack plug-ins.

azasypkin · 2024-11-12T12:09:00Z

src/plugins/files/server/blob_storage_service/adapters/es/integration_tests/es.test.ts

    manageES = await startES();
    manageKbn = await startKibana();
-    esClient = manageKbn.coreStart.elasticsearch.client.asInternalUser;
+    esClient = manageKbn.coreStart.elasticsearch.createClient('es.test').asInternalUser;


question: I'm curious what this change does?

This is changing the test to use a dedicated ES Client, instead of a shared one. I added it to squash some flakiness I was experiencing

That’s weird, as I can’t think of any plausible reason why there would be an issue with the shared ES client, but I’ll defer to the code owners. Maybe they have some idea.

@azasypkin this test was built to run without xpack plugins, and it just so happened that the shared ES client wasn't used by any other non-xpack plugin during the lifespan of the test.

When running this in FIPS mode, we have to include x-pack, and there are a number of x-pack plugins which use the shared client during their initialization phase. For example, the spaces plugin asserts that the default space exists. These additional usages of the shared client were causing some assertions in these tests to fail. By using a dedicated ES client, it removes the noise generated by other running plugins.

Oh, I see that we set up spies for the ES client methods. Got it, thanks for adding more context @legrego, makes sense now.

azasypkin · 2024-11-12T12:16:11Z

...s/files/server/file_client/stream_transforms/file_hash_transform/file_hash_transform.test.ts

-      const fileHash = createFileHashTransform(algorithm);
-      await file.uploadContent(fileContent, undefined, {
-        transforms: [fileHash],
+  if (getFips() !== 1) {


question: can we have something like this to not duplicate the test body?

it.each([ ['sha1', 'a94a8fe5ccb19ba61c4c0873d391e987982fbbd3'], ['sha256', '9f86d081884c7d659a2feaa0c55ad015a3bf4f1b2b0b822cd15d6c15b0f00a08'], [ 'sha512', 'ee26b0dd4af7e749aa1a8ee3c10ae9923f618980772e473f8819a5d4940e0db27ac185f8a0e1d5f84f88bc887fd67b143732c304cc5fa9ad8e6f57f50028a8ff', ], ...(getFips() !== 1 ? [['md5', '098f6bcd4621d373cade4e832627b4f6']] : []), ] as Array<[SupportedFileHashAlgorithm, string]>)(

ahh that is much cleaner - fixing it!

azasypkin · 2024-11-12T12:18:14Z

test/functional/apps/dashboard/group5/dashboard_panel_listing.ts

@@ -17,7 +17,8 @@ export default function ({ getService, getPageObjects }: FtrProviderContext) {
  const kibanaServer = getService('kibanaServer');
  const dashboardAddPanel = getService('dashboardAddPanel');

-  describe('dashboard panel listing', () => {
+  describe('dashboard panel listing', function () {
+    this.tags('skipFIPS');


Tip
Here and for other skips - it would be helpful to have a comment with a justification, and to track this somewhere if we intend to unskip them eventually.

I think a follow up PR to 1) add comments to all skipped tests 2) try to fix the FTR FIPS Role overrides would be worthwhile.

Sounds good to me. As I mentioned in #192632 (comment), it would only be worth adding comments if we can clearly articulate the problem to help future readers. Otherwise, it’s fine to keep it as is.

src/plugins/files/server/file_client/create_es_file_client.ts

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

juliaElastic

LGTM

Dosant

sharedux lgtm, sorry for the delay

elasticmachine · 2024-11-19T11:57:20Z

💚 Build Succeeded

Buildkite Build
Commit: 95969a9
Kibana UBI FIPS Image: docker.elastic.co/kibana-ci/kibana-ubi-fips:9.0.0-SNAPSHOT-95969a985fe329cdbce9cd721b2a03e4fa174366
Storybooks Preview

Metrics [docs]

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`@kbn/es`	4	2	-2

Total ESLint disabled count

id	before	after	diff
`@kbn/es`	4	2	-2

History

💚 Build #251008 succeeded a8be638
💛 Build #249945 was flaky e4b6636
💚 Build #249643 succeeded 08561e4
💔 Build #249619 failed 7454e8a
💔 Build #249341 failed f6829f6

cc @kc13greiner

jloleysens

Thanks for explaining and addressing my feedback. Great work man!

kibanamachine · 2024-11-19T16:32:47Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11917488539

kibanamachine · 2024-11-19T16:32:48Z

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/11917488475

kibanamachine · 2024-11-19T16:37:24Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 192632

Questions ?

Please refer to the Backport tool documentation

kibanamachine · 2024-11-19T16:37:24Z

💔 All backports failed

Status	Branch	Result
❌	8.x	Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 192632

Questions ?

Please refer to the Backport tool documentation

kc13greiner · 2024-11-19T17:15:53Z

💚 All backports created successfully

Status	Branch	Result
✅	8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

…ine (elastic#192632) ## Summary Closes elastic#192233 Just in time for Thanksgiving - a full buffet of FIPS testing fixes Usage of non-compliant algorithms manifest as runtime errors, so it is imperative that we attempt to run all tests possible with Kibana in FIPS mode. However, several overrides are needed to run Kibana in FIPS mode, resulting in setup that make it impossible to run. ## In this PR - Enable Unit tests for FIPS pipeline - Enable Integration Tests for FIPS pipeline - Enable Full FTR suite for FIPS pipeline (smoke test had originally run a subset) - Skip tests that break with overrides - Fix/change tests to work in FIPS mode to maximize coverage - Examine necessity of MD5 when installing from source (TBD based Ops PR feed back, see self review below) - Remove md5 from es_file_client options ## Latest Successful FIPS Test Run https://buildkite.com/elastic/kibana-fips/builds/268 --------- Co-authored-by: Brad White <Ikuni17@users.noreply.github.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co> (cherry picked from commit ac0b0b4) # Conflicts: # packages/kbn-es/src/install/install_source.ts # src/core/server/integration_tests/config/check_dynamic_config.test.ts # src/core/server/integration_tests/core_app/default_route_provider_config.test.ts

… Pipeline (#192632) (#200780) # Backport This will backport the following commits from `main` to `8.x`: - [Enabling Full FTR, Integration, and Unit tests to the FIPS Test Pipeline (#192632)](#192632)  ### Questions ? Please refer to the [Backport tool documentation](https://github.com/sqren/backport)

…ine (elastic#192632) ## Summary Closes elastic#192233 Just in time for Thanksgiving - a full buffet of FIPS testing fixes Usage of non-compliant algorithms manifest as runtime errors, so it is imperative that we attempt to run all tests possible with Kibana in FIPS mode. However, several overrides are needed to run Kibana in FIPS mode, resulting in setup that make it impossible to run. ## In this PR - Enable Unit tests for FIPS pipeline - Enable Integration Tests for FIPS pipeline - Enable Full FTR suite for FIPS pipeline (smoke test had originally run a subset) - Skip tests that break with overrides - Fix/change tests to work in FIPS mode to maximize coverage - Examine necessity of MD5 when installing from source (TBD based Ops PR feed back, see self review below) - Remove md5 from es_file_client options ## Latest Successful FIPS Test Run https://buildkite.com/elastic/kibana-fips/builds/268 --------- Co-authored-by: Brad White <Ikuni17@users.noreply.github.com> Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com> Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com> Co-authored-by: Larry Gregory <larry.gregory@elastic.co>

Replacing smoke test step with FTR pick order step

77697db

kc13greiner added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v8.16.0 labels Sep 11, 2024

Adding unit and integration

35fa148

Ikuni17 reviewed Sep 11, 2024

View reviewed changes

.buildkite/pipelines/fips.yml Outdated Show resolved Hide resolved

Update .buildkite/pipelines/fips.yml

ec83314

Co-authored-by: Brad White <Ikuni17@users.noreply.github.com>

kc13greiner self-assigned this Sep 11, 2024

kc13greiner added 6 commits October 1, 2024 09:49

Removing extra args env var

c54e919

removing config limitations

07b21c0

Merge branch 'main' into feature/Run_Full_FIPS_FTR

f43f7d5

adding extra functions back

4fe0825

Skipping new failing tests

ddab617

Merge branch 'main' into feature/Run_Full_FIPS_FTR

4fe127d

kc13greiner removed the Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! label Oct 2, 2024

kc13greiner marked this pull request as ready for review October 2, 2024 12:02

kc13greiner requested review from a team as code owners October 2, 2024 12:02

Update fips.yml to include jest

d33488d

jbudz reviewed Oct 2, 2024

View reviewed changes

attempting to add fips node options cmd line args

6daa6c8

nreese approved these changes Oct 2, 2024

View reviewed changes

kc13greiner added 2 commits October 2, 2024 10:16

Correcting node options to the front

17ba872

trying diff hash alg

83766b6

kc13greiner requested a review from a team as a code owner October 3, 2024 14:48

botelastic bot added the Team:Fleet Team label for Observability Data Collection Fleet team label Oct 3, 2024

kc13greiner changed the title ~~Changing FIPS Smoke Test Pipeline to be Full FTR~~ [Work in Progress] Changing FIPS Smoke Test Pipeline to be Full FTR Oct 3, 2024

azasypkin approved these changes Nov 12, 2024

View reviewed changes

kc13greiner commented Nov 13, 2024

View reviewed changes

src/plugins/files/server/file_client/create_es_file_client.ts Show resolved Hide resolved

kc13greiner and others added 4 commits November 13, 2024 13:47

PR feedback

c362dd7

Update .buildkite/scripts/steps/test/jest_parallel.sh

abff161

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>

Merge branch 'main' into feature/Run_Full_FIPS_FTR

7a8ce60

[CI] Auto-commit changed files from 'node scripts/notice'

a8be638

kc13greiner added v8.17.0 and removed v8.16.0 labels Nov 13, 2024

juliaElastic approved these changes Nov 14, 2024

View reviewed changes

Ikuni17 self-requested a review November 15, 2024 00:22

Ikuni17 approved these changes Nov 15, 2024

View reviewed changes

Dosant approved these changes Nov 19, 2024

View reviewed changes

Merge branch 'main' into feature/Run_Full_FIPS_FTR

95969a9

jloleysens approved these changes Nov 19, 2024

View reviewed changes

kc13greiner merged commit ac0b0b4 into elastic:main Nov 19, 2024
31 checks passed

kibanamachine added the v9.0.0 label Nov 19, 2024

kibanamachine mentioned this pull request Nov 19, 2024

[LLM tasks] Add product documentation retrieval task #194379

Merged

kc13greiner added backport:version Backport to applied version labels and removed backport:skip This commit does not require backporting labels Nov 19, 2024

kc13greiner mentioned this pull request Nov 19, 2024

[8.x] Enabling Full FTR, Integration, and Unit tests to the FIPS Test Pipeline (#192632) #200780

Merged

	if [[ -f "$KIBANA_DIR/config/node.options" ]]; then
	echo -e '\n--enable-fips' >>"$KIBANA_DIR/config/node.options"
	echo "--openssl-config=$HOME/nodejs.cnf" >>"$KIBANA_DIR/config/node.options"
	fi

Enabling Full FTR, Integration, and Unit tests to the FIPS Test Pipeline #192632

Enabling Full FTR, Integration, and Unit tests to the FIPS Test Pipeline #192632

Conversation

kc13greiner commented Sep 11, 2024 • edited by kibanamachine Loading

Summary

In this PR

Latest Successful FIPS Test Run

kc13greiner commented Oct 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nreese left a comment

Choose a reason for hiding this comment

elasticmachine commented Oct 3, 2024

azasypkin 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

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

kc13greiner Nov 13, 2024 • 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

juliaElastic left a comment

Choose a reason for hiding this comment

Dosant left a comment

Choose a reason for hiding this comment

elasticmachine commented Nov 19, 2024 • edited Loading

💚 Build Succeeded

Metrics [docs]

ESLint disabled line counts

Total ESLint disabled count

History

jloleysens left a comment

Choose a reason for hiding this comment

kibanamachine commented Nov 19, 2024

kibanamachine commented Nov 19, 2024

kibanamachine commented Nov 19, 2024

💔 All backports failed

Manual backport

Questions ?

kibanamachine commented Nov 19, 2024

💔 All backports failed

Manual backport

Questions ?

kc13greiner commented Nov 19, 2024

💚 All backports created successfully

Questions ?

kc13greiner commented Sep 11, 2024 •

edited by kibanamachine

Loading

kc13greiner Nov 13, 2024 •

edited

Loading

elasticmachine commented Nov 19, 2024 •

edited

Loading