Regression test for auth bug#37326
Closed
SangJunBak wants to merge 11 commits into
Closed
Conversation
Copy over the test Dennis created for this bug
I've outlined the root cause of the bug. It would be easy to do a one-off fix and do an authorization check for the internal routes, however that would violate the semantics of the listener config and make our implementation more confusing than it already is. The solution I propose aims to make our server code simpler while preserving the config's semantics, at the cost of a bigger refactor.
We modify the schema of the listener config: - Creates new RouteGroup struct rather than a bool representing if a group of routes is enabled - Lowers `allowed_roles` to each RouteGroup - Creates a separate JSON serialization representing Rust's associated enum type We also implement a single middleware for all authorization and decouple it from authentication - We take advantage that all authentication creates an AuthedUser. Since Authz/authorization is just a function of the user name and the route's policy, We can create a middleware to keep concerns separate. We already pass along the AuthedUser as an extension, so we can attach another extension to carry over the route's policy.
We update test utils and misc to follow the new schema
- Version gate the new listener config - Implement a migration from the old listener config to the current one to avoid having to redefine one in orchestratord when gating. - This is the change that will actually have an effect for all deployments in Cloud and self managed
We now match what we'd deploy in production and set allowed_roles to Internal for the internal/profiling routes
Almost all of these changes are simple one line refactors / moving files around. - For Password/SASL/OIDC listener configs, set internal from NormalAndInternal to Internal to maintain consistency - Move v0_145_0 configs to its own folder, as well as v26_31_0 - Update materialized docker image to copy a version's folder
We create a separate function for resolving the listener config depending on the version / whether there's an upgrade. This is important such that our upgrade tests don't break. For anything that above that overrides these listener configs, we override these too
We can now remove the plan. It was mostly for the purpose of documentation / helping the reviewer.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Test off #37260
The nightly that this PR is meant for https://buildkite.com/materialize/nightly/builds/16906