edns: implement %certificate kickstart feature #6045
Conversation
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
753748f to
97f8ddf
Compare
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
97f8ddf to
e564a96
Compare
rvykydal
had a problem deploying
to
gh-cockpituous
GitHub Actions
Failure
rvykydal
had a problem deploying
to
gh-cockpituous
GitHub Actions
Failure
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
61de0f5 to
e564a96
Compare
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
rvykydal
changed the title
|
/build-image --boot.iso |
|
Images built based on commit e564a96:
Download the images from the bottom of the job status page. |
| [Unit] | ||
| Description=Import of certificates added in initramfs stage of Anaconda via kickstart | ||
| Before=NetworkManager.service | ||
| Before=anaconda.target |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'll create a follow-up issue for this. I think it would be better to handle it separately.
There was a problem hiding this comment.
scripts/handle-sshpw
Outdated
| @@ -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")) | |||
There was a problem hiding this comment.
This is some leftover from my initial stage of implementation. It is actually not needed. I'll remove it.
| @Certificates.setter | ||
| @emits_properties_changed | ||
| def Certificates(self, certificates: List[Structure]): |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
Is there a reason to hide this class like this? Usually we are creating classes as top level.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Maybe also better name would be SectionDataListStrWrapper. And put it at top level in the file?
| if not cert.dir: | ||
| raise SecurityInstallationError( | ||
| "Certificate destination is missing for {}".format(cert.filename) | ||
| ) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
Given that we don't yet have a category support in trust anchor, having simple --dir and --filename is handy.
dracut/parse-kickstart
Outdated
| def process_certificates(handler): | ||
| """Import certificates defined in %certificate sections.""" | ||
| for cert in handler.certificates: | ||
|
|
There was a problem hiding this comment.
I guess an excessive blank line?
There was a problem hiding this comment.
On purpose, feeling more readable to me, but I'll remove it for consistency.
| # to anaconda environment after switchroot. | ||
|
|
||
| # certificates dumped to the specified file are copied to root | ||
| cp -rv /run/install/certificates/path/* / |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Is the
pathat 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.
There was a problem hiding this comment.
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
|
I am going to clean up the structure a bit, squash, rename to --dir and --filename from the beginning, etc. |
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
e564a96 to
ae12329
Compare
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
ae12329 to
3c054f2
Compare
rvykydal
had a problem deploying
to
gh-cockpituous
GitHub Actions
Failure
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
3c054f2 to
01f977f
Compare
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
c3f1356 to
ad50f7f
Compare
rvykydal
had a problem deploying
to
gh-cockpituous
GitHub Actions
Failure
@jkonecny12 Now all the 3 failing tests are due to the pykickstart update not being there (tested locally to work with updated pykickstart). I'd prefer reviewing before the pykickkstart is available so that I can procede with rhel10 ports of PRs. |
Resolves: INSTALLER-4030
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
ad50f7f to
a10649c
Compare
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
|
/kicksart-test --testtype smoke |
|
/kickstart-test --testtype smoke |
|
/build-image --boot.iso |
|
Images built based on commit a10649c:
Download the images from the bottom of the job status page. |
|
/build-image --boot.iso --webui |
|
Images built based on commit a10649c:
Download the images from the bottom of the job status page. |
|
/kickstart-test --skip-testtypes none |
|
Regarding the anaconda-webui test, it is most probably failing due to missing pykickstart. |
The Web UI test issue was addressed in rhinstaller/anaconda-webui#589 |
All tests have passed except for |
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
Related: INSTALLER-4030
rvykydal
force-pushed
the
edns-certs-stage2-w-initramfs
branch
from
6a146fd to
844c561
Compare
rvykydal
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
|
/kickstart-test --testtype smoke |
KKoukiou
temporarily deployed
to
gh-cockpituous
GitHub Actions
Inactive
|
/kickstart-test --testtype smoke |
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 (
--filenameand--diroptions) at various stages: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
--categoryoption: https://github.com/rvykydal/anaconda/commits/edns-certs-stage2-w-initramfs-category/ and pykickstart rvykydal/pykickstart@54b2592TODO: