Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @diconico07. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
Putting as draft for now, as I didn't get time to test it works as expected in all situations (in particular I'll try to setup a PXE environment to test it works with PXE). |
f51be45 to
09155fc
Compare
|
/ok-to-test |
|
/test metal3-centos-e2e-integration-test-main |
scripts/tls-common.sh
Outdated
| cp "${IPA_CACERT_FILE}" etc/ironic-python-agent/ironic.crt | ||
|
|
||
| find . | cpio -o -H newc --reproducible >> "${output_path}" | ||
| ) No newline at end of file |
scripts/tls-common.sh
Outdated
| EOF | ||
| cp "${IPA_CACERT_FILE}" etc/ironic-python-agent/ironic.crt | ||
|
|
||
| find . | cpio -o -H newc --reproducible >> "${output_path}" |
There was a problem hiding this comment.
Just checking.
If I remember correctly you have mentioned this only works with an initrd that is compressed with "zstd" or some other specific compression method right?
There was a problem hiding this comment.
Not this one, the way we currently do it at SUSE in the IPA downloader only works with "zstd" or other compression methods where you can simply append to the file.
Here it is a different initrd file that is loaded by PXE in addition to the IPA one (I do not compress it actually since I don't think it would be relevant, but we could also compress it here if needed)
There was a problem hiding this comment.
Pull request overview
This PR implements a mechanism to inject the Ironic CA certificate and additional CA certificates into the IPA (Ironic Python Agent) image, enabling proper TLS certificate verification. The key changes make the ipa-insecure flag conditional (controlled by the IRONIC_INSECURE environment variable) instead of being always enabled, improving the security posture of both PXE and virtual media deployments.
- Creates an initrd bundle containing the CA certificates and IPA configuration
- Modifies kernel parameters across multiple boot methods to conditionally enable TLS verification
- Adds support for dynamic certificate updates via watchmedo monitoring
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/tls-common.sh | Defines IPA certificate paths, creates CA bundle combining custom and Ironic CAs, and implements generate_cacert_bundle_initrd function to create initrd with certificate configuration |
| scripts/runironic | Adds watchmedo monitoring to update IPA_CACERT_FILE when IPA certificates change |
| scripts/rundnsmasq | Generates ipa-cacert-bundle initrd files for both TFTP and HTTP paths, and sets up certificate update monitoring |
| ironic-config/ironic.conf.j2 | Configures api_ca_cert parameter and makes ipa-insecure flag conditional across pxe, redfish, and irmc drivers |
| ironic-config/ipxe_config.template | Adds ipa-cacert-bundle initrd loading to deploy, anaconda, and ramdisk boot paths |
| ironic-config/inspector.ipxe.j2 | Adds ipa-cacert-bundle initrd loading and makes ipa-insecure flag conditional for inspection boot |
| README.md | Documents the new IRONIC_INSECURE environment variable and /certs/ca/ipa mount point |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
scripts/tls-common.sh
Outdated
| [DEFAULT] | ||
| cafile = /etc/ironic-python-agent/ironic.crt | ||
| EOF | ||
| cp "${IPA_CACERT_FILE}" etc/ironic-python-agent/ironic.crt |
There was a problem hiding this comment.
The cp command for IPA_CACERT_FILE doesn't check if the source file exists or if the copy was successful. If IPA_CACERT_FILE doesn't exist or the copy fails, the function will continue to create an incomplete initrd bundle, which could cause TLS verification failures in IPA.
Consider adding validation that IPA_CACERT_FILE exists and the copy succeeds before proceeding to create the cpio archive.
| cp "${IPA_CACERT_FILE}" etc/ironic-python-agent/ironic.crt | |
| if [ ! -f "${IPA_CACERT_FILE}" ]; then | |
| echo "ERROR: IPA_CACERT_FILE '${IPA_CACERT_FILE}' does not exist. Cannot create initrd bundle." >&2 | |
| exit 1 | |
| fi | |
| if ! cp "${IPA_CACERT_FILE}" etc/ironic-python-agent/ironic.crt; then | |
| echo "ERROR: Failed to copy '${IPA_CACERT_FILE}' to 'etc/ironic-python-agent/ironic.crt'." >&2 | |
| exit 1 | |
| fi |
scripts/rundnsmasq
Outdated
| generate_cacert_bundle_initrd /shared/tftpboot/ipa-cacert-bundle | ||
| generate_cacert_bundle_initrd /shared/html/images/ipa-cacert-bundle |
There was a problem hiding this comment.
The generate_cacert_bundle_initrd function calls are missing error handling. If the function fails (e.g., due to disk space issues, permission problems, or missing IPA_CACERT_FILE), the script will continue without any indication of the failure. This could lead to deployments using an outdated or missing CA certificate bundle.
Consider checking the return value of these function calls or adding set -e error handling to catch failures.
ironic-config/ironic.conf.j2
Outdated
|
|
||
| [irmc] | ||
| kernel_append_params = nofb nomodeset vga=normal ipa-insecure=1 {% if env.ENABLE_FIPS_IPA %}fips={{ env.ENABLE_FIPS_IPA|trim }}{% endif %} {% if env.IRONIC_RAMDISK_SSH_KEY %}sshkey="{{ env.IRONIC_RAMDISK_SSH_KEY|trim }}"{% endif %} {{ env.IRONIC_KERNEL_PARAMS|trim }} systemd.journald.forward_to_console=yes | ||
| kernel_append_params = nofb nomodeset vga=normal {% if env.IRONIC_INSECURE %}ipa-insecure=1{% endif %} {% if env.ENABLE_FIPS_IPA %}fips={{ env.ENABLE_FIPS_IPA|trim }}{% endif %} {% if env.IRONIC_RAMDISK_SSH_KEY %}sshkey="{{ env.IRONIC_RAMDISK_SSH_KEY|trim }}"{% endif %} {{ env.IRONIC_KERNEL_PARAMS|trim }} systemd.journald.forward_to_console=yes |
There was a problem hiding this comment.
The kernel_append_params for the irmc driver is missing the initrd=ipa-cacert-bundle parameter that was added to the pxe driver. This means virtual media deployments using the irmc driver won't have the CA certificate bundle injected, breaking TLS verification for IPA in irmc-based deployments.
The initrd=ipa-cacert-bundle parameter should be added to this line, similar to line 231.
| kernel_append_params = nofb nomodeset vga=normal {% if env.IRONIC_INSECURE %}ipa-insecure=1{% endif %} {% if env.ENABLE_FIPS_IPA %}fips={{ env.ENABLE_FIPS_IPA|trim }}{% endif %} {% if env.IRONIC_RAMDISK_SSH_KEY %}sshkey="{{ env.IRONIC_RAMDISK_SSH_KEY|trim }}"{% endif %} {{ env.IRONIC_KERNEL_PARAMS|trim }} systemd.journald.forward_to_console=yes | |
| kernel_append_params = nofb nomodeset vga=normal initrd=ipa-cacert-bundle {% if env.IRONIC_INSECURE %}ipa-insecure=1{% endif %} {% if env.ENABLE_FIPS_IPA %}fips={{ env.ENABLE_FIPS_IPA|trim }}{% endif %} {% if env.IRONIC_RAMDISK_SSH_KEY %}sshkey="{{ env.IRONIC_RAMDISK_SSH_KEY|trim }}"{% endif %} {{ env.IRONIC_KERNEL_PARAMS|trim }} systemd.journald.forward_to_console=yes |
09155fc to
dfe04e5
Compare
|
/retest |
|
/test metal3-centos-e2e-integration-test-main |
dfe04e5 to
e2bc693
Compare
|
/test metal3-centos-e2e-integration-test-main |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| cd "${temp_dir}" || return | ||
|
|
||
| mkdir -p etc/ironic-python-agent.d etc/ironic-python-agent |
There was a problem hiding this comment.
The mkdir -p commands for creating the directory structure do not have error checking. If the directory creation fails (e.g., due to permissions), the subsequent cp command will fail. While the function will return an error due to the subshell ( usage and bash's error propagation, it would be clearer to check the result explicitly or ensure the script has proper error handling enabled with set -e within the subshell.
| certificates_path = {{ env.IRONIC_GEN_CERT_DIR }} | ||
| api_ca_file = {{ env.IPA_CACERT_FILE }} |
There was a problem hiding this comment.
The api_ca_file configuration is unconditionally set to {{ env.IPA_CACERT_FILE }}, but this file may not exist in all scenarios (e.g., when no IPA CA certificates are provided, no default CA bundle exists, and no Ironic CA certificate is available). This could cause Ironic to fail if it expects a valid file path. Consider adding a conditional check similar to the webserver_verify_ca pattern above (lines 32-36) to only set this when the file exists.
| certificates_path = {{ env.IRONIC_GEN_CERT_DIR }} | |
| api_ca_file = {{ env.IPA_CACERT_FILE }} | |
| certificates_path = {{ env.IRONIC_GEN_CERT_DIR }} | |
| {% if env.IPA_CACERT_FILE %} | |
| api_ca_file = {{ env.IPA_CACERT_FILE }} | |
| {% endif %} |
There was a problem hiding this comment.
to me an empty file would denote a configuration issue (default bundle path not correctly set) and thus I'm not sure we want to make it conditional here, but I'm not opposed to do so if you see other benefits to it.
|
|
||
| cd "${temp_dir}" || return | ||
|
|
||
| mkdir -p etc/ironic-python-agent.d etc/ironic-python-agent |
There was a problem hiding this comment.
The generate_cacert_bundle_initrd function may fail silently if the IPA_CACERT_FILE does not exist. This can happen when there are no IPA CA certificates and no default CA bundle available. The function should check if the source file exists before attempting to copy it, or handle the error explicitly.
| mkdir -p etc/ironic-python-agent.d etc/ironic-python-agent | |
| mkdir -p etc/ironic-python-agent.d etc/ironic-python-agent | |
| if [ ! -f "${IPA_CACERT_FILE}" ]; then | |
| echo "ERROR: IPA CA certificate bundle '${IPA_CACERT_FILE}' not found; cannot generate initrd bundle." >&2 | |
| return 1 | |
| fi |
README.md
Outdated
| - `/certs/ca/ipa` - The storage path of IPA CA certificates. If the path exists | ||
| and `IRONIC_INSECURE` variable is not set to `true`, the certificates in this | ||
| path will be used by the Ironic Python Agent. The Ironic CA bundle will be | ||
| added to the trusted CA automatically. |
There was a problem hiding this comment.
The documentation states that when IRONIC_INSECURE is not set to true, the IPA certificates will be used. However, this is misleading because the default value of IRONIC_INSECURE is set to false in scripts/tls-common.sh line 3. The documentation should clarify that the default behavior is to use TLS certificate verification (i.e., secure by default), and setting IRONIC_INSECURE=true explicitly disables it.
ironic-config/ipxe_config.template
Outdated
| kernel {% if pxe_options.ipxe_timeout > 0 %}--timeout {{ pxe_options.ipxe_timeout }} {% endif %}{{ aki_path_https }} selinux=0 troubleshoot=0 text {{ pxe_options.pxe_append_params|default("", true) }} BOOTIF=${mac} initrd={{ pxe_options.initrd_filename|default("deploy_ramdisk", true) }} || goto retry | ||
|
|
||
| initrd {% if pxe_options.ipxe_timeout > 0 %}--timeout {{ pxe_options.ipxe_timeout }} {% endif %}{{ ari_path_https }} || goto retry | ||
| # Load ipa-cacert-bundle, path is relative to the ipxe script that will be located in /shared/html/{node_id}/ |
There was a problem hiding this comment.
The initrd reference path ../ipa-cacert-bundle is relative and assumes a specific directory structure. This may break if the iPXE script location changes. Consider using an absolute path or documenting the expected directory structure more explicitly in the comment.
| # Load ipa-cacert-bundle, path is relative to the ipxe script that will be located in /shared/html/{node_id}/ | |
| # Load ipa-cacert-bundle. The "../ipa-cacert-bundle" path is intentionally relative to the iPXE script, | |
| # which is expected to be located in /shared/html/{node_id}/, with ipa-cacert-bundle in its parent directory. | |
| # If the script location or directory layout changes, this relative path must be updated accordingly. |
There was a problem hiding this comment.
Path is intentionally relative to the script, but as we generate the ipa-cacert-bundle in another script, not sure if it's useful to say more here.
e2bc693 to
a38467e
Compare
|
/test metal3-centos-e2e-integration-test-main |
|
/test metal3-ubuntu-e2e-integration-test-main |
| {%- set aki_path_https_elements = pxe_options.deployment_aki_path.split(':') %} | ||
| {%- set aki_port_and_path = aki_path_https_elements[2].split('/') %} | ||
| {%- set aki_afterport = aki_port_and_path[1:]|join('/') %} | ||
| {%- set aki_path_https = ['https:', aki_path_https_elements[1], ':8084/', aki_afterport]|join %} | ||
| {%- endif %} | ||
| {%- if pxe_options.deployment_ari_path %} | ||
| {%- set ari_path_https_elements = pxe_options.deployment_ari_path.split(':') %} | ||
| {%- set ari_port_and_path = ari_path_https_elements[2].split('/') %} | ||
| {%- set ari_afterport = ari_port_and_path[1:]|join('/') %} | ||
| {%- set ari_path_https = ['https:', ari_path_https_elements[1], ':8084/', ari_afterport]|join %} | ||
| {%- endif %} |
There was a problem hiding this comment.
Is there any particular reason why we don't need this section anymore? This is for TLS enabled IPXE firmware and not related to the certs within the IPA image.
There was a problem hiding this comment.
To properly inject the CA when using iPXE it needs to always use the custom templated version (see the change in ironic.conf.j2).
I then remembered #898 that stated this is no longer needed, if this is not true (wasn't able to check as I don't have TLS enabled on my local PXE setup), then we'll likely need to use a different template whether we are in TLS enabled setup or not.
There was a problem hiding this comment.
Looked again, more closely and found it safer to go back to having this, put a conditional to only use it when we're in HTTPS iPXE (look at the change in runironic for how it's handled)
286d905 to
5966f7c
Compare
5966f7c to
6ddb3ea
Compare
|
Made a bunch of changes to how it works:
With these changes, as we fall back to IPA_INSECURE if we are not able to verify certificate both IrSO and regular e2e test suite should work. |
|
/test metal3-centos-e2e-integration-test-main |
6ddb3ea to
a9bf65e
Compare
|
/retest |
|
/test metal3-centos-e2e-integration-test-main |
a9bf65e to
da46845
Compare
|
/test metal3-centos-e2e-integration-test-main |
da46845 to
39902d5
Compare
|
/test metal3-centos-e2e-integration-test-main |
scripts/tls-common.sh
Outdated
| cafile = /etc/ironic-python-agent/ironic.crt | ||
| EOF | ||
|
|
||
| find . | cpio -o -H newc -R +0:+0 --reproducible >> "${output_path}" |
There was a problem hiding this comment.
I wonder if we could find an alternative to a piped method
if we can't, let's maybe sort before using cpio
There was a problem hiding this comment.
The piped method is the easiest one, as other methods would involve either manual listing of the files (error prone) or creating a filelist using the exact same find mechanism, added a sort though
tuminoid
left a comment
There was a problem hiding this comment.
Some nits, but some concerns as well. Hope you can follow the chain of though through the review comments (I use ... to signal the next step is next comment)
scripts/configure-ironic.sh
Outdated
| fi | ||
|
|
||
| if [[ "${IPXE_TLS_SETUP,,}" == "true" ]]; then | ||
| sed -i 's/set ipxe_tls_setup = false/set ipxe_tls_setup = true/' /templates/ipxe_config.template |
There was a problem hiding this comment.
/templates is on read-only filesystem when read-only filesystem hardening is enabled.
There was a problem hiding this comment.
Moved the template to /conf then
| copy_atomic "${WEBSERVER_CACERT_FILE}" "${IPA_CACERT_FILE}" | ||
| elif [ -f "${DEFAULT_CACERT_BUNDLE}" ]; then | ||
| copy_atomic "${DEFAULT_CACERT_BUNDLE}" "${IPA_CACERT_FILE}" | ||
| fi |
There was a problem hiding this comment.
would a
else
echo "Missing TLS CA Bundle"
exit 1
be okay for you, or should I find another way ?
scripts/tls-common.sh
Outdated
| copy_atomic "${DEFAULT_CACERT_BUNDLE}" "${IPA_CACERT_FILE}" | ||
| fi | ||
|
|
||
| if [ -f "${IRONIC_CACERT_FILE}" ]; then |
There was a problem hiding this comment.
and we might not have this either....
|
|
||
| if ! openssl verify -CAfile "${IPA_CACERT_FILE}" "${IRONIC_CERT_FILE}" > /dev/null 2>&1; then | ||
| # if we are unable to verify the Ironic cert file set IPA_INSECURE to true | ||
| export IRONIC_IPA_INSECURE="1" |
There was a problem hiding this comment.
...which leads to IPA being insecure, fine...
| cd "${temp_dir}" || return | ||
|
|
||
| mkdir -p etc/ironic-python-agent.d etc/ironic-python-agent | ||
| cp "${IPA_CACERT_FILE}" etc/ironic-python-agent/ironic.crt |
There was a problem hiding this comment.
...then we end up here copying non-existent files, and doing bad configs....
| # ironic-inspector-image and configuration in configure-ironic.sh | ||
| {{ kernel_cmdline('${ipa_kernel}', '${ipa_ramdisk_name}') }} | ||
| initrd --timeout 60000 ${ipa_ramdisk} || goto retry_boot | ||
| initrd --timeout 60000 {{ env.IRONIC_HTTP_URL }}/ipa-cacert-bundle || goto retry_boot |
There was a problem hiding this comment.
...and eventually we end up here doing reboot loop?
39902d5 to
af4ca2a
Compare
- Use WEBSERVER_CACERT_FILE as base bundle if set - Use image's default otherwise - Append Ironic CA if available - Check if Ironic cert can be verified to set IPA insecure mode - Always use templated version of iPXE script Signed-off-by: Nicolas Belouin <nicolas.belouin@suse.com>
af4ca2a to
41e8dab
Compare
|
/test metal3-centos-e2e-integration-test-main |
What this PR does / why we need it:
This PR adds a way to inject the Ironic CA as well as additional CA certificates into the IPA image, it allows removing the
ipa-insecureflag and actually validate the Ironic certificate from the IPA.This solution should work for both virtualmedia and PXE based deployments.
Checklist: