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

Generate app/secret only if SECRET_KEY is unset #538

Merged
merged 1 commit into from
Sep 2, 2024

Conversation

pwalkr
Copy link
Contributor

@pwalkr pwalkr commented Aug 27, 2024

If we already have a secret key, we don't need to create another.

I caught this trying to reduce the number of volume mounts (gramps-project/gramps-web#478) while simultaneously running as non-root. Compose failed to start anything:

grampsweb_celery  | mkdir: cannot create directory ‘/app/secret’: Permission denied
grampsweb-1       | mkdir: cannot create directory ‘/app/secret’: Permission denied

Injecting this docker-compose.sh let me move my secret to env and the service starts and runs normally.

@DavidMStraub
Copy link
Member

Thanks! The problem with this is that using env variables prefixed with GRAMPSWEB_ is only one of several configuration options; if the secret key is set in a config file, this is not detected here.

@pwalkr
Copy link
Contributor Author

pwalkr commented Aug 27, 2024

If the secret key is set in a config file, it's likely mkdir -p /app/secret still won't be doing the right thing. In that case one could set GRAMPSWEB_SECRET_KEY=unused to avoid any mkdir errors on start. But it's not the cleanest solution.

What's ideal? Something like python3 -m gramps_webapi --config /app/config/config.cfg export_config that bash could grok? Move mkdir into python? I'd be interested to apply it in resolving similar indexdir warnings:

grampsweb_celery  | ls: cannot access '/app/indexdir/*.db': No such file or directory
grampsweb-1       | ls: cannot access '/app/indexdir/*.db': No such file or directory

*edit: Ah, maybe none of this needs to happen if GRAMPS_API_CONFIG is set and points to a valid file? touch /app/config/config.cfg would need to move here from Dockerfile... does it need to be populated?

@DavidMStraub
Copy link
Member

Your solution is in any case better than the status quo, so let's merge. Force-pushing to try to get tests to run through.

@pwalkr
Copy link
Contributor Author

pwalkr commented Sep 2, 2024

I've been tinkering with a python-enhanced alternative, would this be closer to ideal?

#!/bin/sh

# mkdirs and pick up any variables from flask config
eval "$(python3 - <<EOC
import os
import sys
from flask import Flask
from gramps_webapi.config import DefaultConfig
from gramps_webapi.const import ENV_CONFIG_FILE
import secrets

# Merge config
app = Flask('config_dump')
app.config.from_object(DefaultConfig)
app.config.from_envvar(ENV_CONFIG_FILE)
app.config.from_prefixed_env(prefix="GRAMPSWEB")

# Handle secret - prints "export" so shell can handle~env before exec
SECRET_PATH = '/app/secret/secret'
if not app.config["SECRET_KEY"] and not os.path.isfile()
    #secrets.token_urlsafe(32) ... write to file
    # Export for shell to eval
    print("export GRAMPSWEB_SECRET_KEY={}".format(cdir))

# mkdirs
for cdir in [
        app.config["REPORT_DIR"],
        app.config["EXPORT_DIR"],
        app.config["THUMBNAIL_CACHE_CONFIG"]["CACHE_DIR"],
        app.config["MEDIA_BASE_DIR"]]:
    if not os.path.isdir(cdir):
        print("+ mkdir {}".format(cdir), file=sys.stderr)
        os.makedirs(cdir)
EOC
)"

# TBD user migration and index here in shell or in python, whichever cleaner

exec "$@"

Could be cleaned up with config-merge logic consolidated in a gramps_webapi method. I'm stuck on how best to set SECRET_KEY and avoid race conditions between api and celery containers (maybe shell's atomic mkdir)

If we already have a secret key, we don't need to create another. This also allows the container to run as non-root without a writable volume mounted at app/secret.
@DavidMStraub
Copy link
Member

Let's start with the simple version for now 🙂

@DavidMStraub DavidMStraub merged commit 3fb1ab5 into gramps-project:master Sep 2, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants