Fix and enhance invalid symlink handling when installing certificates to target container

[dkubek]

In response to the identified flaws in the originally delivered fix, for feature enabling http repositories, this commit addresses the following issues:

1. Previously, files installed via RPMs that were originally symlinks were being switched to standard files. This issue has been resolved by preserving symlinks within the /etc/pki directory. Any symlink pointing to a file within the /etc/pki directory (whether present in the source system or installed by a package in the container) will be present in the container, ensuring changes to certificates are properly propagated.

2. Lists of trusted CAs were not being updated, as the update-ca-trust call was missing inside the container. This commit now includes the necessary update-ca-trust call.

The solution specification has been modified as follows:

• Certificate files in /etc/pki (excluding symlinks) are copied to the container as in the original solution.
• Files installed by packages within the container are preserved and given higher priority.
• Handling of symlinks is enhanced, ensuring that symlinks within the /etc/pki directory are preserved, while any symlink pointing outside the /etc/pki directory will be copied as a file.
• Certificates are updated using update-ca-trust.

JIRA: OAMG-6388

Note: Also fixes OAMG-5073

[github-actions]

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

• review please @oamg/developers to notify leapp developers of the review request
• /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp from PR#42, use /packit test oamg/leapp#42

It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported, beaker-minimal and kernel-rt, both can be used to be run on all upgrade paths or just a couple of specific ones.
To launch on-demand tests with packit:

• /packit test --labels kernel-rt to schedule kernel-rt tests set for all upgrade paths
• /packit test --labels beaker-minimal-8.9to9.3,kernel-rt-8.9to9.3 to schedule kernel-rt and beaker-minimal test sets for 8.9->9.3 upgrade path

[Deprecated] To launch on-demand regression testing public members of oamg organization can leave the following comment:

• /rerun to schedule basic regression tests using this pr build and latest upstream leapp build as artifacts
• /rerun 42 to schedule basic regression tests using this pr build and leapp*PR42* as artifacts
• /rerun-sst to schedule sst tests using this pr build and latest upstream leapp build as artifacts
• /rerun-sst 42 to schedule sst tests using this pr build and leapp*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

[abadger]

We need to make sure that this fixes this: #1136 (comment) and that the original issue there doesn't regress with this change. Perhaps a unitest checking that a dangling symlink in /etc/pki is copied over as a dangling symlink to the target container?

[dkubek]

Regarding #1136 , it should not be possible to copy any dangling symlink from the source system now. I have also added a unit test for this situation already so it should be handled

[abadger]

Right. I'm not sure if ignoring dangling symlinks or not breaking when we do so is the correct thing to do. But since pstodulka solved that particular issue by not creating a dangling symlink, it's not something we have to decide on now.

[abadger]

We should probably look into why, right? Is this something that I can replicate using a failing unittest and replacing the run(chroot....) with context.call([update-ca-trust...)?

[pirat89]

I think this is not suitable for unit-tests (in the way that unit-tests should uncover this problem) as we want to omit unit-tests that are actually somehow messing with the system (it would require an installation of a container somewhere, ....). This is something what should be covered by integration tests.

One difference between the run and context.call is, that the run operation executes chroot, in which the command is performed. Whereas the context.call performs systemd-nspawn. Also env & some sys/run/... files could be different. I am curious what is the real reason behind that difference. We should definitely check that. @Jakuje any thought?

[abadger]

What is the difference in what the following code is attempting to achieve and the code in _copy_decouple? It feels like the link detection and possible dereferencing and copying would be the same and the main difference is just that we're looping over different files? The implementations are a bit different so I'm not sure if I'm understanding it, but if the purpose is the same, we should look at making it a helper function that we call from both places.

[dkubek]

Yes I agree that the implementation is quite similar and I have considered to abstract it to to a common function, however there is a subtle difference in checking whether the symlink is broken. When checking a symlink /etc/pki/A -> /etc/pki/B on the source system, it is easy to dereference it to file /etc/pki/B, however consider a symlink installed in the container $target_container/etc/pki/A -> /etc/pki/B - every time we dereference a link we have to manually update the path to point to a file within the the context of the container. Another small difference is that we have made the decision to respect the files installed by RPMs. So whenever they point to a file outside /etc/pki we keep this symlink there, even though when copying certificates from the source system we would replace this link with the file. I am not sure whether this happens or not or makes a significant difference. With these small differences I felt justified in keeping the two procedures separate (even though very similar). However if you still feel differently I will figure something out.

[pirat89]

lgtm - discovered just one small corner case, that is ok to be merged if not fixed - see my comment.

I've also tested the behaviour when TLS configuration is broken after the update-ca-trust execution (can be reproduced when upgrade RHEL 6 -> 7 and use an older releases)

[pirat89]

@dkubek in this case it's not accorate as the srcdir is nowadays /etc/pki instead of /etc/pki/. From this point, if I have symlink to /etc/pki-custom, it will be missleading. From this piont, I suggest to update the call of the function to _copy_decouple('/etc/pki/', target_pki), which will resolve this very corner case. In some other actors, we have this check resolved, but as TBH, I would not go so far in this case to have Py2 & Py3 compatible code (py3 has a nice functions for that in os.path, but py2 doesn't).

it's very corner (I do not expect customers to create own /etc/pki..../ dir) so it's not blocking the merge nor it's required to be applied from my side, but keeping the space for the application if you want to. I want to merge the PR ideally tomorrow (Wed).

[pirat89]

@dkubek let's continue with the merge, and in case of another changes, let's do them in another iteration, so we unblock the testing for CTC1.

@abadger it seems that all required changes have been applied. let us know in case you find something is missing, so it could be addressed in the next iteration.

Thank you guys for the good work!

seems that all problems have been adressed (see my comments above).

[abadger]

+1. Merging is good with me. I had some thoughts on code structure but I don't know if they'll pan out. Since I'm on PTO and then a US-only holiday, I'll see if I can make any of that work for fun and submit a PR for @dkubek to review if so :-)