-
Notifications
You must be signed in to change notification settings - Fork 333
Improve self-hosting experience with Docker refactor and auth fixes #1231
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
base: newjitsu
Are you sure you want to change the base?
Conversation
…rd flow fixes - Major Docker infrastructure overhaul replacing devenv/ with improved docker/ setup. - Added database seeding capabilities with initial admin user creation. - Fixed change password flow to support changeAtNextLogin flag, forcing password reset. - Consolidated all Docker documentation into README.md and cleaned up docker-compose.yml to contain only implementation notes.
| # Run seed if SEED_DEMO_CONFIGURATION is set | ||
| if [ ! -z "$SEED_DEMO_CONFIGURATION" ]; then | ||
| echo "SEED_DEMO_CONFIGURATION is set, seeding demo configuration..." | ||
| node /app/webapps/console/build/manage.js seed || echo "Seed failed or skipped (this is ok if already seeded)" |
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.
Code Quality: Ineffective error handling
The || echo "Seed failed or skipped..." pattern will always return exit code 0, even if seeding actually fails with a critical error. This masks failures and makes debugging harder.
Consider:
- Checking the actual exit code
- Logging the error properly
- Deciding if seeding failure should be fatal or just a warning
| # Run seed if SEED_DEMO_CONFIGURATION is set | |
| if [ ! -z "$SEED_DEMO_CONFIGURATION" ]; then | |
| echo "SEED_DEMO_CONFIGURATION is set, seeding demo configuration..." | |
| node /app/webapps/console/build/manage.js seed || echo "Seed failed or skipped (this is ok if already seeded)" | |
| if [ ! -z "$SEED_DEMO_CONFIGURATION" ]; then | |
| echo "SEED_DEMO_CONFIGURATION is set, seeding demo configuration..." | |
| if ! node /app/webapps/console/build/manage.js seed; then | |
| echo "WARNING: Seed failed, but continuing startup" | |
| fi | |
| fi |
|
|
||
| // Seed demo connections after successful healthcheck (only once) | ||
| if (!hasErrors && !seedAttempted) { | ||
| seedAttempted = true; | ||
| // Run seed in background, don't block healthcheck response | ||
| seedDemoConnections().catch(err => { | ||
| log.atError().withCause(err).log("Background seed failed"); | ||
| }); | ||
| } |
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.
Architecture Issue: Side effects in healthcheck endpoint
Performing database seeding as a side effect in the healthcheck endpoint is problematic:
- Race conditions: Multiple healthcheck calls could trigger duplicate seeding attempts
- Timeout risk: Seeding might be slow and cause healthcheck timeouts
- Observability: Healthcheck endpoints should be idempotent and fast
- Container orchestration: This could interfere with Kubernetes liveness/readiness probes
Recommendation: Move seeding to a dedicated initialization step in the startup script (docker-start-console.sh) which you've already added at line 93-96. Remove this seeding logic from the healthcheck entirely.
| // Seed demo connections after successful healthcheck (only once) | |
| if (!hasErrors && !seedAttempted) { | |
| seedAttempted = true; | |
| // Run seed in background, don't block healthcheck response | |
| seedDemoConnections().catch(err => { | |
| log.atError().withCause(err).log("Background seed failed"); | |
| }); | |
| } | |
| // Removed: Seeding should happen during container initialization, not in healthcheck | |
| res.status(hasErrors ? 503 : 200).send({ status: hasErrors ? "error" : "ok", ...result }); |
| } | ||
| if (!checkHash(password.hash, body.currentPassword)) { | ||
| //If changeAtNextLogin set to true, do not check current password |
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.
Security Issue: Missing validation for optional current password
When changeAtNextLogin is true, the current password is optional, but there's no explicit validation that ensures body.currentPassword is provided when changeAtNextLogin is false.
If someone sends a request with an empty currentPassword and changeAtNextLogin is false, checkHash() will be called with undefined, which might have unexpected behavior.
| } | |
| if (!checkHash(password.hash, body.currentPassword)) { | |
| //If changeAtNextLogin set to true, do not check current password | |
| //If changeAtNextLogin set to true, do not check current password | |
| if (!password.changeAtNextLogin) { | |
| if (!body.currentPassword) { | |
| throw new Error("Current password is required"); | |
| } | |
| if (!checkHash(password.hash, body.currentPassword)) { | |
| throw new Error("Current password is invalid"); | |
| } | |
| } |
| DISABLE_SIGNUP: false | ||
| ENABLE_CREDENTIALS_LOGIN: true | ||
| SEED_USER_EMAIL: [email protected] |
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.
Configuration Issue: Shared authentication tokens across services
Using the same token (${BULKER_TOKEN:-dev-auth-key}) for multiple different services (Bulker, Rotor, Ingest, Console) reduces security isolation. If one service is compromised, all services are at risk.
Recommendation: Use separate tokens for each service:
BULKER_AUTH_KEYfor BulkerROTOR_AUTH_KEYfor RotorINGEST_AUTH_KEYfor IngestCONSOLE_AUTH_KEYfor Console
This follows the principle of least privilege and improves security boundaries.
| # Core Services | ||
|
|
||
| dep-postgres: |
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.
Security Issue: Weak default passwords
The default passwords like postgres-mqf3nzx, clickhouse-pass, and mongodb-pass are predictable and easily guessable. While they can be overridden, many users will run with defaults in production.
Recommendations:
- Generate random passwords on first run (store in a secrets file)
- Add prominent warnings in documentation about changing defaults
- Consider requiring explicit password setting (no defaults) for production profile
- Add a Docker Compose profile for "production" that requires all passwords to be set
|
@vklimontovich |
This PR significantly improves the self-hosting experience by overhauling the Docker setup,
fixing authentication flows, and adding database seeding capabilities. The changes make it
easier to deploy and maintain self-hosted Jitsu instances.
Self-hosting Jitsu had several friction points: the Docker setup was split between devenv/ and
docker/ directories causing confusion, the change password flow didn't work for initial password
setup or resets, there was no automated way to seed initial admin users, and Docker documentation
was scattered across multiple files with duplication.
Docker Infrastructure Overhaul
Authentication Improvements
changeAtNextLoginflag, allowing password resetswithout requiring the current password
SEED_DEMO_CONFIGURATIONDocumentation and Maintenance
docker/README.mddocker-compose.yml, keeping only implementation notes