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

Fix security config race condition #1718

Merged
merged 48 commits into from
Nov 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
48 commits
Select commit Hold shift + click to select a range
22df5c9
Fix security config race condition
Nov 8, 2022
05d9ed5
Fix pylint error
Nov 8, 2022
d3e06e0
Merge remote-tracking branch 'origin/main' into ps-config
Nov 9, 2022
aace363
Refactor to allow loading security settings earlier
Nov 9, 2022
56bc381
Use local database settings
Nov 9, 2022
f063e1f
Pre-load database settings
Nov 9, 2022
73f9cad
Desperate attempt at debugging CI
Nov 10, 2022
3f07a2c
Remove unsuccessful debugging attempt
Nov 10, 2022
850b202
Bump pydantic to see if it will fix CI issue
Nov 10, 2022
d3971c1
More CI debugging
Nov 10, 2022
a47e718
Fix python version in debugging
Nov 10, 2022
b65ff5f
Fix typo
Nov 10, 2022
dfb4181
Isolate docker build
Nov 10, 2022
68fe613
Remove debugging
Nov 10, 2022
0060dd1
Pull more fideslib code directly in
Nov 10, 2022
4f1abd3
Fix install error
Nov 10, 2022
1bea527
Add missing argument
Nov 10, 2022
bf67a37
Clear pydantic validator cache
Nov 10, 2022
0aeafa1
Move cache clearing
Nov 10, 2022
8f074a7
Add required security settings to test file
Nov 10, 2022
6e444f9
Add environment vairables to docs
Nov 10, 2022
16549d5
Fix docs environment vairables
Nov 10, 2022
5e0a47f
Update CHANGELOG
Nov 10, 2022
ed99d1f
Merge remote-tracking branch 'origin/main' into ps-config
Nov 10, 2022
76de64c
init the security settings if they don't exist in the toml
ThomasLaPiana Nov 11, 2022
69e2385
Add minimal config test
Nov 11, 2022
ef82a73
Merge remote-tracking branch 'origin/ps-config' into ps-config
Nov 11, 2022
390481e
Remove commented code
Nov 11, 2022
52c6f0f
Merge remote-tracking branch 'origin/main' into ps-config
Nov 11, 2022
5e0c8dc
Add minimial config test to CI
Nov 11, 2022
6417639
Add requests to pip install
Nov 11, 2022
6e44108
Add required dependency
Nov 11, 2022
6dd9243
Fix path
Nov 11, 2022
a99694d
Change test to use docker compose wait
Nov 12, 2022
d7ac321
Remove unused import
Nov 12, 2022
1c82941
Rename test step
Nov 12, 2022
fd5f833
code comment cleanup
ThomasLaPiana Nov 13, 2022
35a2ecc
refactor the new minimal config check to use the existing matrix pattern
ThomasLaPiana Nov 13, 2022
f8d4b81
remove the default security settings from the toml
ThomasLaPiana Nov 13, 2022
8e85f7c
get the config loading with partial env definition
ThomasLaPiana Nov 13, 2022
2b23404
fix the docs check
ThomasLaPiana Nov 13, 2022
ffe09b4
fix some of the config tests
ThomasLaPiana Nov 14, 2022
d39dec6
fix the remaining config test failures
ThomasLaPiana Nov 14, 2022
00ac243
fix mypy errors
ThomasLaPiana Nov 14, 2022
118f739
remove the explicit security setting field from the config file
ThomasLaPiana Nov 14, 2022
e70c5ea
fix "check_install", skip validation for the initial security setting…
ThomasLaPiana Nov 14, 2022
9433107
re-add the required vars to the toml, since it is being docker ignored
ThomasLaPiana Nov 14, 2022
8a2db7c
Merge branch 'main' into ps-config
ThomasLaPiana Nov 15, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
Expand Up @@ -48,4 +48,5 @@ venv/

# Dev files
.pre-commit-config.yaml
docker/docker-compose.minimal-config.yml
.fides/
6 changes: 3 additions & 3 deletions .fides/fides.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,14 +30,14 @@ ssl = false
ssl_cert_reqs = "required"

[security]
app_encryption_key = "OLMkv91j8DHiDAULnK5Lxx3kSCov30b3"
cors_origins = [ "http://localhost", "http://localhost:8080", "http://localhost:3000", "http://localhost:3001",]
encoding = "UTF-8"
root_username = "root_user"
root_password = "Testpassword1!"
app_encryption_key = "OLMkv91j8DHiDAULnK5Lxx3kSCov30b3"
oauth_root_client_id = "fidesadmin"
oauth_root_client_secret = "fidesadminsecret"
drp_jwt_secret = "secret"
root_username = "root_user"
root_password = "Testpassword1!"

[execution]
masking_strict = true
Expand Down
18 changes: 15 additions & 3 deletions .github/workflows/backend_checks.yml
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,9 @@ jobs:
- name: Run Static Check
run: nox -s ${{ matrix.session_name }}

################
## Safe Tests ##
################
#################
## Safe Checks ##
#################
Safe-Tests:
needs: Build

Expand All @@ -87,6 +87,7 @@ jobs:
- "check_fides_annotations"
- "fides_db_scan"
- "docs_check"
- "minimal_config_startup"
exclude:
# Not all jobs need to run against the Python matrix
- python_version: "3.8.14"
Expand All @@ -104,6 +105,11 @@ jobs:
- python_version: "3.9.14"
test_selection: "docs_check"

- python_version: "3.8.14"
test_selection: "minimal_config_startup"
- python_version: "3.9.14"
test_selection: "minimal_config_startup"

runs-on: ubuntu-latest
timeout-minutes: 15
continue-on-error: true
Expand All @@ -120,6 +126,12 @@ jobs:
- name: Checkout
uses: actions/checkout@v3

- name: Set Up Python
uses: actions/setup-python@v4
with:
python-version: ${{ env.DEFAULT_PYTHON_VERSION }}
cache: "pip"

