Add API to refresh authc headers and retry ES request when 401 is encountered #120677

pgayvallet · 2021-12-07T20:27:08Z

Summary

~~POC to discuss design and implementation of #108346 (and related to #104893)~~
Fix #108346

Add a new setUnauthorizedErrorHandler public API to ElasticsearchServiceSetup, to allow registering a handler that will be invoked when a scoped ES client returns a 401 for any API call.

The handler can return 2 results: retry or notHandled

When a retry result is returned:
- we update the httpServer's authRequestHeaders bound to the scoped client's request with the returned headers
- we update the scoped ES Client's headers with the returned headers and try the request again
  - if the request fails again, we do not reiterate the process
When a notHandled result is returned:
- The underlying error is surfaced to the ES client's API consumer as if no handler was present
If the handler throws or rejects, we behave as if notHandled was returned.

Usage would look like:

const handler: UnauthorizedErrorHandler = ({ request, error }, toolkit) => {
   const reauthenticationResult = await authenticator.reauthenticate(request, error);
   if (reauthenticationResult.succeeded()) {
     return toolkit.retry({
       authHeaders: reauthenticationResult.authHeaders,
     });
  }
  return toolkit.notHandled();
}

coreSetup.elasticsearch.setUnauthorizedErrorHandler(handler);

pgayvallet

Discussion items and remarks about the design and the proposed implementation

package.json

pgayvallet · 2021-12-08T07:13:20Z

src/core/server/http/types.ts

  getAuthHeaders: GetAuthHeaders;
+  setAuthHeaders: SetAuthHeaders;


As @mshustov already mentioned on the issue, now that we need to update the headers from outside of the http/login lifecycle, it would probably make sense to extract parts of the authc from the http service. Maybe a new auth internal service?

To start simple though, I was thinking of extracting all the Request->AuthState storages from httpServer, or at least to create a higher-level API/storage to keep/isolate them in a single place. We could KISS and keep this new AuthStorage in the http server/service for now, and just expose it on the internal http contract to be used by elasticsearch service.

The AuthStorage would be responsible to keep the state of what is currently stored in 3 different objects:

kibana/src/core/server/http/http_server.ts

Lines 101 to 103 in 815ab04

private readonly authState: AuthStateStorage;

private readonly authRequestHeaders: AuthHeadersStorage;

private readonly authResponseHeaders: AuthHeadersStorage;

pgayvallet · 2021-12-08T07:14:25Z

src/core/server/elasticsearch/types.ts

 export interface ElasticsearchServiceSetup {
+  /**
+   * TODO: doc
+   */
+  setUnauthorizedErrorHandler: (handler: UnauthorizedErrorHandler) => void;


The entrypoint for the new API, to be consumed by the security plugin.

@elastic/kibana-security could you check the signature of the API (and of the UnauthorizedErrorHandler types), and tell us if the API seems acceptable for your needs?

As discussed sync on slack, and explained on the issue's description, the handler currently only allow to update the authRequestHeaders (and not authResponseHeaders or authState)

TL;DR: the API looks good to me, thanks!

As I mentioned in Slack, we can safely ignore authResponseHeaders since we use it only during Kerberos "handshake" that we don't want to perform in this case.

I also confirmed with the ES security team that during refresh the user info (== authState, Kibana Security plugin leverages the core's authState whenever it wants to know who the current user is and what roles they have, it's also a part of public Security plugin API that other plugins rely on) should not change for Token, SAML, OIDC and Kerberos authentication (it's cached in .security-tokens index) which is great.

The only case when user info can potentially change during refresh is PKI authentication since instead of using refresh token (we don't have one for PKI) we exchange existing client certificate to a new token. We know that certificate is the same (otherwise we should have caught that during authentication stage), and the chance that the role mappings have changed during access token lifetime (max 60 minutes) is very low.

Even though it'd be nice to update authState during refresh as well (for the consistency at least), considering all the above it feels safe enough to ignore it assuming it requires significant effort to support (is it correct assessment?).

pgayvallet · 2021-12-08T07:19:12Z

src/core/server/elasticsearch/client/create_transport.ts

+    updateHeaders(headers: AuthHeaders) {
+      this[kHeaders] = { ...this[kHeaders], ...(headers as IncomingHttpHeaders) };
+    }


When the handler returns a retry result with updated authorization headers, I update the Client's headers (using the workaround from @delvedor #108346 (comment))

Note that I currently keep the existing headers and just spread the new ones. I wonder if we should rerun the ClusterClient.getScopedHeaders logic and then totally replace the client's headers instead?

I wonder if we should rerun the ClusterClient.getScopedHeaders logic and then totally replace the client's headers instead?

I don't think it's safe. elasticsearch-js can set client-specific headers. Here it sets headers during the instantiation process.
https://github.com/elastic/elasticsearch-js/blob/main/src/client.ts#L202
https://github.com/elastic/elastic-transport-js/blob/main/src/Transport.ts#L228

Maybe Transport can support updateHeaders API in the future? @delvedor

pgayvallet · 2021-12-08T07:21:47Z

src/core/server/elasticsearch/client/create_transport.ts

+      try {
+        return (await super.request(params, opts)) as TransportResult<any, any>;
+      } catch (e) {
+        if (isUnauthorizedError(e) && unauthorizedErrorHandler) {
+          const result = await unauthorizedErrorHandler(e);
+          if (isRetryResult(result)) {
+            const headers = result.authHeaders;
+            this.updateHeaders(headers);
+            return (await super.request(params, opts)) as TransportResult<any, any>;
+          }
+        }
+        throw e;


This is the logic of the retry mechanism implemented directly into the client's Transport.

pgayvallet · 2021-12-08T07:28:30Z

src/core/server/elasticsearch/client/cluster_client.ts

+    const transportClass = createTransport({
+      getExecutionContext: this.getExecutionContext,
+      unauthorizedErrorHandler: internalErrorHandler,
+    });
+
    const scopedClient = this.rootScopedClient.child({
      headers: scopedHeaders,
+      Transport: transportClass,


Due to each client being bound to a specific request, we're forced to create a new transport class for every scoped client. However I don't think a class is treated differently than any object in javascript, so I don't think this is a danger for memory usage or leak.

As a side note, I think in the final implementation we may want to move the logic related to the scoped client (both the part that was already present such as getScopedHeaders and the new logic introduced in this PR) from ClusterClient to ScopedClusterClient for better isolation of concerns.

so I don't think this is a danger for memory usage or leak.

The connection pool is shared among child clients, so you won't leak file descriptors.

pgayvallet · 2021-12-08T07:37:17Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+  if (!isRealRequest(request)) {
+    return () => toolkit.notHandled();
+  }


This part is to address

Only when request to Elasticsearch is attributed with Authorization header provided by the AuthenticationHandler (the one registered via registerAuth hook). If consumers override client's Authorization header with FakeRequest and alike we shouldn't interfere.
Only when request isn't made by the Security plugin itself (otherwise we can get into infinite recursion 🙂 ). This will be probably solved automatically by the solution for the previous point since we always override Authorization header in security plugin. But if not, maybe we'd need a special TransportRequestOptions option?

from the initial requirements expressed here: #104893 (comment)

@elastic/kibana-security Could you confirm (or infirm) that disabling the handler IIF the request used for the scoped client is not a real request (KibanaRequest) would be sufficient?

If it's not, could you provide the logic we want to use to disable the handler depending on the request? We would ideally have this condition ONLY depends on the request (and not on the state of the request auth headers (httpServer.authRequestHeaders) associated to it. But if required, I can adapt the code to also access the auth storage from this function.

@elastic/kibana-security Could you confirm (or infirm) that disabling the handler IIF the request used for the scoped client is not a real request (KibanaRequest) would be sufficient?

If the proposed solution works for the Kibana Security team, 👍 from me. That was not the simplest logic to implement 😅

Yeah, I believe it should be enough, here is the only place I'm aware of where we'd want to prevent automatic retry logic and we don't use real request there (.asScoped({ headers: { ...request.headers, ...authHeaders } })).

pgayvallet · 2021-12-08T07:46:22Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+  }
+  return async (error) => {
+    try {
+      const result = await handler({ request, error }, toolkit);


I'm not sure this is really an issue, but I need to mention it: we're now keeping a reference to request within this closure/handler, meaning that we keep a reference between the ScopedClusterClient and the associated request. Previously it wasn't the case, as the request was just used to generate the headers via getScopedHeaders and then the reference could be GC'ed as the end of ClusterClient.asScoped.

This means that any reference kept on the instance of the ClusterClient that was created to instantiate the ScopedClusterClient via asScoped will block the GC from disposing of the request object. In theory, it shouldn't be an issue as these scoped clients are supposed to be kept only for the duration of a request (created in the request handler, meaning that it can be disposed at the end of the execution of the handler).

I wonder if we should use a WeakRef internally, as done with WeakMap for AuthHeadersStorage, just to be safe?

Now, if we were to do that, that would we do if the request has been GC'ed when entering the handler? Just return notHandled as we do if !isRealRequest(request)?

In theory, it shouldn't be an issue as these scoped clients are supposed to be kept only for the duration of a request (created in the request handler, meaning that it can be disposed at the end of the execution of the handler).

I think we designed most of our API relying on this condition.

pgayvallet · 2021-12-08T07:55:27Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+      if (isRetryResult(result)) {
+        setAuthHeaders(request, result.authHeaders);
+      }
+      return result;


These 3 lines are probably the most important part. This createInternalErrorHandler function converts the UnauthorizedErrorHandler as registered by our API consumer (e.g security) to the InternalUnauthorizedErrorHandler used by the ES client.

When the client encounter a 401, it will call the internal handler, which will call the registered handler, and then, if the result is a retry, it will update the auth headers (httpServer.authRequestHeaders) before forwarding the result to the ES client.

The ES client, in case of retry, will then update its headers and perform the request again.

pgayvallet · 2021-12-08T08:01:16Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+    try {
+      const result = await handler({ request, error }, toolkit);
+      if (isRetryResult(result)) {
+        setAuthHeaders(request, result.authHeaders);


setAuthHeaders just updates the authRequestHeaders entry for the given request with the returned headers.

I initially adapted AuthHeadersStorage to hold an observable of the AuthHeaders

-- private authHeadersCache = new WeakMap<Request, AuthHeaders>(); ++ private authHeadersCache = new WeakMap<Request, Observable<AuthHeaders>>();

to allow reactively responding to auth headers changes. This would have potentially allowed to have multiple scoped clients bound to the same request to reactively update their headers when any client is updated.

e.g (not working in current implementation)

const client1 = clusterClient.asScoped(request); const client2 = clusterClient.asScoped(request); // causing the 401 scenario because of expired access token const responseReturnedAfterRetry = await client1.search({}); // client2 should ideally already be updated with the new headers and avoid to retrigger the 401 await client2.search({});

However, I couldn't find any other scenario where being able to subscribe to auth headers change would provide any value, so I reverted the change to remove the increased complexity.

WDYT? Do we have a reason/need to change the AuthHeadersStorage implementation to hold an observable instead of only the current value of the auth headers?

Considering #108346 (comment). I'd say we can live with it.

delvedor

Another solution to avoid playing with the internal symbols is to keep track of the headers in your own custom transport and pass them via transport request options when needed.

eg:

class KibanaTransport extends Transport {
 constructor (opts) {
    super(opts)
    this.kibanaHeaders = {}
  }

  async request (params, options = {}) {
    options.headers = { ...options.headers, ...this.kibanaHeaders }
    try {
      return await super.request(params, options)
    } catch (err) {
      // update this.kibanaHeaders if needed
    }
  }
}

pgayvallet · 2021-12-09T10:39:05Z

Another solution to avoid playing with the internal symbols is to keep track of the headers in your own custom transport and pass them via transport request options when needed.

Yea, I thought about this after my initial POC, and that's probably the way to go. We'll need to remove the headers passed in the constructor to the parent's call though

constructor (opts) {
    const { headers, ...superOpts } = opts
    super(superOpts)
    this.kibanaHeaders = headers ?? {};
  }

but this avoid messing with internals of the Client, so that's probably way more elegant.

mshustov · 2021-12-09T14:51:11Z

src/core/server/http/types.ts

@@ -399,6 +399,7 @@ export interface InternalHttpServiceSetup
  registerRouterAfterListening: (router: IRouter) => void;
  registerStaticDir: (path: string, dirPath: string) => void;
  getAuthHeaders: GetAuthHeaders;
+  setAuthHeaders: SetAuthHeaders;


Maybe it is simpler to pass a single AuthHeadersStorage interface instead of passing its methods?
We can continue keeping the Auth service in Http for time being.

Yea, just waiting for @elastic/kibana-security to confirm that only updating the request headers is sufficient (no response headers and no auth state), in which case I agree that introducing a IAuthHeadersStorage interface and passing it down to the ES service seems the most pragmatic solution.

Replied in https://github.com/elastic/kibana/pull/120677/files#r768617303. If updating authState in addition to authRequestHeaders requires significant changes or just too risky, we can move forward with just updating authRequestHeaders.

mshustov · 2021-12-10T12:03:50Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+  setAuthHeaders: SetAuthHeaders;
+}): InternalUnauthorizedErrorHandler => {
+  // we don't want to support 401 retry for fake requests
+  if (!isRealRequest(request)) {


Can we keep this logic isolated in auth service? it rather belongs there.

Well, this is impacting the ES client's behavior, as we need to totally disable the logic and not retry the 401 in case the provided request is not a 'real' one, so I don't really see a way to isolate this in the auth/http service.

We could eventually just check perform this check in ClusterClient.asScoped, and, if it's a fake request, just not create the handler at all, but this will be be done inside the elasticsearch's service code.

Or do you have a suggestion?

I was thinking about something as simple as:

class Auth { canBeReAuthenticated(request) { return isRealRequest(request) } }

mshustov · 2021-12-10T12:07:59Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+  }
+  return async (error) => {
+    try {
+      const result = await handler({ request, error }, toolkit);


In theory, it shouldn't be an issue as these scoped clients are supposed to be kept only for the duration of a request (created in the request handler, meaning that it can be disposed at the end of the execution of the handler).

I think we designed most of our API relying on this condition.

mshustov · 2021-12-10T12:15:32Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+    try {
+      const result = await handler({ request, error }, toolkit);
+      if (isRetryResult(result)) {
+        setAuthHeaders(request, result.authHeaders);


Considering #108346 (comment). I'd say we can live with it.

mshustov · 2021-12-10T12:22:18Z

src/core/server/elasticsearch/client/cluster_client.ts

    this.allowListHeaders = ['x-opaque-id', ...this.config.requestHeadersWhitelist];
  }

  asScoped(request: ScopeableRequest) {
    const scopedHeaders = this.getScopedHeaders(request);
+
+    const internalErrorHandler = this.unauthorizedErrorHandler


Can we create the handler lazily? We really need it only when 401 happened.

We probably can if we want to. Do you have any concern creating is eagerly? It doesn't seems like a costly operation?

It's not super expensive, but asScoped might be considered as a hot path because Security uses the scoped client(s) on every incoming request. But Kibana will need internalErrorHandler handler very rarely only when 401 happens after auth lifecycle.

mshustov · 2021-12-10T12:30:32Z

src/core/server/elasticsearch/client/create_transport.ts

+    updateHeaders(headers: AuthHeaders) {
+      this[kHeaders] = { ...this[kHeaders], ...(headers as IncomingHttpHeaders) };
+    }


I wonder if we should rerun the ClusterClient.getScopedHeaders logic and then totally replace the client's headers instead?

I don't think it's safe. elasticsearch-js can set client-specific headers. Here it sets headers during the instantiation process.
https://github.com/elastic/elasticsearch-js/blob/main/src/client.ts#L202
https://github.com/elastic/elastic-transport-js/blob/main/src/Transport.ts#L228

Maybe Transport can support updateHeaders API in the future? @delvedor

src/core/server/elasticsearch/client/create_transport.ts

…authc-headers

azasypkin · 2021-12-14T10:13:41Z

ACK: reviewing...

azasypkin

The public core API proposed in this PR should cover what Security plugin would need to refresh access token after authentication stage, I've left a few comments with more details.

Thanks!

azasypkin · 2021-12-14T12:44:28Z

src/core/server/http/types.ts

@@ -399,6 +399,7 @@ export interface InternalHttpServiceSetup
  registerRouterAfterListening: (router: IRouter) => void;
  registerStaticDir: (path: string, dirPath: string) => void;
  getAuthHeaders: GetAuthHeaders;
+  setAuthHeaders: SetAuthHeaders;


Replied in https://github.com/elastic/kibana/pull/120677/files#r768617303. If updating authState in addition to authRequestHeaders requires significant changes or just too risky, we can move forward with just updating authRequestHeaders.

…authc-headers

azasypkin · 2021-12-30T09:25:12Z

src/core/server/elasticsearch/client/retry_unauthorized.ts

+  }
+  return async (error) => {
+    try {
+      const result = await handler({ request, error }, toolkit);


question: I see that right now the request that is being passed to handler contains auth headers (Authorization: xxx) that were set by the Core and weren't present in the original request. Would it be possible to "unset" these headers before passing request object to the handler?

Right now Security basically skips such requests since it assumes that it was the client who specified authentication headers explicitly. We can try to workaround this in Security plugin and unset these headers on our own (not sure if it's possible since headers is readonly), but I'm wondering if you have a better way of passing original "pristine" request object instead?

Update: that actually happens because we mutate request with Authorization header after authentication stage for BWC reasons. Hmmm, I'll experiment with this a bit more to see if there anything we can do on our end.

Did you progress on your investigations?

As you said, the mutation is done here:

kibana/src/core/server/http/http_server.ts

Lines 447 to 452 in 8ffc872

if (requestHeaders) {

this.authRequestHeaders.set(req, requestHeaders);

// we mutate headers only for the backward compatibility with the legacy platform.

// where some plugin read directly from headers to identify whether a user is authenticated.

Object.assign(req.headers, requestHeaders);

}

But I don't see any way to differentiate headers that have been added by this snippet from other headers, so apart from just removing this part of the code (which would likely break some stuffs relying on these headers to forge fake requests, right?) I'm not sure how we could answer your ask?

Did you progress on your investigations?

I can think of only one clumsy hack on Security side: https://github.com/elastic/kibana/pull/122155/files#diff-df4d732995701a873f86257177d6440fa562a820a0c849624cc1e54de2bbfbf1R238-R244 (Object.fromEntries(Object.entries(...)) is needed because I cannot just remove authorization header from the headers object since it's readonly).

(request.headers as Record<string, unknown>) = Object.fromEntries( Object.entries(request.headers).filter( ([headerName]) => headerName.toLowerCase() !== 'authorization' ) );

so apart from just removing this part of the code (which would likely break some stuffs relying on these headers to forge fake requests, right?

Yeah, reporting (and probably Upgrade Assistant in some cases) still depends on this I believe.

But I don't see any way to differentiate headers that have been added by this snippet from other headers

I was thinking that you might have a copy of the auth headers returned during authentication stage so that you know which headers are original and which were added as a part of the authentication flow, or it's not the case?

I was thinking that you might have a copy of the auth headers returned during authentication stage so that you know which headers are original and which were added as a part of the authentication flow, or it's not the case?

Nope, not really.

Well, we do set the headers returned from the auth hook inside the authRequestHeaders map, so in a way, we can identify which entries in req.headers were injected by looking at the keys of the associated entry in authRequestHeaders, but:

This will be quite ugly if we do that from the ES error interception logic. We're already leaking responsibilities all over the place, and it will just increase it.

In case of overrides (e.g a header returned from the login hook overriding a header initially present on the request), we won't be able to provide the initial value of such headers.

KibanaRequest is immutable, and we won't be able to easily recreate one to provide it to the hook

In addition, the login HAPI hook is performed very early, before we actually wrap HAPI's request to a KibanaRequest, so if we want to store these original headers somewhere, we'll need to do that (also) directly on the base request. We would be forced to do something like

diff --git a/src/core/server/http/http_server.ts b/src/core/server/http/http_server.ts --- a/src/core/server/http/http_server.ts (revision f6370f413172821ea8e06dfc0d80ff5b5a6e90bd) +++ b/src/core/server/http/http_server.ts (date 1641814909929) @@ -448,6 +448,7 @@ this.authRequestHeaders.set(req, requestHeaders); // we mutate headers only for the backward compatibility with the legacy platform. // where some plugin read directly from headers to identify whether a user is authenticated. + req.app.originalHeaders = { ...req.headers }; Object.assign(req.headers, requestHeaders); } }

and to expose these originalHeaders as a new property of KibanaRequest.

This is doable, but I would take any other alternative if possible to be honest.

@mshustov wdyt?

This will be quite ugly if we do that from the ES error interception logic. We're already leaking responsibilities all over the place, and it will just increase it.

Yeah, I see. It feels like there is no way to avoid an ugly workaround of some sort, it's just a matter where it will live. I'm fine with keeping it in Security if there is no elegant way to accomplish this directly in the Core.

In case of overrides (e.g a header returned from the login hook overriding a header initially present on the request), we won't be able to provide the initial value of such headers.

It's something that should never happen, at least Security will never override existing headers.

KibanaRequest is immutable, and we won't be able to easily recreate one to provide it to the hook

If we agree to keep workaround (e.g. rewriting request.headers), is it safe to assume that request.headers will stay writable?

is it safe to assume that request.headers will stay writable?

KibanaRequest.headers is defined as readonly

kibana/src/core/server/http/router/request.ts

Line 156 in 338fe1a

public readonly headers: Headers;

and its value is frozen

kibana/src/core/server/http/router/request.ts

Line 198 in 338fe1a

this.headers = deepFreeze({ ...request.headers });

however as discussed sync, the request.headers property is not technically frozen and can be overriden if forcecasted.

…authc-headers

elasticmachine · 2022-01-10T14:33:47Z

Pinging @elastic/kibana-core (Team:Core)

azasypkin

The API shape looks good to me, the WIP base integration test suite is passing with this PR as well.

As we agreed, we'll be filtering out authorization header from the request instance before re-authentication in the Security plugin for now. Once we stop mutating requests with the headers returned at the authentication stage, we'll be able to get rid of this workaround.

Thanks!

…authc-headers

mshustov

The overall design looks good. Is it possible to add an integration test to make sure everything works as expected?

src/core/server/elasticsearch/client/retry_unauthorized.ts

mshustov · 2022-01-18T10:45:11Z

src/core/server/elasticsearch/client/create_transport.ts

+      // add stored headers to the options
+      opts.headers = {
+        ...this.headers,
+        ...optionHeaders,


nit: we copy opts.headers twice.

Suggested change

...optionHeaders,

...opts.headers,

I had incredible issues in unit tests because of the way the client keep references / mutates the headers that forced me to do that to assert against the values of the used headers for each individual call. but I will try again.

mshustov · 2022-01-18T10:50:40Z

src/core/server/elasticsearch/client/create_transport.test.ts

+        );
+      });
+
+      it('merges the headers from the constructor and from the options', async () => {


could you add a test that request method doesn't mutate the arguments?

pgayvallet · 2022-01-18T11:09:35Z

Is it possible to add an integration test to make sure everything works as expected?

We talked about it with @azasypkin and agreed that it would make more sense (and would be way easier) to add such tests in the follow-up PR from the security team that will actually leverage the feature. The FTR tests are ready (and passing) in #122155

pgayvallet · 2022-01-18T12:20:20Z

Will be backported as it is required to address #104893 that is a bug fix

kibana-ci · 2022-01-18T13:33:51Z

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id	before	after	diff
`core`	953	968	+15

Unknown metric groups

API count

id	before	after	diff
`core`	2333	2353	+20

History

💚 Build #17840 succeeded 78bee88
💔 Build #17809 failed 782e1b0
💚 Build #16359 succeeded f6370f4
💚 Build #14988 succeeded a40d41d
💚 Build #14820 succeeded a067f4e
💔 Build #14803 failed 0a96665

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

kibanamachine · 2022-01-18T13:43:15Z

💔 All backports failed

Status	Branch	Result
❌	8.0	Backport failed because of merge conflicts

How to fix

Re-run the backport manually:

node scripts/backport --pr 120677

Questions ?

Please refer to the Backport tool documentation

…ountered (elastic#120677) * initial POC * remove test code * update the header holding logic * add new API to plugin context * introduce the IAuthHeadersStorage interface * fix some types, mocks and tests * export types from server entrypoint * also export error type * more doc * update generated doc * Fix ES service tests * add tests for createInternalErrorHandler * fix type in cli_setup * generated doc * add tests for configureClient * add unit tests for custom transport class * fix handler propagation to initial clients * lint * address review comments (cherry picked from commit b606054) # Conflicts: # src/core/server/http/auth_headers_storage.ts

…ountered (elastic#120677) * initial POC * remove test code * update the header holding logic * add new API to plugin context * introduce the IAuthHeadersStorage interface * fix some types, mocks and tests * export types from server entrypoint * also export error type * more doc * update generated doc * Fix ES service tests * add tests for createInternalErrorHandler * fix type in cli_setup * generated doc * add tests for configureClient * add unit tests for custom transport class * fix handler propagation to initial clients * lint * address review comments (cherry picked from commit b606054) # Conflicts: # src/cli_setup/utils.ts # src/core/server/elasticsearch/client/cluster_client.ts # src/core/server/elasticsearch/client/configure_client.ts # src/core/server/elasticsearch/client/errors.ts # src/core/server/elasticsearch/elasticsearch_service.ts # src/core/server/elasticsearch/index.ts # src/core/server/http/auth_headers_storage.ts # src/core/server/index.ts # src/core/server/server.api.md

pgayvallet · 2022-01-18T14:42:36Z

Tried to backport to 7.17, but the 7.x ES client is still using their custom TransportRequestPromise with additional properties, making it impossible for our custom transport to try/catch and retry the requests, as this would result on returning vanilla promises without the additional abort property.

…is encountered (#120677) (#123227) * Add API to refresh authc headers and retry ES request when 401 is encountered (#120677) * initial POC * remove test code * update the header holding logic * add new API to plugin context * introduce the IAuthHeadersStorage interface * fix some types, mocks and tests * export types from server entrypoint * also export error type * more doc * update generated doc * Fix ES service tests * add tests for createInternalErrorHandler * fix type in cli_setup * generated doc * add tests for configureClient * add unit tests for custom transport class * fix handler propagation to initial clients * lint * address review comments (cherry picked from commit b606054) # Conflicts: # src/core/server/http/auth_headers_storage.ts * try to please typescript

pgayvallet added 2 commits December 7, 2021 21:25

initial POC

dd6f9b6

remove test code

815ab04

pgayvallet commented Dec 8, 2021

View reviewed changes

pgayvallet requested review from azasypkin, a team and mshustov December 8, 2021 08:02

pgayvallet mentioned this pull request Dec 8, 2021

Provide API to refresh token after authc lifecycle #108346

Closed

delvedor reviewed Dec 9, 2021

View reviewed changes

mshustov reviewed Dec 10, 2021

View reviewed changes

pgayvallet added 3 commits December 13, 2021 15:18

Merge remote-tracking branch 'upstream/main' into kbn-108346-refresh-…

17d172d

…authc-headers

update the header holding logic

307f187

add new API to plugin context

b941c8a

azasypkin reviewed Dec 14, 2021

View reviewed changes

pgayvallet added 4 commits December 22, 2021 10:39

Merge remote-tracking branch 'upstream/main' into kbn-108346-refresh-…

e9114ab

…authc-headers

introduce the IAuthHeadersStorage interface

8ffc872

fix some types, mocks and tests

ae4fd7c

export types from server entrypoint

5ecbe36

pgayvallet added Feature:elasticsearch release_note:skip Skip the PR/issue when compiling release notes Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc v8.1.0 labels Dec 22, 2021

pgayvallet added 6 commits December 22, 2021 14:29

also export error type

a1e1091

more doc

8b6f7df

update generated doc

f02ca13

Fix ES service tests

0a96665

add tests for createInternalErrorHandler

8194c62

fix type in cli_setup

e4e8633

azasypkin reviewed Dec 30, 2021

View reviewed changes

pgayvallet added 2 commits January 10, 2022 11:24

Merge remote-tracking branch 'upstream/main' into kbn-108346-refresh-…

0f655a3

…authc-headers

fix handler propagation to initial clients

f6370f4

pgayvallet marked this pull request as ready for review January 10, 2022 14:33

pgayvallet requested review from a team, mshustov and azasypkin January 10, 2022 14:33

azasypkin approved these changes Jan 14, 2022

View reviewed changes

pgayvallet added 2 commits January 17, 2022 15:17

Merge remote-tracking branch 'upstream/main' into kbn-108346-refresh-…

782e1b0

…authc-headers

lint

78bee88

mshustov approved these changes Jan 18, 2022

View reviewed changes

address review comments

d4172c5

pgayvallet added the v8.0.0 label Jan 18, 2022

pgayvallet added the auto-backport Deprecated - use backport:version if exact versions are needed label Jan 18, 2022

pgayvallet merged commit b606054 into elastic:main Jan 18, 2022

pgayvallet mentioned this pull request Jan 18, 2022

[8.0] Add API to refresh authc headers and retry ES request when 401 is encountered (#120677) #123227

Merged

pgayvallet added v7.17.0 and removed v7.17.0 labels Jan 18, 2022

pgayvallet mentioned this pull request Jan 18, 2022

[7.17] Add API to refresh authc headers and retry ES request when 401 is encountered (#120677) #123230

Closed

		getAuthHeaders: GetAuthHeaders;
		setAuthHeaders: SetAuthHeaders;

	private readonly authState: AuthStateStorage;
	private readonly authRequestHeaders: AuthHeadersStorage;
	private readonly authResponseHeaders: AuthHeadersStorage;

	if (requestHeaders) {
	this.authRequestHeaders.set(req, requestHeaders);
	// we mutate headers only for the backward compatibility with the legacy platform.
	// where some plugin read directly from headers to identify whether a user is authenticated.
	Object.assign(req.headers, requestHeaders);
	}

Add API to refresh authc headers and retry ES request when 401 is encountered #120677

Add API to refresh authc headers and retry ES request when 401 is encountered #120677

Conversation

pgayvallet commented Dec 7, 2021 • edited Loading

Summary

pgayvallet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Dec 8, 2021 • edited Loading

Choose a reason for hiding this comment

azasypkin Dec 14, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

delvedor left a comment

Choose a reason for hiding this comment

pgayvallet commented Dec 9, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mshustov Dec 10, 2021 • edited Loading

Choose a reason for hiding this comment

azasypkin commented Dec 14, 2021

azasypkin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azasypkin Dec 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet Jan 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Jan 10, 2022

azasypkin left a comment

Choose a reason for hiding this comment

mshustov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgayvallet commented Jan 18, 2022

pgayvallet commented Jan 18, 2022

kibana-ci commented Jan 18, 2022

💚 Build Succeeded

Metrics [docs]

Public APIs missing comments

API count

History

kibanamachine commented Jan 18, 2022

💔 All backports failed

How to fix

Questions ?

pgayvallet commented Jan 18, 2022

pgayvallet commented Dec 7, 2021 •

edited

Loading

pgayvallet Dec 8, 2021 •

edited

Loading

azasypkin Dec 14, 2021 •

edited

Loading

mshustov Dec 10, 2021 •

edited

Loading

pgayvallet Dec 10, 2021 •

edited

Loading

mshustov Dec 10, 2021 •

edited

Loading

azasypkin Dec 30, 2021 •

edited

Loading

pgayvallet Jan 10, 2022 •

edited

Loading