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

Mutual tls support for falcosidekick authentication #231

Merged
merged 3 commits into from
Apr 30, 2021

Conversation

jasiam
Copy link

@jasiam jasiam commented Apr 15, 2021

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:

It adds mutual tls support to be used in alertmanager, elasticsearch, influxdb, NATs and webhook outputs

Which issue(s) this PR fixes:

Fixes #216

Special notes for your reviewer:

@poiana poiana added kind/feature New feature or request dco-signoff: yes labels Apr 15, 2021
@poiana poiana requested review from cpanato and leodido April 15, 2021 15:02
@poiana
Copy link

poiana commented Apr 15, 2021

Welcome @jasiam! It looks like this is your first PR to falcosecurity/falcosidekick 🎉

@Issif
Copy link
Member

Issif commented Apr 15, 2021

Can you complete the description of your PR please.

@Issif Issif added this to the 2.23.0 milestone Apr 15, 2021
@Issif Issif changed the title Mutual tls support for falcosidekick authentication WIP: Mutual tls support for falcosidekick authentication Apr 15, 2021
Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

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

I know it's a WIP, I just added few suggestions and comments.

I like the client_test.go with crt/key generation for unit tests. 👍

config_example.yaml Outdated Show resolved Hide resolved
config_example.yaml Show resolved Hide resolved
outputs/client.go Show resolved Hide resolved
config_example.yaml Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated
@@ -282,7 +282,7 @@ func init() {

if config.CloudEvents.Address != "" {
var err error
cloudeventsClient, err = outputs.NewClient("CloudEvents", config.CloudEvents.Address, config, stats, promStats, statsdClient, dogstatsdClient)
cloudeventsClient, err = outputs.NewClient("CloudEvents", config.CloudEvents.Address, false, config, stats, promStats, statsdClient, dogstatsdClient)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
cloudeventsClient, err = outputs.NewClient("CloudEvents", config.CloudEvents.Address, false, config, stats, promStats, statsdClient, dogstatsdClient)
cloudeventsClient, err = outputs.NewClient("CloudEvents", config.CloudEvents.Address, config.CloudEvents.MutualTls, config, stats, promStats, statsdClient, dogstatsdClient)

main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
outputs/client.go Outdated Show resolved Hide resolved
@jasiam
Copy link
Author

jasiam commented Apr 16, 2021

Do we really want to add the mutualtls support for all outputs which use the NewClient method? For instance, Loki doesn't support this kind of authentication (or any) and you'd need a reverse proxy in order to use it.

@Issif
Copy link
Member

Issif commented Apr 16, 2021

Do we really want to add the mutualtls support for all outputs which use the NewClient method? For instance, Loki doesn't support this kind of authentication (or any) and you'd need a reverse proxy in order to use it.

Only for outputs which allow it. This is why it's a "by output" setting.

@jasiam
Copy link
Author

jasiam commented Apr 16, 2021

Do we really want to add the mutualtls support for all outputs which use the NewClient method? For instance, Loki doesn't support this kind of authentication (or any) and you'd need a reverse proxy in order to use it.

Only for outputs which allow it. This is why it's a "by output" setting.

So... I'm confused. Do you want to add the config.[OUTPUT].MutualTLS parameter in the NewClient method even if the output doesn't support mutual tls authN? I know the default value is false, but I'm afraid some users think they can use mutual tls when the actually can't.
If you want to, I can contact you over slack and discuss it.

@jasiam
Copy link
Author

jasiam commented Apr 16, 2021

Can you complete the description of your PR please.

It's super weird. If I try to edit the description I can see everything I wrote yesterday when I opened the PR, but nothing is shown when I update it.

config_example.yaml Outdated Show resolved Hide resolved
config_example.yaml Show resolved Hide resolved
outputs/client.go Show resolved Hide resolved
@Issif
Copy link
Member

Issif commented Apr 16, 2021

Do we really want to add the mutualtls support for all outputs which use the NewClient method? For instance, Loki doesn't support this kind of authentication (or any) and you'd need a reverse proxy in order to use it.

Only for outputs which allow it. This is why it's a "by output" setting.

So... I'm confused. Do you want to add the config.[OUTPUT].MutualTLS parameter in the NewClient method even if the output doesn't support mutual tls authN? I know the default value is false, but I'm afraid some users think they can use mutual tls when the actually can't.
If you want to, I can contact you over slack and discuss it.

We can discuss about this on Slack, feel free to contact me 😉

@poiana poiana added size/XXL and removed size/XL labels Apr 23, 2021
Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

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

The README is not up to date with config_example.yaml and it misses variables for MutualTLS and CheckCert in list of env vars.

In config.go, default values are missing for MutualTLS and CheckCert, they should all be there, to only have explicit values.

Thanks. 🙏

config.go Outdated Show resolved Hide resolved
config_example.yaml Outdated Show resolved Hide resolved
config_example.yaml Show resolved Hide resolved
config_example.yaml Outdated Show resolved Hide resolved
config_example.yaml Outdated Show resolved Hide resolved
config_example.yaml Outdated Show resolved Hide resolved
outputs/openfaas.go Outdated Show resolved Hide resolved
outputs/kubeless.go Outdated Show resolved Hide resolved
config.go Outdated Show resolved Hide resolved
@jasiam
Copy link
Author

jasiam commented Apr 30, 2021

The README is not up to date with config_example.yaml and it misses variables for MutualTLS and CheckCert in list of env vars.

In config.go, default values are missing for MutualTLS and CheckCert, they should all be there, to only have explicit values.

Thanks.

I think all required changes are now in the PR

Copy link
Member

@Issif Issif left a comment

Choose a reason for hiding this comment

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

can you check both README and config_example have same variables please

README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
config_example.yaml Show resolved Hide resolved
Signed-off-by: Jose Angel Santiago <>
@jasiam
Copy link
Author

jasiam commented Apr 30, 2021

can you check both README and config_example have same variables please

My fault, I forgot to update the env vars part in README file. Now README and config_example.yaml files are synchronized.

@poiana poiana added the lgtm label Apr 30, 2021
@poiana
Copy link

poiana commented Apr 30, 2021

LGTM label has been added.

Git tree hash: 3d064bd02d27d3037c12c366a66340c2333994b1

@poiana
Copy link

poiana commented Apr 30, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@Issif Issif changed the title WIP: Mutual tls support for falcosidekick authentication Mutual tls support for falcosidekick authentication Apr 30, 2021
@Issif
Copy link
Member

Issif commented Apr 30, 2021

/ready

@poiana poiana merged commit 4461d9b into falcosecurity:master Apr 30, 2021
v.SetDefault("Kafka.HostPort", "")
v.SetDefault("Kafka.Topic", "")
v.SetDefault("Kafka.MinimumPriority", "")
v.SetDefault("Pagerduty.RoutingKey", "")
v.SetDefault("Pagerduty.MinimumPriority", "")
v.SetDefault("Googlechat.MutualTls", false)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be PagerDuty.MutualTls.
@Issif @jasiam

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Add Mutual TLS support as global configuration
4 participants