separate security for control and status actions

[tkishel]

Is your feature request related to a problem? Please describe.

Authentication for control and status actions in Puma::App::Status is implemented via the same control-token option, while the security requirements related to control actions like halt and status actionsstats differ significantly.

Describe the solution you'd like

Separate tokens for control and status actions.
Eliminate the need for a token to access status actions, reserving it for control actions.

Describe alternatives you've considered

An option to set the authorization level (control | status) for the control-token option.
Separate tokens for control and status actions.

[nateberkopec]

What if, instead, we allowed you to configure a callback lambda/proc?

[tkishel]

I'm open to any implementation that allows users to configure querying status independently from controlling the server :) Might you have an example that illustrates what you are thinking about?

[tkishel]

How about this ...

config

control-url "ssl://0.0.0.0:12345? ..."
control-token "hello"
control-actions ['stats']

status.rb

      def initialize(cli, token = nil, actions = [])
        @cli = cli
        @auth_token = token
        @auth_actions = actions
      end

      def authorized(action)
        return true if @auth_actions.empty?
        @auth_actions.include? action
      end

      def not_authorized(action)
        rack_response(401, "Action '#{action}' not authorized", 'text/plain')      
      end

      def call(env)
        unless authenticate(env)
          return rack_response(403, 'Invalid auth token', 'text/plain')
        end

        case env['PATH_INFO']
        when /\/(stop)$/
          action = $1         
          return not_authorized(action) unless authorized(action)
          @cli.stop
          rack_response(200, OK_STATUS)

        when /\/(stats)$/
          action = $1         
          return not_authorized(action) unless authorized(action)
          rack_response(200, @cli.stats)

resulting in

# curl https://puma.example.com:12345/stop?token=hello
Action 'stop' not authorized

# curl https://puma.example.com:12345/stats?token=hello
{ "backlog": 0, "running": 0, "pool_capacity": 10, "max_threads": 10 }

[tkishel]

See #2083

Not a lambda, but very simple, and I assume the array could be replaced ...

[bdewater]

I'm open to any implementation that allows users to configure querying status independently from controlling the server :)

Isn't this possible to implement in your application?

You might need to JSON.parse(Puma.stats) in your own application code to get a hash to for data manipulation/extraction, but that requirement will go away with #2086

[tkishel]

It is, but ...

1. Having the token provide (remote) access to both control and status in the default app is an inherent security issue.
2. Empirically (this is where I may be in over my head) querying status outside our app(s) doesn't consume a thread (or that thread is excluded or exits quickly) while querying it inside our app(s) results in the thread lingering and therefore modifies what we are measuring by measuring ...

# Via PR 2083:
curl -k https://example.puppetdebug.vlan:62657/stats?
{ "started_at": "2019-12-17T00:16:04Z", "backlog": 0, "running": 0, "pool_capacity": 100, "max_threads": 100 }

# Via another app:
curl -k https://https://example.puppetdebug.vlan:62658/admin/status
{ "started_at": "2019-12-17T00:16:04Z", "backlog": 0, "running": 1, "pool_capacity": 99, "max_threads": 100 }

#2083 allows users to configure fine-grained configuration of what actions are authorized by the token, and I'd like to propose closing this in favor of that PR ... unless you decline both :)

[nateberkopec]

Having the token provide (remote) access to both control and status in the default app is an inherent security issue.

No, it just means we're over-securing status. That's not a "security issue", just inconvenient. Just want to be clear here.

[tkishel]

Yes: it would be lovely to not need a token to access the stats and gc_stats endpoints.

No, it just means we're over-securing status.

So, should I close both of my PRs as non-starters? Or would you be interested in me simplifying one of them, to remove the check for a token for the stats and gc_stats actions? (I originally considered that, but thought it would be a backwards-incompatible change in security.) That should be as simple as moving unless authenticate(env) into each of the case conditions involving control actions ...

[bdewater]

Empirically (this is where I may be in over my head) querying status outside our app(s) doesn't consume a thread (or that thread is excluded or exits quickly)

You're right, I didn't know but it is running a separate server for that here:

puma/lib/puma/runner.rb

Lines 61 to 65 in f5ccd03

	app = Puma::App::Status.new @launcher, token
	control = Puma::Server.new app, @launcher.events
	control.min_threads = 0
	control.max_threads = 1

[tkishel]

Yay! I remember seeing that but did not make the connection.