(PE-27794) add a puma status app #1452
Conversation
|
No longer requires vanagon changes. |
|
CLA signed by all contributors. |
lib/bolt_server/config.rb
Outdated
| @@ -21,6 +21,8 @@ def int_keys | |||
| def defaults | |||
| super.merge( | |||
| 'port' => 62658, | |||
| 'status-port' => 62659, | |||
| 'status-token' => "bolt", | |||
There was a problem hiding this comment.
Is the token sensitive? Would we lay down one with pe? Can we use the ssl-{cert,key,ca-cert} for auth?
There was a problem hiding this comment.
Puma 4.3.0 implements that in the built-in Status app. We could too, if we model:
But I kept the logic of this app identical to the app in the version of puma we currently ship.
There was a problem hiding this comment.
I think we need to update our puma stack anyway. If we got on 4.3.0 we would still need the puma-stats gem?
There was a problem hiding this comment.
Yes, because the Status app ...
https://github.com/puma/puma/blob/master/lib/puma/app/status.rb
... includes both Control and Status, which, in principle, I think is inappropriate: the security requirements for querying status (metrics) are significantly different that the same for controlling (restarting / shutting down) the service.
There was a problem hiding this comment.
Note that the metrics from Puppet Server do not require a client certificate/whitelist:
[root@pe-201921-replica-ha ~]# curl -k https://pe-201921-master.puppetdebug.vlan:8140/status/v1/services?level=debug
{"puppet-profiler":{"service_version":"6.7.1","service_status_version":1,"detail_level":"debug","state"
...
[root@pe-201921-master ~]# grep -rl pe-201921-replica-ha /etc/puppetlabs | wc -l
0
But I may be able to implement ssl:// in the plugin ...
There was a problem hiding this comment.
OK. I ticketed updating puma https://tickets.puppetlabs.com/browse/PE-27840
There was a problem hiding this comment.
As a POC, I added the MiniSSL::ContextBuilder class from the current version of Puma to the plugin (I could simply remove it if we upgrade to Puma 4.3+) and can serve status via:
if config['status-host'] && config['status-port'] && config['status-token']
plugin 'stats'
bind_addr = +"ssl://#{config['status-host']}:#{config['status-port']}?"
bind_addr << "cert=#{config['ssl-cert']}"
bind_addr << "&key=#{config['ssl-key']}"
bind_addr << "&ca=#{config['ssl-ca-cert']}"
bind_addr << "&verify_mode=none"
bind_addr << "&ssl_cipher_filter=#{config['ssl-cipher-suites'].join(':')}"
stats_url bind_addr
stats_token config['status-token']
end
And access that via:
[root@pe-201921-master ~]# curl https://pe-201921-master.puppetdebug.vlan:44634/puma-stats?token=ace
curl: (60) Peer's certificate issuer has been marked as not trusted by the user.
[root@pe-201921-master ~]# curl --insecure https://pe-201921-master.puppetdebug.vlan:44634/puma-stats?token=ace
{ "backlog": 0, "running": 0, "pool_capacity": 10, "max_threads": 10 }
[root@pe-201921-master ~]# curl --cacert /etc/puppetlabs/puppet/ssl/certs/ca.pem https://pe-201921-master.puppetdebug.vlan:44634/puma-stats?token=ace
{ "backlog": 0, "running": 0, "pool_capacity": 10, "max_threads": 10 }
[root@pe-201921-master ~]# curl --cacert /etc/puppetlabs/puppet/ssl/certs/ca.pem https://pe-201921-master.puppetdebug.vlan:44634/puma-stats?token=zzz
Invalid stats auth token
Is there a direction to move forward that you prefer/recommend?
There was a problem hiding this comment.
I will defer to @branan or @mcdonaldseanp on that. I don't fully understand the impact of having to manage a token for this.
There was a problem hiding this comment.
Okay. Note that not specifying a token in config/transport_service_config.rb or conf.d/bolt-server.conf will disable the token requirement :)
|
Resulting in: The connection is encrypted, but the client does not need to provide a certificate, same as with our other services. The token remains, to be parallel with the default Control/Status app. |
|
If we're gonna maintain this puma stats thing, I'd prefer to pull it into our namespace and ensure it's a known gem to RE, etc. But really, I think I'd prefer we get our version of Puma up-to-date, then try to upstream the ability to enable status without control. That way we're not maintaining anything 😎 |
|
I hear you ... I've submitted a feature and pr to the puma repository to implement separate tokens for control and status actions in the (sigh) Control Server implemented as the Status app. If it is accepted, I can easily modify the associated PRs in our repositories to match. That said, the puma-stats plugin/gem is a trivial fork of another (metrics) plugin that I would happily move into our namespace, and if we updated our Puma to 4.3.x or newer, would be almost identical to the implementation of the Status app (minus the control actions). An argument for implementing our own status code would be if we wanted to extend it beyond what puma offers. |
|
Updated POC. Smaller footprint. Requires Puma 4.3.x |
Include the puma-stats app/plugin locally rather than as a gem.
With this commit, a puma status app (the default or ours) is loaded. Requires Puma 4.3.x
tkishel
changed the title
|
Closing in favor of the simpler #1503 |
Adds a status app, which can be removed when puma improves security in its status app.