Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. #82817

azasypkin · 2020-11-06T10:49:15Z

See the detailed description of the problem in #78260. But in short the idea is following:

PKI and Kerberos should always try to initiate a new session for a non-API requests that require authentication.
PKI and Kerberos should NOT try to initiate a new session for an API requests unless there is a session associated with them.

When we re-initiate session because of expired access/refresh token it doesn't matter if it's an API request or not since the user-facing session is still valid and the fact that we're internally using tokens is just an implementation detail. Same for requests that don't require authentication, if we have a valid Kibana session and we can re-acquire tokens without user involvement then we should do it.

Fixes: #78260

Release note: previously API request could create a new PKI or Kerberos session without user even realizing it. With this fix we use a bit more sophisticated heuristic to know when a new session is really desired.

azasypkin · 2020-11-06T10:59:00Z

.github/CODEOWNERS

@@ -253,9 +253,6 @@
 /x-pack/test/ui_capabilities/ @elastic/kibana-security
 /x-pack/test/encrypted_saved_objects_api_integration/ @elastic/kibana-security
 /x-pack/test/functional/apps/security/ @elastic/kibana-security
-/x-pack/test/kerberos_api_integration/ @elastic/kibana-security


note: just continue moving the remaining tests whenever I have chance.

azasypkin · 2020-11-06T14:02:28Z

x-pack/plugins/security/server/authentication/providers/kerberos.ts

@@ -75,11 +77,8 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
      return AuthenticationResult.notHandled();
    }

-    let authenticationResult = authorizationHeader


note: I'm not sure why we weren't checking the canStartNewSession here before, but I cannot find any reason why we shouldn't do it now. And we also should give a higher priority to the current session as well.

azasypkin · 2020-11-06T14:05:12Z

x-pack/plugins/security/server/authentication/providers/pki.test.ts

@@ -399,55 +408,102 @@ describe('PKIAuthenticationProvider', () => {
          LegacyElasticsearchErrorHelpers.decorateNotAuthorizedError(new Error())
        )
        // In response to a call with a new token.
+        .mockResolvedValueOnce(user) // In response to call with an expired token.


note: I'll remove this stupid setup in the scope of #80952 soon.

azasypkin · 2020-11-06T14:08:07Z

x-pack/test/security_api_integration/tests/kerberos/kerberos_login.ts

      });
    });

    describe('finishing SPNEGO', () => {
      it('should properly set cookie and authenticate user', async () => {
        const response = await supertest
-          .get('/internal/security/me')
+          .get('/security/account')


note: recently we started to treat all /internal/.... as API requests (canRedirectRequest === false).

elasticmachine · 2020-11-06T15:46:27Z

Pinging @elastic/kibana-security (Team:Security)

…unauthenticated XHR requests.

azasypkin · 2020-11-06T16:33:54Z

Took the liberty of force-pushing to this branch to resolve conflicts with the most recent master as likely no one is reviewing this yet, but that was the last force-push in this PR 🙈

legrego · 2020-11-06T17:10:08Z

ACK: will review on Monday or Tuesday next week

legrego

LGTM!

legrego · 2020-11-09T14:02:16Z

x-pack/plugins/security/server/authentication/providers/kerberos.ts

@@ -264,12 +267,12 @@ export class KerberosAuthenticationProvider extends BaseAuthenticationProvider {
      return AuthenticationResult.failed(err);
    }

-    // If refresh token is no longer valid, then we should clear session and renegotiate using SPNEGO.
+    // If refresh token is no longer valid, let's check if the request is allowed to start a new session, and if so try


Is this comment still valid? It seems like we don't have to check if the request is allowed to start a new session anymore, since authentication via refresh token is extending an existing session.

You're absolutely right! This looks like a leftover from one of my initial WIPs, will fix, thanks!

kibanamachine · 2020-11-09T16:31:53Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 0670897

Metrics [docs]

✅ unchanged

History

💚 Build #86665 succeeded 1a7f5a1
💚 Build #86452 succeeded 0b6369e
💚 Build #86395 succeeded 1291abb8c52b9d4d0cbe2f60184609e74388bd39
💔 Build #86357 failed 9c6a51c0ecde7a92e9e9be1c037936fbeb7ff74d
💔 Build #86356 failed 78c878f2ee4ca2b8bc06ae9600a4afeef27b0aae

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

…unauthenticated XHR/API requests. (elastic#82817) * Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR requests. * Review#1: fix comment. # Conflicts: # .github/CODEOWNERS # x-pack/scripts/functional_tests.js

azasypkin · 2020-11-09T18:26:02Z

7.x/7.11.0: 129f4df

* master: (39 commits) Fix ilm navigation (elastic#81664) [Lens] Distinct icons for XY and pie chart value labels toolbar (elastic#82927) [data.search.aggs] Throw an error when trying to create an agg type that doesn't exist. (elastic#81509) Index patterns api - load field list on server (elastic#82629) New events resolver (elastic#82170) [App Search] Misc naming tech debt (elastic#82770) load empty_kibana in test to have clean starting point (elastic#82772) Remove data <--> expressions circular dependencies. (elastic#82685) Update 8.0 breaking change template to gather information on how to programmatically detect it. (elastic#82905) Add alerting as codeowners to related documentation folder (elastic#82777) Add captions to user and space grid pages (elastic#82713) add alternate path for x-pack/Cloud test for Lens (elastic#82634) Uses asCurrentUser in getClusterUuid (elastic#82908) [Alerting][Connectors] Add new executor subaction to get 3rd party case fields (elastic#82519) Fix test import objects (elastic#82767) [ML] Add option for anomaly charts for metric detector should plot min, mean or max as appropriate (elastic#81662) Update alert type selection layout to rows instead of grid (elastic#73665) Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. (elastic#82817) Update grunt and related packages (elastic#79327) Allow the repository to search across all namespaces (elastic#82863) ...

azasypkin added release_note:fix Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Security/Authentication Platform Security - Authentication v7.11.0 labels Nov 6, 2020

azasypkin commented Nov 6, 2020

View reviewed changes

azasypkin force-pushed the issue-78260-new-session-pki-kerberos branch 3 times, most recently from 9430012 to 1291abb Compare November 6, 2020 13:53

azasypkin commented Nov 6, 2020

View reviewed changes

azasypkin marked this pull request as ready for review November 6, 2020 15:46

azasypkin requested a review from a team as a code owner November 6, 2020 15:46

Prevent Kerberos and PKI providers from initiating a new session for …

0b6369e

…unauthenticated XHR requests.

azasypkin force-pushed the issue-78260-new-session-pki-kerberos branch from 1291abb to 0b6369e Compare November 6, 2020 16:30

legrego self-requested a review November 6, 2020 17:10

Merge branch 'master' into issue-78260-new-session-pki-kerberos

1a7f5a1

legrego approved these changes Nov 9, 2020

View reviewed changes

azasypkin added 2 commits November 9, 2020 15:27

Merge branch 'master' into issue-78260-new-session-pki-kerberos

cb9b05c

Review#1: fix comment.

0670897

azasypkin merged commit 45ddd69 into elastic:master Nov 9, 2020

azasypkin deleted the issue-78260-new-session-pki-kerberos branch November 9, 2020 16:34

azasypkin added the v8.0.0 label Nov 9, 2020

azasypkin mentioned this pull request Nov 9, 2020

[7.x] Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. (#82817) #82977

Merged

azasypkin added the backported label Nov 9, 2020

n0othing mentioned this pull request Mar 3, 2021

Support PKI auth against Kibana APIs #93452

Open

mbarretta mentioned this pull request Apr 7, 2021

[Docs] Update Metricbeat and Filebeat documentation to remove mentions of PKI authentication for Kibana APIs elastic/beats#24980

Closed

EamonnTP mentioned this pull request Aug 17, 2021

[DOCS] Remove PKI mentions elastic/beats#27422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. #82817

Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. #82817

azasypkin commented Nov 6, 2020 •

edited

Loading

azasypkin Nov 6, 2020

azasypkin Nov 6, 2020 •

edited

Loading

azasypkin Nov 6, 2020

azasypkin Nov 6, 2020

elasticmachine commented Nov 6, 2020

azasypkin commented Nov 6, 2020

legrego commented Nov 6, 2020

legrego left a comment

legrego Nov 9, 2020

azasypkin Nov 9, 2020

kibanamachine commented Nov 9, 2020

azasypkin commented Nov 9, 2020

Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. #82817

Prevent Kerberos and PKI providers from initiating a new session for unauthenticated XHR/API requests. #82817

Conversation

azasypkin commented Nov 6, 2020 • edited Loading

azasypkin Nov 6, 2020

Choose a reason for hiding this comment

azasypkin Nov 6, 2020 • edited Loading

Choose a reason for hiding this comment

azasypkin Nov 6, 2020

Choose a reason for hiding this comment

azasypkin Nov 6, 2020

Choose a reason for hiding this comment

elasticmachine commented Nov 6, 2020

azasypkin commented Nov 6, 2020

legrego commented Nov 6, 2020

legrego left a comment

Choose a reason for hiding this comment

legrego Nov 9, 2020

Choose a reason for hiding this comment

azasypkin Nov 9, 2020

Choose a reason for hiding this comment

kibanamachine commented Nov 9, 2020

💚 Build Succeeded

Metrics [docs]

History

azasypkin commented Nov 9, 2020

azasypkin commented Nov 6, 2020 •

edited

Loading

azasypkin Nov 6, 2020 •

edited

Loading