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

Don't require KEY or SECRET to be set on startup #22320

Merged
merged 11 commits into from May 6, 2024
Merged

Don't require KEY or SECRET to be set on startup #22320

merged 11 commits into from May 6, 2024

Conversation

rijkvanzanten
Copy link
Member

Scope

What's changed:

Dropped the requirement to have KEY and SECRET set.

KEY hasn't really been used for anything meaningful that PUBLIC_URL isn't already used for so can be removed. SECRET should be set for production uses, but can be autogenerated on startup for local Docker container debugging and testing.

Potential Risks / Drawbacks

  • The biggest risk is people forgetting to set the SECRET env var for production use. This is a similar issue to forgetting to set any of the other prod env vars though, so the solution should be the same (a prod checklist in docs)

Review Notes / Questions

Copy link

changeset-bot bot commented Apr 25, 2024

🦋 Changeset detected

Latest commit: 25e0b9a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@directus/env Patch
@directus/api Patch
directus Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@rijkvanzanten rijkvanzanten changed the title Key secret Don't require KEY or SECRET to be set on startup Apr 25, 2024
@paescuj paescuj self-requested a review April 25, 2024 19:50
.changeset/weak-chefs-sniff.md Outdated Show resolved Hide resolved
docs/self-hosted/config-options.md Outdated Show resolved Hide resolved
docs/self-hosted/config-options.md Outdated Show resolved Hide resolved
@binaryben

This comment was marked as resolved.

@br41nslug
Copy link
Member

br41nslug commented Apr 26, 2024

I think it would still be good to explicitly warn users when the secret gets auto-generated. When starting a demo docker without any settings relying on the defaults it is expected that the container is ephemeral and loses all data upon restart. However this is a different story for an instance that does have database and persistence settings and just happens to forget the secret.

Similar to the auto-generated admin email/password when bootstrapping a new Directus project:

INFO: No admin email provided. Defaulting to "[email protected]"
INFO: No admin password provided. Defaulting to "gHepardRfX7Z"

We should also a log which secret was generated so the user can potentially use the same database without having to redo the hashed passwords of users.

INFO: No SECRET provided. Defaulting to "[some-generated-value]"

@paescuj
Copy link
Member

paescuj commented Apr 26, 2024

I think it would still be good to explicitly warn users when the secret gets auto-generated. When starting a demo docker without any settings relying on the defaults it is expected that the container is ephemeral and loses all data upon restart. However this is a different story for an instance that does have database and persistence settings and just happens to forget the secret.

Such a warning has already been implemented, see https://github.com/directus/directus/pull/22320/files#diff-8b022aa0e0965067888eadfbc456aade35f80d85aafe1e05f5fdbb060f900653R19-R21.

Similar to the auto-generated admin email/password when bootstrapping a new Directus project:

INFO: No admin email provided. Defaulting to "[email protected]"
INFO: No admin password provided. Defaulting to "gHepardRfX7Z"

We should also a log which secret was generated so the user can potentially use the same database without having to redo the hashed passwords of users.

INFO: No SECRET provided. Defaulting to "[some-generated-value]"

I like that idea 👍 Would be in line with the "admin password".

@binaryben

This comment was marked as resolved.

@br41nslug

This comment was marked as resolved.

@rijkvanzanten
Copy link
Member Author

I like that idea 👍 Would be in line with the "admin password".

The difference is that you need the admin password to be able to login to the project, whereas you don't need to know what the secret is for it to be able to run 🙂

@paescuj
Copy link
Member

paescuj commented May 3, 2024

I like that idea 👍 Would be in line with the "admin password".

The difference is that you need the admin password to be able to login to the project, whereas you don't need to know what the secret is for it to be able to run 🙂

Hmm, right! And after reading Tim's comment again: The secret is only used for the JWTs, so it would still be possible to use the same database, the users just need to get a new token 👍

My only remaining concern is that the warning about the missing secret is only logged as soon as it is actually used. The warning could therefore be overlooked. Do you think we can do the check & warn at server startup, along the following lines?

directus/api/src/app.ts

Lines 77 to 79 in 84ba56e

if (!new Url(env['PUBLIC_URL'] as string).isAbsolute()) {
logger.warn('PUBLIC_URL should be a full URL');
}

@rijkvanzanten
Copy link
Member Author

My only remaining concern is that the warning about the missing secret is only logged as soon as it is actually used. The warning could therefore be overlooked. Do you think we can do the check & warn at server startup, along the following lines?

Yeah lets do it 👍🏻

@paescuj paescuj enabled auto-merge (squash) May 6, 2024 12:57
@paescuj paescuj merged commit ec2604f into main May 6, 2024
5 checks passed
@paescuj paescuj deleted the key-secret branch May 6, 2024 13:00
@github-actions github-actions bot added this to the Next Release milestone May 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants