[Connector] Adding internal route for requesting ad-hoc ServiceNow access token

[ymao1]

Towards #130306

Summary

Addresses item 6 from #130306: Expose new actions API HTTP route, which allow to fetch an access token for not saved connectors.

This PR adds an internal route to request an ad-hoc access token from a ServiceNow instance without requiring a saved connector. This allows the Create ServiceNow connector UI to request information from the api/x_elas2_inc_int/elastic_api/health endpoint during the ServiceNow connector creation process.

To Verify

1. Run this PR and submit the following POST request:

POST https://localhost:5601/internal/actions/connector/_servicenow_access_token
{
  "apiUrl": "https://ven04334.service-now.com/",
  "config": {
    "clientId": <clientId>,
    "userIdentifierValue": "elastic_integration",
    "jwtKeyId": <jwtKeyId>
  },
  "secrets": {
    "clientSecret": <clientSecret>,
    "privateKey": <privateKey>
  }
}

2. Verify you get an access token in response in the form: Bearer <accessToken>

Checklist

• Unit or functional tests were updated or added to match the most common scenarios

[mikecote]

I finished my review, the overall structure looks great but I have two questions before giving it a 👍

[jportner]

@jportner We are adding this internal endpoint for generating ad-hoc access tokens for querying the health API for ServiceNow instances. Wanted to make sure @elastic/kibana-security was aware and wondering if there are any concerns y'all might have.

Thanks for the ping, I'll review today

[jportner]

I responded to one of Mike's comments, other than that I don't have any concerns about this PR.

I think the overall approach makes sense 👍

There's an SSRF risk here, but I think it's pretty minimal, with the combination of:

• Authorization checks (as suggested in one of the comments above)
• The optional hostname allow-list for connectors
• Additional URL validation (as suggested in my comment below)
• The inability for users to specify arbitrary parameters with the token request
• The logic which does not return the response body to users, it parses the response from the remote server and only returns the access token to users

In reviewing this, I noticed a couple small problems outside of the scope of this PR:

1. URL validation

Here's the implementation for the tokenUrl validation:

kibana/x-pack/plugins/actions/server/actions_config.ts

Lines 70 to 84 in 0998b67

	function isAllowed({ allowedHosts }: ActionsConfig, hostname: string | null): boolean {
	const allowed = new Set(allowedHosts);
	if (allowed.has(AllowedHosts.Any)) return true;
	if (hostname && allowed.has(hostname)) return true;
	return false;
	}
	function isHostnameAllowedInUri(config: ActionsConfig, uri: string): boolean {
	return pipe(
	tryCatch(() => url.parse(uri)),
	map((parsedUrl) => parsedUrl.hostname),
	mapNullable((hostname) => isAllowed(config, hostname)),
	getOrElse<boolean>(() => false)
	);
	}

This code ignores network-path references (see URL Confusion Vulnerabilities). In other words, a URL of //my-domain.com/foo would be parsed and the validation function would treat as if it has no hostname, but it actually has a hostname of my-domain.com (in terms of how HTTP clients typically behave).

Fortunately the validation function fails securely; if an allow-list is configured and a URL doesn't contain a hostname, it fails to validate. However, I think you should change the usage of url.parse to include the slashesDenoteHost parameter:

