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.

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

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

Merged
merged 59 commits into from
Nov 19, 2024
Merged
Show file tree
Hide file tree
Changes from 58 commits
Commits
Show all changes
59 commits
Select commit Hold shift + click to select a range
77697db
Replacing smoke test step with FTR pick order step
kc13greiner Sep 11, 2024
35fa148
Adding unit and integration
kc13greiner Sep 11, 2024
ec83314
Update .buildkite/pipelines/fips.yml
kc13greiner Sep 11, 2024
c54e919
Removing extra args env var
kc13greiner Oct 1, 2024
07b21c0
removing config limitations
kc13greiner Oct 1, 2024
f43f7d5
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Oct 1, 2024
4fe0825
adding extra functions back
kc13greiner Oct 1, 2024
ddab617
Skipping new failing tests
kc13greiner Oct 2, 2024
4fe127d
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Oct 2, 2024
d33488d
Update fips.yml to include jest
kc13greiner Oct 2, 2024
6daa6c8
attempting to add fips node options cmd line args
kc13greiner Oct 2, 2024
17ba872
Correcting node options to the front
kc13greiner Oct 2, 2024
83766b6
trying diff hash alg
kc13greiner Oct 3, 2024
b2b354b
testing change to signing/digest algorithm, skipping known issues
kc13greiner Oct 3, 2024
dfd9154
Switching hash back and changing curve
kc13greiner Oct 4, 2024
c17e569
trying PEM instead of DER
kc13greiner Oct 4, 2024
001651c
Trying different cipher on pk
kc13greiner Oct 4, 2024
67d8092
cipher threw unsupported envelope, try w/o cipher for now
kc13greiner Oct 4, 2024
e0f7ebc
switching back to DER with the cipher
kc13greiner Oct 4, 2024
8aaf935
adding initialization to ensure its the proper len
kc13greiner Oct 4, 2024
1f93c2f
already imported
kc13greiner Oct 4, 2024
7feec4f
Trying longer rsa
kc13greiner Oct 4, 2024
4cc907e
Attempting to log ciphers to see what is available
kc13greiner Oct 5, 2024
af02d5b
comma
kc13greiner Oct 5, 2024
6a9d171
Clearing up other errors to focus on signing
kc13greiner Oct 6, 2024
0046e88
Flipping condition
kc13greiner Oct 6, 2024
bf5113f
removing log
kc13greiner Oct 6, 2024
7158794
Testing original
kc13greiner Oct 7, 2024
20d4b7b
Commenting out scryptParams
kc13greiner Oct 7, 2024
412a745
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Oct 7, 2024
a863849
Temporary hotfixes to test integration tests in pipeline
kc13greiner Oct 10, 2024
aed8025
import disappeared
kc13greiner Oct 10, 2024
c796660
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Oct 10, 2024
52b5f3c
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Oct 11, 2024
f1504ac
More override tests
kc13greiner Oct 15, 2024
2b5a712
Moving fips config
kc13greiner Oct 15, 2024
29a79db
Skipping PKCS12 and MD5 fs tests
kc13greiner Oct 16, 2024
ae4293e
Fix for unused config issue
kc13greiner Oct 16, 2024
36da348
Dont register
kc13greiner Oct 17, 2024
3e078fa
disable oss in FIPS mode, revert security service, clean up other ove…
kc13greiner Oct 25, 2024
e620cf2
clean up
kc13greiner Oct 25, 2024
608b2f9
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Oct 25, 2024
7d840a4
adding back trial license since some calls don't use the servers factory
kc13greiner Oct 25, 2024
be62e5e
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Oct 30, 2024
2ce8759
Skipping data archive test
kc13greiner Nov 4, 2024
f290b6e
Changing some fips test overrides and skipping some tests
kc13greiner Nov 5, 2024
b2a38ab
Last skips and fixing trial/es overrides for fips
kc13greiner Nov 6, 2024
d444894
skipping serverless test due to overrieds
kc13greiner Nov 7, 2024
f6829f6
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Nov 7, 2024
cf928c8
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Nov 7, 2024
84fa1aa
ensuring there is only 1 oss on the cli
kc13greiner Nov 8, 2024
7454e8a
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Nov 8, 2024
08561e4
Fixing test license type
kc13greiner Nov 8, 2024
e4b6636
Adding comment to clarify test license usage
kc13greiner Nov 11, 2024
c362dd7
PR feedback
kc13greiner Nov 13, 2024
abff161
Update .buildkite/scripts/steps/test/jest_parallel.sh
kc13greiner Nov 13, 2024
7a8ce60
Merge branch 'main' into feature/Run_Full_FIPS_FTR
kc13greiner Nov 13, 2024
a8be638
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Nov 13, 2024
95969a9
Merge branch 'main' into feature/Run_Full_FIPS_FTR
legrego Nov 19, 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
7 changes: 4 additions & 3 deletions .buildkite/pipelines/fips.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,14 +40,15 @@ steps:
machineType: n2-standard-2
preemptible: true

