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

Allow optional secrets.json during Docker build #1000

Closed
wants to merge 1 commit into from

Conversation

afwolfe
Copy link

@afwolfe afwolfe commented Jun 30, 2024

Allow optional secrets.json during Docker build

🗣 Description

This PR allows the Dockerfile to take advantage of build secrets, allowing the pre-installed distribution build to support both environment variables and the secrets.json in a similar fashion to the entrypoint script.

💭 Motivation and context

If a user has already created a secrets file for use with their local container, this change allows the user to have a single source of truth for their credentials and reuse the secrets file for any future updates and builds of the image without needing to explicitly pass in the FOUNDRY_USERNAME and FOUNDRY_PASSWORD build arguments.

🧪 Testing

These changes were tested by building the image locally in a few different configurations. I tested the default build path, providing the FOUNDRY_USERNAME and FOUNDRY_PASSWORD build arguments, and the secrets file.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All future TODOs are captured in issues, which are referenced
    in code comments.
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
    • NOTE: While running the pre-commit hook. I had a failure due to ansible-lint v6.14.0a0 not existing, but it passed otherwise.
  • These code changes follow cisagov code standards.
  • All relevant repo and/or project documentation has been updated
    to reflect the changes in this PR.
  • Tests have been added and/or modified to cover the changes in this PR.
  • All new and existing tests pass.

✅ Pre-merge checklist

  • Revert dependencies to default branches.
  • Finalize version.

✅ Post-merge checklist

  • Add a tag or create a release.

@afwolfe afwolfe requested a review from felddy as a code owner June 30, 2024 20:13
@felddy
Copy link
Owner

felddy commented Jul 9, 2024

Thanks for the contribution! This is a cool idea. I should have a chance to kick the tires this week, and hopefully get it merged.
Thank you again!

@afwolfe
Copy link
Author

afwolfe commented Aug 21, 2024

Hello @felddy, have you had a chance to review this yet? No rush, you had just mentioned trying it out awhile back.

I have rebased the branch on top of the latest changes and updated the version references in the README.

@felddy felddy mentioned this pull request Nov 8, 2024
11 tasks
@felddy
Copy link
Owner

felddy commented Nov 8, 2024

I still think this is a cool idea, and I truly appreciate the PR. I was going to merge your changes into the v13 prototype branch but ran into an issue when I tried to test it. It wasn't a bug in your implementation. That is totally solid.

This issue I ran into was passing the credentials as a JSON blob from GitHub Actions secrets. It is an anti-pattern to pass structured data as it could lead to leakage of the secrets inside.

Structured data can cause secret redaction within logs to fail, because redaction largely relies on finding an exact match for the specific secret value. For example, do not use a blob of JSON, XML, or YAML (or similar) to encapsulate a secret value, as this significantly reduces the probability the secrets will be properly redacted. Instead, create individual secrets for each sensitive value.

See:

I was going to build a step that would build the JSON file from the two secrets in the repository, but that too became a bit problematic as my reusable-workflows required changing and could also lead to leakage.

I ended up going with a hybrid approach. Passing the credentials as two separate Docker build secrets, and documenting in the README how to extract credentials with jq from the command line. It isn't as elegant as your solution, but I think it will be safer, easier to maintain, and not lead to too much pain for the users.

See:

docker build \
  --secret id=foundry_username,src=<(jq -r '.foundry_username' path/to/credentials.json) \
  --secret id=foundry_password,src=<(jq -r '.foundry_password' path/to/credentials.json) \
  --build-arg VERSION=13.332.0 \
  --tag felddy/foundryvtt:13.332.0 \
  https://github.com/felddy/foundryvtt-docker.git#develop

I suspect that you are one of only a few people that are pre-installing distributions, and that the group as a whole is pretty savvy. I hope this solutions works well enough for you.

Thank you again for the PR and making this project that much better. You'll find a shout out on the pre-release:
https://github.com/felddy/foundryvtt-docker/releases/tag/v13.332.0

Cheers!

@felddy felddy closed this Nov 8, 2024
@afwolfe afwolfe deleted the buildx-secret branch November 12, 2024 00:52
@afwolfe
Copy link
Author

afwolfe commented Nov 12, 2024

Thanks for the thoughtful review and taking the time to share the knowledge on the GitHub Actions secret best practices.
I agree that while not as seamless, the final implementation is more secure. Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants