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.

Sign up for GitHub

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

Jump to bottom

fix: removing outdated code in Kibana client auth #4495

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fix: removing outdated code in Kibana client auth #4495

wants to merge 5 commits into from

Conversation

traut
Copy link
Contributor

@traut traut commented Feb 25, 2025

Pull Request

Issue link(s):

Summary - What I changed

  • dropped support for username / password / cookies in Kibana client as insecure
  • adjust the code that uses Kibana class to use api_key

How To Test

  • unit tests pass

Checklist

  • Added a label for the type of pr: bug, enhancement, schema, maintenance, Rule: New, Rule: Deprecation, Rule: Tuning, Hunt: New, or Hunt: Tuning so guidelines can be generated
  • Added the meta:rapid-merge label if planning to merge within 24 hours
  • Secret and sensitive material has been managed correctly
  • Automated testing was updated or added to match the most common scenarios
  • Documentation and comments were added for features that require explanation

Contributor checklist

@traut traut added the bug Something isn't working label Feb 25, 2025
@github-actions GitHub Actions
Copy link
Contributor

Bug - Guidelines

These guidelines serve as a reminder set of considerations when addressing a bug in the code.

Documentation and Context

  • Provide detailed documentation (description, screenshots, reproducing the bug, etc.) of the bug if not already documented in an issue.
  • Include additional context or details about the problem.
  • Ensure the fix includes necessary updates to the release documentation and versioning.

Code Standards and Practices

  • Code follows established design patterns within the repo and avoids duplication.
  • Code changes do not introduce new warnings or errors.
  • Variables and functions are well-named and descriptive.
  • Any unnecessary / commented-out code is removed.
  • Ensure that the code is modular and reusable where applicable.
  • Check for proper exception handling and messaging.

Testing

  • New unit tests have been added to cover the bug fix or edge cases.
  • Existing unit tests have been updated to reflect the changes.
  • Provide evidence of testing and detecting the bug fix (e.g., test logs, screenshots).
  • Validate that any rules affected by the bug are correctly updated.
  • Ensure that performance is not negatively impacted by the changes.
  • Verify that any release artifacts are properly generated and tested.

Additional Checks

  • Ensure that the bug fix does not break existing functionality.
  • Review the bug fix with a peer or team member for additional insights.
  • Verify that the bug fix works across all relevant environments (e.g., different OS versions).
  • Confirm that all dependencies are up-to-date and compatible with the changes.
  • Confirm that the proper version label is applied to the PR patch, minor, major.

@traut traut added the patch label Feb 25, 2025
@traut traut marked this pull request as ready for review February 25, 2025 13:50
@eric-forte-elastic
Copy link
Contributor

Just noting so that we do not forget, if we are dropping support for user/password we need to update our documentation in this repo and in https://github.com/elastic/DaC-Reference.

Mikaayenson
Mikaayenson reviewed Mar 5, 2025
View reviewed changes
detection_rules/misc.py
help="Elastic Cloud providers: basic and saml (for SSO)"),
'provider_name': click.Option(['--provider-name'], default=getdefault('provider_name'),
help="Elastic Cloud providers: cloud-basic and cloud-saml (for SSO)"),
'cloud_id': click.Option(['--cloud-id'], default=getdefault('cloud_id'), help="ID of the cloud instance."),
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason why we dont keep username/password etc for legacy support or local stacks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Kibana API does not officially support username/password auth (docs), and has an explicit warning:

Access to internal Kibana API endpoints will be restricted in 9.0. Please move any integrations to publicly documented APIs.

I suggest we drop the support for username/password, and if there are requests from the customers, we bring this auth method back, but mark it clearly as a not-recommended-way

@botelastic botelastic bot added the python Internal python for the repository label Mar 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Mikaayenson Mikaayenson Mikaayenson left review comments

@eric-forte-elastic eric-forte-elastic Awaiting requested review from eric-forte-elastic eric-forte-elastic is a code owner

@terrancedejesus terrancedejesus Awaiting requested review from terrancedejesus terrancedejesus is a code owner

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
backport: auto bug Something isn't working patch python Internal python for the repository
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@traut @eric-forte-elastic @Mikaayenson