[Elasticsearch] Add APIKey configuration for Elasticsearch output #980
Conversation
Signed-off-by: Aleksandr Maus <aleksandr.maus@elastic.co>
aleksmaus
force-pushed
the
feature/elasticsearch_apikey
branch
from
f2888a0
to
8307903
Compare
| if c.Config.Elasticsearch.ApiKey != "" { | ||
| req.Header.Set("Authorization", "APIKey "+c.Config.Elasticsearch.ApiKey) | ||
| } | ||
|
|
||
| if c.Config.Elasticsearch.Username != "" && c.Config.Elasticsearch.Password != "" { | ||
| req.SetBasicAuth(c.Config.Elasticsearch.Username, c.Config.Elasticsearch.Password) | ||
| } |
There was a problem hiding this comment.
Should we not make them exclusive? APIKey or Username + Password but not both?
There was a problem hiding this comment.
I was thinking about this.
The original implementation of setting the headers
https://github.com/falcosecurity/falcosidekick/blob/2.29.0/outputs/elasticsearch.go#L62
allowed the custom headers to overwrite the basic auth header. Effectively users could set the username and password in the configuration and then add "Authorization" header to the customHeaders and overwrite that header. It is a subtle detail, but it is the current behavior.
So I decided to follow the same behavior here, the headers are just applied in order, and not trying to figure out if the header is already set or not.
What is going to happen in this case: if the user has both apikey and the username and password in the configuration, the username and password will overwrite the "Authorization" header here:
https://github.com/golang/go/blob/ffb3e574012ce9d3d5193d7b8df135189b8a6671/src/net/http/request.go#L1023
thus will end up using basic auth.
Let me know if you still want to change.
And if we change, should we apply the custom headers before auth or after? They were applied after authorization header before (and now still). If we continue to apply them after, should we check if the "Authorization" header was already set or let the user overwrite it if they want? If we let the user to overwrite, we might as well keep the implementation as already is.
There was a problem hiding this comment.
It's ok to me to keep the behavior like it is. Thanks
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aleksmaus, Issif The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
LGTM label has been added. Git tree hash: f3e2dcc027e1d6315fbbcbae3c0b6fdf0ce48c99
|
What type of PR is this?
/kind feature
Any specific area of the project related to this PR?
/area outputs
What this PR does / why we need it:
Adds
APIKeyconfiguration for Elasticsearch outputAPIKeyis a preferred way to authenticated with Elasticsearch, since users can limit it's permissions, assign the expiration, revoke, etc.Currently user can configure it with the
customHeaders, for example:Exposing
APIKeythrough a dedicated configuration option would hopefully encourage and make easier for users to use APIKey instead of basic auth.Special notes for your reviewer:
Good convenience configuration option