server: require web login if an env var is set #25057
Conversation
|
This change is |
vilterp
force-pushed
the
require-login-env-var
branch
from
d7eca5c
to
7541d77
Compare
There was a problem hiding this comment.
One thing to consider: there is an interaction with the "insecure" flag which is called out in the RFC for web session logins. Once login is released, it will be on by default for secure servers, but disabled for insecure servers. This is important because, in turn, it allows our frontend to detect whether or not authentication is enabled simply by checking the protocol (HTTP vs HTTPS). Otherwise, you need an additional mechanism to communicate this setting to the frontend so that it knows whether or not to require a login.
In other words: I think the system for enabling/disabling login is more complicated than is expressed here, i'm not sure we should be checking anything in until its fully considered.
pkg/server/config.go
Outdated
| @@ -386,7 +386,7 @@ func MakeConfig(ctx context.Context, st *cluster.Settings) Config { | |||
| ScanInterval: defaultScanInterval, | |||
| ScanMaxIdleTime: defaultScanMaxIdleTime, | |||
| EventLogEnabled: defaultEventLogEnabled, | |||
| EnableWebSessionAuthentication: defaultEnableWebSessionAuthentication, | |||
| EnableWebSessionAuthentication: envutil.EnvOrDefaultBool("COCKROACH_REQUIRE_WEB_LOGIN", false), | |||
There was a problem hiding this comment.
Move this to a top-level var and reference it here.
|
@mrtracy the env var only applies if we're in secure mode. Just updated the description to reflect that. It's true that this is just to avoid myself, @couchand, and anyone else who is trying to test login having to twiddle a hardcoded boolean over the next couple months, so we can do that if necessary. |
vilterp
force-pushed
the
require-login-env-var
branch
from
7541d77
to
c8e0305
Compare
|
LGTM |
There was a problem hiding this comment.
As I mentioned to you yesterday, I'm not sure this is worth it. It's doesn't seem to make things that much easier for us, and there's a risk involved.
Changing a line of code screams "this really isn't done yet". Adding an env var (even one with EXPERIMENTAL in the name) seems like maybe it's okay to do.
pkg/server/config.go
Outdated
| @@ -376,6 +375,8 @@ func MakeConfig(ctx context.Context, st *cluster.Settings) Config { | |||
| panic(err) | |||
| } | |||
|
|
|||
| requireWebLogin := envutil.EnvOrDefaultBool("COCKROACH_REQUIRE_WEB_LOGIN", false) | |||
There was a problem hiding this comment.
If this is added it should definitely have EXPERIMENTAL in the name.
Release note (admin ui change): require web login if the environment variable COCKROACH_REQUIRE_WEB_LOGIN is set to true.
vilterp
force-pushed
the
require-login-env-var
branch
from
c8e0305
to
a35b8d3
Compare
|
Changed to bors r+ |
Build failed |
|
bors r+ |
25057: server: require web login if an env var is set r=vilterp a=vilterp This PR introduces the temporary environment variable `COCKROACH_REQUIRE_LOGIN` to enable testing login functionality before a fully functional login/logout UI is merged. Supersedes #24944, which hardcoded this. Once we have a functional login/logout UI, we will probably hardcode this variable to true, so that login will always be required when the cluster is in secure mode. Before this change: - Admin server endpoints don't care whether there is a logged-in user or not, even in secure mode; they always run as the root user (#8767) - There is no way to log in — the `/_auth/v1/login` endpoint 404s After this change: - secure mode - with env var `COCKROACH_REQUIRE_LOGIN=true`: - Admin server endpoints require _some user_ to be logged in, although they still run queries (e.g. against `crdb_internal`) as the root user. - `/_auth/v1/login` is exposed, allowing clients such as the admin UI to log in. - else: - no auth (same as before) - insecure mode - no auth (same as before) Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com>
Build succeeded |
25005: ui: add top-level LoginContainer: require login before rendering anything r=couchand a=vilterp Depends on #25057 Touches #24939  Shown above: 1. go to admin UI; see login screen 2. error message when you type in the wrong password 3. you can't hit an authenticated endpoint because you don't have a valid session (this checking is turned on by #24944, only in secure mode) 4. once you login with the right password, you can see the UI (the temporary "connection lost" banner shouldn't be there) 5. now you (and the UI itself) can hit endpoints, because you have a valid session Todos: - [ ] redirect to login page instead of current wrapper component switching thing - [ ] logout - [ ] logout button (make API call; redirect to login & blow away redux store) - [ ] log out other tabs (if they get a 403, redirect to login & blow away redux store) - [ ] styling Release note: None 25628: distsql: support lookup join on secondary index r=solongordon a=solongordon joinReader now supports lookup joins on secondary indexes. This was a trivial change for queries where all the output columns are included in the secondary index. I just modified the physical planner to specify the secondary index in the JoinReaderSpec and removed checks which prevented secondary indexes from being used. The more complicated situation is when we want to do a lookup join against a non-covering index. In this case, the logical planner plans an index join before the inner join, but we want to perform the lookup join first. We now handle this by only planning the lookup join during physical planning, not the index join. During execution, the joinReader detects that there are output columns not covered by the secondary index, and it performs primary index lookups as necessary to retrieve the additional columns. Fixes #25431 Co-authored-by: Pete Vilter <vilterp@cockroachlabs.com> Co-authored-by: Andrew Couch <andrew@cockroachlabs.com> Co-authored-by: Solon Gordon <solon@cockroachlabs.com>
This PR introduces the temporary environment variable
COCKROACH_EXPERIMENTAL_REQUIRE_WEB_LOGINto enable testing login functionality before a fully functional login/logout UI is merged. Supersedes #24944, which hardcoded this.Once we have a functional login/logout UI, we will probably hardcode this variable to true, so that login will always be required when the cluster is in secure mode.
Before this change:
/_auth/v1/loginendpoint 404sAfter this change:
COCKROACH_EXPERIMENTAL_REQUIRE_WEB_LOGIN=true:crdb_internal) as the root user./_auth/v1/loginis exposed, allowing clients such as the admin UI to log in.