-
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
Add license check for FIPS #181187
Add license check for FIPS #181187
Conversation
…S is enabled in KB and Node
… error for this test
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
/ci |
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.
Core changes looks ok, got a couple questions before approval though:
@@ -6,6 +6,7 @@ | |||
* Side Public License, v 1. | |||
*/ | |||
|
|||
import { CoreFipsService } from './fips'; |
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.
NIT: import type
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.
Fixed in 7b7ed01
@@ -21,6 +22,11 @@ export interface SecurityServiceSetup { | |||
* @remark this should **exclusively** be used by the security plugin. | |||
*/ | |||
registerSecurityDelegate(api: CoreSecurityDelegateContract): void; | |||
|
|||
/** | |||
* The {{@link CoreFipsService | FIPS service}} |
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.
NIT: I don't think doubling {{
}}
even works for tsdoc?
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.
Fixed in 7b7ed01
} | ||
|
||
public setup(): InternalSecurityServiceSetup { | ||
const config = this.getConfig(); | ||
const securityConfig: SecurityServiceConfigType = config.get(['xpack', 'security']); |
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.
Oh, yeah... I overlooked we would need to access the security plugin's config from our core service for that feature 🙈.
With our security-in-core initiative, I think we'll want to move the security config from xpack.security.*
to core.security.*
at some point. It shouldn't be that much work (moving the schema registration + adding config deprecation + exposing the config type from core for the security plugin to be able to use them)
Without doing so, I agree that hacking around and accessing the raw config is the most pragmatic approach. It's absolutely against a dozen Core principles though, so would you mind opening a follow-up issue, just to keep track (and discuss) of moving the security config schema/registration to Core?
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.
Follow up issue: #186863
Ill make a note to update this code once we move the config over
import { SecurityServiceConfigType } from '../utils'; | ||
|
||
export function isFipsEnabled(config: SecurityServiceConfigType): boolean { | ||
return config?.experimental?.fipsMode?.enabled || false; |
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.
NIT: ?? false
is technically more accurate
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.
Fixed in 7b7ed01
let config: SecurityServiceConfigType; | ||
|
||
beforeAll(() => { | ||
config = {}; | ||
}); | ||
|
||
afterEach(() => { | ||
config = {}; | ||
}); |
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.
NIT: I would ditch that part, given each test manually declare the config it need for its assertion.
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.
Fixed in 7b7ed01
@@ -388,6 +388,7 @@ kibana_vars=( | |||
xpack.security.authc.selector.enabled | |||
xpack.security.cookieName | |||
xpack.security.encryptionKey | |||
xpack.security.experimental.fipsMode.enabled |
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.
NIT: Thinking out loud there, but are we sure we want that under that experimental prefix instead of just flagging it as experimental in the documentation?
experimental config prefixes means once the feature leaves its experimental state, the config property name changes too, meaning another config deprecation to move it, and overall more work and maintenance.
So yeah, wouldn't documenting the flag as experimental be enough?
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.
I'm torn; I want to really make clear that this config option should not be used at the moment, but I do see what you mean more work/rework.
@legrego @azasypkin What do you think?
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.
I'm quite torn as well. We really do not want this to be exposed/configured yet, and documenting it goes against that goal.
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.
Yeah, I think I'm in the "experimental-in-config-entry-name" camp mostly due to these reasons:
- We absolutely don't want anyone outside of Elastic to use or even know about it at this point, no usual "deprecation" flow is assumed. It's experimental in the broadest sense, it can theoretically lead to all sorts of consequences from unexpected downtime to data loss or violated security expectations.
- If anyone happens to figure out that such a setting exists and tries to use it before we can commit to supporting FIPS, well, that's on them. But when we're ready to officially support FIPS, we want to make sure they cannot upgrade before they go and read about the scope of FIPS support, expectations, and proper configuration that might potentially include other settings they will need to configure in addition to
fipsMode.enabled
.
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.
👍 fair enough, thanks for explaining the reasoning
isNodeRunningWithFipsEnabled ? 'enabled' : 'disabled' | ||
}` | ||
); | ||
process.exit(78); |
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.
🙈 I gonna do like I did not saw a plugin's configuration validation call process.exit
with a return code not used anywhere else in of our software.
More seriously though, can't this be done at Core's level? I would have way less concerns and issues with Core performing this check (or at least this non-standard process termination). This is done before the license is added to the equation, right? So from my understanding nothing blocks it from being done in Core?
(but if we can't I'll just 🙈)
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.
😅 fair points, I'll take a look to see if there might be a better place for this check in core.
Did you have any place in mind that might be best?
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.
Could this possibly be done in the setup
phase of Core's security
service (where you added the logic to return the isFIPS
API from the contract)?
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.
Done in 2c11610
/ci |
/ci |
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.
LGTM
💛 Build succeeded, but was flaky
Failed CI StepsMetrics [docs]Public APIs missing comments
Page load bundle
Unknown metric groupsAPI count
History
To update your PR or re-run it, just comment with: cc @kc13greiner |
…in CI and Docker (#187533) ## Summary Closes #188272 A check was added to in #181187 which detects if the environment has FIPS enabled NodeJS, but Kibana is not setup properly. This adds the Kibana setting for FIPS in CI and the Docker image. Note there are still license issues on some tests due to #181187 as well, but this will be handled in another PR.
Updates
Latest updates
Consolidating all FIPS PRs into this PR
*Previous PRs were Approved
Changes
xpack.security.experimental.fipsMode.enabled
Summary
Closes #169738
Closes #169739
Closes #169740
Closes #185948
FIPS is a platinum license feature.
KIbana instances must have a platinum or better license to start up in FIPS mode, a lesser license will result in Kibana failing to start up
If the license is degraded, Kibana will still run, but an error will be logged letting the user know that Kibana will not be able to restart.
Config changes
This PR required the changes that were approved from a previous PR, since that PR couldn't be merged into main, I merged it here.
Testing
Locally
In your
kibana.dev.yml
add:xpack.security.experimental.fipsMode.enabled: true
To allow Kibana to start without actually providing a compliant OpenSSL provider, in
x-pack/plugins/security/server/config.ts
change L328 fromif (isFipsEnabled !== isNodeRunningWithFipsEnabled)
toif (false)
You are now configured to run in FIPS-spoof mode!
Run:
yarn es snapshot
andyarn start
> You should see Kibana fail to start with an error about using a basic license.Run:
yarn es snapshot --license trial
andyarn start
> Kibana should start.Login as
elastic
and navigate to Stack Management > License ManagementSwitch your license to
basic
and accept.In your logs, you will see an error letting users know that you no longer have an appropriate license and Kibana will not restart.
For FIPS enthusiasts
Start an ES instance in a method of your choosing, but not using
yarn es snapshot
. I like to use an 8.15.0-snapshot from the.es/cache
directory by runningtar -xzvf elasticsearch-8.15.0-SNAPSHOT-darwin-aarch64.tar.gz
and cd into the new directory'sbin
folder to run./elasticsearch
Ensure you have Docker running locally.
From any command line, run:
docker run --rm -it -e XPACK_SECURITY_FIPSMODE_ENABLED='true' -p 5601:5601/tcp docker.elastic.co/kibana-ci/kibana-ubi-fips:8.15.0-SNAPSHOT-bc3150316ed317c08d57c6bd785ba39586072e1d
This will start Kibana into Interactive Setup mode, copy and paste the token from the ES startup logs.
Kibana should fail to start and you should see Kibana fail to start with an error about using a basic license.
Repeat the above process except before you paste the token from ES, do the following to enable a trial license on your ES instance:
In a new terminal window, navigate to your the top level of your elasticsearch folder and run
curl -X POST --cacert config/certs/http_ca.crt -u elastic:YOUR_PASSWORD_HERE "https://localhost:9200/_license/start_trial?acknowledge=true&pretty"
You should receive a successful response.
Now paste the token from the ES startup logs into the Kibana Interactive Setup window and Kibana should start.
Login as
elastic
and navigate to Stack Management > License ManagementSwitch your license to
basic
and accept.In your logs, you will see an error letting users know that you no longer have an appropriate license and Kibana will not restart.