-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
Changes from 54 commits
77697db
35fa148
ec83314
c54e919
07b21c0
f43f7d5
4fe0825
ddab617
4fe127d
d33488d
6daa6c8
17ba872
83766b6
b2b354b
dfd9154
c17e569
001651c
67d8092
e0f7ebc
8aaf935
1f93c2f
7feec4f
4cc907e
af02d5b
6a9d171
0046e88
bf5113f
7158794
20d4b7b
412a745
a863849
aed8025
c796660
52b5f3c
f1504ac
2b5a712
29a79db
ae4293e
36da348
3e078fa
e620cf2
608b2f9
7d840a4
be62e5e
2ce8759
f290b6e
b2a38ab
d444894
f6829f6
cf928c8
84fa1aa
7454e8a
08561e4
e4b6636
c362dd7
abff161
7a8ce60
a8be638
95969a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,93 +16,100 @@ 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 { getFips } from 'crypto'; | ||
|
||
const createStubInternalContract = (): CoreSecurityDelegateContract => { | ||
return Symbol('stubContract') as unknown as CoreSecurityDelegateContract; | ||
}; | ||
|
||
describe('SecurityService', () => { | ||
let coreContext: ReturnType<typeof mockCoreContext.create>; | ||
let service: SecurityService; | ||
describe('SecurityService', function () { | ||
if (getFips() !== 1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: can we add a code comment here explaining why we're doing this? Do all these tests fail in FIPS? Even There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) |
||
let coreContext: ReturnType<typeof mockCoreContext.create>; | ||
let service: SecurityService; | ||
|
||
beforeEach(() => { | ||
coreContext = mockCoreContext.create(); | ||
service = new SecurityService(coreContext); | ||
beforeEach(() => { | ||
coreContext = mockCoreContext.create(); | ||
service = new SecurityService(coreContext); | ||
|
||
convertSecurityApiMock.mockReset(); | ||
getDefaultSecurityImplementationMock.mockReset(); | ||
}); | ||
convertSecurityApiMock.mockReset(); | ||
getDefaultSecurityImplementationMock.mockReset(); | ||
}); | ||
|
||
describe('#setup', () => { | ||
describe('#registerSecurityDelegate', () => { | ||
it('throws if called more than once', () => { | ||
const { registerSecurityDelegate } = service.setup(); | ||
describe('#setup', () => { | ||
describe('#registerSecurityDelegate', () => { | ||
it('throws if called more than once', () => { | ||
const { registerSecurityDelegate } = service.setup(); | ||
|
||
const contract = createStubInternalContract(); | ||
registerSecurityDelegate(contract); | ||
const contract = createStubInternalContract(); | ||
registerSecurityDelegate(contract); | ||
|
||
expect(() => registerSecurityDelegate(contract)).toThrowErrorMatchingInlineSnapshot( | ||
`"security API can only be registered once"` | ||
); | ||
expect(() => registerSecurityDelegate(contract)).toThrowErrorMatchingInlineSnapshot( | ||
`"security API can only be registered once"` | ||
); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('#fips', () => { | ||
describe('#isEnabled', () => { | ||
it('should return boolean', () => { | ||
const { fips } = service.setup(); | ||
describe('#fips', () => { | ||
describe('#isEnabled', () => { | ||
it('should return boolean', () => { | ||
const { fips } = service.setup(); | ||
|
||
expect(fips.isEnabled()).toBe(false); | ||
expect(fips.isEnabled()).toBe(false); | ||
}); | ||
}); | ||
}); | ||
}); | ||
}); | ||
|
||
describe('#start', () => { | ||
it('logs a warning if the security API was not registered', () => { | ||
service.setup(); | ||
service.start(); | ||
describe('#start', () => { | ||
it('logs a warning if the security API was not registered', () => { | ||
service.setup(); | ||
service.start(); | ||
|
||
expect(loggerMock.collect(coreContext.logger as MockedLogger).warn).toMatchInlineSnapshot(` | ||
expect(loggerMock.collect(coreContext.logger as MockedLogger).warn).toMatchInlineSnapshot(` | ||
Array [ | ||
Array [ | ||
"Security API was not registered, using default implementation", | ||
], | ||
] | ||
`); | ||
}); | ||
}); | ||
|
||
it('calls convertSecurityApi with the registered API', () => { | ||
const { registerSecurityDelegate } = service.setup(); | ||
it('calls convertSecurityApi with the registered API', () => { | ||
const { registerSecurityDelegate } = service.setup(); | ||
|
||
const contract = createStubInternalContract(); | ||
registerSecurityDelegate(contract); | ||
const contract = createStubInternalContract(); | ||
registerSecurityDelegate(contract); | ||
|
||
service.start(); | ||
service.start(); | ||
|
||
expect(convertSecurityApiMock).toHaveBeenCalledTimes(1); | ||
expect(convertSecurityApiMock).toHaveBeenCalledWith(contract); | ||
}); | ||
expect(convertSecurityApiMock).toHaveBeenCalledTimes(1); | ||
expect(convertSecurityApiMock).toHaveBeenCalledWith(contract); | ||
}); | ||
|
||
it('calls convertSecurityApi with the default implementation when no API was registered', () => { | ||
const contract = createStubInternalContract(); | ||
getDefaultSecurityImplementationMock.mockReturnValue(contract); | ||
it('calls convertSecurityApi with the default implementation when no API was registered', () => { | ||
const contract = createStubInternalContract(); | ||
getDefaultSecurityImplementationMock.mockReturnValue(contract); | ||
|
||
service.setup(); | ||
service.start(); | ||
service.setup(); | ||
service.start(); | ||
|
||
expect(convertSecurityApiMock).toHaveBeenCalledTimes(1); | ||
expect(convertSecurityApiMock).toHaveBeenCalledWith(contract); | ||
}); | ||
expect(convertSecurityApiMock).toHaveBeenCalledTimes(1); | ||
expect(convertSecurityApiMock).toHaveBeenCalledWith(contract); | ||
}); | ||
|
||
it('returns the result of convertSecurityApi as contract', () => { | ||
const convertedContract = { stub: true }; | ||
convertSecurityApiMock.mockReturnValue(convertedContract); | ||
it('returns the result of convertSecurityApi as contract', () => { | ||
const convertedContract = { stub: true }; | ||
convertSecurityApiMock.mockReturnValue(convertedContract); | ||
|
||
service.setup(); | ||
const startContract = service.start(); | ||
service.setup(); | ||
const startContract = service.start(); | ||
|
||
expect(startContract).toEqual(convertedContract); | ||
expect(startContract).toEqual(convertedContract); | ||
}); | ||
}); | ||
} else { | ||
test('fips is enabled', function () { | ||
expect(getFips() === 1).toEqual(true); | ||
}); | ||
}); | ||
} | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, | ||
|
@@ -75,6 +77,13 @@ export function createRootWithSettings( | |
pkg.version = customKibanaVersion; | ||
} | ||
|
||
let oss = true; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tip Also a comment here would be helpful. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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, | ||
{ | ||
|
@@ -84,10 +93,10 @@ export function createRootWithSettings( | |
watch: false, | ||
basePath: false, | ||
runExamples: false, | ||
oss: true, | ||
disableOptimizer: true, | ||
cache: true, | ||
dist: false, | ||
oss, | ||
...cliArgs, | ||
}, | ||
repoPackages: getPackages(REPO_ROOT), | ||
|
@@ -255,7 +264,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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Warning I think it’s fine to default to Do we have many test suites that specify a license different from There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 Weakly held opinion, WDY both T? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I agree!
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.
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') { | ||
|
@@ -292,6 +307,7 @@ export function createTestServers({ | |
hosts: es.getHostUrls(), | ||
username: kibanaServerTestUser.username, | ||
password: kibanaServerTestUser.password, | ||
...(getFips() ? kbnSettings.elasticsearch : {}), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. question: why do we need this? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 😆) |
||
}; | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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}`; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'; | ||
|
||
|
@@ -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'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you add a comment explaining the logic for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added here e4b6636 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment as for There was a problem hiding this comment. Choose a reason for hiding this commentThe 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.
Many tests default to
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, | ||
|
There was a problem hiding this comment.
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.
kibana/.buildkite/scripts/common/env.sh
Lines 143 to 146 in 833a267
NODE_OPTIONS for jest at
kibana/.buildkite/scripts/steps/test/jest_parallel.sh
Line 63 in 833a267
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI in progress: https://buildkite.com/elastic/kibana-fips/builds/197#01924db0-7734-405c-bc8c-c26facdcbb60