- command: .buildkite/scripts/steps/fips/smoke_test.sh
label: 'Pick Smoke Test Group Run Order'
- command: .buildkite/scripts/steps/test/pick_test_group_run_order.sh
label: 'Pick Test Group Run Order'
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'
Copy link
Member

Choose a reason for hiding this comment

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

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

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I think I have it setup properly now!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

JEST_INTEGRATION_SCRIPT: '.buildkite/scripts/steps/test/jest_integration.sh'
retry:
automatic:
- exit_status: '*'
Expand Down
24 changes: 0 additions & 24 deletions .buildkite/scripts/steps/fips/smoke_test.sh

This file was deleted.

9 changes: 8 additions & 1 deletion .buildkite/scripts/steps/test/jest_parallel.sh
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,14 @@ while read -r config; do
# --trace-warnings to debug
# Node.js process-warning detected:
# Warning: Closing file descriptor 24 on garbage collection
cmd="NODE_OPTIONS=\"--max-old-space-size=12288 --trace-warnings\" node ./scripts/jest --config=\"$config\" $parallelism --coverage=false --passWithNoTests"
cmd="NODE_OPTIONS=\"--max-old-space-size=12288 --trace-warnings"

if [ "${KBN_ENABLE_FIPS:-}" == "true" ]; then
cmd=$cmd" --enable-fips --openssl-config=$HOME/nodejs.cnf"
fi

cmd=$cmd"\" node ./scripts/jest --config=\"$config\" $parallelism --coverage=false --passWithNoTests"

echo "actual full command is:"
echo "$cmd"
echo ""
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,32 @@ import { loggerMock, MockedLogger } from '@kbn/logging-mocks';
import { mockCoreContext } from '@kbn/core-base-server-mocks';
import type { CoreSecurityDelegateContract } from '@kbn/core-security-server';
import { SecurityService } from './security_service';
import { configServiceMock } from '@kbn/config-mocks';
import { getFips } from 'crypto';

const createStubInternalContract = (): CoreSecurityDelegateContract => {
return Symbol('stubContract') as unknown as CoreSecurityDelegateContract;
};

