Zitadel with Login V2 - Minor fixes following code review by remz1337 · Pull Request #1428 · community-scripts/ProxmoxVED

remz1337 · 2026-02-07T13:20:55Z

✍️ Description

Fixes some bugs found in code review

✅ Prerequisites (X in brackets)

Self-review completed – Code follows project standards.
Tested thoroughly – Changes work as expected.
No breaking changes – Existing functionality remains intact.
No security risks – No hardcoded secrets, unnecessary privilege escalations, or permission issues.

🛠️ Type of Change (X in brackets)

🐞 Bug fix – Resolves an issue without breaking functionality.
✨ New feature – Adds new, non-breaking functionality.
💥 Breaking change – Alters existing functionality in a way that may require updates.
🆕 New script – A fully functional and tested script or script set.
🌍 Website update – Changes to website-related JSON files or metadata.
🔧 Refactoring / Code Cleanup – Improves readability or maintainability without changing functionality.
📝 Documentation update – Changes to README, AppName.md, CONTRIBUTING.md, or other docs.

🔍 Code & Security Review (X in brackets)

Follows Code_Audit.md & CONTRIBUTING.md guidelines
Uses correct script structure (AppName.sh, AppName-install.sh, AppName.json)
No hardcoded credentials

📋 Additional Information (optional)

greptile-apps · 2026-02-07T13:22:38Z

Greptile Overview

Greptile Summary

This PR applies small follow-up fixes to the Zitadel CT setup and install scripts:

ct/zitadel.sh: updates the update flow to stop/start the API and Login V2 services separately, with a short delay between them.
install/zitadel-install.sh: removes an extra dependency install step, writes the masterkey without a trailing newline and locks down permissions, runs zitadel init/setup as the zitadel system user, switches the API service to use --masterkeyFile, and updates the rerun helper script to stop/restart the correct services.

Main functional concern is systemd unit activation: the installer writes new unit files but no longer reloads systemd before enabling them, which can break fresh installs.

Confidence Score: 2/5

This PR has a real risk of breaking fresh installs due to systemd not loading newly written unit files before enabling them.
While the changes are small and mostly improve correctness (service separation, masterkey handling, running init/setup as the service user), removing systemctl daemon-reload after creating unit files can make systemctl enable -q --now fail on clean systems. There is also a likely ownership gap for ${CONFIG_DIR} when running init/setup as zitadel. Fixing these would materially reduce merge risk.
install/zitadel-install.sh

Important Files Changed

Filename	Overview
ct/zitadel.sh	Splits stop/start into explicit api/login units and adds a startup delay; update flow should consistently target the installed unit names to avoid failures on systems without a legacy `zitadel` service.
install/zitadel-install.sh	Tightens masterkey handling and runs Zitadel init/setup as the `zitadel` user, but removing `systemctl daemon-reload` after writing unit files can cause service enable/start to fail; also config dir ownership may be inconsistent for files created during init/setup.

greptile-apps

_{2 files reviewed, 5 comments}

_{Edit Code Review Agent Settings | Greptile}

greptile-apps · 2026-02-07T13:22:41Z

    msg_info "Starting Service"
-    systemctl start zitadel
+    systemctl start zitadel-api
+    sleep 5
+    systemctl start zitadel-login


Wrong service name

update_script now starts zitadel-api and zitadel-login, but it previously started zitadel and no longer starts a unit named zitadel. The install script only creates zitadel-api.service and zitadel-login.service, so starting zitadel will fail on existing installs unless a legacy zitadel.service still exists. Consider starting/stopping the same unit names consistently (e.g., zitadel-api.service / zitadel-login.service) to avoid failed updates on systems without a zitadel unit.

greptile-apps · 2026-02-07T13:22:42Z

 msg_info "Create zitadel-rerun.sh"
 cat <<EOF >~/zitadel-rerun.sh
-systemctl stop zitadel
-timeout --kill-after=5s 15s zitadel setup --masterkeyFile ${CONFIG_DIR}/.masterkey --config ${CONFIG_DIR}/config.yaml"
-systemctl restart zitadel
+systemctl stop zitadel-api zitadel-login
+timeout --kill-after=5s 15s /opt/zitadel/zitadel setup --masterkeyFile ${CONFIG_DIR}/.masterkey --config ${CONFIG_DIR}/config.yaml
+systemctl restart zitadel-api zitadel-login


Stray quote removed

In the generated ~/zitadel-rerun.sh, the old command had a trailing " which would make the script fail to run. The PR fixed it, but the heredoc currently writes a command without quoting paths/vars; if ${CONFIG_DIR} ever contains spaces (unlikely here) it would break. To keep it robust, quote the expanded paths in the heredoc.

Suggested change

msg_info "Create zitadel-rerun.sh"

cat <<EOF >~/zitadel-rerun.sh

systemctl stop zitadel

timeout --kill-after=5s 15s zitadel setup --masterkeyFile ${CONFIG_DIR}/.masterkey --config ${CONFIG_DIR}/config.yaml"

systemctl restart zitadel

systemctl stop zitadel-api zitadel-login

timeout --kill-after=5s 15s /opt/zitadel/zitadel setup --masterkeyFile ${CONFIG_DIR}/.masterkey --config ${CONFIG_DIR}/config.yaml

systemctl restart zitadel-api zitadel-login

timeout --kill-after=5s 15s /opt/zitadel/zitadel setup --masterkeyFile "${CONFIG_DIR}/.masterkey" --config "${CONFIG_DIR}/config.yaml"

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}

greptile-apps · 2026-02-07T13:22:43Z

 mkdir -p ${CONFIG_DIR}
-echo "${MASTERKEY}" > ${CONFIG_DIR}/.masterkey
+echo -n "${MASTERKEY}" > ${CONFIG_DIR}/.masterkey
+chmod 600 "${CONFIG_DIR}/.masterkey"
+chown "${ZITADEL_USER}:${ZITADEL_GROUP}" "${CONFIG_DIR}/.masterkey"



Masterkey newline/ownership

echo -n "${MASTERKEY}" > ${CONFIG_DIR}/.masterkey avoids adding a newline, but the file is created as root and only later chowned; if any step between creation and chown reads the file (e.g., a parallel service start), it will be unreadable by the zitadel user. Safer is to create/write the file as the target user (or ensure no reads happen before ownership is set) and keep the path quoted.

Also, mkdir -p ${CONFIG_DIR} is unquoted; use quotes for consistency.

Suggested change

mkdir -p ${CONFIG_DIR}

echo "${MASTERKEY}" > ${CONFIG_DIR}/.masterkey

echo -n "${MASTERKEY}" > ${CONFIG_DIR}/.masterkey

chmod 600 "${CONFIG_DIR}/.masterkey"

chown "${ZITADEL_USER}:${ZITADEL_GROUP}" "${CONFIG_DIR}/.masterkey"

mkdir -p "${CONFIG_DIR}"

greptile-apps · 2026-02-07T13:22:44Z

 chown "${ZITADEL_USER}:${ZITADEL_GROUP}" "${CONFIG_DIR}/config.yaml"

 # Initialize database as zitadel user (no masterkey needed for init)
-$STD ./zitadel init --config ${CONFIG_DIR}/config.yaml
+$STD sudo -u ${ZITADEL_USER} ./zitadel init --config ${CONFIG_DIR}/config.yaml

 # Run setup phase as zitadel user (with masterkey and steps)
-$STD ./zitadel setup --config ${CONFIG_DIR}/config.yaml --steps ${CONFIG_DIR}/config.yaml --masterkey "${MASTERKEY}"
+$STD sudo -u ${ZITADEL_USER} ./zitadel setup --config ${CONFIG_DIR}/config.yaml --steps ${CONFIG_DIR}/config.yaml --masterkey "${MASTERKEY}"



Init/setup permissions

Switching to sudo -u ${ZITADEL_USER} for ./zitadel init / ./zitadel setup is correct for file ownership, but these commands write artifacts into the working directory and config paths; ensure ${ZITADEL_DIR} and ${CONFIG_DIR} are already writable by zitadel at this point. Right now mkdir -p ${CONFIG_DIR} happens before the chown calls for ${CONFIG_DIR} (only .masterkey and config.yaml are chowned), so other files created under ${CONFIG_DIR} by init/setup may end up root-owned.

Consider chowning the whole config dir before running init/setup.

Suggested change

chown "${ZITADEL_USER}:${ZITADEL_GROUP}" "${CONFIG_DIR}/config.yaml"

# Initialize database as zitadel user (no masterkey needed for init)

$STD ./zitadel init --config ${CONFIG_DIR}/config.yaml

$STD sudo -u ${ZITADEL_USER} ./zitadel init --config ${CONFIG_DIR}/config.yaml

# Run setup phase as zitadel user (with masterkey and steps)

$STD ./zitadel setup --config ${CONFIG_DIR}/config.yaml --steps ${CONFIG_DIR}/config.yaml --masterkey "${MASTERKEY}"

$STD sudo -u ${ZITADEL_USER} ./zitadel setup --config ${CONFIG_DIR}/config.yaml --steps ${CONFIG_DIR}/config.yaml --masterkey "${MASTERKEY}"

chown -R "${ZITADEL_USER}:${ZITADEL_GROUP}" "${CONFIG_DIR}"

greptile-apps · 2026-02-07T13:22:45Z

 WantedBy=multi-user.target
 EOF

-# Reload systemd
-systemctl daemon-reload
-
 # Enable and start API service
 systemctl enable -q --now zitadel-api.service

 # Wait for API to start
-sleep 10
+sleep 5

 # Enable and start Login service
 systemctl enable -q --now zitadel-login.service


Missing daemon-reload

The PR removed systemctl daemon-reload after writing new unit files. On a fresh install, systemd may not recognize zitadel-api.service / zitadel-login.service yet, causing systemctl enable -q --now ... to fail. If the repo standard is to avoid daemon-reload, the install should use the project’s helper for creating services (if available) or otherwise ensure units are loaded before enabling.

(As-is, this will definitely fail on systems where systemd hasn’t noticed the new unit files yet.)

CrazyWolf13 · 2026-02-07T16:35:43Z

+    systemctl stop zitadel-api
+    systemctl stop zitadel-login


if I may ask, what values does it provide to make them a single line?

Just in case someone updates from the original script where zitadel-login service doesn’t exist but i might be wrong

But then it fails with exit Code?

I don’t know, something to test. But I have no issue merging this in a single command. Anyway people with older installation would likely need to reinstall from scratch

if that's a problem, then just add a check or add || true ?

I merge it, i can test tomorrow at company with an cloned snapshot of an older install

Minor fixes following code review

abaca28

remz1337 requested a review from a team as a code owner February 7, 2026 13:20

github-actions Bot added bugfix update script labels Feb 7, 2026

greptile-apps Bot reviewed Feb 7, 2026

View reviewed changes

CrazyWolf13 reviewed Feb 7, 2026

View reviewed changes

tremor021 reviewed Feb 7, 2026

View reviewed changes

Comment thread install/zitadel-install.sh Outdated

Apply suggestion from @tremor021

146f681

MickLesk merged commit 32682b9 into community-scripts:main Feb 8, 2026
1 check passed

remz1337 mentioned this pull request Feb 14, 2026

Zitadel #1458

Closed

Uh oh!

Uh oh!

Conversation

remz1337 commented Feb 7, 2026

✍️ Description

✅ Prerequisites (X in brackets)

🛠️ Type of Change (X in brackets)

🔍 Code & Security Review (X in brackets)

📋 Additional Information (optional)

Uh oh!

greptile-apps Bot commented Feb 7, 2026

Greptile Overview

Greptile Summary

Confidence Score: 2/5

Important Files Changed

Uh oh!

greptile-apps Bot left a comment

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

CrazyWolf13 Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

remz1337 Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

MickLesk Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

remz1337 Feb 7, 2026

Choose a reason for hiding this comment

Uh oh!

CrazyWolf13 Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

MickLesk Feb 8, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants