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

Enable bearer scheme by default to support service token authorization #112654

Merged
merged 8 commits into from
Oct 11, 2021

Conversation

joshdover
Copy link
Contributor

@joshdover joshdover commented Sep 21, 2021

Summary

Related to #112648, required to allow Fleet Server to use its service account token for authorization against Fleet APIs in Kibana.

  • Updates the default for xpack.security.authc.http.schemes to include the bearer scheme
  • Adds functional tests for HTTP Bearer authorization (supports API calls, but not login or logout)
  • Updated documentation to reflect the changes

I wasn't sure if this change should have a release note or not, @elastic/kibana-security please let me know if I should add one.

Checklist

Delete any items that are not applicable to this PR.

  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list

For maintainers

@joshdover
Copy link
Contributor Author

@elastic/kibana-security Any concerns with this change? Also, do you have any recommendations on where to add some functional test coverage for this?

@jportner
Copy link
Contributor

@elastic/kibana-security Any concerns with this change? Also, do you have any recommendations on where to add some functional test coverage for this?

Just to be clear, this is related to the Fleet server changing to use a service account, yes?

We most recently changed how schemes are configured in 7.7 via #58126.

I'm not sure it makes sense for the bearer scheme to be automatically enabled for all providers. It does conflict with our docs on HTTP authentication. But I'm also not sure how else we could best bsupport Fleet functionality out of the box.

@azasypkin I hate to suggest this, but would it make sense to introduce a Kibana-specific scheme to support service accounts? Or maybe there's another option that I'm missing. I'll defer to you.

@jportner jportner requested a review from azasypkin September 27, 2021 15:36
@azasypkin
Copy link
Member

@azasypkin I hate to suggest this, but would it make sense to introduce a Kibana-specific scheme to support service accounts? Or maybe there's another option that I'm missing. I'll defer to you.

If it's to support service accounts then I think it'd fine to include bearer to the default list of supported HTTP schemes so that API requests to both Kibana and ES use the same scheme. It's already implicitly enabled in many cases anyway because of default autoSchemesEnabled: true flag. But as you mentioned, we'll need to a) update our docs, and b) update kibana.yml template in Cloud to include bearer if needed (Cloud overrides default value for xpack.security.authc.http.schemes to include basic)

Also, do you have any recommendations on where to add some functional test coverage for this?

It'd be great if you can add a couple of tests for this to x-pack/test/security_api_integration (e.g. create http_auth.config.ts with default xpack.security.authc config). We have several similar tests here.

@joshdover
Copy link
Contributor Author

Just to be clear, this is related to the Fleet server changing to use a service account, yes?

Yep this is to support #112648

But as you mentioned, we'll need to a) update our docs, and b) update kibana.yml template in Cloud to include bearer if needed (Cloud overrides default value for xpack.security.authc.http.schemes to include basic)

Makes sense. I'll update this PR with the additional testing and update the relevant docs. I'll also open a PR for updating the Cloud stack pack for 7.x and 8.0.

Thanks for the guidance here, everyone.

@joshdover joshdover force-pushed the add-support-for-service-token branch from 43dbd25 to 9d29ef5 Compare October 5, 2021 09:57
@joshdover joshdover added Feature:Security/Authentication Platform Security - Authentication release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.16.0 v8.0.0 labels Oct 5, 2021
@joshdover joshdover marked this pull request as ready for review October 5, 2021 10:08
@joshdover joshdover requested a review from a team as a code owner October 5, 2021 10:08
@azasypkin
Copy link
Member

ACK: will review today

Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

Looks good to me, just a couple of nits and one suggestion.

@joshdover joshdover requested a review from azasypkin October 11, 2021 09:35
Copy link
Member

@azasypkin azasypkin left a comment

Choose a reason for hiding this comment

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

LGTM! Please don't forget to update Cloud kibana.yml template.

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
@joshdover joshdover enabled auto-merge (squash) October 11, 2021 14:14
@joshdover joshdover added the auto-backport Deprecated - use backport:version if exact versions are needed label Oct 11, 2021
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

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

cc @joshdover

@joshdover joshdover merged commit b03237a into elastic:master Oct 11, 2021
@kibanamachine
Copy link
Contributor

💔 Backport failed

Status Branch Result
7.x Commit could not be cherrypicked due to conflicts

To backport manually run:
node scripts/backport --pr 112654

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 112654 or prevent reminders by adding the backport:skip label.

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 13, 2021
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 112654 or prevent reminders by adding the backport:skip label.

2 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 112654 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 112654 or prevent reminders by adding the backport:skip label.

joshdover added a commit to joshdover/kibana that referenced this pull request Oct 19, 2021
…ion (elastic#112654)

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
# Conflicts:
#	x-pack/plugins/security/server/usage_collector/security_usage_collector.test.ts
@kibanamachine
Copy link
Contributor

Looks like this PR has a backport PR but it still hasn't been merged. Please merge it ASAP to keep the branches relatively in sync.

@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Oct 19, 2021
joshdover added a commit that referenced this pull request Oct 19, 2021
…orization (#112654) (#115557)

Co-authored-by: Aleh Zasypkin <aleh.zasypkin@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Security/Authentication Platform Security - Authentication release_note:skip Skip the PR/issue when compiling release notes Team:Security Team focused on: Auth, Users, Roles, Spaces, Audit Logging, and more! v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants