-
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
[ML] Adds secondary authorization header to Transforms in Fleet #154665
Conversation
5c5f862
to
4086164
Compare
4086164
to
605b27b
Compare
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.
Minor suggestions, otherwise UI text LGTM
...c/applications/integrations/sections/epm/screens/detail/assets/deferred_assets_accordion.tsx
Outdated
Show resolved
Hide resolved
...lic/applications/integrations/sections/epm/screens/detail/assets/deferred_assets_warning.tsx
Outdated
Show resolved
Hide resolved
...lic/applications/integrations/sections/epm/screens/detail/assets/deferred_assets_warning.tsx
Outdated
Show resolved
Hide resolved
...plications/integrations/sections/epm/screens/detail/assets/deferred_transforms_accordion.tsx
Outdated
Show resolved
Hide resolved
b4112ff
to
5bb5690
Compare
@qn895 Where I can found the hot risk score integration to test this locally? |
@qn895 I did some local tests and overall it seems to work well. I may have one suggestion one we click on reauthorize all it could be nice to reload the assets tab what do you think? |
transformId: string; | ||
}> | null>(null); | ||
|
||
useEffect(() => { |
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: do we really need a useEffect
here can we call an authorizeTransforms
directly from the click events?
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.
Updated here 4e54dee
(#154665)
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.
Tested locally and LGTM 🚀
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.
One small suggestion, otherwise UI text LGTM!
'xpack.fleet.epm.packageDetails.assets.deferredTransformInstallationsDescription', | ||
{ | ||
defaultMessage: | ||
'{assetCount, plural, one {Transform was installed but requires} other {# transforms were installed but require}} additional permissions to run. You must have the transform_admin built-in role or manage_transform cluster privileges to start operations.', |
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.
Based on the discussion here, I suggest slightly modifying this message:
'{assetCount, plural, one {Transform was installed but requires} other {# transforms were installed but require}} additional permissions to run. You must have the transform_admin built-in role or manage_transform cluster privileges to start operations.', | |
'{assetCount, plural, one {Transform was installed but requires} other {# transforms were installed but require}} additional permissions to run. Contact your administrator to request the required privileges.', |
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.
Updated here 4e54dee
(#154665)
- Add comment to route - Update settings to swallow error without blocking - Remove redundant update/delete index alias code - Better error handling, and scenario if security is not defined
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.
Tested with your example Host Risk Score package and LGTM
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.
Taking a second look I'm wondering if some of the warnings need changes?
...plications/integrations/sections/epm/screens/detail/assets/deferred_transforms_accordion.tsx
Outdated
Show resolved
Hide resolved
...lic/applications/integrations/sections/epm/screens/detail/assets/deferred_assets_warning.tsx
Show resolved
Hide resolved
@hop-dev you can find the package here https://github.com/qn895/integrations/tree/host_risk_score_versioned |
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.
Text changes for the warnings LGTM
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.
Tested locally, looks good to me!
Were you able to test the scenario where the package is reinstalled as system user? Do we retain the previous auth header?
|
||
// Extended version of x-pack/plugins/security/server/authentication/http_authentication/http_authorization_header.ts | ||
// to prevent bundle being required in security_solution | ||
// FIXME: Put this in a package |
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.
For this FIXME comment do we have an issue for the fix? When do we expect to fix it?
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 removed this FIXME for now as the file is modified from the original copy of security_solution's code 0187c5f
(#154665)
count: number; | ||
}> = ({ count }) => { | ||
return ( | ||
<EuiCallOut color="primary"> |
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 think callouts should always have a title with an icon , check out the EUI examples https://elastic.github.io/eui/#/display/callout
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.
Updated here 0187c5f
(#154665)
@@ -432,3 +450,60 @@ export const getVerificationKeyIdHandler: FleetRequestHandler = async ( | |||
return defaultFleetErrorHandler({ error, response }); | |||
} | |||
}; | |||
|
|||
/** |
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.
thanks for the comments here 👍
As discussed in Slack with you and Nicolas, I'll create a smaller follow-up PR to address force reinstallation of packages containing transforms with the new spec. Thanks all for your help testing and reviewing this PR 🥳 |
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
Saved Objects .kibana field count
Unknown metric groupsAPI count
ESLint disabled line counts
Total ESLint disabled count
History
To update your PR or re-run it, just comment with: cc @qn895 |
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
I'm happy to merge this and fix the inefficiency in a followup.
Summary
The PR updates how credentials are created and managed for packages including Transforms. Previously, everything will be installed as
kibana_system
user, which has limited permissions to a specific set of indices defined internally. This PR changes so that a secondary authorization is passed to the creation of Transforms, making the permissions/privileges dependent on the logged-in user.Installing a package containing transforms
-It will parse the authorization header (schema and credentials) from the Kibana request to the package handlers.
transform/_put
requests.Transforms will be successfully created and started. They will be marked in the saved object reference with
deferred: false
Transform

_meta
will haveinstalled_by: {username}
Package will be successfully installed
deferred: true
Deferred installations
If a package has deferred installations (a.k.a assets that were included in the package, but require additional permissions to operate correctly), it will:
Show a warning on the

Installed integrations
page:Show a warning badge with explanation on the tab:

Show a new

Deferred installations
section as well as call out message to prompt user to re-authorize inside theAssets
tab:If the currently logged-in user has sufficient permissions (
manage_transform
ES cluster privilege/transform_admin
Kibana role), the Reauthorize buttons will be enabled:Reauthorizing installations
For transforms:
Reauthorize
button will send an_transform/_update
API request with aheaders: {es-secondary-authorization: 'ApiKey {encoded_api}'
and then a_transform/_start
to start operations._meta
will be updated with addition oflast_authorized_by: {username}
order
is specified in_meta
of the transform, they will be updated and started sequentially. Else, they will be executed concurrently.Reviewers note:
-For kibana-core: saved object for Fleet's EsAsset was extended with
deferred: boolean
, thus changing the hash.Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
Delete this section if it is not applicable to this PR.
Before closing this PR, invite QA, stakeholders, and other developers to identify risks that should be tested prior to the change/feature release.
When forming the risk matrix, consider some of the following examples and how they may potentially impact the change:
For maintainers