-
Notifications
You must be signed in to change notification settings - Fork 786
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
[Snyk] Security upgrade PyJWT from 1.4.2 to 1.5.1 #729
Changes from 6 commits
ab0c9c5
1827cfa
1f26d7f
1072607
c66fd0f
dd13204
2bbb2d2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,7 +10,8 @@ | |
from monkey_island.cc.database import database, mongo | ||
from monkey_island.cc.resources.attack.attack_config import AttackConfiguration | ||
from monkey_island.cc.resources.attack.attack_report import AttackReport | ||
from monkey_island.cc.resources.auth.auth import init_jwt | ||
from monkey_island.cc.resources.auth.auth import Authenticate, init_jwt | ||
from monkey_island.cc.resources.auth.registration import Registration | ||
from monkey_island.cc.resources.bootloader import Bootloader | ||
from monkey_island.cc.resources.client_run import ClientRun | ||
from monkey_island.cc.resources.edge import Edge | ||
|
@@ -31,7 +32,6 @@ | |
from monkey_island.cc.resources.node_states import NodeStates | ||
from monkey_island.cc.resources.pba_file_download import PBAFileDownload | ||
from monkey_island.cc.resources.pba_file_upload import FileUpload | ||
from monkey_island.cc.resources.registration import Registration | ||
from monkey_island.cc.resources.remote_run import RemoteRun | ||
from monkey_island.cc.resources.reporting.report import Report | ||
from monkey_island.cc.resources.root import Root | ||
|
@@ -71,9 +71,12 @@ def serve_home(): | |
|
||
def init_app_config(app, mongo_url): | ||
app.config['MONGO_URI'] = mongo_url | ||
app.config['SECRET_KEY'] = str(uuid.getnode()) | ||
app.config['JWT_AUTH_URL_RULE'] = '/api/auth' | ||
app.config['JWT_EXPIRATION_DELTA'] = env_singleton.env.get_auth_expiration_time() | ||
|
||
# See https://flask-jwt-extended.readthedocs.io/en/stable/options | ||
app.config['JWT_TOKEN_LOCATION'] = ['headers'] | ||
app.config['JWT_ACCESS_TOKEN_EXPIRES'] = env_singleton.env.get_auth_expiration_time() | ||
# Invalidate the signature of JWTs between server resets. | ||
app.config['JWT_SECRET_KEY'] = str(uuid.uuid4()) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's stored in mongodb between resets? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nope - I meant Python process restarted, fixed the doc. |
||
|
||
|
||
def init_app_services(app): | ||
|
@@ -96,6 +99,7 @@ def init_app_url_rules(app): | |
def init_api_resources(api): | ||
api.add_resource(Root, '/api') | ||
api.add_resource(Registration, '/api/registration') | ||
api.add_resource(Authenticate, '/api/auth') | ||
api.add_resource(Environment, '/api/environment') | ||
api.add_resource(Monkey, '/api/monkey', '/api/monkey/', '/api/monkey/<string:guid>') | ||
api.add_resource(Bootloader, '/api/bootloader/<string:os>') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,40 +1,67 @@ | ||
import json | ||
import logging | ||
from functools import wraps | ||
|
||
from flask import abort, current_app | ||
from flask_jwt import JWT, JWTError, _jwt_required | ||
import flask_jwt_extended | ||
import flask_restful | ||
from flask import make_response, request | ||
from flask_jwt_extended.exceptions import JWTExtendedException | ||
from jwt import PyJWTError | ||
from werkzeug.security import safe_str_cmp | ||
|
||
import monkey_island.cc.environment.environment_singleton as env_singleton | ||
import monkey_island.cc.resources.auth.user_store as user_store | ||
|
||
__author__ = 'itay.mizeretz' | ||
logger = logging.getLogger(__name__) | ||
|
||
|
||
def init_jwt(app): | ||
user_store.UserStore.set_users(env_singleton.env.get_auth_users()) | ||
_ = flask_jwt_extended.JWTManager(app) | ||
logger.debug("Initialized JWT with secret key that started with " + app.config["JWT_SECRET_KEY"][:4]) | ||
|
||
def authenticate(username, secret): | ||
|
||
class Authenticate(flask_restful.Resource): | ||
""" | ||
Resource for user authentication. The user provides the username and hashed password and we give them a JWT. | ||
See `AuthService.js` file for the frontend counterpart for this code. | ||
""" | ||
@staticmethod | ||
def _authenticate(username, secret): | ||
user = user_store.UserStore.username_table.get(username, None) | ||
if user and safe_str_cmp(user.secret.encode('utf-8'), secret.encode('utf-8')): | ||
return user | ||
|
||
def identity(payload): | ||
user_id = payload['identity'] | ||
return user_store.UserStore.user_id_table.get(user_id, None) | ||
|
||
JWT(app, authenticate, identity) | ||
|
||
def post(self): | ||
""" | ||
Example request: | ||
{ | ||
"username": "my_user", | ||
"password": "343bb87e553b05430e5c44baf99569d4b66..." | ||
} | ||
""" | ||
credentials = json.loads(request.data) | ||
# Unpack auth info from request | ||
username = credentials["username"] | ||
secret = credentials["password"] | ||
# If the user and password have been previously registered | ||
if self._authenticate(username, secret): | ||
access_token = flask_jwt_extended.create_access_token(identity=user_store.UserStore.username_table[username].id) | ||
logger.debug(f"Created access token for user {username}: {access_token}") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we really want to log access token? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Changed to only log first 4 chars (useful for debugging as I've discovered) |
||
return make_response({"access_token": access_token, "error": ""}, 200) | ||
else: | ||
return make_response({"error": "Invalid credentials"}, 401) | ||
|
||
def jwt_required(realm=None): | ||
def wrapper(fn): | ||
@wraps(fn) | ||
def decorator(*args, **kwargs): | ||
try: | ||
_jwt_required(realm or current_app.config['JWT_DEFAULT_REALM']) | ||
return fn(*args, **kwargs) | ||
except JWTError: | ||
abort(401) | ||
|
||
return decorator | ||
# See https://flask-jwt-extended.readthedocs.io/en/stable/custom_decorators/ | ||
def jwt_required(fn): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't fit as a replacement? https://flask-jwt-extended.readthedocs.io/en/stable/api/#flask_jwt_extended.jwt_required There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It doesn't since it raises 500 instead of a more accurate 401 on JWT errors (like invalidation due to key change). |
||
@wraps(fn) | ||
def wrapper(*args, **kwargs): | ||
try: | ||
flask_jwt_extended.verify_jwt_in_request() | ||
return fn(*args, **kwargs) | ||
# Catch authentication related errors in the verification or inside the called function. All other exceptions propagate | ||
except (JWTExtendedException, PyJWTError) as e: | ||
return make_response({"error": f"Authentication error: {str(e)}"}, 401) | ||
|
||
return wrapper |
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.
Already a default, why specify?
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.
Removed