Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow using JWT credentials to grant API keys. #172444

Merged
merged 6 commits into from Dec 4, 2023

Conversation

azasypkin
Copy link
Member

@azasypkin azasypkin commented Dec 4, 2023

Summary

In this PR we:

NOTE: We're not gating this functionality with the config flag (xpack.security.authc.http.jwt.taggedRoutesOnly) as we did for the Serverless offering. It'd be a breaking change as we already implicitly support JWT authentication without client authentication, and to be honest, it's not really necessary anyway.

Testing

Refer to the Testing section in this PR description: #159117.

Or run already pre-configured Kibana functional test server:

  1. node scripts/functional_tests_server.js --config x-pack/test/security_api_integration/api_keys.config.ts
  2. Create a role mapping for JWT user:
curl -X POST --location "http://localhost:9220/_security/role_mapping/jwt" \
    -H "Authorization: Basic ZWxhc3RpYzpjaGFuZ2VtZQ==" \
    -H "Accept: application/json" \
    -H "Content-Type: application/json" \
    -d "{
          \"roles\": [ \"superuser\" ],
          \"enabled\": true,
          \"rules\": { \"all\": [{\"field\" : { \"realm.name\" : \"jwt_with_secret\" }}] }
        }"
  1. Send any Kibana API request with the following credentials:
curl -X POST --location "xxxx"
  -H 'Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL2tpYmFuYS5lbGFzdGljLmNvL2p3dC8iLCJzdWIiOiJlbGFzdGljLWFnZW50IiwiYXVkIjoiZWxhc3RpY3NlYXJjaCIsIm5hbWUiOiJFbGFzdGljIEFnZW50IiwiaWF0Ijo5NDY2ODQ4MDAsImV4cCI6NDA3MDkwODgwMH0.P7RHKZlLskS5DfVRqoVO4ivoIq9rXl2-GW6hhC9NvTSkwphYivcjpTVcyENZvxTTvJJNqcyx6rF3T-7otTTIHBOZIMhZauc5dob-sqcN_mT2htqm3BpSdlJlz60TBq6diOtlNhV212gQCEJMPZj0MNj7kZRj_GsECrTaU7FU0A3HAzkbdx15vQJMKZiFbbQCVI7-X2J0bZzQKIWfMHD-VgHFwOe6nomT-jbYIXtCBDd6fNj1zTKRl-_uzjVqNK-h8YW1h6tE4xvZmXyHQ1-9yNKZIWC7iEaPkBLaBKQulLU5MvW3AtVDUhzm6--5H1J85JH5QhRrnKYRon7ZW5q1AQ'
  -H 'ES-Client-Authentication: SharedSecret my_super_secret'

....for example....
curl -X GET --location "http://localhost:5620/internal/security/me" \
    -H 'Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodHRwczovL2tpYmFuYS5lbGFzdGljLmNvL2p3dC8iLCJzdWIiOiJlbGFzdGljLWFnZW50IiwiYXVkIjoiZWxhc3RpY3NlYXJjaCIsIm5hbWUiOiJFbGFzdGljIEFnZW50IiwiaWF0Ijo5NDY2ODQ4MDAsImV4cCI6NDA3MDkwODgwMH0.P7RHKZlLskS5DfVRqoVO4ivoIq9rXl2-GW6hhC9NvTSkwphYivcjpTVcyENZvxTTvJJNqcyx6rF3T-7otTTIHBOZIMhZauc5dob-sqcN_mT2htqm3BpSdlJlz60TBq6diOtlNhV212gQCEJMPZj0MNj7kZRj_GsECrTaU7FU0A3HAzkbdx15vQJMKZiFbbQCVI7-X2J0bZzQKIWfMHD-VgHFwOe6nomT-jbYIXtCBDd6fNj1zTKRl-_uzjVqNK-h8YW1h6tE4xvZmXyHQ1-9yNKZIWC7iEaPkBLaBKQulLU5MvW3AtVDUhzm6--5H1J85JH5QhRrnKYRon7ZW5q1AQ' \
    -H 'ES-Client-Authentication: SharedSecret my_super_secret' \
    -H "Accept: application/json"
----
{
  "username": "elastic-agent",
  "roles": [
    "superuser"
  ],
  "full_name": null,
  "email": null,
  "metadata": {
    "jwt_claim_sub": "elastic-agent",
    "jwt_token_type": "access_token",
    "jwt_claim_iss": "https://kibana.elastic.co/jwt/",
    "jwt_claim_name": "Elastic Agent",
    "jwt_claim_aud": [
      "elasticsearch"
    ]
  },
  "enabled": true,
  "authentication_realm": {
    "name": "jwt_with_secret",
    "type": "jwt"
  },
  "lookup_realm": {
    "name": "jwt_with_secret",
    "type": "jwt"
  },
  "authentication_type": "realm",
  "authentication_provider": {
    "type": "http",
    "name": "__http__"
  },
  "elastic_cloud_user": false
}

Fixes: #171522


Release note: The default value of the elasticsearch.requestHeadersWhitelist configuration option has been expanded to include the es-client-authentication HTTP header, in addition to authorization.

@azasypkin azasypkin self-assigned this Dec 4, 2023
@azasypkin azasypkin added Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! Feature:Users/Roles/API Keys NeededFor:ResponseOps v8.12.0 labels Dec 4, 2023
@@ -94,7 +94,7 @@ export const configSchema = schema.object({
}),
],
{
defaultValue: ['authorization'],
defaultValue: ['authorization', 'es-client-authentication'],
Copy link
Member Author

Choose a reason for hiding this comment

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

@azasypkin azasypkin marked this pull request as ready for review December 4, 2023 13:28
@azasypkin azasypkin requested review from a team as code owners December 4, 2023 13:28
@elasticmachine
Copy link
Contributor

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

@azasypkin azasypkin added the release_note:skip Skip the PR/issue when compiling release notes label Dec 4, 2023
Copy link
Member

@jbudz jbudz left a comment

Choose a reason for hiding this comment

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

ftr_configs.yml

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Tested locally, LGTM with one docs comment and one test question.

I recommend adding a Release Notes section to this PR description, which includes a note about the change to elasticsearch.requestHeadersWhitelist's default value.

`xpack.security.authc.realms.jwt.jwt_with_secret.pkc_jwkset_path=${jwksPath}`,
`xpack.security.authc.realms.jwt.jwt_with_secret.token_type=access_token`,

// JWT WITHOUT shared secret
Copy link
Member

Choose a reason for hiding this comment

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

🏅 thanks for testing both with and without shared secret

// separate `describe` since `this.tags` only works on a test suite level.
this.tags(['skipMKI']);

it('should properly grant API key (with client authentication)', async function () {
Copy link
Member

Choose a reason for hiding this comment

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

question should we include negative tests to ensure that end-to-end error handling works correctly if we:

  • Fail to provide a ES-Client-Authentication header
  • Provide a malformed ES-Client-Authentication header
  • Provide a well-formed, but incorrect ES-Client-Authentication header
  • Provide a valid ES-Client-Authentication header with a valid ES bearer token instead of a JWT

While this should be handled more or less automatically, we've benefited in the past from these types of "redundant" tests.

Copy link
Member Author

@azasypkin azasypkin Dec 4, 2023

Choose a reason for hiding this comment

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

All these tests will fail during authentication stage, so we'll effectively be testing authentication, not grantApiKey API, but I agree that these tests will be useful, so I added them to a separate test suite. Done in e795826!

@azasypkin azasypkin added release_note:enhancement and removed release_note:skip Skip the PR/issue when compiling release notes labels Dec 4, 2023
@azasypkin
Copy link
Member Author

I recommend adding a Release Notes section to this PR description, which includes a note about the change to elasticsearch.requestHeadersWhitelist's default value.

Good idea, thanks. I'll add a release note.

Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

ObsUX changes LGTM

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Security Solution Cypress Tests #11 / Detection ES|QL rules, creation creation creates an ES|QL rule and overrides its name creates an ES|QL rule and overrides its name

Metrics [docs]

Unknown metric groups

API count

id before after diff
security 401 402 +1

History

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

cc @azasypkin

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

LGTM!

@azasypkin azasypkin merged commit 7421034 into elastic:main Dec 4, 2023
38 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 4, 2023
@azasypkin azasypkin deleted the issue-171522-grant-api-keys branch December 4, 2023 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting Feature:Users/Roles/API Keys NeededFor:ResponseOps release_note:enhancement Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v8.12.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants