From 2ae99fe215616334dfa17861196859b42e8b6c78 Mon Sep 17 00:00:00 2001 From: Andrew Jackson Date: Wed, 29 Jun 2022 15:34:48 -0400 Subject: [PATCH] Password Hashing update (#749) --- CHANGELOG.md | 5 +- .../admin-ui/src/features/auth/auth.slice.ts | 3 +- clients/admin-ui/src/features/common/utils.ts | 5 ++ .../user-management/user-management.slice.ts | 6 +- create_superuser.py | 3 +- docs/fidesops/docs/ui/user_management.md | 9 ++- src/fidesops/schemas/user.py | 29 +++++++-- src/fidesops/util/cryptographic_util.py | 12 ++++ tests/api/v1/endpoints/test_user_endpoints.py | 59 ++++++++++++------- tests/scripts/test_create_superuser.py | 9 +-- tests/util/test_cryptographic_util.py | 34 ++++++++++- 11 files changed, 137 insertions(+), 37 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 749d58edf..32bb92037 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,10 @@ The types of changes are: * Parallelize CI safe checks to reduce run time [#717](https://github.com/ethyca/fidesops/pull/717) * Add dependabot to keep dependencies up to date [#718](https://github.com/ethyca/fidesops/pull/718) -* ### Docs +### Changed +* Base64 encode passwords on frontend [#749](https://github.com/ethyca/fidesops/pull/749) + +### Docs * Updated the tutorial installation to use main in fidesdemo [#715](https://github.com/ethyca/fidesops/pull/715) * Added a page on how to use the datastore UI [#742](https://github.com/ethyca/fidesops/pull/742) diff --git a/clients/admin-ui/src/features/auth/auth.slice.ts b/clients/admin-ui/src/features/auth/auth.slice.ts index 91ef465c3..a672910b7 100644 --- a/clients/admin-ui/src/features/auth/auth.slice.ts +++ b/clients/admin-ui/src/features/auth/auth.slice.ts @@ -8,6 +8,7 @@ import { createApi, fetchBaseQuery } from "@reduxjs/toolkit/query/react"; import type { RootState } from "../../app/store"; import { BASE_URL, STORED_CREDENTIALS_KEY } from "../../constants"; import { addCommonHeaders } from "../common/CommonHeaders"; +import { utf8ToB64 } from "../common/utils"; import { User } from "../user-management/types"; import { LoginRequest, LoginResponse } from "./types"; @@ -88,7 +89,7 @@ export const authApi = createApi({ query: (credentials) => ({ url: "login", method: "POST", - body: credentials, + body: { ...credentials, password: utf8ToB64(credentials.password) }, }), invalidatesTags: () => ["Auth"], }), diff --git a/clients/admin-ui/src/features/common/utils.ts b/clients/admin-ui/src/features/common/utils.ts index 0594f3405..7f7166532 100644 --- a/clients/admin-ui/src/features/common/utils.ts +++ b/clients/admin-ui/src/features/common/utils.ts @@ -2,3 +2,8 @@ export function capitalize(text: string): string { return text.replace(/^\w/, (c) => c.toUpperCase()); } + +// eslint-disable-next-line import/prefer-default-export +export function utf8ToB64(str: string): string { + return window.btoa(unescape(encodeURIComponent(str))); +} diff --git a/clients/admin-ui/src/features/user-management/user-management.slice.ts b/clients/admin-ui/src/features/user-management/user-management.slice.ts index dcce88a36..0959f9d4c 100644 --- a/clients/admin-ui/src/features/user-management/user-management.slice.ts +++ b/clients/admin-ui/src/features/user-management/user-management.slice.ts @@ -5,6 +5,7 @@ import type { RootState } from "../../app/store"; import { BASE_URL } from "../../constants"; import { selectToken } from "../auth"; import { addCommonHeaders } from "../common/CommonHeaders"; +import { utf8ToB64 } from "../common/utils"; import { User, UserPasswordUpdate, @@ -157,7 +158,10 @@ export const userApi = createApi({ query: ({ id, old_password, new_password }) => ({ url: `user/${id}/reset-password`, method: "POST", - body: { old_password, new_password }, + body: { + old_password: utf8ToB64(old_password!), + new_password: utf8ToB64(new_password!), + }, }), invalidatesTags: ["User"], }), diff --git a/create_superuser.py b/create_superuser.py index b52dc5ca2..e3279264d 100644 --- a/create_superuser.py +++ b/create_superuser.py @@ -14,6 +14,7 @@ from fidesops.models.fidesops_user import FidesopsUser from fidesops.models.fidesops_user_permissions import FidesopsUserPermissions from fidesops.schemas.user import UserCreate +from fidesops.util.cryptographic_util import str_to_b64_str def get_username(prompt: str) -> str: @@ -25,7 +26,7 @@ def get_username(prompt: str) -> str: def get_password(prompt: str) -> str: """Prompt the user for a password""" password = getpass.getpass(prompt) - return password + return str_to_b64_str(password) def get_input(prompt: str) -> str: diff --git a/docs/fidesops/docs/ui/user_management.md b/docs/fidesops/docs/ui/user_management.md index a77b63956..69efe2a16 100644 --- a/docs/fidesops/docs/ui/user_management.md +++ b/docs/fidesops/docs/ui/user_management.md @@ -47,8 +47,13 @@ User permissions are managed through access tokens, which contain scopes associa Creating a user currently provides access to all scopes. + +### User Passwords +All user passwords must by Base64 encoded before creating a new user, logging in, or changing a users password. This can be done with [base64encode.org](https://www.base64encode.org/). After Base64 encoding, the password `Suitablylongwithnumber8andsymbol$` would become `U3VpdGFibHlsb25nd2l0aG51bWJlcjhhbmRzeW1ib2wk`. + ### Logging in + ``` POST api/v1/login ``` @@ -56,7 +61,7 @@ POST api/v1/login ```json title="Request Body" { "username": "test_username", - "password": "Suitablylongwithnumber8andsymbol$" + "password": "U3VpdGFibHlsb25nd2l0aG51bWJlcjhhbmRzeW1ib2wk" } ``` @@ -83,7 +88,7 @@ POST api/v1/user ```json title="Request Body" { "username": "new_username", - "password": "new_Suitablylongwithnumber8andsymbol$" + "password": "U3VpdGFibHlsb25nd2l0aG51bWJlcjhhbmRzeW1ib2wk" } ``` diff --git a/src/fidesops/schemas/user.py b/src/fidesops/schemas/user.py index bc8674b02..041ed318e 100644 --- a/src/fidesops/schemas/user.py +++ b/src/fidesops/schemas/user.py @@ -6,6 +6,7 @@ from fidesops.schemas.base_class import BaseSchema from fidesops.schemas.oauth import AccessToken +from fidesops.util.cryptographic_util import b64_str_to_str class UserUpdate(BaseSchema): @@ -33,18 +34,19 @@ def validate_username(cls, username: str) -> str: @validator("password") def validate_password(cls, password: str) -> str: """Add some password requirements""" - if len(password) < 8: + decoded_password = b64_str_to_str(password) + if len(decoded_password) < 8: raise ValueError("Password must have at least eight characters.") - if re.search("[0-9]", password) is None: + if re.search("[0-9]", decoded_password) is None: raise ValueError("Password must have at least one number.") - if re.search("[A-Z]", password) is None: + if re.search("[A-Z]", decoded_password) is None: raise ValueError("Password must have at least one capital letter.") - if re.search("[a-z]", password) is None: + if re.search("[a-z]", decoded_password) is None: raise ValueError("Password must have at least one lowercase letter.") - if re.search(r"[\W]", password) is None: + if re.search(r"[\W]", decoded_password) is None: raise ValueError("Password must have at least one symbol.") - return password + return decoded_password class UserLogin(BaseSchema): @@ -53,6 +55,11 @@ class UserLogin(BaseSchema): username: str password: str + @validator("password") + def validate_password(cls, password: str) -> str: + """Convert b64 encoded password to normal string""" + return b64_str_to_str(password) + class UserPasswordReset(BaseSchema): """Contains both old and new passwords when resetting a password""" @@ -60,6 +67,16 @@ class UserPasswordReset(BaseSchema): old_password: str new_password: str + @validator("old_password") + def validate_old_password(cls, old_password: str) -> str: + """Convert b64 encoded old_password to normal string""" + return b64_str_to_str(old_password) + + @validator("new_password") + def validate_new_password(cls, new_password: str) -> str: + """Convert b64 encoded new_password to normal string""" + return b64_str_to_str(new_password) + class UserResponse(BaseSchema): """Response after requesting a User""" diff --git a/src/fidesops/util/cryptographic_util.py b/src/fidesops/util/cryptographic_util.py index 3cdab96b1..b614c7d24 100644 --- a/src/fidesops/util/cryptographic_util.py +++ b/src/fidesops/util/cryptographic_util.py @@ -32,3 +32,15 @@ def bytes_to_b64_str(bytestring: bytes) -> str: def b64_str_to_bytes(encoded_str: str) -> bytes: """Converts encoded string into bytes""" return b64decode(encoded_str.encode(config.security.ENCODING)) + + +def b64_str_to_str(encoded_str: str) -> str: + """Converts encoded string into str""" + return b64decode(encoded_str).decode(config.security.ENCODING) + + +def str_to_b64_str(string: str) -> str: + """Converts str into a utf-8 encoded string""" + return b64encode(string.encode(config.security.ENCODING)).decode( + config.security.ENCODING + ) diff --git a/tests/api/v1/endpoints/test_user_endpoints.py b/tests/api/v1/endpoints/test_user_endpoints.py index 0515e827c..5aadcbbf6 100644 --- a/tests/api/v1/endpoints/test_user_endpoints.py +++ b/tests/api/v1/endpoints/test_user_endpoints.py @@ -42,6 +42,7 @@ JWE_PAYLOAD_CLIENT_ID, JWE_PAYLOAD_SCOPES, ) +from fidesops.util.cryptographic_util import str_to_b64_str from fidesops.util.oauth_util import extract_payload, generate_jwe from tests.conftest import generate_auth_header_for_user @@ -84,7 +85,7 @@ def test_username_exists( ) -> None: auth_header = generate_auth_header([USER_CREATE]) - body = {"username": "test_user", "password": "TestP@ssword9"} + body = {"username": "test_user", "password": str_to_b64_str("TestP@ssword9")} user = FidesopsUser.create(db=db, data=body) response = api_client.post(url, headers=auth_header, json=body) @@ -103,7 +104,7 @@ def test_create_user_bad_password( ) -> None: auth_header = generate_auth_header([USER_CREATE]) - body = {"username": "test_user", "password": "short"} + body = {"username": "test_user", "password": str_to_b64_str("short")} response = api_client.post(url, headers=auth_header, json=body) assert HTTP_422_UNPROCESSABLE_ENTITY == response.status_code assert ( @@ -111,7 +112,7 @@ def test_create_user_bad_password( == "Password must have at least eight characters." ) - body = {"username": "test_user", "password": "longerpassword"} + body = {"username": "test_user", "password": str_to_b64_str("longerpassword")} response = api_client.post(url, headers=auth_header, json=body) assert HTTP_422_UNPROCESSABLE_ENTITY == response.status_code assert ( @@ -119,7 +120,7 @@ def test_create_user_bad_password( == "Password must have at least one number." ) - body = {"username": "test_user", "password": "longer55password"} + body = {"username": "test_user", "password": str_to_b64_str("longer55password")} response = api_client.post(url, headers=auth_header, json=body) assert HTTP_422_UNPROCESSABLE_ENTITY == response.status_code assert ( @@ -127,7 +128,7 @@ def test_create_user_bad_password( == "Password must have at least one capital letter." ) - body = {"username": "test_user", "password": "LoNgEr55paSSworD"} + body = {"username": "test_user", "password": str_to_b64_str("LoNgEr55paSSworD")} response = api_client.post(url, headers=auth_header, json=body) assert HTTP_422_UNPROCESSABLE_ENTITY == response.status_code assert ( @@ -143,7 +144,7 @@ def test_create_user( url, ) -> None: auth_header = generate_auth_header([USER_CREATE]) - body = {"username": "test_user", "password": "TestP@ssword9"} + body = {"username": "test_user", "password": str_to_b64_str("TestP@ssword9")} response = api_client.post(url, headers=auth_header, json=body) @@ -164,7 +165,7 @@ def test_create_user_with_name( auth_header = generate_auth_header([USER_CREATE]) body = { "username": "test_user", - "password": "TestP@ssword9", + "password": str_to_b64_str("TestP@ssword9"), "first_name": "Test", "last_name": "User", } @@ -354,7 +355,7 @@ def test_get_users(self, api_client: TestClient, generate_auth_header, url, db): for i in range(total_users): body = { "username": f"user{i}@example.com", - "password": "Password123!", + "password": str_to_b64_str("Password123!"), "first_name": "Test", "last_name": "User", } @@ -389,7 +390,10 @@ def test_get_filtered_users( saved_users: List[FidesopsUser] = [] total_users = 50 for i in range(total_users): - body = {"username": f"user{i}@example.com", "password": "Password123!"} + body = { + "username": f"user{i}@example.com", + "password": str_to_b64_str("Password123!"), + } resp = api_client.post(url, headers=create_auth_header, json=body) assert resp.status_code == HTTP_201_CREATED user = FidesopsUser.get_by(db, field="username", value=body["username"]) @@ -566,8 +570,8 @@ def test_update_different_user_password( f"{url_no_id}/{user.id}/reset-password", headers=auth_header, json={ - "old_password": OLD_PASSWORD, - "new_password": NEW_PASSWORD, + "old_password": str_to_b64_str(OLD_PASSWORD), + "new_password": str_to_b64_str(NEW_PASSWORD), }, ) assert resp.status_code == HTTP_401_UNAUTHORIZED @@ -599,8 +603,8 @@ def test_update_user_password_invalid( f"{url_no_id}/{application_user.id}/reset-password", headers=auth_header, json={ - "old_password": "mismatching password", - "new_password": NEW_PASSWORD, + "old_password": str_to_b64_str("mismatching password"), + "new_password": str_to_b64_str(NEW_PASSWORD), }, ) assert resp.status_code == HTTP_401_UNAUTHORIZED @@ -629,8 +633,8 @@ def test_update_user_password( f"{url_no_id}/{application_user.id}/reset-password", headers=auth_header, json={ - "old_password": OLD_PASSWORD, - "new_password": NEW_PASSWORD, + "old_password": str_to_b64_str(OLD_PASSWORD), + "new_password": str_to_b64_str(NEW_PASSWORD), }, ) assert resp.status_code == HTTP_200_OK @@ -646,19 +650,28 @@ def url(self, oauth_client: ClientDetail) -> str: return V1_URL_PREFIX + LOGIN def test_user_does_not_exist(self, db, url, api_client): - body = {"username": "does not exist", "password": "idonotknowmypassword"} + body = { + "username": "does not exist", + "password": str_to_b64_str("idonotknowmypassword"), + } response = api_client.post(url, headers={}, json=body) assert response.status_code == HTTP_404_NOT_FOUND def test_bad_login(self, db, url, user, api_client): - body = {"username": user.username, "password": "idonotknowmypassword"} + body = { + "username": user.username, + "password": str_to_b64_str("idonotknowmypassword"), + } response = api_client.post(url, headers={}, json=body) assert response.status_code == HTTP_403_FORBIDDEN def test_login_creates_client(self, db, url, user, api_client): # Delete existing client for test purposes user.client.delete(db) - body = {"username": user.username, "password": "TESTdcnG@wzJeu0&%3Qe2fGo7"} + body = { + "username": user.username, + "password": str_to_b64_str("TESTdcnG@wzJeu0&%3Qe2fGo7"), + } assert user.client is None # client does not exist assert user.permissions is not None @@ -681,7 +694,10 @@ def test_login_creates_client(self, db, url, user, api_client): user.client.delete(db) def test_login_updates_last_login_date(self, db, url, user, api_client): - body = {"username": user.username, "password": "TESTdcnG@wzJeu0&%3Qe2fGo7"} + body = { + "username": user.username, + "password": str_to_b64_str("TESTdcnG@wzJeu0&%3Qe2fGo7"), + } response = api_client.post(url, headers={}, json=body) assert response.status_code == HTTP_200_OK @@ -690,7 +706,10 @@ def test_login_updates_last_login_date(self, db, url, user, api_client): assert user.last_login_at is not None def test_login_uses_existing_client(self, db, url, user, api_client): - body = {"username": user.username, "password": "TESTdcnG@wzJeu0&%3Qe2fGo7"} + body = { + "username": user.username, + "password": str_to_b64_str("TESTdcnG@wzJeu0&%3Qe2fGo7"), + } existing_client_id = user.client.id user.client.scopes = [PRIVACY_REQUEST_READ] diff --git a/tests/scripts/test_create_superuser.py b/tests/scripts/test_create_superuser.py index 566734b27..972c401e2 100644 --- a/tests/scripts/test_create_superuser.py +++ b/tests/scripts/test_create_superuser.py @@ -9,6 +9,7 @@ from fidesops.models.fidesops_user import FidesopsUser from fidesops.models.fidesops_user_permissions import FidesopsUserPermissions from fidesops.schemas.user import UserCreate +from fidesops.util.cryptographic_util import str_to_b64_str class TestCreateSuperuserScript: @@ -23,7 +24,7 @@ def test_collect_username_and_password( db, ): GENERIC_INPUT = "some_input" - mock_pass.return_value = "TESTP@ssword9" + mock_pass.return_value = str_to_b64_str("TESTP@ssword9") mock_user.return_value = "test_user" mock_input.return_value = GENERIC_INPUT user: UserCreate = collect_username_and_password(db) @@ -47,7 +48,7 @@ def test_collect_username_and_password_user_exists( db=db, data={"username": "test_user", "password": "test_password"}, ) - mock_pass.return_value = "TESTP@ssword9" + mock_pass.return_value = str_to_b64_str("TESTP@ssword9") mock_user.return_value = "test_user" mock_input.return_value = "some_input" @@ -66,7 +67,7 @@ def test_collect_username_and_password_bad_data( mock_user, db, ): - mock_pass.return_value = "bad_password" + mock_pass.return_value = str_to_b64_str("bad_password") mock_user.return_value = "test_user" mock_input.return_value = "some_input" @@ -83,7 +84,7 @@ def test_create_user_and_client( mock_user, db, ): - mock_pass.return_value = "TESTP@ssword9" + mock_pass.return_value = str_to_b64_str("TESTP@ssword9") mock_user.return_value = "test_user" mock_input.return_value = "some_input" diff --git a/tests/util/test_cryptographic_util.py b/tests/util/test_cryptographic_util.py index 8d5d8fb36..f2833c6c5 100644 --- a/tests/util/test_cryptographic_util.py +++ b/tests/util/test_cryptographic_util.py @@ -1,5 +1,11 @@ from fidesops.core.config import config -from fidesops.util.cryptographic_util import hash_with_salt +from fidesops.util.cryptographic_util import ( + b64_str_to_bytes, + b64_str_to_str, + bytes_to_b64_str, + hash_with_salt, + str_to_b64_str, +) def test_hash_with_salt() -> None: @@ -13,3 +19,29 @@ def test_hash_with_salt() -> None: ) assert hashed == expected_hash + + +def test_bytes_to_b64_str() -> None: + byte_string = b"https://www.google.com" + b64_string = "aHR0cHM6Ly93d3cuZ29vZ2xlLmNvbQ==" + result = bytes_to_b64_str(byte_string) + assert b64_string == result + + +def test_b64_str_to_bytes() -> None: + b64_string = "aHR0cHM6Ly93d3cuZ29vZ2xlLmNvbQ==" + result = b64_str_to_bytes(b64_string) + assert b"https://www.google.com" == result + + +def test_b64_str_to_str() -> None: + b64_string = "aHR0cHM6Ly93d3cuZ29vZ2xlLmNvbQ==" + result = b64_str_to_str(b64_string) + assert "https://www.google.com" == result + + +def test_str_to_b64_str() -> None: + orig_string = "https://www.google.com" + b64_string = "aHR0cHM6Ly93d3cuZ29vZ2xlLmNvbQ==" + result = str_to_b64_str(orig_string) + assert b64_string == result