Skip to content

Add ability to skip TLS verification for amtool#2663

Merged
roidelapluie merged 5 commits intoprometheus:mainfrom
nedvna:main
Aug 6, 2021
Merged

Add ability to skip TLS verification for amtool#2663
roidelapluie merged 5 commits intoprometheus:mainfrom
nedvna:main

Conversation

@nedvna
Copy link
Contributor

@nedvna nedvna commented Aug 1, 2021

Just like with prometheus or alertmanager endpoints, we sometimes need to disable TLS certificate check. We can do that providing <tls_config> section there.
amtool is a simpler thing but it does provide us some basic features like providing custom port and performing basic auth. I think skipping tls verification will fit in here too.

nedvna added 2 commits August 3, 2021 12:26
Signed-off-by: Nikita Nedvetskii <72229464+nedvna@users.noreply.github.com>
Signed-off-by: Nikita Nedvetskii <72229464+nedvna@users.noreply.github.com>
cli/root.go Outdated
app.Flag("alertmanager.url", "Alertmanager to talk to").URLVar(&alertmanagerURL)
app.Flag("output", "Output formatter (simple, extended, json)").Short('o').Default("simple").EnumVar(&output, "simple", "extended", "json")
app.Flag("timeout", "Timeout for the executed command").Default("30s").DurationVar(&timeout)
app.Flag("skip.verify", "Skip TLS certificate verification").BoolVar(&skipVerify)
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
app.Flag("skip.verify", "Skip TLS certificate verification").BoolVar(&skipVerify)
app.Flag("tls.insecure.skip.verify", "Skip TLS certificate verification").BoolVar(&skipVerify)

To match go tls name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for suggestion, applied it.
A bit more verbose but also clearer.

Signed-off-by: Nikita Nedvetskii <72229464+nedvna@users.noreply.github.com>
cli/root.go Outdated

cr := clientruntime.New(address, path.Join(amURL.Path, defaultAmApiv2path), schemes)

if amURL.Scheme == "https" && skipVerify {
Copy link
Member

Choose a reason for hiding this comment

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

Now I notice this. We should not check the scheme, but apply this all the time. You could go to an HTTP endpoint that redirects you (HTTP 302) to HTTPS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't think we automatically followed them.
Thanks, removed it.

Signed-off-by: Nikita Nedvetskii <72229464+nedvna@users.noreply.github.com>
Copy link
Member

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

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

Sorry, I noticed one thing that was not updated, so I added comments from 2 other small things :) Then it will be OK :)

cli/root.go Outdated
date.format
Sets the output format for dates. Defaults to "2006-01-02 15:04:05 MST"

skip.verify
Copy link
Member

Choose a reason for hiding this comment

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

This was not updated

README.md Outdated
receiver: team-X-pager

# Skip TLS certificate verification
tls.insecure.skip.verify: true
Copy link
Member

Choose a reason for hiding this comment

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

Let's get this out of the example configuration to keep it simple and not promote insecure settings.

cli/root.go Outdated
alertmanagerURL *url.URL
output string
timeout time.Duration
skipVerify bool
Copy link
Member

Choose a reason for hiding this comment

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

Can we have tlsInsecureSkipVerify ?

Signed-off-by: Nikita Nedvetskii <72229464+nedvna@users.noreply.github.com>
@nedvna
Copy link
Contributor Author

nedvna commented Aug 4, 2021

Sorry, I noticed one thing that was not updated, so I added comments from 2 other small things :) Then it will be OK :)

No problem.
I hope I'm not breaking any more naming conventions :)

@roidelapluie roidelapluie merged commit c72c4d7 into prometheus:main Aug 6, 2021
@roidelapluie
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants