Skip to content

Commit

Permalink
Kuery api strict validation (elastic#161064)
Browse files Browse the repository at this point in the history
## Summary

Require validation for endpoints accepting `kuery` as a parameter in
POST or PUT requests.

**IMPORTANT**: This PR is part of the work needed to prepare the APIs
for Serverless.

### Some context
The initial idea was to completely remove any KQL queries from being
exposed in the endpoints, but after some discussion we came to the
agreement that they can stay but need to be validated, so only allowed
parameters can be sent. A similar approach is being followed by other
teams as well.

Impacted endpoints:
- `GET api/fleet/agents`
- `GET api/fleet/agent_status`
- `GET api/fleet/agent_policies`
- `GET api/fleet/package_policies`
- `GET api/fleet/enrollment_api_keys`
- `GET api/fleet/agent_status`

All these endpoints accept as a parameter `ListWithKuery`. It was
originally being deprecated but it was then decided to keep it and add
validation to the endpoints instead.

The endpoint `api/fleet/agents/action_status` doesn't accept `kuery`
anymore, since it was not being passed internally.


### What's changing

The KQL passed to these endpoints will be accepted in two possible
formats:
```
GET kbn:api/fleet/agents?kuery=local_metadata.agent.version="8.8.0"

GET kbn:api/fleet/agents?kuery=fleet-agents.local_metadata.agent.version="8.8.0"
```
Note that originally only the second format was going to accepted, but
we decided to avoid enforcing it as it would introduce a breaking
change, possibly breaking many customers automations.

### How it works
The code for `ValidateFilterKueryNode` has been adapted from a [similar
function](https://github.com/elastic/kibana/blob/45a483f49643bcca4ff130d9f100c38a1a2181e7/packages/core/saved-objects/core-saved-objects-api-server-internal/src/lib/search/utils/filter_utils.ts#L102)
already used in Kibana core. I added several tests where with some
common queries that are performed in the UI just to be sure that they
would pass validation. Additional queries can be validated by these
tests in the future.

`ValidateFilterKueryNode` needs to have the SO or index and a mapping
with the parameters to validate against. I copied over the mappings for
the necessary entities; if in the future we intend to expose a new
mapping parameter in the endpoints, it will be necessary to add it there
as well, or the validation will fail.

### UI
I also checked that the UI doesn't fail when using the KQL search boxes
for Agents, Agent policies and Enrollment keys and made sure that they
expose the same values present in the mappings.


### Testing

From dev tools, you can test the affected endpoints by entering the
following queries:
```
# agents
GET kbn:api/fleet/agents?kuery=fleet-agents.active:true
GET kbn:api/fleet/agents?kuery=active:true

# tags
GET kbn:api/fleet/agents/tags?kuery=fleet-agents.tags:tag1
GET kbn:api/fleet/agents/tags?kuery=tags:tag1

# agent status
GET kbn:/api/fleet/agent_status?kuery=fleet-agents.policy_id:fleet-server-policy
GET kbn:/api/fleet/agent_status?kuery=policy_id:fleet-server-policy

# package policies
GET kbn:/api/fleet/package_policies?kuery=ingest-package-policies.package.name:fleet_server

# agent policies
GET kbn:/api/fleet/agent_policies?kuery=ingest-agent-policies.name:"Fleet Server Policy"
GET kbn:/api/fleet/agent_policies?kuery=name:"Fleet Server Policy"

# enrollment keys
GET kbn:/api/fleet/enrollment_api_keys?kuery=fleet-enrollment-api-keys.policy_id:policy1
GET kbn:/api/fleet/enrollment_api_keys?kuery=policy1
```

These should all pass validation; modifying the parameters (for instance
with non existing ones) should fail validation

### Checklist
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios

---------

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Patryk Kopycinski <contact@patrykkopycinski.com>
  • Loading branch information
3 people committed Jul 24, 2023
1 parent e298d2c commit 82a1776
Show file tree
Hide file tree
Showing 28 changed files with 2,179 additions and 109 deletions.
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/common/index.ts
Expand Up @@ -58,6 +58,7 @@ export {
ENDPOINT_PRIVILEGES,
// dashboards ids
DASHBOARD_LOCATORS_IDS,
FLEET_ENROLLMENT_API_PREFIX,
} from './constants';
export {
// Route services
Expand Down
Expand Up @@ -8,7 +8,7 @@ get:
- schema:
type: integer
default: 5
in: query
in: query
name: errorSize
responses:
'200':
Expand Down Expand Up @@ -78,7 +78,7 @@ get:
type: string
description: policy id (POLICY_CHANGE action)
revision:
type: string
type: string
description: new policy revision (POLICY_CHANGE action)
creationTime:
type: string
Expand All @@ -90,11 +90,11 @@ get:
type: object
properties:
agentId:
type: string
type: string
error:
type: string
timestamp:
type: string
type: string
required:
- actionId
- complete
Expand Down
6 changes: 6 additions & 0 deletions x-pack/plugins/fleet/common/types/models/agent.ts
Expand Up @@ -415,3 +415,9 @@ export interface FleetServerAgentAction {

[k: string]: unknown;
}

export interface ActionStatusOptions {
errorSize: number;
page?: number;
perPage?: number;
}
3 changes: 0 additions & 3 deletions x-pack/plugins/fleet/common/types/rest_spec/common.ts
Expand Up @@ -7,9 +7,6 @@

import type { HttpFetchQuery } from '@kbn/core/public';

/**
* @deprecated will be replaced by a "narrow" set of parameters
*/
export interface ListWithKuery extends HttpFetchQuery {
page?: number;
perPage?: number;
Expand Down
Expand Up @@ -208,14 +208,6 @@ describe('SearchBar', () => {
});

describe('filterAndConvertFields', () => {
it('prepends the fieldPrefix if passed and hides some fields ', async () => {
expect(filterAndConvertFields(fields, '.test-index', 'test-index')).toEqual({
'test-index.api_key': { esTypes: ['keyword'], name: 'test-index.api_key', type: 'string' },
'test-index.name': { esTypes: ['keyword'], name: 'test-index.name', type: 'string' },
'test-index.version': { esTypes: ['keyword'], name: 'test-index.version', type: 'string' },
});
});

it('leaves the fields names unchanged and does not hide any fields if fieldPrefix is not passed', async () => {
expect(filterAndConvertFields(fields, '.test-index')).toEqual({
_id: { esTypes: ['_id'], name: '_id', type: 'string' },
Expand Down
Expand Up @@ -49,22 +49,14 @@ export const filterAndConvertFields = (
if (indexPattern === INDEX_NAME) {
filteredFields = fields.filter((field) => field.name.startsWith(fieldPrefix));
} else {
// Concatenate the fields with the prefix
const withPrefix = fields.map((field) => {
return !field.name.startsWith(fieldPrefix)
? { ...field, name: `${fieldPrefix}.${field.name}` }
: field;
});
// filter out fields that have names to be hidden
filteredFields = withPrefix.filter((field) => {
if (field.name.startsWith(fieldPrefix)) {
for (const hiddenField of HIDDEN_FIELDS) {
if (field.name.includes(hiddenField)) {
return false;
}
filteredFields = fields.filter((field) => {
for (const hiddenField of HIDDEN_FIELDS) {
if (field.name.includes(hiddenField)) {
return false;
}
return true;
}
return true;
});
}
} else {
Expand Down
Expand Up @@ -37,7 +37,7 @@ export function useFleetServerUnhealthy() {

if (agentPolicyIds.length > 0) {
const agentStatusesRes = await sendGetAgentStatus({
kuery: agentPolicyIds.map((policyId) => `policy_id:"${policyId}"`).join(' or '),
kuery: agentPolicyIds.map((policyId) => `policy_id:${policyId}`).join(' or '),
});

if (agentStatusesRes.error) {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugins/fleet/server/constants/index.ts
Expand Up @@ -95,3 +95,4 @@ export {
} from './fleet_es_assets';
export { FILE_STORAGE_DATA_AGENT_INDEX } from './fleet_es_assets';
export { FILE_STORAGE_METADATA_AGENT_INDEX } from './fleet_es_assets';
export * from './mappings';

0 comments on commit 82a1776

Please sign in to comment.