-
Notifications
You must be signed in to change notification settings - Fork 80
Run Copr infrastructure with podman kube play #4100
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: main
Are you sure you want to change the base?
Conversation
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 Review
This pull request introduces an impressive and comprehensive setup for running the COPR infrastructure locally using podman kube play. The use of an orchestration script, detailed Containerfiles, and Kubernetes manifests is a great step towards modernizing the development environment. My review focuses on enhancing security, maintainability, and robustness. Key areas for improvement include removing hardcoded secrets, reducing container privileges, and simplifying complex commands within manifests and container definitions. While this is a work-in-progress, addressing these points will establish a more secure and maintainable foundation for local COPR development.
| SQLALCHEMY_DATABASE_URI = 'postgresql+psycopg2://copr-fe:coprpass@database:5432/coprdb' | ||
|
|
||
| # Backend authentication | ||
| BACKEND_PASSWORD = '1234' |
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.
Hardcoding secrets like the database URI (with password) and the backend password directly in the configuration file is a critical security vulnerability. These credentials should be managed securely, for example by reading them from environment variables. You will need to import os at the top of this file.
Example:
import os
SQLALCHEMY_DATABASE_URI = os.environ['DATABASE_URI']
BACKEND_PASSWORD = os.environ['BACKEND_PASSWORD']| - containerPort: 22 | ||
| protocol: TCP | ||
| securityContext: | ||
| privileged: true |
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.
Running a container with privileged: true is a major security risk as it grants the container nearly all the capabilities of the host machine. This is likely to allow mock to function, but it's overly permissive. You should identify the specific capabilities mock requires (e.g., CAP_SYS_ADMIN, CAP_SYS_CHROOT) and grant only those in the securityContext.capabilities.add field.
| - name: POSTGRESQL_USER | ||
| value: "copr-fe" | ||
| - name: POSTGRESQL_PASSWORD | ||
| value: "coprpass" |
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.
| pg_ctl -D "$PGDATA" -w start | ||
|
|
||
| # Create user and database | ||
| psql -c "CREATE USER \"$POSTGRESQL_USER\" WITH PASSWORD '$POSTGRESQL_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.
This psql command is vulnerable to syntax errors or SQL injection if the password contains special characters like a single quote. The password should be properly escaped before being used in the SQL query.
A safer way to do this is to escape single quotes within the password variable:
ESCAPED_PASSWORD=$(echo "$POSTGRESQL_PASSWORD" | sed "s/'/''/g")
psql -c "CREATE USER \"$POSTGRESQL_USER\" WITH PASSWORD '$ESCAPED_PASSWORD';"| RUN echo 'root:passwd' | chpasswd && \ | ||
| chmod 700 /root /root/.ssh && \ | ||
| touch /root/.ssh/authorized_keys && chmod 600 /root/.ssh/authorized_keys |
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.
Hardcoding the root password 'passwd' is a significant security risk. Since this container is for a builder and can be accessed via SSH, it's much safer to rely exclusively on SSH key authentication. Please remove the password setting.
RUN chmod 700 /root /root/.ssh && \
touch /root/.ssh/authorized_keys && chmod 600 /root/.ssh/authorized_keys
| chmod 0660 /etc/sign.conf | ||
|
|
||
| # Workaround for user check | ||
| RUN sed -i "s|getpass.getuser() != 'copr-signer'|False|" /usr/bin/gpg-copr |
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.
This sed command to patch the gpg-copr script is a brittle workaround. It disables a user check, but it might break if the script is updated in a future package version. It would be more robust to investigate why getpass.getuser() is not returning the expected 'copr-signer' user and address the root cause, if possible.
| RUN dirs="/var/run/signd /var/run/httpd" ; \ | ||
| for dir in $dirs; do \ | ||
| mkdir -p "$dir" && \ | ||
| chown root:root "$dir" && \ | ||
| chmod 0770 "$dir" ; \ | ||
| done |
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.
This for loop can be simplified. The mkdir, chown, and chmod commands can all accept multiple paths, which makes the script cleaner and more efficient by reducing shell invocations.
RUN mkdir -p /var/run/signd /var/run/httpd && \
chown root:root /var/run/signd /var/run/httpd && \
chmod 0770 /var/run/signd /var/run/httpd
| CMD cd $(rpm -ql resalloc-server |grep alembic.ini |xargs dirname) \ | ||
| && alembic-3 upgrade head \ | ||
| && /usr/bin/resalloc-server |
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.
This complex CMD logic is brittle and hard to read. The command rpm -ql resalloc-server |grep alembic.ini |xargs dirname could fail if the package contents change unexpectedly. It's better practice to move this logic into a dedicated entrypoint.sh script. This improves readability, maintainability, and allows for more robust error handling.
| command: | ||
| - /usr/bin/tini | ||
| - "--" | ||
| - /bin/bash | ||
| - "-c" | ||
| - "mkdir --mode=0777 -p /var/lib/copr-keygen/phrases /var/lib/copr-keygen/gnupg && mkdir --mode=0777 -p /var/run/httpd && exec /usr/sbin/httpd -DFOREGROUND" |
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.
| location ~* .*\.gz$ { | ||
| add_header Content-Encoding gzip; | ||
| } | ||
| } |
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.
This location block for .gz files is not the standard way to serve pre-compressed content. It adds the Content-Encoding: gzip header, but browsers may not decompress it correctly. The recommended approach is to use the gzip_static module, which transparently serves a pre-compressed .gz file when the original is requested.
To implement this, you would need to:
- Install the
nginx-mod-http-gzip-staticpackage in thebackend-httpdContainerfile. - Remove this
locationblock. - Add
gzip_static on;inside thelocation /block above.
Pull Request validationFailed🔴 Review - Missing review from a member (2 required) Success🟢 CI - All checks have passed |
very WIP