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

[eset_protect] Update grant type to password #9600

Merged
merged 5 commits into from
Apr 18, 2024

Conversation

janvi-elastic
Copy link
Contributor

Type of change

  • Enhancement

What does this PR do?

  • Update grant type to password for detection and device_dask as client_credentials grant type is deprecated.

Checklist

  • I have reviewed tips for building integrations and this pull request is aligned with them.
  • I have verified that all data streams collect metrics or logs.
  • I have added an entry to my package's changelog.yml file.
  • I have verified that Kibana version constraints are current according to guidelines.

How to test this PR locally

  • Clone integrations repo.
  • Install elastic package locally.
  • Start elastic stack using elastic-package.
  • Move to integrations/packages/eset_protect directory.
  • Run the following command to run tests.

elastic-package test

Automated Test

image

@janvi-elastic janvi-elastic requested a review from a team as a code owner April 15, 2024 16:38
@elasticmachine
Copy link

🚀 Benchmarks report

To see the full report comment with /test benchmark fullreport

client.id: ' '
client.secret: ' '
user: {{username}}
password: {{password}}
Copy link
Member

Choose a reason for hiding this comment

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

Consider using escape_string1 in case values contain characters that require special handling in YAML.

  user: {{escape_string username}}
  password: {{escape_string password}}

Footnotes

  1. https://github.com/elastic/package-spec/issues/387

@@ -15,8 +15,10 @@ resource.timeout: {{http_client_timeout}}
{{/if}}
resource.url: https://{{region}}.incident-management.eset.systems
auth.oauth2:
client.id: {{username}}
client.secret: {{password}}
client.id: ' '
Copy link
Member

Choose a reason for hiding this comment

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

Why are these empty strings?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Without empty string it is not working.

Copy link
Member

Choose a reason for hiding this comment

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

Can you clarify what is not working? Can you share the error message? Is it some kind of validation problem from the Agent side? Or is a something from the API service?

If you view the request tracer logs, is the input POSTing client_id=%20&client_secret=%20 and the API is accepting that?

At a minimum we need a comment explaining why the empty strings exists so that future maintainers understand.

Copy link
Contributor

@efd6 efd6 Apr 15, 2024

Choose a reason for hiding this comment

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

The issue here is over zealous validation in the auth config code for the cel package (the same issue will exist in the httpjson package which is the origin of this code). When password auth was added, the token auth logic in the validation was not updated to allow a fallthrough.

https://github.com/elastic/beats/blob/11bc06ca1f0f82510a38e88f978b9aed577a3e5e/x-pack/filebeat/input/cel/config_auth.go#L266-L268

I'm putting together a change to fix this.

Copy link
Contributor Author

@janvi-elastic janvi-elastic Apr 16, 2024

Choose a reason for hiding this comment

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

I was facing below error:
both token_url and client credentials must be provided accessing 'auth.oauth2'

Additionally, it does not function with an empty string. It requires one space to work properly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, elastic/beats#38962 will fix that, but for the moment you'll need to do what you have done. A comment explaining it and giving a time frame for removal would be good (I expect the PR for the fix would get in to v8.14.0).

TBH I'm amazed that his has gone unnoticed for so long. The bug was introduced in to HTTPJSON when password grant auth was added. I guess no-one has ever had password-only uses.

Copy link
Member

@andrewkroh andrewkroh Apr 17, 2024

Choose a reason for hiding this comment

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

A comment explaining it and giving a time frame for removal would be good (I expect the PR for the fix would get in to v8.14.0).

@janvi-elastic Can you please add a comment to the two cel.yml.hbs files.

@andrewkroh andrewkroh requested a review from a team April 15, 2024 17:02
@andrewkroh andrewkroh added Integration:eset_protect ESET PROTECT Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations] labels Apr 15, 2024
@elasticmachine
Copy link

Pinging @elastic/security-service-integrations (Team:Security-Service Integrations)

andrewkroh and others added 2 commits April 17, 2024 16:01
As per https://eu.esetconnect.eset.systems/swagger/

> Grant type client_credentials is deprecated and will be removed on Apr 15th 2024
@elasticmachine
Copy link

💚 Build Succeeded

History

Copy link

Quality Gate passed Quality Gate passed

Issues
0 New issues
0 Fixed issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarQube

@efd6 efd6 merged commit cdeaa8a into elastic:main Apr 18, 2024
5 checks passed
@elasticmachine
Copy link

Package eset_protect - 0.5.0 containing this change is available at https://epr.elastic.co/search?package=eset_protect

@narph narph added the Crest label Apr 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Crest enhancement New feature or request Integration:eset_protect ESET PROTECT Team:Security-Service Integrations Security Service Integrations Team [elastic/security-service-integrations]
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants