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

Support authorization_credentials #272

Merged
merged 1 commit into from Feb 17, 2021

Conversation

roidelapluie
Copy link
Member

This backward-compatible patch enables authorization header type to be
set.

For consistency, bearer_token is renamed to authorization_credentials
and a new authorization_type is introduced.

The terminology is taken from
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization

refs:
#271
prometheus/prometheus#5107

Signed-off-by: Julien Pivotto roidelapluie@inuits.eu

@roidelapluie
Copy link
Member Author

Questions:
Do we want this?

Do we want the form:

authorization_credentials: superlongbearer
authorization_type: APIKEY

or

authorization:
   credentials: superlongbearer
   type: APIKEY

Note: the latter would mean always 2 lines (in pretty yaml) and therefore you can't easily sed your existing configuration file.

authorization_credentials: superlongbearer

or

authorization:
   credentials: superlongbearer

@dazinator
Copy link

My vote would be for the latter format

authorization:
   credentials: superlongbearer

@roidelapluie
Copy link
Member Author

I have implemented the latter. cc @beorn7

@roidelapluie roidelapluie force-pushed the add-auth-credentials branch 2 times, most recently from fac8ca6 to 34fd980 Compare February 15, 2021 21:13
@beorn7
Copy link
Member

beorn7 commented Feb 15, 2021

I'll review properly ASAP.

In the meantime, you can fix the CI failures. (o:

This backward-compatible patch enables authorization header type to be
set.

For consistency, bearer_token is renamed to authorization credentials
and a new authorization type is introduced.

The terminology is taken from
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Authorization

Signed-off-by: Julien Pivotto <roidelapluie@inuits.eu>
if len(c.Authorization.Type) == 0 {
c.Authorization.Type = "Bearer"
}
if strings.ToLower(c.Authorization.Type) == "basic" {

Choose a reason for hiding this comment

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

What's the benefit for preventing "basic" as the auth type here? Just thinking the two config mechanisms are not strictly equivalent - as with one the user must save the username and password in the config in plain text. In the other the user can save the credentials pre encoded as they are transmitted over the wire. It may be preferable to the user to use the second mechanism?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is to reduce support requests. That prevents users to put clear text password here and expect it to work. We have a well defined way to do Basic auth, let's not add a second.

Copy link
Member Author

Choose a reason for hiding this comment

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

I would also note that some users might have the fake sense of security that this method would use an encrypted password while it's not the case.

@roidelapluie roidelapluie merged commit 7a93127 into prometheus:master Feb 17, 2021
This was referenced Mar 11, 2021
This was referenced Mar 15, 2021
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.

None yet

3 participants