Skip to content
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

[DNM] server: require web login by default in secure mode #24944

Closed
wants to merge 1 commit into from

Conversation

vilterp
Copy link
Contributor

@vilterp vilterp commented Apr 19, 2018

Note: this change or something like it is required for web login to work, but shouldn't be merged before a UI login screen exists.

Before this change:

After this change:

  • 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.

UX note: some users might want a cluster which is in secure mode but doesn't require web login. To support that, we'd need to introduce a new cluster setting, e.g. disable_web_login. do not merge this until we've made that decision.

Implementation note: Flipping this boolean causes the authentication mux to be installed on the server when in secure mode, which (a) handles the login endpoint and (b) checks requests to the admin and status servers for a valid session. Here's where it's installed:

cockroach/pkg/server/server.go

Lines 1181 to 1184 in 75d44eb

var authHandler http.Handler = gwMux
if s.cfg.RequireWebSession() {
authHandler = newAuthenticationMux(s.authentication, authHandler)
}

@vilterp vilterp requested a review from a team April 19, 2018 20:40
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@vilterp vilterp added the do-not-merge bors won't merge a PR with this label. label Apr 19, 2018
@vilterp vilterp changed the title server: require web login by default in secure mode [DNM] server: require web login by default in secure mode Apr 19, 2018
@vilterp vilterp requested review from mrtracy and couchand April 19, 2018 20:51
@vilterp
Copy link
Contributor Author

vilterp commented Apr 24, 2018

Superseded by #25057

@vilterp vilterp closed this Apr 24, 2018
@vilterp vilterp deleted the require-web-login branch April 24, 2018 20:15
craig bot pushed a commit that referenced this pull request Apr 26, 2018
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 <[email protected]>
craig bot pushed a commit that referenced this pull request May 16, 2018
24945: server: exempt healthcheck endpoint from authentication check r=couchand a=vilterp

Otherwise, you get the red "connection lost" banner until you log in.

This will have no effect until the auth mux is put on the request path (#24944).

Fixes #24942 

25581: storage: fix use of context with closed trace r=a-robinson a=a-robinson

Fixes #25575

Release note: None

Co-authored-by: Pete Vilter <[email protected]>
Co-authored-by: Alex Robinson <[email protected]>
craig bot pushed a commit that referenced this pull request May 22, 2018
25005: ui: add top-level LoginContainer: require login before rendering anything r=couchand a=vilterp

Depends on #25057 
Touches #24939

![](https://user-images.githubusercontent.com/7341/39150096-38b4cd28-470f-11e8-9d67-e1832d35a211.gif)

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 <[email protected]>
Co-authored-by: Andrew Couch <[email protected]>
Co-authored-by: Solon Gordon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do-not-merge bors won't merge a PR with this label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants