[Filebeat] Adds oauth2 support for httpjson input by marc-gr · Pull Request #18892 · elastic/beats

marc-gr · 2020-06-02T10:07:12Z

What does this PR do?

Adds oauth2 support to filebeat HTTPJSON input.

Why is it important?

This will let us pull data from oauth2 authenticated sources, like AzureAD or GSuite.

Checklist

My code follows the style guidelines of this project
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
~~I have made corresponding change to the default configuration files~~
I have added tests that prove my fix is effective or that my feature works
I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

Related issues

Closes #18415

elasticmachine · 2020-06-02T10:07:14Z

Pinging @elastic/siem (Team:SIEM)

elasticmachine · 2020-06-02T10:54:35Z

💚 Build Succeeded

Expand to view the summary

Build stats

Build Cause: [Pull request #18892 updated]
Start Time: 2020-06-09T09:20:52.521+0000
Duration: 85 min 1 sec

Test stats 🧪

Test	Results
Failed	0
Passed	9431
Skipped	1574
Total	11005

Steps errors

Expand to view the steps failures

Name: Report to Codecov
- Description: curl -sSLo codecov https://codecov.io/bash for i in auditbeat filebeat heartbeat libbeat metricbeat packetbeat winlogbeat journalbeat do FILE="${i}/build/coverage/full.cov" if [ -f "${FILE}" ]; then bash codecov -f "${FILE}" fi done
- Duration: 2 min 22 sec
- Start Time: 2020-06-09T10:05:21.006+0000
- log

P1llus

Just my 2 cents, rest looks good from our testing

x-pack/filebeat/input/httpjson/config.go

x-pack/filebeat/input/httpjson/input.go

marc-gr · 2020-06-04T15:11:43Z

@P1llus please take a look to see if this looks somehow good. I scoped the provider specific options with google_ azure_ etc. to make it easy to grasp while reading the config or setting it, even though I do not have strong feelings towards doing it or not. If it feels good will move on with testing, docs etc.

P1llus

Just a small question, rest LGTM :)

x-pack/filebeat/input/httpjson/config.go

andrewkroh

Just a little early feedback. Looks like it coming along well.

andrewkroh · 2020-06-04T22:22:39Z

x-pack/filebeat/input/httpjson/config.go

Suggested change

if c.OAuth2 != nil {

if c.OAuth2.IsEnabled() {

Here I followed the same pattern as Pagination, which checks for it being enabled at the moment of using it, but still validates its config. To me it kind of made sense since even if disabled a user needs to introduce a valid config for it. Otherwise I would change also how Pagination is handled just to be consistent here and check for IsEnabled in both config validation and usage. WDYT?

I think using IsEnabled() is correct now that you've refactored it a bit (before you we're correct). For example it would be invalid to use api_key or authentication only when oauth is enabled.

Good point! 👍

x-pack/filebeat/input/httpjson/config.go

x-pack/filebeat/input/httpjson/config_oauth.go

P1llus

Added one comment, rest LGTM

P1llus · 2020-06-08T08:01:54Z

x-pack/filebeat/input/httpjson/config.go

Could you add validation for any raw json strings in your config? Example from my http_endpoint input:

func (c *config) Validate() error { if !json.Valid([]byte(c.ResponseBody)) { return errors.New("response_body must be valid JSON") } return nil }

Added it in the places where it is expected: https://github.com/elastic/beats/pull/18892/files#diff-e3d27d33f878a5fb0a720224c80ad7aaR135 and https://github.com/elastic/beats/pull/18892/files#diff-e3d27d33f878a5fb0a720224c80ad7aaR171

andrewkroh · 2020-06-08T20:59:57Z

x-pack/filebeat/input/httpjson/config_oauth.go

I like having a case OAuth2ProviderDefault, OAuth2ProviderAzure: and then an explicit default: case that returns an error. Then we know every case is covered or it results in a error.

andrewkroh · 2020-06-08T21:18:58Z

x-pack/filebeat/docs/inputs/input-httpjson.asciidoc

I'd like to minimize the use of api_key and possibly deprecate it soon because you can just use http_headers. Having two ways to configure something usually leads to confusion and more edge cases to test. So can you leave this example out?

andrewkroh · 2020-06-08T21:25:42Z

x-pack/filebeat/docs/inputs/input-httpjson.asciidoc

This and some of the other NOTEs can be incorporated into the paragraph. NOTEs get some special rendering and I'd reserve them for exceptional cases where the behavior might not be what the user expected.

andrewkroh · 2020-06-08T21:46:03Z

x-pack/filebeat/docs/inputs/input-httpjson.asciidoc

Maybe "For Azure either token_url or azure.tenant_id is required." From looking at the validation it looks like one or the other is required.

andrewkroh · 2020-06-08T22:00:57Z

x-pack/filebeat/docs/inputs/input-httpjson.asciidoc

Suggested change

https://docs.microsoft.com/en-us/azure/active-directory/develop/howto-create-service-principal-portal

https://docs.microsoft.com/en-us/azure/active-directory/develop/howto-create-service-principal-portal.

andrewkroh · 2020-06-08T22:07:19Z

x-pack/filebeat/input/httpjson/config.go

I think using IsEnabled() is correct now that you've refactored it a bit (before you we're correct). For example it would be invalid to use api_key or authentication only when oauth is enabled.

