chore: update packaging to use config files #119
Conversation
Summary of ChangesHello @kgiusti, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the configuration management for the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the server configuration to use dedicated YAML files instead of relying heavily on sysconfig environment variables and command-line arguments. This is a significant improvement for configuration management and clarity. The implementation is well-executed, including updates to documentation, RPM packaging, and systemd services.
My review focuses on ensuring the new configuration mechanism is secure, robust, and maintainable. I've identified a few areas for improvement:
- The permissions on the configuration directory in the RPM spec file are too permissive and should be tightened for security.
- The default YAML configuration files have minor inconsistencies (integer vs. string for port numbers) and one insecure default setting that should be changed.
- There is some configuration duplication remaining in the
sysconfigfiles, which could be confusing. I've suggested adding comments to clarify this.
Overall, this is a valuable change, and with these adjustments, it will be even better.
kgiusti
force-pushed
the
config-systemd-107
branch
from
15c6c84 to
d1bb649
Compare
test/fmf/tests/utils.sh
Outdated
| sudo sed -i 's|^ADDITIONAL_OPTS=".*"|ADDITIONAL_OPTS=""|' "${sysconfig_file}" || true | ||
| # Reset CMDLINE_OPTS back to empty instead of deleting the file | ||
| if grep -q '^CMDLINE_OPTS=' "${sysconfig_file}"; then | ||
| sudo sed -i 's|^CMDLINE_OPTS=".*"|CMDLINE_OPTS=""|' "${sysconfig_file}" || true |
There was a problem hiding this comment.
just to make sure, sysconfigs have to go away 😄 we don't want them, systemd files shouldn't reference them, everything has to come from our new shiny config yaml/toml - it's not a breaking change either so we can do whatever we want w/o breaking nobody
There was a problem hiding this comment.
Right now they're only used by the generate certs scripts to figure out where the keys belong. Which is wrong - that script should use the config file instead.
There was a problem hiding this comment.
@runcom I've removed all the sysconfigs! The certificate generation code has been refactored quite a bit and nary a sysconfig to be seen.
I had to absolutely murder your fmf http script to do it. Sorry. But... no more sysconfigs!!!
There was a problem hiding this comment.
awesome, @mmartinv wdyt about the changes to the scripts
kgiusti
force-pushed
the
config-systemd-107
branch
from
d1bb649 to
e581e86
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the server packaging and configuration to use configuration files (YAML/TOML) instead of sysconfig environment files. This is a significant improvement, making configuration more structured and aligned with modern practices. The changes include updating the RPM spec file, systemd service files, and test utilities to support the new configuration loading mechanism. Default configuration files are now provided for each server role. My review found one medium-severity issue in a test script related to temporary file creation. Overall, the changes are well-implemented and improve the project's maintainability.
kgiusti
force-pushed
the
config-systemd-107
branch
from
132fe7c to
bc86463
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant improvement, refactoring the server configuration to use YAML files instead of environment variables and command-line arguments. The changes are extensive and well-executed, covering documentation, RPM packaging, systemd services, and tests. The move to file-based configuration enhances usability and maintainability. The updated RPM spec and systemd services follow modern best practices, such as using libexec for helper scripts, /etc/pki for certificates, and recommending systemd drop-ins for customization. The documentation updates are clear and helpful. I've identified a couple of minor issues in the test utility script related to code cleanup and a bug in the file removal logic. Overall, this is an excellent contribution.
test/fmf/tests/utils.sh
Outdated
| @@ -1,47 +1,47 @@ | |||
| #!/bin/bash | |||
| # shellcheck disable=SC2034 | |||
There was a problem hiding this comment.
This directive is added to suppress unused variable warnings. It would be cleaner to remove the unused variables instead. The following variables seem to be unused after the refactoring:
rpm_manufacturer_db_typerpm_manufacturer_db_dsnrpm_rendezvous_db_typerpm_rendezvous_db_dsnrpm_owner_db_typerpm_owner_db_dsn
Consider removing them from the script, along with this shellcheck directive.
Signed-off-by: Kenneth Giusti <[email protected]>
Remove sysconfig files in favor of systemd drop-ins. Refactor default certificate and key generation. Establish /etc/pki/go-fdo-server as the default directory for storing keys. Trigger key generation via a one-shot systemd service. Update tests accordingly. Install certificate tooling to private /usr/libexec directory. Correct minor rpmlint errors. Fixes: fido-device-onboard#107 Signed-off-by: Kenneth Giusti <[email protected]>
The version of systemctl on centos9 does not support stdin for the edit command. Fixed spelling errors and various review cleanup. Signed-off-by: Kenneth Giusti <[email protected]>
Signed-off-by: Kenneth Giusti <[email protected]>
Signed-off-by: Kenneth Giusti <[email protected]>
Signed-off-by: Kenneth Giusti <[email protected]>
kgiusti
force-pushed
the
config-systemd-107
branch
from
bc86463 to
990c83f
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the server configuration to use YAML/TOML files instead of sysconfig environment variables, which is a great improvement for usability and consistency. The changes are well-implemented across the documentation, packaging (RPM spec), and systemd service files. I've found a critical issue in the test utility scripts that needs to be addressed, as it will likely cause CI failures. I also found a medium-severity issue with the certificate auto-generation logic that could lead to a broken state in an edge case. Details are in the comments below.
test/fmf/tests/utils.sh
Outdated
| configure_service_manufacturer() { | ||
| sudo bash -c " | ||
| cp ${manufacturer_key} ${rpm_manufacturer_key} | ||
| cp ${manufacturer_crt} ${rpm_manufacturer_crt} | ||
| chown ${rpm_manufacturer_user}:${rpm_group} ${rpm_manufacturer_key} ${rpm_manufacturer_crt} | ||
| chmod g+r ${rpm_manufacturer_crt} | ||
| cp ${device_ca_crt} ${rpm_device_ca_crt} | ||
| cp ${device_ca_key} ${rpm_device_ca_key} | ||
| chown ${rpm_device_ca_user}:${rpm_group} ${rpm_device_ca_key} ${rpm_device_ca_crt} | ||
| chmod g+r ${rpm_device_ca_crt} | ||
| > ${rpm_manufacturer_sysconfig_file} | ||
| echo 'LISTEN_IP=0.0.0.0' >> ${rpm_manufacturer_sysconfig_file} | ||
| echo 'LISTEN_PORT=${manufacturer_port}' >> ${rpm_manufacturer_sysconfig_file} | ||
| echo 'DATABASE_TYPE=${rpm_manufacturer_db_type}' >> ${rpm_manufacturer_sysconfig_file} | ||
| echo 'DATABASE_DSN=${rpm_manufacturer_db_dsn}' >> ${rpm_manufacturer_sysconfig_file} | ||
| echo 'MANUFACTURER_KEY=${rpm_manufacturer_key}' >> ${rpm_manufacturer_sysconfig_file} | ||
| echo 'OWNER_CRT=${rpm_owner_crt}' >> ${rpm_manufacturer_sysconfig_file} | ||
| echo 'DEVICE_CA_CRT=${rpm_device_ca_crt}' >> ${rpm_manufacturer_sysconfig_file} | ||
| echo 'DEVICE_CA_KEY=${rpm_device_ca_key}' >> ${rpm_manufacturer_sysconfig_file} | ||
| # Add additional options to manufacturer if https is used | ||
| # Enable HTTP if https protocol is used | ||
| if [ '${manufacturer_protocol}' = 'https' ]; then | ||
| mkdir -p ${rpm_certs_base_dir} | ||
| cp ${manufacturer_https_key} ${rpm_manufacturer_https_key} | ||
| cp ${manufacturer_https_crt} ${rpm_manufacturer_https_crt} | ||
| chown ${rpm_manufacturer_user}:${rpm_group} ${rpm_manufacturer_https_key} ${rpm_manufacturer_https_crt} | ||
| additional_opts=\"--http-cert=${rpm_manufacturer_https_crt} --http-key=${rpm_manufacturer_https_key}\" | ||
| echo ADDITIONAL_OPTS=\\\"\${additional_opts}\\\" >> ${rpm_manufacturer_sysconfig_file} | ||
| mkdir -p ${rpm_systemd_drop_in_dir}/go-fdo-server-manufacturer.service.d | ||
| cat <<EOF | tee ${rpm_systemd_drop_in_dir}/go-fdo-server-manufacturer.service.d/10-https.conf | ||
| [Service] | ||
| ExecStart= | ||
| ExecStart=/usr/bin/go-fdo-server manufacturing --http-cert=${rpm_manufacturer_https_crt} --http-key=${rpm_manufacturer_https_key} | ||
| EOF | ||
| systemctl daemon-reload | ||
| fi | ||
| " | ||
| } |
There was a problem hiding this comment.
The test configuration for the services (manufacturer, owner) appears to be incomplete after the refactoring. The logic to copy test-specific certificates and keys (e.g., manufacturer key, owner key, device CA cert) to the locations expected by the services has been removed. The functions now only configure HTTPS certificates.
Without this, the services will fall back to using the auto-generated example certificates, which will likely cause tests that rely on specific credentials to fail.
This logic needs to be restored in configure_service_manufacturer and configure_service_owner. You'll need to:
- Define variables for the target certificate paths based on the new
.yamlconfiguration files (e.g.,/etc/pki/go-fdo-server/manufacturer-example.key). - In each
configure_service_*function, copy the test-specific certificates (from variables like${manufacturer_key}) to these target paths, overwriting the auto-generated ones. - Ensure correct ownership and permissions are set on the copied files.
This issue affects at least configure_service_manufacturer and configure_service_owner.
There was a problem hiding this comment.
One of the purposes of these fmf tests is to validate that our RPM packaging and the systemd services included in that packaging is correct.
One of the functions performed by that packaging/systemd stuff is the generation of default keys/certs. These tests need to verify that we correctly generate and configure these keys/certs when the packages are deployed and the services started.
Restoring the code as advised would overwrite all keys/certs deployed by the packaging and the service initialization code, basically removing any chance of verifying that our packaging and service configuration of keys/certs is functioning correctly.
If a test needs to special case key/cert generation then that test should be responsible for doing so by providing a custom configuration/overrides for that test.
Be aware that the ci/container test code needs to manage certificates because that test code is not deploying via RPMs/systemd services. Perhaps the fmf code originated from that codebase but going forward unless special key/cert handling is necessary the fmf test should use what is provided via the packages.
@runcom @mmartinv @miabbott let me know if you think otherwise.
There was a problem hiding this comment.
I agree with Ken's explanation; but I defer to the rest of the team for the final call
djach7
left a comment
djach7
left a comment
There was a problem hiding this comment.
Successfully tested the PR locally by running various scenarios:
Configuration Discovery:
- Explicit config files (
--config) load correctly with debug logging showing file discovery - Automatic discovery works: searches
~/.config/go-fdo-server/→/etc/go-fdo-server/→/usr/share/go-fdo-server/ - Command-based naming (e.g.,
manufacturing.yaml) resolves properly
Backward Compatibility:
- Graceful fallback when no config exists - logs "configuration file not found" and accepts CLI flags
- All existing CLI arguments work unchanged
YAML Config Loading:
- All three server types (manufacturing/owner/rendezvous) load their respective configs
- New certificate paths (
/etc/pki/go-fdo-server/*-example.{key,crt}) resolve correctly
Build & Runtime:
- make build completes cleanly
- Simplified systemd services work as expected
Note: Claude kept complaining about a path inconsistency between /usr/share/ in search vs /etc/ in RPM and storing fallback configs. I thought this was intentional but maybe I'm wrong, just wanted to mention it.
test/fmf/tests/utils.sh
Outdated
| mkdir -p ${rpm_systemd_drop_in_dir}/go-fdo-server-manufacturer.service.d | ||
| cat <<EOF | tee ${rpm_systemd_drop_in_dir}/go-fdo-server-manufacturer.service.d/10-https.conf | ||
| [Service] | ||
| ExecStart= | ||
| ExecStart=/usr/bin/go-fdo-server manufacturing --http-cert=${rpm_manufacturer_https_crt} --http-key=${rpm_manufacturer_https_key} | ||
| EOF | ||
| systemctl daemon-reload | ||
| fi |
There was a problem hiding this comment.
This is the utils.sh and it should provide a generic way of configuring the services, not just a drop-in file with a hardcoded commandline.
Previous functions generated the sysconfig files based on env variables. Now we need to do the same but with config files and we need to generate the config files in a place where the go-fdo-server can find it automatically (it might worth to add /run/go-fdo-server/ to the list of default locations and generate the files there)
There was a problem hiding this comment.
I agree that the fmf tests should have the ability to generate test-specific configuration files (and yes good idea to add /run/go-fdo-server/ to the config search path).
But I wouldn't make the FMF tests generate configurations or keys/certs by default.
I'm under the impression that the goal of the FMF testing is to validate that the RPM-based deployment works. Is that not the case?
If it is then the FMF tests should be using the configurations, keys, and certs as they are deployed by the RPM as much as possible, right?
So in the case of the FMF tests I wouldn't define functions configure_service_XXX in fmf/utils.h in order to enforce use of the packaged config by default. I would add configuration functions to test-onboarding-https.sh since that test needs a modified config.
There was a problem hiding this comment.
test/fmf/tests/utils.sh
Outdated
| # Default cert/keys for FDO and the Device CA are automatically | ||
| # generated by systemd on service start if they do not exist. | ||
| generate_service_certs() { | ||
| return 0 | ||
| } | ||
|
|
There was a problem hiding this comment.
I am not sure about this, I would rather leave the default function and copy the generated certs to the configuration directory we choose (/run/.. or ${HOME}.config or /etc/...) and adjust the permissions accordingly.
There was a problem hiding this comment.
Related to my last comment: doing that would mean the keys/certs deployed by the RPM will never be validated. Unless there's a specific FMF test case that requires special keys and certs we should use the ones that are RPM deployed.
There was a problem hiding this comment.
Disregard my last comment - certs are now moved to the server's homedir as needed.
| # Sysuser | ||
| %{_sysusersdir}/go-fdo-server-manufacturer.conf | ||
| # Default config | ||
| %config(noreplace) %attr(644, root, go-fdo-server) %{_sysconfdir}/%{name}/manufacturing.yaml |
There was a problem hiding this comment.
I believe this expands to /etc/go-fdo-server-manufacturer/manufacturing.yaml not sure if that's what you want
There was a problem hiding this comment.
Interesting - that does look suspicious. lemme check...
$ rpm -qlvp ./rpmbuild/rpms/noarch/go-fdo-server-manufacturer-0-git3f6e91c.noarch.rpm
-rw-r--r-- 1 root go-fdo-s 833 Nov 18 19:00 /etc/go-fdo-server/manufacturing.yaml
-rw-r--r-- 1 root root 879 Nov 18 19:00 /usr/lib/systemd/system/go-fdo-server-manufacturer.service
-rw-r--r-- 1 root root 118 Nov 18 19:00 /usr/lib/sysusers.d/go-fdo-server-manufacturer.conf
$ rpmspec -P ./build/package/rpm/go-fdo-server.spec | grep manufacturing.yaml
install -m 0644 -vp configs/manufacturing.yaml /home/kgiusti/rpmbuild/BUILD/go-fdo-server-0-build/BUILDROOT/etc/go-fdo-server
%config(noreplace) %attr(644, root, go-fdo-server) /etc/go-fdo-server/manufacturing.yaml
I think it's working - I'm on f41 btw.
Remove useless shell exec from ExecStart command in systemd service files Signed-off-by: Kenneth Giusti <[email protected]>
Issue identified and fix provided by Gemini-code-assist. Signed-off-by: Kenneth Giusti <[email protected]>
Manage scratch home directories for per-test configuration files and state Signed-off-by: Kenneth Giusti <[email protected]>
6 participants
No description provided.