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

Pushgateway basic auth with special characters in password #170

Closed
dkirilenko opened this issue Dec 4, 2019 · 6 comments · Fixed by #242
Closed

Pushgateway basic auth with special characters in password #170

dkirilenko opened this issue Dec 4, 2019 · 6 comments · Fixed by #242
Assignees
Milestone

Comments

@dkirilenko
Copy link

Looks like we can't to make basic auth with special characters in the password. I've implemented fast monkey-patching

module PrometheusClientPush
  attr_reader :basic_auth

  def set_basic_auth(username, password)
    @basic_auth = OpenStruct.new(username: username, password: password)
  end

  private

  def request(req_class, registry = nil)
    req = req_class.new(@uri)
    req.content_type = Prometheus::Client::Formats::Text::CONTENT_TYPE
    req.basic_auth(@uri.user || basic_auth.username, @uri.password || basic_auth.password) if @uri.user || basic_auth
    req.body = Prometheus::Client::Formats::Text.marshal(registry) if registry

    @http.request(req)
  end
end

Prometheus::Client::Push.prepend(PrometheusClientPush)

Will be great to see this possibility in a new version of gem, perhaps with better implementation

@dmagliola
Copy link
Collaborator

@dkirilenko I don't know the PushGateway very well...
Would you be able to open a PR with this improvement?

@Sinjo Sinjo self-assigned this Mar 25, 2021
@Sinjo
Copy link
Member

Sinjo commented Mar 25, 2021

Hey @dkirilenko, just going through old issues and I thought I'd take a look into this one.

I'm struggling to see how your monkey patch affects the handling of special characters in basic auth passwords. The current code gets the username and password as parsed by URI.parse and sets them on the request by calling the basic_auth method on whichever Net::HTTP class we happen to be using (i.e. Post/Put/Delete).

As far as I can tell, your monkey patch allows setting a username and password separately to the gateway URL, but I'm not clear how this would change the handling of special characters. Could you clarify?

@Sinjo
Copy link
Member

Sinjo commented Jan 2, 2022

@dmagliola I decided to have a bit of a play with this, and I managed to find an edge case that I suspect may be what @dkirilenko ran into, or at least close to it.

Right now, the only way to pass us basic auth credentials is by supplying them as part of the gateway parameter using the http://user:password@localhost:9091 URL syntax. We parse that parameter with URI.parse, and then pass the user and password fields in to the basic_auth method on Net::HTTP::Post (or another Net::HTTP class, depending on the HTTP method).

That syntax doesn't have a way to supply an @ symbol in the password. Searching the internet, there are suggestions that you can use percent encoding, but it seems highly tooling-dependent. Ruby's URI.parse specifically doesn't decode them for you:

irb(main):045:0> URI.parse("http://atsign:password%40@localhost:9091").password
=> "password%40"

However, if do pass the decoded form (i.e. password@) to the basic_auth method, it does work. That's because in reality all it's doing is base64 encoding the user:password string and setting it as a HTTP header value. It doesn't matter whether or not it's valid URL syntax once you're making the request itself.

I'm not sure if there are other characters that this affects. Surprisingly, colon seems just fine:

irb(main):046:0> URI.parse("http://colon:password:@localhost:9091").password
=> "password:"

but that doesn't hold true for other characters that you'd typically need to encode in a URL:

irb(main):047:0> URI.parse("http://colon:password?@localhost:9091").password
home/sinjo/.rbenv/versions/3.1.0/lib/ruby/3.1.0/uri/rfc3986_parser.rb:67:in `split': bad URI(is not URI?): "http://colon:password?@localhost:9091" (URI::InvalidURIError)
...
irb(main):049:0> URI.parse("http://colon:password%3F@localhost:9091").password
=> "password%3F"

I'm not 100% sure which way I want to go with this, but I'm leaning towards discouraging the use of the in-URL basic auth syntax. My reasoning there is that while in theory we could URL decode the password (and possibly the username, I'd need to go read up on what those are allowed to contain), we'd be reliant on users encoding them correctly in the first place, which seems like a huge footgun.

We could introduce a separate method (or a parameter in initialize) to set them, and perhaps throw an exception if they're provided in the gateway URL, telling users why we don't accept them that way and what they should do instead.

We're making a whole bunch of breaking changes to this class in #234 which will land in version 3.0.0 of the library. If you agree on the approach, this would be a good time to land it.

@dmagliola
Copy link
Collaborator

Yeah, I agree with this plan. I think it's better to set these creds in either the initializer or a specific method (i'd choose one or the other depending on how common we think it is to set creds)

And I agree we should either raise or at the very least log a warning if you pass them in the URL, this is a hard one to debug otherwise if you get bitten by this!

@Sinjo
Copy link
Member

Sinjo commented Jan 9, 2022

@dkirilenko Thank you for the report! This has been fixed in #242, which will be part of our 3.0.0 release.

@dkirilenko
Copy link
Author

thanks, guys)

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 a pull request may close this issue.

3 participants