Write real Rack middleware for authorization

[ekohl]

This is to share my work in progress code. The idea is to utilize Rack middleware to implement authorization checking. Modules can then simply call it. However, this is not finished or even working. That's why it's a draft.

[lzap]

Thanks for pushing this early, what is your motivation to do this?

What is the benefit here?

There is one major drawback, some plugins (e.g. OpenSCAP) performs different authorization in a single SinatraApp for different paths. It is an unfortunate design, but it is what it is. Middleware approach not only does not allow to do per-path authorization, you literally deleted those methods so there is no way to call it explicitly.

This patch would work nicely if we choose to introduce a new port for clients, then we would simply use middleware for all requests coming to the endpoint from Foreman. Or maybe better, if there was a prefix for all Foreman calls, let's say /foreman, we could put such middleware only for this patch and then migrate all calls from foreman there.

But unless you find a solution for OpenSCAP, this can't be done. The code logic could be extracted so it could be called from OpenSCAP, but that is not ideal. In my opinion, I think we should start moving all requests which should be authorized differently to a different path. We can either move foreman calls (/foreman) or clients (/clients) or both. I think there are fewer endpoints for clients, that could be a better way, however on the other hand it is more difficult to upgrade all clients so the transition could be actually easier if we moved foreman endpoints

[ekohl]

Thanks for pushing this early, what is your motivation to do this?

This feels like a much more native way to perform authentication. I believe it'll end up working with less code. IMHO it should be easy for module maintainers to do it right and secure.

From what I've seen, in practice all modules use either both authentication methods (HTTPS verification & authorize against trusted hosts) or neither.

Another motivation was to make it easier to use Puma. If feels like handling Webrick vs Puma in middleware is easier then separate methods.

There is one major drawback, some plugins (e.g. OpenSCAP) performs different authorization in a single SinatraApp for different paths. It is an unfortunate design, but it is what it is. Middleware approach not only does not allow to do per-path authorization, you literally deleted those methods so there is no way to call it explicitly.

I'm not sure this is a fundamental limitation of Rack middleware. It really depends on how you want to use it. That happens in the rackup file. Take the example of https://www.rubydoc.info/github/rack/rack/master/Rack/Builder

require 'rack/lobster'
app = Rack::Builder.new do
  use Rack::CommonLogger
  use Rack::ShowExceptions
  map "/lobster" do
    use Rack::Lint
    run Rack::Lobster.new
  end
end

run app

That means you could only use the middleware on some paths. Note that I haven't converted any module yet so time will tell if that really works.

This patch would work nicely if we choose to introduce a new port for clients, then we would simply use middleware for all requests coming to the endpoint from Foreman. Or maybe better, if there was a prefix for all Foreman calls, let's say /foreman, we could put such middleware only for this patch and then migrate all calls from foreman there.

A common prefix is not how Rack works and I don't think it's technically feasible. However, I do think we should have a secure-by-default mode. Looking at the modules, the most common thing that you want is all requests authenticated.

But unless you find a solution for OpenSCAP, this can't be done. The code logic could be extracted so it could be called from OpenSCAP, but that is not ideal. In my opinion, I think we should start moving all requests which should be authorized differently to a different path. We can either move foreman calls (/foreman) or clients (/clients) or both. I think there are fewer endpoints for clients, that could be a better way, however on the other hand it is more difficult to upgrade all clients so the transition could be actually easier if we moved foreman endpoints

I agree there may be some challenges with mixed mode. I knew there were some out there, but thanks for naming an explicit example.

[lzap]

Overall I like it where this is going, make no mistake. I just want to be precautions so you know there might be struggles ahead. Per path authorization is a terrible idea, we had to introduce it into OpenSCAP due to security vulnerability. Clients and Foreman API should have been strictly split from day one.

[ekohl]

I did some more basic testing:

$ curl https://$HOSTNAME:8443/dns --cacert ~/dev/ownca/cacert.crt -i ; echo
HTTP/1.1 401 Unauthorized
Server: foreman-proxy/2.6.0-develop
Date: Thu, 06 May 2021 12:57:24 GMT
Content-Length: 12
Connection: Keep-Alive

Unauthorized
$ curl https://$HOSTNAME:8443/dns --cacert ~/dev/ownca/cacert.crt --cert ~/dev/ownca/$HOSTNAME/$HOSTNAME.crt --key ~/dev/ownca/$HOSTNAME/$HOSTNAME.key -i ; echo
HTTP/1.1 404 Not Found
Content-Type: application/json
X-Cascade: pass
Content-Length: 27
X-Content-Type-Options: nosniff
Server: foreman-proxy/2.6.0-develop
Date: Thu, 06 May 2021 12:58:55 GMT
Connection: Keep-Alive

Requested url was not found

So this is roughly where I want to go.

Technically you can call use in the rackup file (http_config.ru) or in the API itself. I now opted to do it in the rackup file to keep the API files smaller but I'm not sure what is really the best way.

I'll also look into the mixed case like OpenSCAP.

Still considered a project under investigation but the goal is an easier to use API for Smart Proxy module authors.

[lzap]

Frankly, I was hoping that this patch will improve security in a way that there would be one generic middleware that would secure all requests. Therefore plugin author who would forget to solve authorization for a new endpoint would automatically be covered without even knowing it. The current version just moves the problem from Sinatra before method to Rack middleware which does not gets anywhere.

For the generic middleware idea, there would need to be some mechanism (e.g. API or configuration) where plugins (and core modules) could specify which paths or Sinatra apps are actually allowed without particular authorization type (SSL, trusted hosts). I believe this would be huge step fotward.

[ekohl]

We are on the same page for that but it's something I still need to look into. You can register middleware by default, as we do here:

smart-proxy/lib/smart_proxy_main.rb

Lines 46 to 47 in 6c0ee87

	::Sinatra::Base.use ::Proxy::RequestIdMiddleware
	::Sinatra::Base.use ::Proxy::LoggerMiddleware

My idea was to make it somehow configurable to make it opt-out instead of opt in, but I don't know if that's possible. That would be secure by default, unless you explicitly configure an endpoint to be insecure.

We do need to check if all modules inherit from Sinatra::Base. The built in modules do, but I haven't checked external ones.