describe('SecurityService', () => {
describe('SecurityService', function () {
let coreContext: ReturnType<typeof mockCoreContext.create>;
let configService: ReturnType<typeof configServiceMock.create>;
let service: SecurityService;

beforeEach(() => {
coreContext = mockCoreContext.create();
const mockConfig = {
xpack: {
security: {
experimental: {
fipsMode: {
enabled: !!getFips(),
},
},
},
},
};
configService = configServiceMock.create({ getConfig$: mockConfig });
coreContext = mockCoreContext.create({ configService });
service = new SecurityService(coreContext);

convertSecurityApiMock.mockReset();
Expand All @@ -51,8 +66,11 @@ describe('SecurityService', () => {
describe('#isEnabled', () => {
it('should return boolean', () => {
const { fips } = service.setup();

expect(fips.isEnabled()).toBe(false);
if (getFips() === 0) {
expect(fips.isEnabled()).toBe(false);
} else {
expect(fips.isEnabled()).toBe(true);
}
});
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,5 +22,6 @@
"@kbn/core-base-server-mocks",
"@kbn/config",
"@kbn/core-logging-server-mocks",
"@kbn/config-mocks",
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ import loadJsonFile from 'load-json-file';
import { defaultsDeep } from 'lodash';
import { BehaviorSubject } from 'rxjs';
import supertest from 'supertest';
import { set } from '@kbn/safer-lodash-set';

import { getPackages } from '@kbn/repo-packages';
import { ToolingLog } from '@kbn/tooling-log';
import { REPO_ROOT } from '@kbn/repo-info';
import { getFips } from 'crypto';
import {
createTestEsCluster,
CreateTestEsClusterOptions,
Expand Down Expand Up @@ -75,6 +77,17 @@ export function createRootWithSettings(
pkg.version = customKibanaVersion;
}

/*
* Most of these integration tests expect OSS to default to true, but FIPS
* requires the security plugin to be enabled
*/
let oss = true;
Copy link
Member

Choose a reason for hiding this comment

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

Tip

Also a comment here would be helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++ will do

if (getFips() === 1) {
set(settings, 'xpack.security.experimental.fipsMode.enabled', true);
oss = false;
delete cliArgs.oss;
}

const env = Env.createDefault(
REPO_ROOT,
{
Expand All @@ -84,10 +97,10 @@ export function createRootWithSettings(
watch: false,
basePath: false,
runExamples: false,
oss: true,
disableOptimizer: true,
cache: true,
dist: false,
oss,
...cliArgs,
},
repoPackages: getPackages(REPO_ROOT),
Expand Down Expand Up @@ -255,7 +268,13 @@ export function createTestServers({
if (!adjustTimeout) {
throw new Error('adjustTimeout is required in order to avoid flaky tests');
}
const license = settings.es?.license ?? 'basic';
let license = settings.es?.license ?? 'basic';

if (getFips() === 1) {
// Set license to 'trial' if Node is running in FIPS mode
license = 'trial';
}
Comment on lines +273 to +276
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

++ 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)


const usersToBeAdded = settings.users ?? [];
if (usersToBeAdded.length > 0) {
if (license !== 'trial') {
Expand Down Expand Up @@ -292,6 +311,7 @@ export function createTestServers({
hosts: es.getHostUrls(),
username: kibanaServerTestUser.username,
password: kibanaServerTestUser.password,
...(getFips() ? kbnSettings.elasticsearch : {}),
Copy link
Member

Choose a reason for hiding this comment

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

question: why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

};
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
"@kbn/repo-packages",
"@kbn/es",
"@kbn/dev-utils",
"@kbn/safer-lodash-set",
],
"exclude": [
"target/**/*",
Expand Down
4 changes: 2 additions & 2 deletions packages/kbn-es/src/install/install_source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -84,15 +84,15 @@ async function sourceInfo(cwd: string, license: string, log: ToolingLog = defaul
log.info('on %s at %s', chalk.bold(branch), chalk.bold(sha));
log.info('%s locally modified file(s)', chalk.bold(status.modified.length));

const etag = crypto.createHash('md5').update(branch); // eslint-disable-line @kbn/eslint/no_unsafe_hash
const etag = crypto.createHash('sha256').update(branch);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Self Review: I think these can be changed (yarn es source worked) and it would be nice to remove all md5 possible. Thoughts @jbudz @Ikuni17 .

Copy link
Contributor

Choose a reason for hiding this comment

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

lgtm

etag.update(sha);

// for changed files, use last modified times in hash calculation
status.files.forEach((file) => {
etag.update(fs.statSync(path.join(cwd, file.path)).mtime.toString());
});

const cwdHash = crypto.createHash('md5').update(cwd).digest('hex').substr(0, 8); // eslint-disable-line @kbn/eslint/no_unsafe_hash
const cwdHash = crypto.createHash('sha256').update(cwd).digest('hex').substr(0, 8);

const basename = `${branch}-${task}-${cwdHash}`;
const filename = `${basename}.${ext}`;
Expand Down
6 changes: 5 additions & 1 deletion packages/kbn-test/src/es/test_es_cluster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import type { ToolingLog } from '@kbn/tooling-log';
import { REPO_ROOT } from '@kbn/repo-info';
import type { ArtifactLicense } from '@kbn/es';
import type { ServerlessOptions } from '@kbn/es/src/utils';
import { getFips } from 'crypto';
import { CI_PARALLEL_PROCESS_PREFIX } from '../ci_parallel_process_prefix';
import { esTestConfig } from './es_test_config';

Expand Down Expand Up @@ -200,12 +201,15 @@ export function createTestEsCluster<

const esArgs = assignArgs(defaultEsArgs, customEsArgs);

// Use 'trial' license if FIPS mode is enabled, otherwise use the provided license or default to 'basic'
const testLicense: ArtifactLicense = getFips() === 1 ? 'trial' : license ? license : 'basic';
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment explaining the logic for licence pick in different cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added here e4b6636

Copy link
Member

Choose a reason for hiding this comment

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

Same comment as for packages/core/test-helpers/core-test-helpers-kbn-server/src/create_root.ts - I’m a bit concerned that we’re ignoring the license requested by the test suite. For example, if a test is meant to verify that certain functionality is available in basic, running it in FIPS would be pointless. Would it be possible to override this at the consumer/test-suite level and maybe even exclude suites that aren't relevant to trial, and hence FIPS?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more tests we can force to run = more assurance that we don't hit an OpenSSL error.

For example, if a test is meant to verify that certain functionality is available in basic, running it in FIPS would be pointless.

Many tests default to basic and run fine when overridden with trial, if they fail due to configuration, I skip them for FIPS one way or another (depending on unit/integration/ftr). A test that is asserting something specific to a basic license in this case would fail and be skipped.

Would it be possible to override this at the consumer/test-suite level and maybe even exclude suites that aren't relevant to trial, and hence FIPS?

There are so many tests, IMO, it would be very messy to have a FIPS check in every suite - and out of all the Unit/Integration there were so few that actually required a skip (usually due to OSS=false, not the license!)

To summarize, for the FIPS pipeline we just want to make sure we maximize our coverage to prevent FIPS related errors. Everyday CI is to assert working functionality.


const config = {
version: esVersion,
installPath: Path.resolve(basePath, clusterName),
sourcePath: Path.resolve(REPO_ROOT, '../elasticsearch'),
license: testLicense,
password,
license,
basePath,
esArgs,
resources: files,
Expand Down
Loading