-
Notifications
You must be signed in to change notification settings - Fork 60
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add gateway authentication #1067
base: main
Are you sure you want to change the base?
Conversation
I will squash before merge |
First question, just based on the PR description. You mention:
How does this work with multiple load-balanced worker service nodes (which is the normal way to set up a real golem cluster)? |
Yes. Absolutely. As I mentioned in the PR description the plan is to add redis backend. But as another PR. Cloud will not work without this anyway and I will be forced to add (and there won't be accidental skipping). Happy to push that into the current PR. Just stopped committing once the tests becoming green after fixing other corner cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will continue tomorrow
golem-worker-service-base/src/gateway_api_definition/http/http_api_definition.rs
Show resolved
Hide resolved
golem-worker-service-base/src/gateway_api_definition_transformer/mod.rs
Outdated
Show resolved
Hide resolved
golem-worker-service-base/src/gateway_execution/gateway_session.rs
Outdated
Show resolved
Hide resolved
golem-worker-service-base/src/gateway_execution/gateway_session.rs
Outdated
Show resolved
Hide resolved
golem-worker-service-base/src/gateway_middleware/middleware_in.rs
Outdated
Show resolved
Hide resolved
golem-worker-service-base/src/gateway_middleware/middleware_out.rs
Outdated
Show resolved
Hide resolved
@mschuwalow Tried to resolve many of your comments thanks for the review. Transformers are top level functions and so for middleware in and out (initially I had troubles, propagation of some history there, but fixed it). I think these are useful comments. I would prefer doing the rest (if any later), especially IdentityProvider. Initially provider is a trait, but not each one of them. Initially they were in the commit history, and later to moved to a single IdentityProvider implementation. If we have diverging implementations we can consider it as each provider as a trait. Or we can think about it later |
* Add redis * Update tests * Update tests * Update tests * Simulate session expiry without introducing sleep or test flakiness * Add redis config
Add worker gateway authentication resolving. Please excuse the verbose PR description, but I request you to go through it before reviewing code.
#1061
#1062
#1063
#1064
#1060
#1065
As part of these tickets, there are some important changes done here - mainly focussing on user experience with authentication, CORS and similar middleware(s) in API gateway, that can be very difficult to fix in future if not taken care now.
Notes on OpenAPI
This was also a learning as to how much we can keep up with the real OpenAPI spec. The native API definition (user-facing part) is now mostly inspired from OpenAPI spec itself.
API Gateway is powerful enough, that you don't need all the details required in OpenAPI spec all the time. Trying to add them to align strongly with OpenAPI spec can be really confusing. Example: There is no need of
openIdConnectUrl
which is part of the OpenAPI spec - just the security scheme id is enough. The reasoning here is, not many API gateways really support this level of authentication as far as I see. While OpenAPI spec is designed and geared to be part of the actual application and not the gateway. Another example is, there is no clear path on how to setup cors in a standard OpenAPI spec - while it is aware of some headers, but not entirely.API Gateway doesn't have all of the features that an advanced OpenAPI spec may refer to. However, by the time we support the major ones (which we are already in progress with), we will end up having the above situation. Example: We support only OpenID based authentication, while OpenAPI can list a set of securities to be applied. I haven't done the sequential application of security yet. Supporting a list right now can only confuse developers.
Hints on what's in PR
HttpApiDefinitionRequest
(user facing internal API definition structure which is formed either through direct native-api-definition upload or OpenAPI import). As an example, this will have concise information about security (there is more to it, which is further explained below)HttpApiDefinition
(explained below). Transformations are idempotent.HttpApiDefinition
. This will have detailed information about security, added auth call back endpoints based on the redirect-url of user's security scheme with Google (as an example), cors-header-additions based on preflight endpoints etc.CompiledHttpApiDefinition
(not much change here except compiler driven changes). This is HttpApiDefinition itself, but with pre compiled rib, input type information for each Rib used (worker name and response mapping)HttpApiDefinitionResponse
- This is user facing response (unexposed byte code, unexposed identity provider metadata, unexposed secrets etc). There is an extra HttpApiDefinitionRequestData which is almost 1-1 with HttpApiDefinitionRequest, however, I didn't focus on fixing this. Duplicate data came into existence for some reason, but such details can be dealt later, as this is not affecting user's hot path.None
. It can beNone
for certain other cases too such as if the middleware results inUnauthorized
response or aredirect
response.ToResponse<R>
was hindering development to a great extent and it only got worse in last couple of days, so I made it explicit forHttp
last day. Basically the workflow is much more easier to understand now. History: This was mainly done to reuse the workflow (resolve the binding and execute the binding while handling middlewares etc) between different protocols long time back, however, it was giving more pain than needed. We will figure out how other protocols can be incorporated later. It's not a concern of this PR. In other words,gateway_input_executor
is changed to gateway_http_input_executorExperimental in nature
Being able to tag a security which is a one-liner in configuration (OpenAPI as well as native API definition), and then specify things like
request.auth.email
in a Rib script makes the whole workflow production grade. That is, not anyone can simply invoke a worker even in a publicly exposed API.The changes done are still experimental as there are numerous possibilities of configurations and usage of these features. It' less ROI to go through all of those possibilities in the first attempt, and therefore the PR is more of getting one thing right with focus on extensibility, and not focussing much on minor details which can be added/fixed based on testing in coming weeks. Example: Adding cookies is just inlined straight away for the auth call back responses - we are not making it configurable and complicating it further - because we don't know if this is worth it yet.
I took an extra week for this work, but I strongly believe the details figured out are worth it, and offloading them to future would result in weeks of downtime.