Skip to content

Commit

Permalink
[Hardening] Kibana Feature API Privileges Names (elastic#208067)
Browse files Browse the repository at this point in the history
## Summary

As part of our effort to harden API action definitions and enforce
standards this PR adds an utility `ApiPrivileges` class.
It is supposed to be used for both feature registration and API route
definition to construct the privilege name.
```ts
plugins.features.registerKibanaFeature({
  privileges: {
    all: {
      app: [...],
      catalogue: [...],
      api: [ApiPrivileges.manage('subject_name')],
      ...
    },
    read: {
      ...
      api: [ApiPrivileges.read('subject_name')],
      ...
    },
  },
})
....

// route definition
router.get(
  {
    path: 'api_path',
    security: {
      authz: {
        requiredPrivileges: [ApiPrivileges.manage('subject_name')],
      },
    },
  },
  async (ctx, req, res) => {}
);
```

`require_kibana_feature_privileges_naming` eslint rule has been added to
show warning if the API privilege name doesn't satisfy the naming
convention.

### Naming convention

- API privilege should start with valid `ApiOperation`: `manage`,
`read`, `update`, `delete`, `create`
- API privilege should use `_` as separator

❌ `read-entity-a`
❌ `delete_entity-a`
❌ `entity_manage`
✅ `read_entity_a`
✅ `delete_entity_a`
✅ `manage_entity`

> [!IMPORTANT]  
> Serverless ZDT update scenario:
>
> - version N has an endpoint protected with the `old_privilege_read`.
> - version N+1 has the same endpoint protected with a new
`read_privilege`.
> 
> There might be a short period between the time the UI pod N+1 passes
SO migrations and updates privileges and the time it's marked as
ready-to-handle-requests by k8s, and when UI pod N is terminated.
>
> After discussion with @legrego and @azasypkin we decided to ignore it
due to the perceived risk-to-cost ratio:
> 1. The time window users might be affected is very narrow because we
register privileges late in the Kibana startup flow (e.g., after SO
migrations).
> 2. The transient 403 errors users might get won't result in session
termination and shouldn't lead to data loss.
> 3. The roll-out will be performed in batches over the course of
multiple weeks and implemented by different teams. This means the impact
per release shouldn't be significant.

### Checklist

- [x]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [x] [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

__Relates: https://github.com/elastic/kibana/issues/198716__

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
  • Loading branch information
3 people authored Feb 3, 2025
1 parent 78606e0 commit 504510b
Show file tree
Hide file tree
Showing 27 changed files with 454 additions and 25 deletions.
17 changes: 17 additions & 0 deletions dev_docs/key_concepts/api_authorization.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,23 @@ router.get({
}, handler);
```

### Naming conventions for privileges
1. **Privilege should start with a valid `ApiOperation`**:
- **Valid operations**: `manage`, `read`, `update`, `delete`, `create`.
- Use the corresponding methods from the `ApiPrivileges` utility class: `ApiPrivileges.manage`, `ApiPrivileges.read`, etc.
2. **Use `_` as the separator** between the operation and the subject.

**Examples**:
Incorrect privilege names ❌
- `read-entity-a`: Uses `-` instead of `_`.
- `delete_entity-a`: Mixes `_` and `-`.
- `entity_manage`: Places the subject name before the operation.

Correct privilege names ✅
- `read_entity_a`
- `delete_entity_a`
- `manage_entity`

### Configuring operator and superuser privileges
We have two special predefined privilege sets that can be used in security configuration:
1. Operator
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-eslint-config/.eslintrc.js
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,7 @@ module.exports = {
'@kbn/eslint/no_async_promise_body': 'error',
'@kbn/eslint/no_async_foreach': 'error',
'@kbn/eslint/no_deprecated_authz_config': 'error',
'@kbn/eslint/require_kibana_feature_privileges_naming': 'warn',
'@kbn/eslint/no_trailing_import_slash': 'error',
'@kbn/eslint/no_constructor_args_in_property_initializers': 'error',
'@kbn/eslint/no_this_in_property_initializers': 'error',
Expand Down
1 change: 1 addition & 0 deletions packages/kbn-eslint-plugin-eslint/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,5 +21,6 @@ module.exports = {
no_unsafe_console: require('./rules/no_unsafe_console'),
no_unsafe_hash: require('./rules/no_unsafe_hash'),
no_deprecated_authz_config: require('./rules/no_deprecated_authz_config'),
require_kibana_feature_privileges_naming: require('./rules/require_kibana_feature_privileges_naming'),
},
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,223 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

const ts = require('typescript');
const path = require('path');

function getImportedVariableValue(context, name, propertyName) {
const parent = context
.getAncestors()
.find((ancestor) => ['BlockStatement', 'Program'].includes(ancestor.type));

if (!parent) return;

const importDeclaration = parent.body.find(
(statement) =>
statement.type === 'ImportDeclaration' &&
statement.specifiers.some((specifier) => specifier.local.name === name)
);

if (!importDeclaration) return;

const absoluteImportPath = require.resolve(importDeclaration.source.value, {
paths: [path.dirname(context.getFilename())],
});

const program = ts.createProgram([absoluteImportPath], {});
const sourceFile = program.getSourceFile(absoluteImportPath);

if (!sourceFile) return null;

const checker = program.getTypeChecker();
const symbols = checker.getExportsOfModule(sourceFile.symbol);
const symbol = symbols.find((s) => s.name === name);

if (!symbol) return null;

if (propertyName) {
const currentSymbol = checker.getTypeOfSymbolAtLocation(symbol, sourceFile);
const property = currentSymbol.getProperty(propertyName);

if (ts.isStringLiteral(property.valueDeclaration.initializer)) {
return property.valueDeclaration.initializer.text;
}

return null;
}

const initializer = symbol?.valueDeclaration?.initializer;

if (ts.isStringLiteral(initializer)) {
return initializer.text;
}

return null;
}

function validatePrivilegesNode(context, privilegesNode, scopedVariables) {
['all', 'read'].forEach((privilegeType) => {
const privilege = privilegesNode.value.properties.find(
(prop) =>
prop.key && prop.key.name === privilegeType && prop.value.type === 'ObjectExpression'
);

if (!privilege) return;

const apiProperty = privilege.value.properties.find(
(prop) => prop.key && prop.key.name === 'api' && prop.value.type === 'ArrayExpression'
);

if (!apiProperty) return;

apiProperty.value.elements.forEach((element) => {
let valueToCheck = null;

if (element.type === 'Literal' && typeof element.value === 'string') {
valueToCheck = element.value;
} else if (element.type === 'Identifier') {
valueToCheck = scopedVariables.has(element.name)
? scopedVariables.get(element.name)
: getImportedVariableValue(context, element.name);
} else if (element.type === 'MemberExpression') {
valueToCheck = getImportedVariableValue(
context,
element.object.name,
element.property.name
);
}

if (valueToCheck) {
const isValid = /^(manage|create|update|delete|read)/.test(valueToCheck);
const usesValidSeparator = /^[a-z0-9_]+$/.test(valueToCheck);
let method = 'manage';

if (valueToCheck.includes('read')) {
method = 'read';
}

if (valueToCheck.includes('create') || valueToCheck.includes('copy')) {
method = 'create';
}

if (valueToCheck.includes('delete')) {
method = 'delete';
}

if (valueToCheck.includes('update')) {
method = 'update';
}

if (!isValid) {
return context.report({
node: element,
message: `API privilege '${valueToCheck}' should start with [manage|create|update|delete|read] or use ApiPrivileges.${method} instead`,
});
}

if (!usesValidSeparator) {
return context.report({
node: element,
message: `API privilege '${valueToCheck}' should use '_' as a separator`,
});
}
}
});
});
}

module.exports = {
meta: {
type: 'problem',
docs: {
description: 'Ensure API privileges in registerKibanaFeature call follow naming conventions',
category: 'Best Practices',
recommended: true,
},
schema: [],
},

create(context) {
return {
CallExpression(node) {
const isRegisterKibanaFeatureCall =
node.callee.type === 'MemberExpression' &&
node.callee.property.name === 'registerKibanaFeature' &&
((node.callee.object.type === 'MemberExpression' &&
node.callee.object.property.name === 'features') ||
node.callee.object.name === 'features');

if (!isRegisterKibanaFeatureCall) return;

const scopedVariables = new Map();

const sourceCode = context.getSourceCode();

const parent = sourceCode
.getAncestors(node)
.find((ancestor) => ['BlockStatement', 'Program'].includes(ancestor.type));

if (parent) {
parent.body.forEach((statement) => {
if (statement.type === 'VariableDeclaration') {
statement.declarations.forEach((declaration) => {
if (
declaration.id.type === 'Identifier' &&
declaration.init &&
declaration.init.type === 'Literal' &&
typeof declaration.init.value === 'string'
) {
scopedVariables.set(declaration.id.name, declaration.init.value);
}
});
}
});
}

const [feature] = node.arguments;
if (feature?.type === 'ObjectExpression') {
const privilegesProperty = feature.properties.find(
(prop) =>
prop.key && prop.key.name === 'privileges' && prop.value.type === 'ObjectExpression'
);

if (!privilegesProperty) return;

return validatePrivilegesNode(context, privilegesProperty, scopedVariables);
}
},
ExportNamedDeclaration(node) {
if (
node.declaration?.type !== 'VariableDeclaration' ||
!node.declaration.declarations?.length
) {
return;
}

node.declaration.declarations.forEach((declaration) => {
if (declaration.init && declaration.init.type === 'ObjectExpression') {
if (
!['id', 'name', 'privileges', 'scope', 'category'].every((key) =>
declaration.init.properties.find((prop) => prop.key?.name === key)
)
) {
return;
}

const privilegesProperty = declaration.init.properties.find(
(prop) =>
prop.key && prop.key.name === 'privileges' && prop.value.type === 'ObjectExpression'
);

validatePrivilegesNode(context, privilegesProperty, new Map());
}
});
},
};
},
};
Loading

0 comments on commit 504510b

Please sign in to comment.