-
Notifications
You must be signed in to change notification settings - Fork 473
WIP Password auth design doc #32005
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
WIP Password auth design doc #32005
Conversation
WIP password auth design doc
Regarding closing off the internal port for Cloud in the cloud sync:
|
Option A) - preferred Option B) - backup
|
Teleport prefers mutual TLS, and we've actually gone out of our way to disable it. It's been on the roadmap for a long time to get that working, but never got to it. |
Here are my thoughts on implementation:
The rest of it as laid out in the doc makes sense to me |
On system users... We might be able to pass in a bootstrap user that is also considered internal/system. This user should be considered superuser and internal user and it's password should come from a k8s secret. The k8s secret passed in can be used as it's password secrets reference in the catalog. |
Still not clear to me the flow of setting up auth. In my head, it looks like:
Does that sound correct? Then we just make it a limitation that a user can't use the Console unless there's a registered role. In the Console, we'll most likely need the following information
Should also consider the migration path from self managed customers without auth to customers with auth. |
|
||
### 4. Configurable admin system login: | ||
|
||
Passwords for `mz_system` and `mz_support` roles will be settable via environment variables `MZ_MZ_<USER>_EXTERNAL_LOGIN_PASSWORD`. Orchestratord should provide a set of parameters to set these variables via a Kubernetes secret. Additionally, We should enable login of system users through the external ports when they have external login passwords set. Login through the external port must not be possible unless this flag is set, this logic should not rely on whether the internal user has a password. |
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.
This may need an updated Materialize CRD to add a new field for the name of the secret.
@SangJunBak +1 to everything you said.
Maybe we can pass that to the console also? Maybe through an env var? That gets a bit tricky, since I think we update the balancers and console immediately upon getting an updated Materialize, while only updating environmentd if we trigger a rollout. Perhaps this logic needs to change, but that may be difficult too. |
Your initial flow is correct!
Unless they have what? The standard auth for postgres has login and non login users see pg_authid. For logins, one can first check that the role allows login then compare passwords. This would be done fully on the server side. I imagine we want to enable rolcanlogin if a password is set, and by that just check to see |
@alex-hunt-materialize and with the current design, an env variable won't do much good given the Console's already built by the time orchestratord installs the console pod. There's currently no way for orchestratord to edit build variables for the Console, which I think we should fix! |
Console shouldn't need to query pg_authid. It should just need to attempt to connect using username and password. If the role isn't a login role or there's a password mismatch that should all be handled server side. To determine if auth is enabled it could just attempt to run a query and, on failed login, route to the login box, or we can install a configurable endpoint on NGINX that console can hit to tell it what kind of auth it should attempt to use. |
291b450
to
2c5754d
Compare
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.
LGTM on my end! Thanks for writing all this out!
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.
My "hard requirements"
- Set the minimum iteration count >=200,000.
- Do not allow lockouts by default.
- Log successful and failed login attempts by default.
I've made some additional comments and recommendations for discussion / consideration. Thanks for writing this all up and the consideration in this design.
6671220
to
80398a2
Compare
should this include a threat analysis? |
This is the bones of self managed password auth. This is missing integration tests and documentation which will come next. You can test this locally like so: ```shell #this starts materialize with auth enabled on the external port but not the internal port $ bin/environmentd --bazel --reset -- --all-features --unsafe-mode --enable-self-hosted-auth #start a mz_system session, turn on sql support and create a user $ psql -U mz_system -h localhost -p 6877 materialize NOTICE: connected to Materialize v0.139.0-dev.0 Org ID: 1bd2c405-c638-44cc-b917-6d05dfb832ac Region: local/az1 User: mz_system Cluster: mz_system Database: materialize Schema: public Session UUID: fe6d2dbe-3a94-430d-82e1-5e79cb14b91f Issue a SQL query to get started. Need help? View documentation: https://materialize.com/s/docs Join our Slack community: https://materialize.com/s/chat psql (14.17 (Homebrew), server 9.5.0) Type "help" for help. materialize=> alter system set enable_self_managed_auth=true; NOTICE: variable "enable_self_managed_auth" was updated for the system, this will have no effect on the current session ALTER SYSTEM materialize=> create role foo with superuser password 'bar'; CREATE ROLE # Now connect over the port with auth enabled $ psql -U foo -h localhost -p 6875 materialize Password for user foo: ``` This begins implementing #32005 <!-- Describe the contents of the PR briefly but completely. If you write detailed commit messages, it is acceptable to copy/paste them here, or write "see commit messages for details." If there is only one commit in the PR, GitHub will have already added its commit message above. --> ### Motivation <!-- Which of the following best describes the motivation behind this PR? * This PR fixes a recognized bug. [Ensure issue is linked somewhere.] * This PR adds a known-desirable feature. [Ensure issue is linked somewhere.] * This PR fixes a previously unreported bug. [Describe the bug in detail, as if you were filing a bug report.] * This PR adds a feature that has not yet been specified. [Write a brief specification for the feature, including justification for its inclusion in Materialize, as if you were writing the original feature specification.] * This PR refactors existing code. [Describe what was wrong with the existing code, if it is not obvious.] --> ### Tips for reviewer <!-- Leave some tips for your reviewer, like: * The diff is much smaller if viewed with whitespace hidden. * [Some function/module/file] deserves extra attention. * [Some function/module/file] is pure code movement and only needs a skim. Delete this section if no tips. --> ### Checklist - [ ] This PR has adequate test coverage / QA involvement has been duly considered. ([trigger-ci for additional test/nightly runs](https://trigger-ci.dev.materialize.com/)) - [ ] This PR has an associated up-to-date [design doc](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/README.md), is a design doc ([template](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/design/00000000_template.md)), or is sufficiently small to not require a design. <!-- Reference the design in the description. --> - [ ] If this PR evolves [an existing `$T ⇔ Proto$T` mapping](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/command-and-response-binary-encoding.md) (possibly in a backwards-incompatible way), then it is tagged with a `T-proto` label. - [ ] If this PR will require changes to cloud orchestration or tests, there is a companion cloud PR to account for those changes that is tagged with the release-blocker label ([example](MaterializeInc/cloud#5021)). <!-- Ask in #team-cloud on Slack if you need help preparing the cloud PR. --> - [ ] If this PR includes major [user-facing behavior changes](https://github.com/MaterializeInc/materialize/blob/main/doc/developer/guide-changes.md#what-changes-require-a-release-note), I have pinged the relevant PM to schedule a changelog post.
80398a2
to
a91ab68
Compare
This will be a self-managed only feature which will be scoped different for security audits. I'll leave it up to @jasonhernandez to sort out how we deal with this. |
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.
Thanks for setting a 400k iteration count as the default. This looks like a good v1 design to ship.
Motivation
Tips for reviewer
Checklist
$T ⇔ Proto$T
mapping (possibly in a backwards-incompatible way), then it is tagged with aT-proto
label.