Shard: Add support for Authorization header #518
Conversation
There was a problem hiding this comment.
If we need to add Auth for this, instead of limiting this to basic_auth, I'm wondering if it would be better to generalize it, and implement it as an optional Authorization header that can be added. Then, this would support not only Basic Auth, but also if an upstream server was using some kind of Token or other authentication scheme 🤔
Uhm..., yeah, that would be useful. I will change this PR in that direction 👍 |
4eee35c to
f43fde0
Compare
|
@kfdm , I have reworked the PR from scratch because I think is easier to review. It is now ready for review. Thanks. |
| # PromQL Query | ||
| path("promql-query", views.PromqlQuery.as_view(), name="promql-query"), |
There was a problem hiding this comment.
This is more of a 'thinking out loud' comment, but we have a few internal APIs, like this, exporter test, and so on, I wonder if there would be value in making some kind of internal/ prefix for them, just to help organize them. There's also some older API cleanup I would like to so, so picking some kind of internal prefix might help us see what have been migrated so far.
There was a problem hiding this comment.
but we have a few internal APIs, like this, exporter test, and so on, I wonder if there would be value in making some kind of internal/ prefix for them, just to help organize them
We could. But since the others are not already organized under the same internal/ prefix, I guess we could leave this one as it is, and organize all of them in the future.
Or, if you tell me which paths on the urls.py file you want organized under an internal/ prefix, I can do that on this PR.
promgen/views.py
Outdated
| if not all(x in request.GET for x in ["shard", "query"]): | ||
| return HttpResponse("BAD REQUEST", status=400) | ||
|
|
||
| shard = models.Shard.objects.get(pk=request.GET["shard"]) |
There was a problem hiding this comment.
This is mostly a style thing,
| shard = models.Shard.objects.get(pk=request.GET["shard"]) | |
| shard = get_object_or_404(models.Shard, pk=request.GET["shard"]) |
but since we already have get__object_or_404 included, this means if for whatever reason, an invalid shard ID was called, this would result in a 404 instead of a 500.
There was a problem hiding this comment.
Makes sense. Done in 04b06cc. Thanks 👍
This new field in the Shard model will hold the value for the HTTP Authorization header. Although unused at the moment, this will give us the possibility to support Shards that require authentication.
The API end point of some shards are protected with HTTP Basic Auth. This is making the promql-query component to fail receiving a '401 Unauthorized' error response. Now that the Shard model supports an "authorization" field to specify the value of the "Authorization" HTTP header, we can address this issue. However, if we perform the request in the front end, that information would be visible by a simple inspection of the HTTP headers. Because of that we have moved it to the back end.
Now that our Shard model supports an "authorization" field to specify the value of the HTTP "Authorization" header, we can use it in our PrometheusProxy class. Note that we have replace "host" with "shard" since the objects obtained by the 'models.Shard.objects.filter(proxy=True)' instruction are shards, not hosts.
04b06cc to
189e07e
Compare
Thanks. Rebased on |
The API end point of some shards are protected with HTTP Basic Auth. This will make the promql-query component to fail receiving a '401 Unauthorized' error response.
To address that, we have added the possibility to specify HTTP Basic Auth credentials on shards.
Apart from that, the query is performed in the back end for security issues. Since the request's "Authorization" header contains the base64-encoded user and password, if we performed the query in the front end that information would be easy to obtain by inspecting the HTTP requests.