diff --git a/x-pack/plugins/actions/server/actions_config.ts b/x-pack/plugins/actions/server/actions_config.ts
index 35e08bb5cfe..49f1d1fd544 100644
--- a/x-pack/plugins/actions/server/actions_config.ts
+++ b/x-pack/plugins/actions/server/actions_config.ts
@@ -76,7 +76,7 @@ function isAllowed({ allowedHosts }: ActionsConfig, hostname: string | null): bo
 
 function isHostnameAllowedInUri(config: ActionsConfig, uri: string): boolean {
   return pipe(
-    tryCatch(() => url.parse(uri)),
+    tryCatch(() => url.parse(uri, false /* parseQueryString */, true /* slashesDenoteHost */)),
     map((parsedUrl) => parsedUrl.hostname),
     mapNullable((hostname) => isAllowed(config, hostname)),
     getOrElse<boolean>(() => false)

2. JWT assertions

The code to create JWT assertions allows for consumers to specify custom claims that would override the existing claims (subject, audience, issuer, etc.):

kibana/x-pack/plugins/actions/server/builtin_action_types/lib/create_jwt_assertion.ts

Lines 19 to 38 in 53272c6

	export function createJWTAssertion(
	logger: Logger,
	privateKey: string,
	privateKeyPassword: string | null,
	reservedClaims: JWTClaims,
	customClaims?: Record<string, string>
	): string {
	const { subject, audience, issuer, expireInMilliseconds, keyId } = reservedClaims;
	const iat = Math.floor(Date.now() / 1000);
	const headerObj = { algorithm: 'RS256' as Algorithm, ...(keyId ? { keyid: keyId } : {}) };
	const payloadObj = {
	sub: subject, // subject claim identifies the principal that is the subject of the JWT
	aud: audience, // audience claim identifies the recipients that the JWT is intended for
	iss: issuer, // issuer claim identifies the principal that issued the JWT
	iat, // issued at claim identifies the time at which the JWT was issued
	exp: iat + (expireInMilliseconds ?? 3600), // expiration time claim identifies the expiration time on or after which the JWT MUST NOT be accepted for processing
	...(customClaims ?? {}),
	};

It doesn't appear that this customClaims option is being used anywhere. I'm not sure why it was added, but IMO if you want to keep it you should ensure that it can't be used as a vector to overwrite the other claims:

diff --git a/x-pack/plugins/actions/server/builtin_action_types/lib/create_jwt_assertion.ts b/x-pack/plugins/actions/server/builtin_action_types/lib/create_jwt_assertion.ts
index b33a2d17ed9..2fcb17c1e18 100644
--- a/x-pack/plugins/actions/server/builtin_action_types/lib/create_jwt_assertion.ts
+++ b/x-pack/plugins/actions/server/builtin_action_types/lib/create_jwt_assertion.ts
@@ -29,12 +29,12 @@ export function createJWTAssertion(
   const headerObj = { algorithm: 'RS256' as Algorithm, ...(keyId ? { keyid: keyId } : {}) };
 
   const payloadObj = {
+    ...(customClaims ?? {}),
     sub: subject, // subject claim identifies the principal that is the subject of the JWT
     aud: audience, // audience claim identifies the recipients that the JWT is intended for
     iss: issuer, // issuer claim identifies the principal that issued the JWT
     iat, // issued at claim identifies the time at which the JWT was issued
     exp: iat + (expireInMilliseconds ?? 3600), // expiration time claim identifies the expiration time on or after which the JWT MUST NOT be accepted for processing
-    ...(customClaims ?? {}),
   };
 
   try {

This will prevent us from accidentally introducing a vulnerability in the future 😄

[ymao1]

@jportner Thanks for reviewing! I've pushed changes based on your feedback:

1. URL validation

Added slashesDenoteHost in this commit: 769176f

2. JWT assertions

Removed ability to specify customClaims in this commit: b02b197. As you said, we are not currently using this so it makes sense to remove and re-add if the need ever comes up in the future.

[mikecote]

Changes LGTM!

[jportner]

LGTM, thanks for the quick turnaround!

[kibana-ci]

💛 Build succeeded, but was flaky

• Buildkite Build
• Commit: 1b0263c
• Storybooks Preview
• Webpack Bundle Analyzer

Failed CI Steps

• FTR Configs #5

Test Failures

• [job] [logs] FTR Configs #5 / analytics instrumented events from the browser Core Context Providers should have the properties provided by the "license info" context provider

Metrics [docs]

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id	before	after	diff
actions	11	12	+1

History

• 💔 Build #43115 failed 52b94de
• 💚 Build #42593 succeeded b065a5c
• 💚 Build #42149 succeeded 68af17d
• 💔 Build #42106 failed dc65c55
• 💚 Build #42036 succeeded d3422a0
• 💚 Build #41732 succeeded ebf4704

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @ymao1