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

mgr/dashboard: JWT authentication #22833

Merged
merged 4 commits into from Oct 30, 2018

Conversation

Projects
None yet
10 participants
@rjfd
Contributor

rjfd commented Jul 3, 2018

This PR changes the authentication scheme we were using in the dashboard, from a stateful scheme to a stateless scheme based on JWT tokens.

This solves two problems:

  • when the dashboard module is disabled and enabled again (reload) the UI will not require the user to login again.
  • when the ceph-mgr daemon dies and we fallback to another, the UI will not require the user to login again.

The login function now returns a JWT token with a validity of 8 hours (configurable by a dashboard CLI command) that must be added to the request header Authorization in every request to our REST API.
Upon logout the token is blacklisted and further attempts to use that token will return a 401 HTTP code.

This PR takes care of both backend and frontend code to use JWT tokens.

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch 2 times, most recently from c084d2b to 0720837 Jul 4, 2018

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jul 4, 2018

@sebastian-philipp piggybacked the vstart.sh user creation fix to this PR: 70fd021

class JwtManager(object):
JWT_TOKEN_BLACKLIST_KEY = "jwt-token-black-list"
JWT_TOKEN_TTL = 28800 # default 8 hours

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Jul 4, 2018

Member

why 8 hours?

This comment has been minimized.

@rjfd

rjfd Jul 4, 2018

Contributor

The idea is for the token to endure a full working day.

@sebastian-philipp

This comment has been minimized.

Member

sebastian-philipp commented Jul 4, 2018

TBH, I don't see the necessity for adding this. It adds lots of complexity to the code base with little benefit for users:

  • Users typically do not disable and enable the dashboard. I expect users to enable the dashboard once without touching it a second time.
  • ceph-mgr typically does not die. In any case, users have to login every so often.

Is there any real and noticeable benefit for users?

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jul 4, 2018

@sebastian-philipp this feature allows the ceph-dasboard user session to be available when the browser needs to connect to a different backend server. I agree that this case should not be frequent but it may happen.

I don't think the code in this PR adds complexity to existing code. The code added in this PR is limited to the implementation of the token based support and does not change the remaining of the dashboard code. Also, it does not introduce any change in the methodology, or process, of implementing new features to the dashboard.

But if you think this PR will cause problems when developing new features for the dashboard, I'm happy to close this PR.

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jul 4, 2018

jenkins retest this please

@@ -5,6 +5,8 @@ import { BrowserAnimationsModule } from '@angular/platform-browser/animations';
import { ToastModule, ToastOptions } from 'ng2-toastr/ng2-toastr';

This comment has been minimized.

@tspmelo

tspmelo Jul 5, 2018

Contributor

You should remove lines 7 and 9, and add a blank line between 10 and 11.

this.authStorageService.remove();
});
return this.http.delete('api/auth').toPromise().then(() => {
localStorage.removeItem('access_token');

This comment has been minimized.

@tspmelo

tspmelo Jul 5, 2018

Contributor

This line should be moved inside authStorageService.remove()

.toPromise()
.then((resp: Credentials) => {
.then((resp: any) => {
localStorage.setItem('access_token', resp.token);

This comment has been minimized.

@tspmelo

tspmelo Jul 5, 2018

Contributor

This should be moved inside authStorageService.set().

.toPromise()
.then((resp: Credentials) => {
.then((resp: any) => {

This comment has been minimized.

@tspmelo

tspmelo Jul 5, 2018

Contributor

Should we add token to Credentials class? That way we can keep using that type in the response.

This comment has been minimized.

@rjfd

rjfd Jul 24, 2018

Contributor

I would prefer not to do that because the information require to do the login is different from the information returned by a login operation. What about creating a class called LoginResponse for this?

@@ -1,7 +1,7 @@
#!/usr/bin/env bash
set -e
: ${CEPH_ROOT:=$PWD/../../../../}

This comment has been minimized.

@tspmelo

tspmelo Jul 5, 2018

Contributor

Does this always replace the value of CEPH_ROOT?

This comment has been minimized.

@rjfd

rjfd Jul 5, 2018

Contributor

Nope, it's a default value in case $CEPH_ROOT is not defined.

goToApiDocs() {
let tokenInput = null;
if (this.docsFormElement.nativeElement.children.length === 0) {

This comment has been minimized.

@tspmelo

tspmelo Jul 5, 2018

Contributor

I think this can be simplified if you just declare the input field in the html and then you only need to update it's value in the method.

@LenzGr LenzGr added the needs-rebase label Jul 10, 2018

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Jul 13, 2018

Would you mind adding some information about this to the documentation as well? How does this affect me as a user or developer? How do I use this feature?

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jul 15, 2018

@LenzGr this feature is completely transparent to users and developers.

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch from 70fd021 to 216751e Jul 24, 2018

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jul 24, 2018

@tspmelo addressed your comments

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch 2 times, most recently from 1e90bb5 to 0bfb8bc Jul 24, 2018

@@ -9,15 +9,21 @@ import { ServicesModule } from './services.module';
export class AuthStorageService {
constructor() {}
set(username: string, permissions: any = {}) {
set(username: string, token: string, permissions: any = {}) {

This comment has been minimized.

@tspmelo

tspmelo Jul 24, 2018

Contributor

I think we can use object type for the permissions.

@@ -13,10 +14,10 @@ export class AuthService {
login(credentials: Credentials) {
return this.http
.post('api/auth', credentials)
.post('api/auth', { username: credentials.username, password: credentials.password })

This comment has been minimized.

@tspmelo

tspmelo Jul 24, 2018

Contributor

Since we now only have username and password on the Credentials class, maybe we can keep the old code and pass the variable as the body of POST?

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch from 0bfb8bc to 44a100e Jul 25, 2018

@rjfd

This comment has been minimized.

Contributor

rjfd commented Jul 25, 2018

@tspmelo addressed your comments

@votdev

votdev requested changes Jul 27, 2018 edited

In controllers/user.py the removed class Session is still imported.

2018-07-27 07:37:00.287 7f8e4c614700 -1 Traceback (most recent call last):
  File "/ceph/src/pybind/mgr/dashboard/module.py", line 288, in serve
    mapper, parent_urls = generate_routes(self.url_prefix)
  File "/ceph/src/pybind/mgr/dashboard/controllers/__init__.py", line 231, in generate_routes
    ctrls = load_controllers()
  File "/ceph/src/pybind/mgr/dashboard/controllers/__init__.py", line 170, in load_controllers
    package='dashboard')
  File "/usr/lib64/python2.7/importlib/__init__.py", line 37, in import_module
    __import__(name)
  File "/ceph/src/pybind/mgr/dashboard/controllers/user.py", line 11, in <module>
    from ..tools import Session
ImportError: cannot import name Session
@rjfd

This comment has been minimized.

Contributor

rjfd commented Jul 27, 2018

@votdev good catch! that was introduced yesterday with the merge of #22758

elif cmd['prefix'] == 'dashboard set-session-expire':
self.set_config('session-expire', str(cmd['seconds']))
return 0, 'Session expiration timeout updated', ''
elif cmd['prefix'] == 'dashboard set-jwt-token-ttl':

This comment has been minimized.

@votdev

votdev Jul 27, 2018

Contributor

I'm missing a dashboard get-jwt-token-ttl command here.

@classmethod
def gen_token(cls, username):
ttl = mgr.get_config('jwt-token-ttl', cls.JWT_TOKEN_TTL)

This comment has been minimized.

@votdev

votdev Jul 27, 2018

Contributor

s/jwt-token-ttl/jwt_token_ttl

I think config keys should always use underscores, see ceph config-key ls.

class JwtManager(object):
JWT_TOKEN_BLACKLIST_KEY = "jwt-token-black-list"

This comment has been minimized.

@votdev

votdev Jul 27, 2018

Contributor

s/jwt-token-black-list/jwt_token_black_list

Config keys always use underscores as far as i know.

@votdev

This comment has been minimized.

Contributor

votdev commented Jul 27, 2018

🐳 arrakis /ceph/build # ceph dashboard set-jwt-token-ttl 20
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2018-07-27 08:01:56.824 7f6055f88700 -1 WARNING: all dangerous and experimental features are enabled.
2018-07-27 08:01:56.836 7f6055f88700 -1 WARNING: all dangerous and experimental features are enabled.
JWT token TTL updated
🐳 arrakis /ceph/build # ceph config-key ls | grep jwt-token-ttl
*** DEVELOPER MODE: setting PATH, PYTHONPATH and LD_LIBRARY_PATH ***
2018-07-27 08:02:02.972 7fbb6ad60700 -1 WARNING: all dangerous and experimental features are enabled.
2018-07-27 08:02:02.984 7fbb6ad60700 -1 WARNING: all dangerous and experimental features are enabled.
    "config-history/7/+mgr/mgr/dashboard/jwt-token-ttl",
    "config/mgr/mgr/dashboard/jwt-token-ttl",

The config key path config/mgr/mgr/dashboard/jwt-token-ttl looks incorrect to me, i assume it should be config/mgr/dashboard/jwt-token-ttl.

@rjfd

This comment has been minimized.

Contributor

rjfd commented Oct 17, 2018

It would be great if we have a way to invalidate tokens for a certain user.

@rjfd has this been implemented? Is there a commandline option for this?

Yes, but there is no command to do that. What is there is the infrastructure to invalidate a user session/token, and we already use that in this PR for instance when changing the user properties like the password, or the its roles.

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch from ba5a1a7 to afd606d Oct 17, 2018

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch 2 times, most recently from 97fb73a to 080a600 Oct 18, 2018

@callithea callithea removed the needs-rebase label Oct 18, 2018

@callithea

This comment has been minimized.

Member

callithea commented Oct 19, 2018

jenkins test make check

logger.debug('Login successful')
token = JwtManager.gen_token(username)
token = token.decode('utf-8')
logger.debug("JWT Token: %s", token)

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Oct 22, 2018

Member

This looks like a security vulnerability.

IMO, access tokens are considered security critical data that should not be logged.

This comment has been minimized.

@LenzGr

LenzGr Oct 22, 2018

Contributor

Indeed, storing login credentials in logs could be considered a potential security vulnerability. But this seems to be a log entry that only appears when the log level is set to "debug", which is not the production default level, or is it? We may have a similar issue in #24475 then.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Oct 23, 2018

Member

Ok, if this is acceptable, everything else LGTM.

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch from 080a600 to 8b43d10 Oct 23, 2018

@rjfd rjfd removed the needs-rebase label Oct 23, 2018

@LenzGr

This comment has been minimized.

Contributor

LenzGr commented Oct 24, 2018

Yes, but there is no command to do that. What is there is the infrastructure to invalidate a user session/token, and we already use that in this PR for instance when changing the user properties like the password, or the its roles.

Let's add that in a separate PR. There is a tracker issue for this feature here: https://tracker.ceph.com/issues/25229

@rjfd

This comment has been minimized.

Contributor

rjfd commented Oct 25, 2018

@callithea could you re-run the QA tests?

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch from 8b43d10 to 3a33d44 Oct 29, 2018

@rjfd

This comment has been minimized.

Contributor

rjfd commented Oct 29, 2018

@callithea I think I've fixed the problem. Please re-run the QA tests when the make check finishes. Thanks!

rjfd added some commits Jul 3, 2018

mgr/dashboard: backend: JWT based authentication
Signed-off-by: Ricardo Dias <rdias@suse.com>
mgr/dashboard: run-backend-api-request.sh: support for JTW tokens
Signed-off-by: Ricardo Dias <rdias@suse.com>
mgr/dashboard: frontend: JWT authentication implementation
Signed-off-by: Ricardo Dias <rdias@suse.com>
mgr/dashboard: run-frontend-unittests: fix CEPH_ROOT initialization
Signed-off-by: Ricardo Dias <rdias@suse.com>

@rjfd rjfd force-pushed the rjfd:wip-dashboard-jwt branch from 3a33d44 to 61bf117 Oct 29, 2018

@LenzGr LenzGr merged commit 88719af into ceph:master Oct 30, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
@djgalloway

This comment has been minimized.

Contributor

djgalloway commented Nov 1, 2018

This didn't get tested on RHEL and python-jwt isn't available in the RHEL-equivalent base repo like it is in CentOS. It's also not in epel.

Is RHEL supported? Where should users get the package from?

liewegas added a commit to liewegas/ceph that referenced this pull request Nov 1, 2018

Revert "Merge pull request ceph#22833 from rjfd/wip-dashboard-jwt"
This reverts commit 88719af, reversing
changes made to c60bb22.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment