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

Add support for file-based environment variables #19856

Closed
aminosbh opened this issue May 31, 2022 · 2 comments · Fixed by #24832
Closed

Add support for file-based environment variables #19856

aminosbh opened this issue May 31, 2022 · 2 comments · Fixed by #24832
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.

Comments

@aminosbh
Copy link

aminosbh commented May 31, 2022

Feature Description

In the new container-centric kubernetes-oriented world, storing secrets (or other related data) inside the ini settings file as plain text could be a major security threat and could lead to secrets end up being pushed to a git repository. And managing secrets as plain text in environment variables could also represent a security threat in setups that are using monitoring/reporting/orchestrating tools that log and store environment variables. That's why docker/Kubernetes and other tools offer the ability to manage secrets as separate artifacts in a secure way (on-disk encrypted, plain-text inside the container).

When using docker secret or other tools like Kubernetes, HashiCorp Vault, ... and were the secret is mounted as a file in /run/secrets/<SECRET_NAME> there is a need for a mechanism to load those secrets from the mounted-plain-text-files to make them available to gitea.

The proposal is that any settings in app.ini can be set or overridden with the content of a file by defining an environment variable of the form: GITEA__section_name__KEY_NAME__FILE that points to a file (in case of docker secrets it could point to /run/secrets/<SECRET_NAME>).

This technique could be used in docker-compose.yml as follows:

version: '3.9'
services:
  server:
    container_name: gitea
    image: gitea/gitea:1.16.8
    environment:
      USER_UID: 1000
      USER_GID: 1000
      # Database
      GITEA__database__DB_TYPE: postgres
      GITEA__database__HOST: gitea-db:5432
      GITEA__database__NAME: gitea
      GITEA__database__USER: gitea
      GITEA__database__PASSWD__FILE: /run/secrets/gitea_db_password
    secrets:
    - gitea_db_password
    networks:
    - gitea-net
    
  db:
    container_name: gitea-db
    image: postgres:14-alpine
    environment:
      POSTGRES_DB: gitea
      POSTGRES_USER: gitea
      POSTGRES_PASSWORD_FILE: /run/secrets/gitea_db_password
    secrets:
      - gitea_db_password
    networks:
      - gitea-net

secrets:
  gitea_db_password:
    external: true

As discussed with @zeripath in #19858 I will summarize the usability and security implications of the discussed implementations:
🟢 for positive point, 🔴 for negative point and empty when N/A or could be prevented

  1. Save secrets in ini file (Current implementation)
  2. Save secrets in overlaid dedicated ini file
  3. Make every key have an optional __FILE ending which would automatically read from a file.
    a. Implemented inside gitea: Load secrets from files to memory.
    b. Implemented inside environment-to-ini: Load secrets from file and populate ini file inside the container. Add support for file-based environment variables in environment-to-ini #19857
  4. Make a dedicated secrets section in init file and load from files to memory.
Implication 1 (Current) 2 3.a 3.b #19857 4
Secrets as plain text in the host 🔴
Secrets encrypted in the host (docker secret, Kubernetes, HashiCorp Vault, ...) 🟠 🟢 🟢 🟢
Secrets grouped with the other settings 🔴
Secrets separated from the other settings[*] 🟢 🟢 🟢 🟢
Secrets as plain text inside the container[**] 🔴 🔴 🔴 🔴 🔴
Secrets as plain text in volumes[***] 🟡 🟡
Secrets re-usable with other containers (wide usage) 🟢 🟢 🟢
Major update to ini file 🔴
Update only environment-to-ini 🟣

🟠 : Managing the whole overlaid ini file (dedicated for secrets) with docker secret
🟣 : I don't know if it could be a positive or negative point (in the short run) but the risk of breaking gitea is reduced and, for sure, for the long run it is better to implement it gitea-side.
[*]: When the secrets are separated from the general settings they could be managed by docker secret and mounted only when running the container.
[**]: In all cases the secrets mounted as files will be present as plain text inside the container.
[***]: Having secrets as plain text in volumes could be a security risk but could be mitigated if using an encrypted filesystem and setting the correct file-permission for the volume directory to prevent unprivileged processes from accessing it.

Screenshots

No response

@aminosbh aminosbh added type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first. labels May 31, 2022
@aminosbh aminosbh changed the title Add support for file-based environment variables in environment-to-ini Add support for file-based environment variables Jun 1, 2022
@lunny
Copy link
Member

lunny commented May 24, 2023

Maybe we don't need to mapping all the secrets into the ini file. But only some security related, I think they are not too much. If we support over 200 items via envs, it will become difficult when we want to deprecate them.

Maybe these items:

  1. database setting related, we can support 4 items
    DB_TYPE = mysql
    HOST = 127.0.0.1:3306 ; can use socket e.g. /var/run/mysqld/mysqld.sock
    NAME = gitea
    USER = root

  2. [security] INTERNAL_TOKEN_URI & INTERNAL_TOKEN 2 items

  3. [lfs] LFS_JWT_SECRET 1 items

  4. [oauth2] JWT_SECRET 1 items

lunny pushed a commit that referenced this issue May 24, 2023
Replace #19857

Close #19856
Close #10311
Close #10123

Major changes:

1. Move a lot of code from `environment-to-ini.go` to `config_env.go` to
make them testable.
2. Add `__FILE` support
3. Update documents
4. Add tests
@lonix1
Copy link
Contributor

lonix1 commented Jun 17, 2023

Maybe these items:

Please see this. This new feature works, but it writes those secrets into the app.ini file, which leaks the secrets.

From what I can understand (please correct me if I'm wrong) that was not the intention? The secrets should be read directly from the secrets files into memory, without being written to config file.

meissa331 pushed a commit to DomainDrivenArchitecture/c4k-forgejo that referenced this issue Jul 27, 2023
Configured secret env vars for mailer credentials to be loaded at pod runtime.
We may want to write our credentials to an encrypted file so we can use
the __FILE feature described here: go-gitea/gitea#19856.
Or we may want to encrypt our secrets as described here:
https://kubernetes.io/docs/tasks/administer-cluster/encrypt-data/
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
type/feature Completely new functionality. Can only be merged if feature freeze is not active. type/proposal The new feature has not been accepted yet but needs to be discussed first.
Projects
None yet
3 participants