Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,7 @@ describe('checking migration metadata changes on all registered SO types', () =>
"ml-module": "cb77705b41ea0a35d8ba79b19014a30069e0e93a2cfb7ae8c6f20a79207d5daa",
"ml-trained-model": "133305438dc0b60a6660c44f0d8183ad5ba079db8fdd4e4f4b5ab3a09d2f29b8",
"monitoring-telemetry": "fa7c4f2a099b4f0539e571372a598601c2a0c65ba50f6c34df23b4d6925cdc53",
"oauth_state": "9b204196198cd491014364003e68be49ecc193996dca38a49ac685402b757e96",
"oauth_state": "dba837d6453b71f02f3aedc40a85bb161267e50203058ecab83a6b918376065f",
"observability-onboarding-state": "b656db675800bfee8a2ddb5bf73b543542c7a7db64ed268ab5adcae6910773d2",
"osquery-manager-usage-metric": "8833b9f812e9179897444c395761f9911945cfb77de9869c4e9b6ee6eeb0f573",
"osquery-pack": "7ee940ea04c9c562406977efaa213050b2079c5bcc4e06ec56c8be6a85eb5ccd",
Expand Down Expand Up @@ -993,7 +993,7 @@ describe('checking migration metadata changes on all registered SO types', () =>
"oauth_state|global: a665293b001c9e7b6e92c0a50a553b8163dbcd41",
"oauth_state|mappings: 04721e2fa836fed1f3f2e9c343d96ec5304f8f09",
"oauth_state|schemas: da39a3ee5e6b4b0d3255bfef95601890afd80709",
"oauth_state|10.1.0: 5e73676664142f84718b9e578df9d703e47a096a7e12137c166499438b1c6677",
"oauth_state|10.1.0: 6e761629347d967babaf76618a126851715a8156e27de8198f27248bd894edb9",
"====================================================================================",
"observability-onboarding-state|global: c226ba4dd0412c2d7fd7a01976461e9da00b78bf",
"observability-onboarding-state|mappings: d6efe91e6efcc5e1b41fac37b731b715182939ce",
Expand Down
8 changes: 7 additions & 1 deletion x-pack/platform/plugins/shared/actions/server/feature.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
ACTION_SAVED_OBJECT_TYPE,
ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE,
CONNECTOR_TOKEN_SAVED_OBJECT_TYPE,
OAUTH_STATE_SAVED_OBJECT_TYPE,
} from './constants/saved_objects';

const ENDPOINT_SECURITY_EXECUTE_PRIVILEGE_API_TAG = 'actions:execute-endpoint-security-connectors';
Expand Down Expand Up @@ -48,6 +49,7 @@ export const ACTIONS_FEATURE: KibanaFeatureConfig = {
ACTION_SAVED_OBJECT_TYPE,
ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE,
CONNECTOR_TOKEN_SAVED_OBJECT_TYPE,
OAUTH_STATE_SAVED_OBJECT_TYPE,
],
read: [],
},
Expand All @@ -62,7 +64,11 @@ export const ACTIONS_FEATURE: KibanaFeatureConfig = {
},
savedObject: {
// action execution requires 'read' over `actions`, but 'all' over `action_task_params`
all: [ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE, CONNECTOR_TOKEN_SAVED_OBJECT_TYPE],
all: [
ACTION_TASK_PARAMS_SAVED_OBJECT_TYPE,
CONNECTOR_TOKEN_SAVED_OBJECT_TYPE,
OAUTH_STATE_SAVED_OBJECT_TYPE,
],
read: [ACTION_SAVED_OBJECT_TYPE],
},
ui: ['show', 'execute'],
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,10 +104,12 @@ export class OAuthAuthorizationService {
/**
* Gets OAuth configuration for a connector with decrypted secrets
* @param connectorId - The connector ID
* @param namespace - The space ID where to look for the connector. For default space, it's undefined.
Comment thread
seanstory marked this conversation as resolved.
* The namespace is the same as where the authorization was initiated from, which should be the space where the connector was created.
* @returns OAuth configuration including authorizationUrl, clientId, and optional scope
* @throws Error if connector is not found, not OAuth, or missing required config
*/
async getOAuthConfig(connectorId: string): Promise<OAuthConfig> {
async getOAuthConfig(connectorId: string, namespace: string | undefined): Promise<OAuthConfig> {
// Get connector to verify it exists and check auth type
const connector = await this.actionsClient.get({ id: connectorId });
const config = connector.config as OAuthConnectorConfig;
Expand All @@ -119,7 +121,8 @@ export class OAuthAuthorizationService {
const rawAction =
await this.encryptedSavedObjectsClient.getDecryptedAsInternalUser<ConnectorWithOAuth>(
'action',
connectorId
connectorId,
{ namespace }
);

const secrets = rawAction.attributes.secrets;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ interface OAuthStateAttributes {
connectorId: string;
redirectUri: string;
kibanaReturnUrl: string;
spaceId: string;
createdAt: string;
expiresAt: string;
createdBy?: string;
Expand All @@ -39,6 +40,7 @@ interface CreateStateOptions {
connectorId: string;
redirectUri: string;
kibanaReturnUrl: string;
spaceId: string;
createdBy?: string;
}

Expand Down Expand Up @@ -78,6 +80,7 @@ export class OAuthStateClient {
connectorId,
redirectUri,
kibanaReturnUrl,
spaceId,
createdBy,
}: CreateStateOptions): Promise<{
state: OAuthState;
Expand All @@ -101,6 +104,7 @@ export class OAuthStateClient {
connectorId,
redirectUri,
kibanaReturnUrl,
spaceId,
createdAt: now.toISOString(),
expiresAt: expiresAt.toISOString(),
createdBy,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export const oauthAuthorizeRoute = (
});
}

const [coreStart, { encryptedSavedObjects }] = await coreSetup.getStartServices();
const [coreStart, { encryptedSavedObjects, spaces }] = await coreSetup.getStartServices();
const kibanaUrl = coreStart.http.basePath.publicBaseUrl;
if (!kibanaUrl) {
return res.badRequest({
Expand All @@ -94,7 +94,13 @@ export const oauthAuthorizeRoute = (
}),
kibanaBaseUrl: kibanaUrl,
});
const oauthConfig = await oauthService.getOAuthConfig(connectorId);

const spaceId = spaces ? spaces.spacesService.getSpaceId(req) : 'default';
let namespace: string | undefined;
if (spaces && spaceId) {
namespace = spaces.spacesService.spaceIdToNamespace(spaceId);
}
const oauthConfig = await oauthService.getOAuthConfig(connectorId, namespace);
const redirectUri = oauthService.getRedirectUri();

// Validate and build return URL for post-OAuth redirect
Expand Down Expand Up @@ -133,6 +139,7 @@ export const oauthAuthorizeRoute = (
connectorId,
redirectUri,
kibanaReturnUrl,
spaceId,
});

const authorizationUrl = oauthService.buildAuthorizationUrl({
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ export const oauthCallbackRoute = (
}

try {
const [, { encryptedSavedObjects }] = await coreSetup.getStartServices();
const [, { encryptedSavedObjects, spaces }] = await coreSetup.getStartServices();

// Retrieve and validate state
const oauthStateClient = new OAuthStateClient({
Expand All @@ -339,16 +339,20 @@ export const oauthCallbackRoute = (
});
}

// Get connector with decrypted secrets
// Get connector with decrypted secrets using the spaceId from the OAuth state
const connectorEncryptedClient = encryptedSavedObjects.getClient({
includedHiddenTypes: ['action'],
});
const namespace =
spaces && oauthState.spaceId
? spaces.spacesService.spaceIdToNamespace(oauthState.spaceId)
: undefined;
const rawAction = await connectorEncryptedClient.getDecryptedAsInternalUser<{
actionTypeId: string;
name: string;
config: OAuthConnectorConfig;
secrets: OAuthConnectorSecrets;
}>('action', oauthState.connectorId);
}>('action', oauthState.connectorId, { namespace });

const config = rawAction.attributes.config;
const secrets = rawAction.attributes.secrets;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ export function setupSavedObjects(
'redirectUri',
'authorizationUrl',
'scope',
'spaceId',
Copy link
Copy Markdown
Contributor Author

@lorenabalan lorenabalan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In light of @jeramysoucy's comment here, I'm thinking we could potentially drop

  • redirectUri, authorizationUrl, scope - they're already validated/enforced by the OAuth provider, so including them in AAD wouldn't necessarily provide an additional security benefit?
  • expiresAt - metadata; here I have mixed feelings cause I see CONNECTOR_TOKEN_SAVED_OBJECT_TYPE does include it in AAD so there is a precedent

Thoughts? @seanstory @jcger
Can address it in a separate PR though, to have clear scope separation. Same place where I could address #246655 (comment)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm outside my area of expertise, but reading the docs Jeramy linked, I see:

AAD is constructed of key-value pairs composed of the Saved Object Descriptor and any attributes included in attributesToIncludeInAAD when the ESO is registered. The Saved Object Descriptor consists of the object type, ID, and, if applicable, namespace (or space ID).

which reads to me like spaceId doesn't need to be in attributesToIncludeInAAD, because it's automatically in AAD?

I'm thinking we could potentially drop

I think we should drop scope, as it's not crazy for that to change (adding new scopes to the connector). The rest I'd expect to stay static though.

Copy link
Copy Markdown
Contributor Author

@lorenabalan lorenabalan Feb 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah but then it says

The descriptor for space-agnostic types (namespaceType: 'agnostic'), and multi-namespace types (namespaceType: 'multiple-isolated' and namespaceType: 'multiple'), will not include a namespace.

Which is our case here. My understanding is we store oauth_state globally via agnostic.

But even then I think they represent semantically different things:

  • spaceId from descriptor would be where oauth_state SO is stored
  • spaceId we store would be the space where authz flow was started from (which should the same as the space where the action SO exists)

So I would think we want to keep spaceId in AAD.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the docs

AAD, or Additional Authenticated Data, is part of an "Authenticated Encryption" schema. AAD is an unencrypted string that is used during encryption and decryption operations in addition to the supplied encryption key, to protect access to encrypted values. If AAD is used during encryption, it must be provided during decryption, and must be an exact match to the AAD value used during encryption, otherwise decryption will fail

I'm also no expert here, but if AAD it's being used to encrypt, decrypt, I would think we want to keep redirectUri, spaceId, state (IIUC we do not update this) and connectorId

Now, I'm unsure about this:

scope and createdBy, but the docs mention that these are good candidates to be excluded because they are optional. So, out.

Regarding expiresAt, I think it's part of the connector token SO because it's being used to have an immutable timestamp bound to AAD, so maybe we can use it too, or maybe createdAt instead?

'createdAt',
'expiresAt',
'createdBy',
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ export const rawOAuthStateSchema = schema.object({
redirectUri: schema.string(),
scope: schema.maybe(schema.string()),
kibanaReturnUrl: schema.string(), // in case of OAuth success, redirect to this URL
spaceId: schema.string(), // the space where the connector exists
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This SO was introduced in the feature branch for the first time, so no need to add a migration here.

createdAt: schema.string(),
expiresAt: schema.string(),
createdBy: schema.maybe(schema.string()),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import type { EncryptedSavedObjectsService } from '../../server/crypto';
import * as EncryptedSavedObjectsModule from '../../server/saved_objects';

// This will only change if new ESOs are introduced. This number should never get smaller.
export const ESO_TYPES_COUNT = 19 as const;
export const ESO_TYPES_COUNT = 20 as const;

describe('checking changes on all registered encrypted SO types', () => {
let esServer: TestElasticsearchUtils;
Expand Down Expand Up @@ -70,13 +70,14 @@ describe('checking changes on all registered encrypted SO types', () => {
"alert": "d961ff113e2b7995a49483b8937fcbdccfe425ac82b59a050931cd620b043ed1",
"api_key_pending_invalidation": "ce3641d95c31bcc2880a294f0123060dcc5026f0a493befdda74924a7ea5c4a0",
"cloud-connect-api-key": "8c0ae7a780c411145ae4aaf7a70235672c9ccfb56d011c322da3c4eeb258f32d",
"connector_token": "16ca2154c13c5ee3d3a45b55d4ea6cd33aeaceaef3dc229b002d25470bfc9b3b",
"connector_token": "e446f5ff0fbf516f63398e474f126332b4c31e316daa613c6cb8c863400110c5",
"entity-discovery-api-key": "cd3b5230a513d2d3503583223e48362fbbbc7812aa4710579a62acfa5bbc30e6",
"fleet-fleet-server-host": "3b8d0809aaf8a133596307bc29328207c7ceee1dc72233da75141ec47ad8d327",
"fleet-message-signing-keys": "5cdcf6bf85247267f8876bda4226e871dbfefe01f050e898db7cbc267d57a275",
"fleet-uninstall-tokens": "6e7d75921dcce46e566f175eab1b0e3825fe565f20cdb3c984e7037934d61e23",
"ingest-download-sources": "23eb3cf789fe13b4899215c6f919705b8a44b89f8feba7181e1f5db3c7699d40",
"ingest-outputs": "d66716d5333484a25c57f7917bead5ac2576ec57a4b9eb61701b573f35ab62ad",
"oauth_state": "a7f039d0d00612a4228213d3983c86245c09ef7683efed147ea0b3511cfa2f66",
"privmon-api-key": "7d7b76b3bc5287a784518731ba66d4f761052177fc04b1a85e5605846ab9de42",
"synthetics-monitor": "f1c060b7be3b30187c4adcb35d74f1fa8a4290bd7faf04fec869de2aa387e21b",
"synthetics-monitor-multi-space": "39c4c6abd28c4173f77c1c89306e92b6b92492c0029274e10620a170be4d4a67",
Expand Down Expand Up @@ -125,6 +126,7 @@ describe('checking changes on all registered encrypted SO types', () => {
"alert|1",
"api_key_pending_invalidation|1",
"cloud-connect-api-key|1",
"connector_token|2",
"connector_token|1",
"fleet-fleet-server-host|2",
"fleet-fleet-server-host|1",
Expand All @@ -138,6 +140,7 @@ describe('checking changes on all registered encrypted SO types', () => {
"ingest-outputs|3",
"ingest-outputs|2",
"ingest-outputs|1",
"oauth_state|1",
"synthetics-monitor|2",
"synthetics-monitor|1",
"task|7",
Expand Down