- name: Install Nox
run: pip install nox>=2022

Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ The types of changes are:
* Show a helpful error message if Docker daemon is not running during "fides deploy" [#1694](https://github.com/ethyca/fides/pull/1694)
* Allow users to query their own permissions, including root user. [#1698](https://github.com/ethyca/fides/pull/1698)
* Single-select taxonomy fields legal basis and special category can be cleared. [#1712](https://github.com/ethyca/fides/pull/1712)
* Fixes the issue where the security config is not properly loading from environment variables. [#1718](https://github.com/ethyca/fides/pull/1718)
* Correctly handle response from adobe jwt auth endpoint as milliseconds, rather than seconds. [#1754](https://github.com/ethyca/fides/pull/1754)

### Security
Expand Down
10 changes: 8 additions & 2 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -100,8 +100,14 @@ services:
ports:
- "8000:8000"
environment:
- FIDES__DEV_MODE=True
- FIDES__CLI__ANALYTICS_ID=${FIDES__CLI__ANALYTICS_ID:-}
FIDES__DEV_MODE: True
FIDES__CLI__ANALYTICS_ID: ${FIDES__CLI__ANALYTICS_ID:-}

# Required security env vars
FIDES__SECURITY__APP_ENCRYPTION_KEY: OLMkv91j8DHiDAULnK5Lxx3kSCov30b3
FIDES__SECURITY__OAUTH_ROOT_CLIENT_ID: fidesadmin
FIDES__SECURITY__OAUTH_ROOT_CLIENT_SECRET: fidesadminsecret
FIDES__SECURITY__DRP_JWT_SECRET: secret

worker:
image: ethyca/fides:local
Expand Down
67 changes: 67 additions & 0 deletions docker/docker-compose.minimal-config.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
services:
fides:
image: ethyca/fides:local
command: uvicorn --host 0.0.0.0 --port 8080 --reload --reload-dir src fides.api.main:app
healthcheck:
test: [ "CMD", "curl", "-f", "http://0.0.0.0:8080/health" ]
interval: 20s
timeout: 5s
retries: 10
ports:
- "8080:8080"
depends_on:
fides-db:
condition: service_healthy
redis:
condition: service_started
expose:
- 8080
environment:
FIDES__DATABASE__SERVER: "fides-db"
FIDES__DATABASE__USER: "postgres"
FIDES__DATABASE__PASSWORD: "fides"
FIDES__DATABASE__PORT: "5432"
FIDES__DATABASE__DB: "fides"
FIDES__USER__ANALYTICS_OPT_OUT: "True"
FIDES__SECURITY__APP_ENCRYPTION_KEY: "OLMkv91j8DHiDAULnK5Lxx3kSCov30b3"
FIDES__SECURITY__OAUTH_ROOT_CLIENT_ID: "fidesadmin"
FIDES__SECURITY__OAUTH_ROOT_CLIENT_SECRET: "fidesadminsecret"
FIDES__SECRUITY__DRP_JWT_SECRET: "secret"

fides-db:
image: postgres:12
healthcheck:
test: [ "CMD-SHELL", "pg_isready -U postgres" ]
interval: 15s
timeout: 5s
retries: 5
volumes:
- postgres:/var/lib/postgresql/data
expose:
- 5432
ports:
- "5432:5432"
environment:
POSTGRES_USER: "postgres"
POSTGRES_PASSWORD: "fides"
POSTGRES_DB: "fides"
deploy:
placement:
constraints:
- node.labels.fides.app-db-data == true

redis:
image: "redis:6.2.5-alpine"
command: redis-server --requirepass testpassword
environment:
- REDIS_PASSWORD=testpassword
expose:
- 6379
ports:
- "0.0.0.0:6379:6379"

volumes:
postgres: null

networks:
fides_network:
32 changes: 30 additions & 2 deletions noxfiles/ci_nox.py
Original file line number Diff line number Diff line change
Expand Up @@ -103,11 +103,18 @@ def check_install(session: nox.Session) -> None:
"""Check that fides installs works correctly."""
session.install(".")

REQUIRED_ENV_VARS = {
"FIDES__SECURITY__APP_ENCRYPTION_KEY": "OLMkv91j8DHiDAULnK5Lxx3kSCov30b3",
"FIDES__SECURITY__OAUTH_ROOT_CLIENT_ID": "fidesadmin",
"FIDES__SECURITY__OAUTH_ROOT_CLIENT_SECRET": "fidesadminsecret",
"FIDES__SECURITY__DRP_JWT_SECRET": "secret",
}

run_command = ("fides", "--version")
session.run(*run_command)
session.run(*run_command, env=REQUIRED_ENV_VARS)

run_command = ("python", "-c", "from fides.api.main import start_webserver")
session.run(*run_command)
session.run(*run_command, env=REQUIRED_ENV_VARS)


@nox.session()
Expand All @@ -134,6 +141,27 @@ def fides_db_scan(session: nox.Session) -> None:
session.run(*run_command, external=True)


@nox.session()
def minimal_config_startup(session: nox.Session) -> None:
"""
Check that the server can start successfully with a minimal
configuration set through environment vairables.
"""
session.notify("teardown")
compose_file = "docker/docker-compose.minimal-config.yml"
start_command = (
"docker",
"compose",
"-f",
compose_file,
"up",
"--wait",
"-d",
IMAGE_NAME,
)
session.run(*start_command, external=True)


# Pytest
@nox.session()
@nox.parametrize(
Expand Down
5 changes: 3 additions & 2 deletions pyproject.toml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,8 @@ module = [
"sqlalchemy.future.*",
"sqlalchemy_utils.*",
"twilio.*",
"uvicorn.*"
"uvicorn.*",
"validators.*",
]
ignore_missing_imports = true

Expand Down Expand Up @@ -201,4 +202,4 @@ markers = [
"integration_firebase_auth",
"unit_saas"
]
asyncio_mode = "auto"
asyncio_mode = "auto"
3 changes: 2 additions & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ pandas==1.4.3
passlib[bcrypt]==1.7.4
plotly==5.4
psycopg2-binary==2.9.3
pydantic<1.10.0
pydantic<1.10.2
ThomasLaPiana marked this conversation as resolved.
Show resolved Hide resolved
pydash==5.1.1
PyJWT==2.4.0
pymongo==3.13.0
Expand All @@ -44,4 +44,5 @@ SQLAlchemy-Utils==0.38.3
toml>=0.10.1
twilio==7.15.0
Unidecode==1.3.4
validators==0.20.0
ThomasLaPiana marked this conversation as resolved.
Show resolved Hide resolved
versioneer==0.19
5 changes: 5 additions & 0 deletions src/fides/__init__.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,11 @@
"""Fides CLI"""

from fides.ctl.core.config import get_config

from ._version import get_versions

# Load the config here as a work around to the timing issue with environment variables
get_config()

__version__ = get_versions()["version"]
del get_versions
3 changes: 2 additions & 1 deletion src/fides/api/ops/api/v1/endpoints/drp_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
logger = logging.getLogger(__name__)
router = APIRouter(tags=["DRP"], prefix=urls.V1_URL_PREFIX)
CONFIG = get_config()

EMBEDDED_EXECUTION_LOG_LIMIT = 50


Expand All @@ -67,7 +68,7 @@ async def create_drp_privacy_request(
a corresponding Fidesops PrivacyRequest
"""

jwt_key: str = CONFIG.security.drp_jwt_secret
jwt_key = CONFIG.security.drp_jwt_secret
if jwt_key is None:
raise HTTPException(
status_code=HTTP_500_INTERNAL_SERVER_ERROR,
Expand Down
1 change: 1 addition & 0 deletions src/fides/api/ops/api/v1/endpoints/oauth_endpoints.py
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ async def acquire_access_token(
raise AuthenticationFailure(detail="Authentication Failure")

logger.info("Creating access token")

access_code = client_detail.create_access_code_jwe(
CONFIG.security.app_encryption_key
)
Expand Down
2 changes: 1 addition & 1 deletion src/fides/api/ops/models/privacy_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@
from fides.api.ops.schemas.masking.masking_secrets import MaskingSecretCache
from fides.api.ops.schemas.redis_cache import Identity
from fides.api.ops.tasks import celery_app
from fides.api.ops.util.identity_verification import IdentityVerificationMixin
from fides.api.ops.util.cache import (
FidesopsRedis,
get_all_cache_keys_for_privacy_request,
Expand All @@ -69,6 +68,7 @@
)
from fides.api.ops.util.collection_util import Row
from fides.api.ops.util.constants import API_DATE_FORMAT
from fides.api.ops.util.identity_verification import IdentityVerificationMixin
from fides.ctl.core.config import get_config

logger = logging.getLogger(__name__)
Expand Down
1 change: 1 addition & 0 deletions src/fides/api/ops/schemas/encryption_request.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ class AesEncryptionRequest(BaseModel):
@validator("key")
def validate_key(cls, v: str) -> bytes:
"""Convert string into bytes and verify this is the correct length"""

key = v.encode(CONFIG.security.encoding)
verify_encryption_key(key)
return key
Expand Down
1 change: 0 additions & 1 deletion src/fides/api/ops/tasks/storage.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ def create_presigned_url_for_s3(
:param object_name: string
:return: Presigned URL as string.
"""

response = s3_client.generate_presigned_url(
"get_object",
Params={"Bucket": bucket_name, "Key": object_name},
Expand Down
7 changes: 1 addition & 6 deletions src/fides/api/ops/util/identity_verification.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@
from typing import Optional

from fides.api.ops.common_exceptions import IdentityVerificationException
from fides.api.ops.util.cache import (
FidesopsRedis,
get_cache,
)

from fides.api.ops.util.cache import FidesopsRedis, get_cache
from fides.ctl.core.config import get_config


logger = logging.getLogger(__name__)
CONFIG = get_config()

Expand Down
Loading