andrewkroh · 2020-06-08T23:08:38Z

x-pack/filebeat/input/httpjson/config_test.go

I'm really happy to have all the new test cases. I'm not keen on the numerical naming pattern that got established here. Maybe break from this pattern for the new tests. I'd prefer something that describes the requirements/assertion being made about the code (e.g. TestConfigValidateRequiresCreds or use TestConfigValidate with t.Run("validate requires credentials", func(...)).

…entials

marc-gr · 2020-06-09T09:07:31Z

Moved the config tests from TestConfigValidateNN to a table test. Also changed config.Validate to use IsEnabled and added an extra test for it. Did some changes on the docs, please tell me if you think they need any more 👍

P1llus

LGTM, I got nothing more to add than what Andrew already mentioned

andrewkroh

LGTM

…ngs-archive * upstream/master: (119 commits) Update filebeat input docs (elastic#19110) Add ECS fields from log pipeline of PostgreSQL (elastic#19127) Init package libbeat/statestore (elastic#19117) [Ingest Manager] Retryable downloads of beats (elastic#19102) [DOCS] Add output.console to Functionbeat doc and Functionbeat reference file (elastic#18965) Add compatibility info (elastic#18929) Set ecszap version to v0.2.0 (elastic#19106) [filebeat][httpjson] Fix unit test function call (elastic#19124) [Filebeat][httpjson] Adds oauth2 support for httpjson input (elastic#18892) Allow host.* fields to be disabled in Suricata module (elastic#19107) Make selector string casing configurable (elastic#18854) Switch the GRPC communication where Agent is running the server and the beats are connecting back to Agent (elastic#18973) Disable host.* fields by default for netflow module (elastic#19087) Automatically fill zube teams on backports if available (elastic#18924) Fix crash on vsphere module (elastic#19078) [Ingest Manager] Download snapshot artifacts from snapshots repo (elastic#18685) [Ingest Manager] Basic Elastic Agent documentation (elastic#19030) Make user.id a string in system/users, in line with ECS (elastic#19019) [docs] Add 7.8 release highlights placeholder file (elastic#18493) Fix translate_sid's empty target field handling (elastic#18991) ...

…18892) * Filebeat HTTPJSON input initial changes to support oauth2 client_credentials * [Filebeat][httpjson] Add EndpointParams option to oauth config * Add provider specific settings to oauth httpjson * Change config as suggested and add config tests * Add checks for invalid json in google validation * Add documentation and azure.resource * Add oauth2 test and update changelog * Address docs and change new test case into table tests * Check if oauth2 is enabled in config.Validate and add test Closes elastic#18415 (cherry picked from commit b6cd17c)

…19122) * Filebeat HTTPJSON input initial changes to support oauth2 client_credentials * [Filebeat][httpjson] Add EndpointParams option to oauth config * Add provider specific settings to oauth httpjson * Change config as suggested and add config tests * Add checks for invalid json in google validation * Add documentation and azure.resource * Add oauth2 test and update changelog * Address docs and change new test case into table tests * Check if oauth2 is enabled in config.Validate and add test Closes #18415 (cherry picked from commit b6cd17c)

…18892) * Filebeat HTTPJSON input initial changes to support oauth2 client_credentials * [Filebeat][httpjson] Add EndpointParams option to oauth config * Add provider specific settings to oauth httpjson * Change config as suggested and add config tests * Add checks for invalid json in google validation * Add documentation and azure.resource * Add oauth2 test and update changelog * Address docs and change new test case into table tests * Check if oauth2 is enabled in config.Validate and add test Closes elastic#18415

marc-gr added enhancement in progress Pull request is currently in progress. Filebeat Filebeat Team:SIEM v7.9.0 labels Jun 2, 2020

marc-gr requested a review from P1llus June 2, 2020 10:07

botelastic bot added needs_team Indicates that the issue/PR needs a Team:* label and removed needs_team Indicates that the issue/PR needs a Team:* label labels Jun 2, 2020

marc-gr force-pushed the feature_httpjson-oauth2 branch from dd950a3 to 922f906 Compare June 3, 2020 12:39

P1llus reviewed Jun 3, 2020

View reviewed changes

x-pack/filebeat/input/httpjson/config.go Outdated Show resolved Hide resolved

x-pack/filebeat/input/httpjson/input.go Outdated Show resolved Hide resolved

marc-gr force-pushed the feature_httpjson-oauth2 branch from 922f906 to 4a321dc Compare June 4, 2020 15:09

marc-gr force-pushed the feature_httpjson-oauth2 branch 2 times, most recently from 3f8a188 to 868d3b8 Compare June 4, 2020 15:35

P1llus reviewed Jun 4, 2020

View reviewed changes

x-pack/filebeat/input/httpjson/config.go Outdated Show resolved Hide resolved

andrewkroh reviewed Jun 4, 2020

View reviewed changes

marc-gr force-pushed the feature_httpjson-oauth2 branch 4 times, most recently from 0475b64 to 8c68fd0 Compare June 5, 2020 14:08

P1llus reviewed Jun 8, 2020

View reviewed changes

marc-gr force-pushed the feature_httpjson-oauth2 branch from 8c68fd0 to 05581e2 Compare June 8, 2020 08:22

marc-gr requested review from P1llus and andrewkroh June 8, 2020 12:18

marc-gr force-pushed the feature_httpjson-oauth2 branch from 3ded504 to a130cd9 Compare June 8, 2020 12:39

marc-gr added review and removed in progress Pull request is currently in progress. labels Jun 8, 2020

andrewkroh reviewed Jun 8, 2020

View reviewed changes

marc-gr added 8 commits June 9, 2020 10:42

Filebeat HTTPJSON input initial changes to support oauth2 client_cred…

25b1631

…entials

[Filebeat][httpjson] Add EndpointParams option to oauth config

4150b7e

Add provider specific settings to oauth httpjson

488a7f9

Change config as suggested and add config tests

f25a635

Add checks for invalid json in google validation

9eb4f62

Add documentation and azure.resource

53e9b26

Add oauth2 test and update changelog

7108a73

Address docs and change new test case into table tests

246f1d9

marc-gr force-pushed the feature_httpjson-oauth2 branch from a130cd9 to 246f1d9 Compare June 9, 2020 08:43

Check if oauth2 is enabled in config.Validate and add test

dc1c1c4

P1llus reviewed Jun 9, 2020

View reviewed changes

marc-gr requested a review from andrewkroh June 10, 2020 07:39

andrewkroh approved these changes Jun 10, 2020

View reviewed changes

marc-gr merged commit b6cd17c into elastic:master Jun 11, 2020

marc-gr deleted the feature_httpjson-oauth2 branch June 11, 2020 06:45

marc-gr mentioned this pull request Jun 11, 2020

Cherry-pick #18892 to 7.x: [Filebeat] Adds oauth2 support for httpjson input #19122

Merged

5 tasks

	https://docs.microsoft.com/en-us/azure/active-directory/develop/howto-create-service-principal-portal
	https://docs.microsoft.com/en-us/azure/active-directory/develop/howto-create-service-principal-portal.

Conversation

marc-gr commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this PR do?

Why is it important?

Checklist

Related issues

Uh oh!

elasticmachine commented Jun 2, 2020

Uh oh!

elasticmachine commented Jun 2, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

💚 Build Succeeded

Build stats

Test stats 🧪

Steps errors

Uh oh!

P1llus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marc-gr commented Jun 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

P1llus left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

P1llus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewkroh Jun 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

marc-gr commented Jun 9, 2020

Uh oh!

P1llus left a comment

Choose a reason for hiding this comment

Uh oh!

andrewkroh left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

marc-gr commented Jun 2, 2020 •

edited

Loading

elasticmachine commented Jun 2, 2020 •

edited

Loading

marc-gr commented Jun 4, 2020 •

edited

Loading

andrewkroh Jun 8, 2020 •

edited

Loading