Invalid input for update_ip_address

Hi,

There is a lot of errors in /logs, which states there is invalid input for update_ip_address operation.

The discourse is deployed in Azure, with a separated PostgreSQL and Redis service.

Error logs:

/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/rack-mini-profiler-0.10.7/lib/patches/db/pg.rb:90:in `async_exec'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/rack-mini-profiler-0.10.7/lib/patches/db/pg.rb:90:in `async_exec'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/postgresql_adapter.rb:614:in `block (2 levels) in exec_no_cache'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activesupport-5.1.4/lib/active_support/dependencies/interlock.rb:46:in `block in permit_concurrent_loads'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activesupport-5.1.4/lib/active_support/concurrency/share_lock.rb:185:in `yield_shares'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activesupport-5.1.4/lib/active_support/dependencies/interlock.rb:45:in `permit_concurrent_loads'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/postgresql_adapter.rb:613:in `block in exec_no_cache'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/abstract_adapter.rb:612:in `block (2 levels) in log'
/usr/local/lib/ruby/2.4.0/monitor.rb:214:in `mon_synchronize'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/abstract_adapter.rb:611:in `block in log'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activesupport-5.1.4/lib/active_support/notifications/instrumenter.rb:21:in `instrument'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/abstract_adapter.rb:603:in `log'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/postgresql_adapter.rb:612:in `exec_no_cache'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/postgresql_adapter.rb:599:in `execute_and_clear'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/postgresql/database_statements.rb:92:in `exec_delete'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/abstract/database_statements.rb:140:in `update'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/connection_adapters/abstract/query_cache.rb:17:in `update'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/relation.rb:380:in `update_all'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/persistence.rb:333:in `update_columns'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/activerecord-5.1.4/lib/active_record/persistence.rb:306:in `update_column'
/var/www/discourse/app/models/user.rb:529:in `update_ip_address!'
/var/www/discourse/lib/auth/default_current_user_provider.rb:74:in `block in current_user'
/var/www/discourse/lib/scheduler/defer.rb:74:in `block in do_work'
/var/www/discourse/vendor/bundle/ruby/2.4.0/gems/rails_multisite-1.1.2/lib/rails_multisite/connection_management.rb:77:in `with_connection'
/var/www/discourse/lib/scheduler/defer.rb:72:in `do_work'
/var/www/discourse/lib/scheduler/defer.rb:61:in `block (2 levels) in start_thread'
1 Like

looks to me like you are forwarding an incorrect field to ip address. How do you have IP Address forwarding configured?

2 Likes

Thank you Sam.
I didn’t configure that on purpose. Where can I check the config?

In the Azure setup… somewhere. That data is not coming from within Discourse.

1 Like

It’s ip address from our company proxy. I don’t know how is the portal address added. Can we just remove the port info to accept this ip?

1 Like

No, it isn’t an IP address, and that’s the problem. If you’ve got a proxy that’s including a port number in what’s supposed to be an IP address, then the proxy is broken, and you’ll want to get that fixed.

6 Likes

I haven’t find the root cause yet. But I find a defected solution. By adding the following configuration in app.yml. I make the input of ip_address valid.

       filename: /etc/nginx/conf.d/discourse.conf
       from: $proxy_add_x_forwarded_for
       to: $http_your_original_ip_header
       global: true

The defected part is that the user ip_addresses are all 127.0.0.1 now. It’s not perfect, but the 500 errors won’t throw finally.

According to RFC 7239 - Forwarded HTTP Extension

5.2. Forwarded For

The “for” parameter is used to disclose information about the client
that initiated the request and subsequent proxies in a chain of
proxies. When proxies choose to use the “for” parameter, its default
configuration SHOULD contain an obfuscated identifier as described in
Section 6.3. If the server receiving proxied requests requires some
address-based functionality, this parameter MAY instead contain an IP
address (and, potentially, a port number
). A third option is the
“unknown” identifier described in Section 6.2.

If your nginx setting is using $proxy_add_x_forwarded_for as the indicate of user ip. I think ip with port will be a valid input here. You may need to strip the port info out.

1 Like

Well, this needs to be amended upstream then, see:

Feel free to raise a feature request for it at:

1 Like

That RFC describes the format of the Forwarded: HTTP request header. The X-Forwarded-For header, which we’re examining, has different semantics.

5 Likes

Thanks for the correction.
And for Azure Gateway, it does have a different definition for x-forwarded-for.
See Frequently asked questions about Application Gateway | Microsoft Learn

Q. Does Application Gateway support x-forwarded-for headers?

Yes, Application Gateway inserts x-forwarded-for, x-forwarded-proto, and x-forwarded-port headers into the request forwarded to the backend. The format for x-forwarded-for header is a comma-separated list of IP:Port. The valid values for x-forwarded-proto are http or https. X-forwarded-port specifies the port at which the request reached at the Application Gateway.

I will seek solution in rack or Azure Gateway.
Thanks @sam also.

1 Like

Hi,

Just to add to the conversation - this is standard practice in Azure Application Gateways, and it’s infuriating. They add the port to the forwarded-for header even though they also send it in the forwarded port header.

Two feedback/change requests on Microsoft which you can vote for;

Our “fix” is also the same as the above step, we’re manually setting the forwarded-for header to a generic ip as this caused lots of issues with users being logged out or the site not working properly.

Just out of interest, we are using a httpd redirect within the network, does anyone know if it is possible to rewrite the header and remove the port? Failing that can it be done in nginx (I am unfamiliar with nginx)?

I would look at submitting a PR if we get this fixed but the comments above seem to suggest the devs would prefer this fixing upstream - is that the case?

5 Likes

Great news! Microsoft have fixed their insufferable nonsense. Sort of!

2 Likes

Just to make it clear. This will only work for SKU V2. So if you have a V1 AppGateway you won’t be able to rewrite the header.

Yup, for us it was immediately worth upgrading to v2 when it became available for production workloads. Azure Gateways do a lot of weird things, appending ports to forward-for IP’s are just one of the opinionated things that drove us nuts. Static VIP’s are wonderful.

Auto scaling gateway capacity is great to, saved our bacon once or twice already. And zone redundancy finally is very welcome.

Migrating took about twenty minutes and we have hundreds of listeners and some relatively complicated back pools, there is a power shell script to aid you, and it’s super simple.

Not sure if you use certificates on the gateway, but if you do v2 are super super fast - no more waiting 40 minutes to apply a cert, it’s seconds now.

Pricing is far more complicated (Microsoft give with one hand, they will always take with the other) but for us, so far, the bills have been the same or lower.

5 Likes

Lucky you. Because of the higher cost we needed to switch back to SKU V1 as soon as the V2 preview has ended.

Anyway, it seems that this issue/case has already been addressed upstream by a merged PR and should be included since rack 2.0. But according to this issue it is still missing in the current releases.

Temporarily I apply a patch during CI/CD to handle this issue. It’s not that great but it will do the job until we see the fix in an upcoming rack-/discourse-release.

If anyone is interested, that’s the part you need to change to get rid of the extra port in lib/auth/default_current_user_provider.rb:

if current_user && should_update_last_seen?
  u = current_user
  ip_port_split = request.ip.split(':')
  ip_only = ip_port_split.first
  Scheduler::Defer.later "Updating Last Seen" do
    u.update_last_seen!
    u.update_ip_address!(ip_only)
  end
end

I don’t know if it is a good idea to replace any occurrence of request.ip with this quickfix anywhere else in that file or others (email_controller.rb, 006-mini_profiler.rb, request_tracker.rb) but it works for us.
As said, applying it as a patch during your build-/CI-processes keeps the code-base clean and updatable.

Any “prettier” solution is welcome.

3 Likes

A short follow up.
The “fix” I’ve provided above causes some issue like the one described here.

For now we work around this by wrapping this “split” - part into a begin…rescue…end like this:

begin
  ip_port_split = request.ip.split(':')
  ip_only = ip_port_split.first
rescue
  ip_only = request.ip
end

Kind Regards
Sascha

2 Likes

So with this commit (update to rack 2.0.8) the ip:port format of the Azure Application Gateway v1 shouldn’t be a problem anymore.

4 Likes

This topic was automatically closed after 2349 days. New replies are no longer allowed.