Introduce an authentication & authorization scheme #7
Conversation
This commit introduces the exodus_gw.auth module containing
helpers for ensuring that the current request is authenticated
and determining the associated roles.
It is designed to integrate with an existing authentication scheme
shared by various services. In summary, it works as follows:
- exodus-gw itself does not authenticate requests
- exodus-gw should be deployed behind a reverse-proxy which performs
authentication
- the authenticating proxy will add a json-in-base64 header to
incoming requests, of the form (abbreviated):
{
"user": {
"roles": ["some-role"],
"authenticated": true,
"internalUsername": "some-user"
}
}
- exodus-gw parses and trusts this header and uses it to decide
whether or not the current request is authorized.
Thus, exodus-gw supports any authentication & authorization scheme
implemented by the reverse proxy. In practice, mutual TLS is
expected to be used.
This commit adds authentication functionality, but none of the
existing endpoints require authentication yet, so it's only used
from autotests.
Other concepts introduced by this commit include:
- usage of the fastapi dependency injection framework (Depends).
- usage of pydantic settings, which allows all settings to be
overridden by environment variables (12-factor app style).
Things which could be added here but are instead planned for
later commits include:
- documentation on the authentication scheme and the settings.
This appears to fit best in some kind of deployment guide doc,
which doesn't exist yet.
- requiring authentication on the upload API. I think each
target environment for uploads should have a separate role, so
it requires implementing support for multiple environments
first.
|
More details on the internal component this is designed to work with: https://url.corp.redhat.com/df92983 |
|
@rohanpm This topic deserves a meeting to share the story, design, implementation and tests. WDYT? |
@lioramilbaum meetings ruin my productivity, I don't want a workflow where many PRs are triggering a meeting. We have the concept of recording & sharing demos in this project, it covers most of the points you mentioned (but usually happens later). A possible compromise would be preparing the demos earlier (closer to code review time so it's still easier to change things). It'd be good to hear some opinions on this from the other devs on whether we need to adjust the workflow here. |
|
@rohanpm My suggestion drives from the whole team members productivity. When one of team members submit a PR which relates to a new requirement+solution, than the rest of the team should be able to understand the code, be able to review it in an effective and timely manner and even suggest other approaches. This is a more inclusive approach which connects all team members to the task at hand and the goals of the team. This is my 2 cents on this matter. |
|
I think it's perfectly reasonable to request information/discussion on a design decision, and I think the dev behind that decision should accommodate within reason. The medium in which that happens, though, isn't something I think can't be set in stone and depends entirely on the parties involved -- different people prefer different methods of communication. Now, for this situation specifically, I would say some kind of design document may be in order, explaining what we know of the service and how we expect it to integrate. Between what was written in https://projects.engineering.redhat.com/browse/RHELDST-2163, grooming on that issue (recorded? link?), the PR description, and the link to the Sidecar project, I think enough information was presented. It would still be nice to consolidate those into a single document where questions/concerns could be raised. I wouldn't suggest a design spike or anything, just attaching a doc to the Jira issue should suffice. |
|
OK, the way we usually handle this kind of thing is to use a Google doc to explain and propose a design first and collaborate there to resolve all the questions before pushing anything for review. I guess there is no particular reason that step has been skipped this time.
@nathanegillett I read that as "no need to file a separate issue", but the doc format would be similar as what we do for a spike. I don't mind writing such a doc at all, I can put this PR on hold for a bit and prepare the accompanying doc. |
|
@rohanpm Do you have ETA to the write-up you've suggested? |
Early to mid next week is likely now. Should have been earlier but I had some unexpected leave. |
|
Alright, sorry it took so long, here is the promised doc: https://docs.google.com/document/d/1Yt0_pfrigFH9JCXtfA5ffmNbJmfsfQ41AwD-vvwN_C8/edit?usp=sharing I'll aim to submit this early next week if there are no concerns raised here or on the doc. |
This commit introduces the exodus_gw.auth module containing
helpers for ensuring that the current request is authenticated
and determining the associated roles.
It is designed to integrate with an existing authentication scheme
shared by various services. In summary, it works as follows:
exodus-gw itself does not authenticate requests
exodus-gw should be deployed behind a reverse-proxy which performs
authentication
the authenticating proxy will add a json-in-base64 header to
incoming requests, of the form (abbreviated):
whether or not the current request is authorized.
Thus, exodus-gw supports any authentication & authorization scheme
implemented by the reverse proxy. In practice, mutual TLS is
expected to be used.
This commit adds authentication functionality, but none of the
existing endpoints require authentication yet, so it's only used
from autotests.
Other concepts introduced by this commit include:
usage of the fastapi dependency injection framework (Depends).
usage of pydantic settings, which allows all settings to be
overridden by environment variables (12-factor app style).
Things which could be added here but are instead planned for
later commits include:
documentation on the authentication scheme and the settings.
This appears to fit best in some kind of deployment guide doc,
which doesn't exist yet.
requiring authentication on the upload API. I think each
target environment for uploads should have a separate role, so
it requires implementing support for multiple environments
first.