edns: implement %certificate kickstart feature #6045

rvykydal · 2024-12-10T12:40:22Z

This implementation should provide ground for follow-up/final modifications based on how the certificate location and import process/tooling will be specified (https://issues.redhat.com/browse/INSTALLER-4030).

Depends on pykickstart support: pykickstart/pykickstart#517

For the review it can be useful to follow the order of the commits to see (the reasoning behind) some decisions better.

At this point, there is only an implementation just dumping the certificate to the specified location (--filename and --dir options) at various stages:

initramfs
early after switchroot
early anaconda
early system (pre-install)
system

Regarding the use cases the initramfs will be used most often. (Kickstart fetched without network or via network using already available certs).

At the end there is a patch adapting to options renaming in pykickstart. (pykickstart/pykickstart#514 (review), --name -> --filename, --path->--dir).

Currently it seems that at this point only dump of certificate will be needed but considering the options and future, potential import via a tool (ca-certificates, trust), there is possible API extension with --category option: https://github.com/rvykydal/anaconda/commits/edns-certs-stage2-w-initramfs-category/ and pykickstart rvykydal/pykickstart@54b2592

TODO:

kickstart tests

pep8speaks · 2024-12-10T12:40:36Z

Hello @rvykydal! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2025-01-03 15:09:53 UTC

rvykydal · 2024-12-10T13:52:23Z

/build-image --boot.iso

github-actions · 2024-12-10T14:04:41Z

Images built based on commit e564a96:

boot.iso: success

Download the images from the bottom of the job status page.

jstodola · 2024-12-11T08:55:01Z

data/systemd/anaconda-import-initramfs-certs.service

+[Unit]
+Description=Import of certificates added in initramfs stage of Anaconda via kickstart
+Before=NetworkManager.service
+Before=anaconda.target


Do I understand it correctly, that there is a period of time, where certificates are not available in the installation environment - right after initramfs switchroot and before this import service is executed?
Is there an option to import the certificates to the stage2 environment even before switchroot to stage2 to ensure the provided certificates are available throughout the whole boot process?

Interesting idea, I'll think about pushing from initramfs instead of pulling from root.
For just moving files I guess it should be fine, one possible problem is that for other potential actions, like running import tool as in rvykydal@51c67c5#diff-572b90e7fb275a5450a384189ddd4651c02f9ab6059576366cdc25e5cdbec5f9R9
this might be a bit more tricky (running in /sysroot chroot?).
But to be honest I don't know if it is usual / reasonable thing to do so I'll consult dracut people.

I'll create a follow-up issue for this. I think it would be better to handle it separately.

https://issues.redhat.com/browse/INSTALLER-4089

jkonecny12

Sharing my so far results.

jkonecny12 · 2024-12-11T15:30:09Z

scripts/handle-sshpw

@@ -37,6 +37,7 @@ if not os.path.exists(ksfile):
 handler = makeVersion(VERSION)
 ksparser = KickstartParser(handler, missingIncludeIsFatal=False)
 ksparser.registerSection(NullSection(handler, sectionOpen="%addon"))
+ksparser.registerSection(NullSection(handler, sectionOpen="%certificate"))


Why is this required?

This is some leftover from my initial stage of implementation. It is actually not needed. I'll remove it.

jkonecny12 · 2024-12-11T15:49:37Z

pyanaconda/modules/security/certificates/certificates_interface.py

+    @Certificates.setter
+    @emits_properties_changed
+    def Certificates(self, certificates: List[Structure]):


If it is not a requirement for something I would rather avoid setter support for two main reasons:

UI is not expected to be able to set these

security (anyone can use the API to add another certificate to the system)

Yes, I agree.
Initially I added it mostly for testing :).
I didn't think it would be fundamentally more insecure. Maybe anyone can use ReadKickstart(string) API on the security module instead to do the same thing, but yes there seem to be no valid reason to provide setter API.
I'll make it read-only property.

jkonecny12 · 2024-12-11T16:26:26Z

pyanaconda/core/kickstart/specification.py

+            class SectionDataListString():
+                def __init__(self, data_list):
+                    self._data_list = data_list
+
+                def __str__(self):
+                    retval = []
+                    for data_obj in self._data_list:
+                        if isinstance(data_obj, data):
+                            retval.append(data_obj.__str__())
+                    return "".join(retval)


Is there a reason to hide this class like this? Usually we are creating classes as top level.

The reason is I don't feel the class is not ready / safe to be reused. It feels very single-purpose to me. I'll think more about it.

Maybe also better name would be SectionDataListStrWrapper. And put it at top level in the file?

jkonecny12

Looks great to me overall. Great job! I found just a few notes on such a big change.

Also, I would recommend squashing a few commits together which don't have useful change. As fixups or switch to constants from strings.

jkonecny12 · 2024-12-11T21:09:19Z

pyanaconda/modules/security/certificates/installation.py

+        if not cert.dir:
+            raise SecurityInstallationError(
+                "Certificate destination is missing for {}".format(cert.filename)
+            )


Shouldn't there be a default path for certificate given by the pykickstart? I guess that most of the certificates will go to one place so we don't have to force this on users?

What would be the default path?
At this point where we are just dumping a file to the path it does not make sense to me to have any.
I think even the tooling for certificate import is moving from

put the certificate to the directory ( /etc/pki/ca-trust/source/anchors/)

run a tool - update-ca-trust extract

to

run trust anchor <filename>

(https://docs.redhat.com/en/documentation/red_hat_enterprise_linux/9/html/securing_networks/using-shared-system-certificates_securing-networks#managing-trusted-system-certificates_using-shared-system-certificates)
So I think we don't want to own any default path.
It might be different if/when we offer support to define where we should import the cert (like --category global or --category edns) but still ideally we would just pass the category to the import tool (trust). Something I drafted here: rvykydal@51c67c5

Sounds good, so we can follow their example. Do we want to support only this even now?

I mean do we want the path solution instead of using only the category?

@jkonecny12 Hmm. It may be worth considering removing the --dir solution completely. Depends if we feel comfortable offering the API. I think the --dir --name API is not very big burden given it just dumps the file, no more commitments (any more complex actions/imports would be done via --category). And it can provide some flexibility for clients when the things (paths, solutions) are not settled completely. If we offer only --category, (implementing only --category dns with the path and name of the cert baked in anaconda) at this point, adaptations to any changes of paths etc would require rebuild of Anaconda. Also it wouldn't be probably possible to configure the destination via anaconda configuration as it is applied already in initramfs.

@abbra do you think it would be feasible to provide only --category dns API by anaconda kickstart or should we rather offer also simple --dir and --filename? (Please see above).

Given that we don't yet have a category support in trust anchor, having simple --dir and --filename is handy.

jkonecny12 · 2024-12-11T21:30:22Z

dracut/parse-kickstart

+def process_certificates(handler):
+    """Import certificates defined in %certificate sections."""
+    for cert in handler.certificates:
+


I guess an excessive blank line?

On purpose, feeling more readable to me, but I'll remove it for consistency.

jkonecny12 · 2024-12-11T21:35:34Z

scripts/anaconda-import-initramfs-certs

+# to anaconda environment after switchroot.
+
+# certificates dumped to the specified file are copied to root
+cp -rv /run/install/certificates/path/* /


Is the path at the end really expected. It just feels like a leftover.

Also are we sure that this directory will exist in all the cases? I mean the service will fail if the path is missing, isn't it?

Is the path at the end really expected. It just feels like a leftover.

It is on purpose, this subdirectory of /run/install/certificates/path should contain certificates that should be transported to switched root just by copying to the place, ie certificates only specified by --dir in kickstart. Other subdirectories can potentially contain certificates of other types, for example /run/install/certificates/category/global would contain certificates with option --category global as in rvykydal@51c67c5#diff-572b90e7fb275a5450a384189ddd4651c02f9ab6059576366cdc25e5cdbec5f9R9. It is not so clear without the --category patch.
In other words, to be future-proof I don't want to use the root of the transport directory (/run/install/certificates just for copying everything in it to root.

Also are we sure that this directory will exist in all the cases? I mean the service will fail if the path is missing, isn't it?

No it will not, good point, I'll perhaps modify copy to:

cp -rv /run/install/certificates/path/* / || true

rvykydal · 2024-12-13T09:47:46Z

I am going to clean up the structure a bit, squash, rename to --dir and --filename from the beginning, etc.

rvykydal · 2024-12-17T14:36:16Z

/kickstart-test --testtype smoke

jstodola · 2024-12-17T17:25:16Z

dracut/parse-kickstart

+        log.error("Certificate destination is missing for %s", cert.filename)
+        return
+
+    dst_dir = os.path.join(root+dump_dir.lstrip('/'))


why not os.path.join(root, dump_dir.lstrip('/')) ?

jstodola · 2024-12-17T17:26:44Z

data/systemd/anaconda-import-initramfs-certs.service

+[Unit]
+Description=Import of certificates added in initramfs stage of Anaconda via kickstart
+Before=NetworkManager.service
+Before=anaconda.target


jstodola · 2024-12-17T18:08:53Z

pyanaconda/modules/security/certificates/installation.py

+                return
+
+        for cert in self._certificates:
+            log.debug("Importing certificate %s", cert)


Does this log also the content of the certificate itself? I wonder if it is not enough to log just the certificate directory/filename for readability of the log file.

Good point, I'll update the message.

jkonecny12

Looks good to me otherwise the comment below and @jstodola notes. Great job!

jkonecny12 · 2024-12-18T07:46:33Z

dracut/parse-kickstart

@@ -403,6 +405,44 @@ def ksnet_to_dracut(args, lineno, net, bootdev=False):

    return " ".join(line)

+
+def _dump_certificate(cert, root="/", dump_dir=None):


I'm missing a test for this code.

Indeed, I'll add it.

jkonecny12 · 2024-12-19T16:35:23Z

I'll do the final review when the tests are fixed.

M4rtinK

Looks good to me, thanks! :)

Just a couple suggestions. :)

M4rtinK · 2025-01-02T15:47:38Z

tests/unit_tests/pyanaconda_tests/test_kickstart_specification.py

@@ -213,6 +213,16 @@ class SpecificationF(KickstartSpecification):
            "my_test_2": TestData2
        }

+    class SpecificationG(KickstartSpecification):


I think this is a typo ? SpecificationG

It is just following the pattern naming the test classes Specification.

M4rtinK · 2025-01-03T11:36:04Z

pyanaconda/modules/security/certificates/installation.py

+
+    def run(self):
+        for cert in self._certificates:
+            log.debug("Importing certificate with filename: %s dir: %s", cert.filename, cert.dir)


I find this bit a little weird - the task seemingly should be importing the certificates but it seems to just iterate over them and & log the fact they exist ?

Even if this is correct (eq. the files do get auto-imported) I still suggest noting how this works in a docstring or commit message, to prevent confusion. :)

I find this bit a little weird - the task seemingly should be importing the certificates but it seems to just iterate over them and & log the fact they exist ?

I think you are referring to an initial task skeleton commit, in a follow-up/implementation commit _dump_certificate is called in the loop.

M4rtinK · 2025-01-03T11:37:41Z

pyanaconda/startup_utils.py

@@ -66,6 +66,7 @@
 )


I think there is a typo in the commit message:

The certificates imported in initramfs are already imported earlier a service.

It should be "by a service" or something like that I guess ?

M4rtinK · 2025-01-03T11:39:26Z

pyanaconda/startup_utils.py

@@ -642,6 +643,13 @@ def initialize_security():
    if not flags.automatedInstall:
        security_proxy.FingerprintAuthEnabled = True

+    # Import certificates from kickstart
+    # Most probably they have already been imported in iniramfs stage


Maybe "in the initramfs stage" or even make this more verbose about when the certificates were imported and by what ? :)

M4rtinK · 2025-01-03T11:42:29Z

pyanaconda/modules/security/certificates/installation.py

+        """Dump the certificate into specified file and directory."""
+        dst_dir = join_paths(root, cert.dir)
+        if not os.path.exists(dst_dir):
+            log.debug("Path %s for certificate does not exist, creating.", dst_dir)


Maybe also log for which certificate this new folder is being created ? Eq. something like:

log.debug("Path %s for certificate %s does not exist, creating.", dst_dir, cert.filename)

M4rtinK · 2025-01-03T12:09:44Z

pyanaconda/modules/security/certificates/installation.py

@@ -68,6 +73,12 @@ def run(self):

        Dump the certificates into specified files and directories.
        """
+        if self._phase == INSTALLATION_PHASE_PREINSTALL:
+            if self._payload_type != PAYLOAD_TYPE_DNF:
+                log.debug("Not importing certificates in pre install for %s payload.",


Maybe "in pre install phase" for consistency ?

Kickstart %certificate section is used. Submodule, data structures, parsing. Resolves: INSTALLER-4030 Patch modified by rvykydal.

Unlike the %packages section where the data of all sections are merged into single data object the %certificates section holds the per instance section data in a list. Related: INSTALLER-4030

Resolves: INSTALLER-4030

Related: INSTALLER-4030

The certificates imported in initramfs are already imported earlier by a service. Resolves: INSTALLER-4030

Resolved: INSTALLER-4030

Resolves: INSTALLER-4030

In case they are needed or processed by package scriptlets Resolves: INSTALLER-4030

Resolves: INSTALLER-4030

Don't require it on parsing level, don't own a default. Resolves: INSTALLER-4030

Resolves: INSTALLER-4030

Also dump for transfer durig switchroot so that the certificates can be potentially imported early after switchroot by a service. Resolves: INSTALLER-4030

Resolves: INSTALLER-4030

rvykydal temporarily deployed to gh-cockpituous December 10, 2024 12:40 — with GitHub Actions Inactive

github-actions bot added the f42 Fedora 42 label Dec 10, 2024

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from 753748f to 97f8ddf Compare December 10, 2024 13:17

rvykydal temporarily deployed to gh-cockpituous December 10, 2024 13:17 — with GitHub Actions Inactive

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from 97f8ddf to e564a96 Compare December 10, 2024 13:17

rvykydal had a problem deploying to gh-cockpituous December 10, 2024 13:17 — with GitHub Actions Failure

rvykydal had a problem deploying to gh-cockpituous December 10, 2024 13:27 — with GitHub Actions Failure

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from 61de0f5 to e564a96 Compare December 10, 2024 13:28

rvykydal temporarily deployed to gh-cockpituous December 10, 2024 13:28 — with GitHub Actions Inactive

rvykydal changed the title ~~Edns certs stage2 w initramfs~~ edns: implement %certificate kickstart feature Dec 10, 2024

rvykydal requested review from M4rtinK, jkonecny12, KKoukiou and jstodola December 10, 2024 13:48

jstodola reviewed Dec 11, 2024

View reviewed changes

jkonecny12 reviewed Dec 11, 2024

View reviewed changes

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from e564a96 to ae12329 Compare December 17, 2024 12:46

rvykydal temporarily deployed to gh-cockpituous December 17, 2024 12:46 — with GitHub Actions Inactive

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from ae12329 to 3c054f2 Compare December 17, 2024 14:11

rvykydal had a problem deploying to gh-cockpituous December 17, 2024 14:11 — with GitHub Actions Failure

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from 3c054f2 to 01f977f Compare December 17, 2024 14:13

rvykydal temporarily deployed to gh-cockpituous December 17, 2024 14:13 — with GitHub Actions Inactive

rvykydal requested review from jstodola, jkonecny12 and abbra December 17, 2024 14:22

jstodola reviewed Dec 17, 2024

View reviewed changes

jkonecny12 reviewed Dec 18, 2024

View reviewed changes

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from 01f977f to f160aed Compare December 19, 2024 12:29

rvykydal temporarily deployed to gh-cockpituous December 19, 2024 12:29 — with GitHub Actions Inactive

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from f160aed to 3cbdc2d Compare December 19, 2024 13:50

rvykydal temporarily deployed to gh-cockpituous December 19, 2024 13:50 — with GitHub Actions Inactive

rvykydal requested review from jkonecny12 and jstodola December 19, 2024 13:56

rvykydal marked this pull request as ready for review December 19, 2024 13:56

M4rtinK approved these changes Jan 3, 2025

View reviewed changes

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from 3cbdc2d to c3f1356 Compare January 3, 2025 14:19

rvykydal had a problem deploying to gh-cockpituous January 3, 2025 14:19 — with GitHub Actions Failure

KKoukiou and others added 14 commits January 3, 2025 16:09

security: implement the support to install certificates to Anaconda

bb07209

Kickstart %certificate section is used. Submodule, data structures, parsing. Resolves: INSTALLER-4030 Patch modified by rvykydal.

kickstart: extend section specification for list of section data

2da7985

Unlike the %packages section where the data of all sections are merged into single data object the %certificates section holds the per instance section data in a list. Related: INSTALLER-4030

security: add API: Certificate getter

7354c6f

Resolves: INSTALLER-4030

security: add API to import certificates to Anaconda environment

1e76e03

Related: INSTALLER-4030

security: import certificates early after Anaconda start

9d7ff2a

The certificates imported in initramfs are already imported earlier by a service. Resolves: INSTALLER-4030

security: Add API for installation on target system

ad6d018

Resolved: INSTALLER-4030

security: install certificates on target system

2109179

Resolves: INSTALLER-4030

security: add API to install certificates early before payload

7fcd439

In case they are needed or processed by package scriptlets Resolves: INSTALLER-4030

security: pre-install certificates before payload installation

3fd4ebf

Resolves: INSTALLER-4030

security: log a warning when dumping certificate over an existing file

649c6bb

Resolves: INSTALLER-4030

security: raise exception if certificate destination is unknown

ff2a5ce

Don't require it on parsing level, don't own a default. Resolves: INSTALLER-4030

security: install certificates in pre-install phase only for dnf payload

c29a13d

Resolves: INSTALLER-4030

security: import certificates in initramfs

9646836

Also dump for transfer durig switchroot so that the certificates can be potentially imported early after switchroot by a service. Resolves: INSTALLER-4030

security: add service to transfer certificates from initramfs

ad50f7f

Resolves: INSTALLER-4030

rvykydal force-pushed the edns-certs-stage2-w-initramfs branch from c3f1356 to ad50f7f Compare January 3, 2025 15:09

rvykydal had a problem deploying to gh-cockpituous January 3, 2025 15:09 — with GitHub Actions Failure

		@@ -403,6 +405,44 @@ def ksnet_to_dracut(args, lineno, net, bootdev=False):

		return " ".join(line)


		def _dump_certificate(cert, root="/", dump_dir=None):

edns: implement %certificate kickstart feature #6045

Are you sure you want to change the base?

edns: implement %certificate kickstart feature #6045

Conversation

rvykydal commented Dec 10, 2024 • edited Loading

pep8speaks commented Dec 10, 2024 • edited Loading

Comment last updated at 2025-01-03 15:09:53 UTC

rvykydal commented Dec 10, 2024

github-actions bot commented Dec 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal Dec 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

jkonecny12 Dec 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal Dec 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal commented Dec 13, 2024

rvykydal commented Dec 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkonecny12 commented Dec 19, 2024

M4rtinK left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvykydal commented Dec 10, 2024 •

edited

Loading

pep8speaks commented Dec 10, 2024 •

edited

Loading

rvykydal Dec 13, 2024 •

edited

Loading

rvykydal Dec 12, 2024 •

edited

Loading

jkonecny12 Dec 16, 2024 •

edited

Loading

rvykydal Dec 12, 2024 •

edited

Loading