-
Notifications
You must be signed in to change notification settings - Fork 163
Update CA bundle path in krb5 config (el9to10) #1340
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
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.
Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build.
Note that first time contributors cannot run tests automatically - they need to be started by a reviewer. It is possible to schedule specific on-demand tests as well. Currently 2 test sets are supported,
See other labels for particular jobs defined in the 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. |
6512868 to
b3f4974
Compare
|
@jrisc Hi \o thank you for the contribution. I am aware that for first contributors tests are not performed automatically and it could you to wait for our approval always. You can execute tests locally in container using Makefile from the main dir: TEST_CONTAINER=rhel9 make test_containersee |
pirat89
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good in general. I found just one thing which is weird to me and based on the outcome of the discussion there could be another things to change or not.
repos/system_upgrade/el9toel10/actors/krb5conf/convertkrb5conf/actor.py
Outdated
Show resolved
Hide resolved
repos/system_upgrade/el9toel10/actors/krb5conf/convertkrb5conf/libraries/convertpamuserdb.py
Outdated
Show resolved
Hide resolved
| with open(conf_file) as f: | ||
| text = f.read().replace('/etc/ssl/certs/ca-certificates.crt', | ||
| '/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem') | ||
| with open(conf_file, 'w') as f: | ||
| f.write(text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we are sure that all files wil be always present and rw operation will allowed, it's ok. otherwise we should cover it by try-except:
try:
...
except IOError as e:
# log error
return # give it a chance to fix other files still
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the file list is supposed to be collected just before by the scankrb5conf actor, I think we can expect the files to still be there at this point.
I just realize that I forgot to rename this source file though... 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed the file in 724f6be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is Leapp running as root? If that's the case, I don't expect it to face any file permissions problems. However, if it is not and one of the krb5 config file in /etc/krb5.conf.d/ is not publicly readable (which makes no sense, but is technically possible), it will probably cause the scankrb5conf actor to fail. Do you think I should just log a warning and skip the erroneous file in this case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right. yes, it is running as root always and as we could read it before, we should be able to read it later as well - DAC is ignored by root, selinux will be always in disabled or permissive mode, and I do not expect someone would set an ACL on these files (like making them immutable). So only thing here would be if a file is removed or moved/renamed during the RPM transaction.
repos/system_upgrade/el9toel10/actors/krb5conf/convertkrb5conf/libraries/convertkrb5conf.py
Outdated
Show resolved
Hide resolved
|
@pirat89 It seems that failing tests are cause by the absence of RPM database. Is there a way to handle this case, or it just cannot be tested? |
In RHEL10, the historical /etc/ssl/certs/ca-certificates.crt CA bundle file was replaced by /etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem. The CA bundle file might be used as "pkinit_anchors" in MIT krb5 configuration files. The krb5conf actors search for occurrences of the old CA bundle path, and replace them with the new one. jira: RHEL-65265 Co-authored-by: Petr Stodůlka <[email protected]>
MichalHe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The solution looks code-wise very good, thank you. I left some minor comments intended to ease with maintenance of the code.
| if msg.unmanaged_files: | ||
| reporting.create_report([ | ||
| reporting.Title('Unmanaged MIT krb5 configuration file(s) will be ' | ||
| 'updated to point the new X.509 CA bundle file'), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| 'updated to point the new X.509 CA bundle file'), | |
| 'updated to point to the new X.509 CA bundle file'), |
| 'file was modified. The following unmanaged MIT krb5 ' | ||
| 'configuration files have to be updated to point to the new ' | ||
| 'bundle file:' | ||
| '{sep}{files}'.format(sep=FMT_LIST_SEPARATOR, files=FMT_LIST_SEPARATOR.join(msg.unmanaged_files))), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a function that takes a list of entries (e.g. unmanaged files) and outputs the list formatted in human readable form? I see that the same pattern is used in both created reports. The function could improve readability as I had to think a bit about what the output would look like.
|
|
||
| class ConvertKrb5conf(Actor): | ||
| """ | ||
| Update krb5 configuration file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add more details what is the nature of the update?
repos/system_upgrade/el9toel10/actors/krb5conf/convertkrb5conf/libraries/convertkrb5conf.py
Show resolved
Hide resolved
|
|
||
| def fetch_outdated_krb5_conf_files(conf_paths, ca_bundle_path='/etc/ssl/certs/ca-certificates.crt'): | ||
| krb5_conf_files = set() | ||
| odtd_rpm_conf = set() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, what does odtd means?
| else: | ||
| try: | ||
| rpm_nvr = run(['/usr/bin/rpm', '-qf', file_path], split=True)['stdout'] | ||
| # We do not potential files provided by Red Hat RPMs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment is a bit malformed. Maybe something like this?
| # We do not potential files provided by Red Hat RPMs | |
| # We only want to handle files signed by the distribution, because we have no guarantees about third party packages |
In RHEL10, the historical
/etc/ssl/certs/ca-certificates.crtCA bundle file was replaced by/etc/pki/ca-trust/extracted/pem/tls-ca-bundle.pem. The CA bundle file might be used aspkinit_anchorsin MIT krb5 configuration files.The
krb5confactors search for occurrences of the old CA bundle path, and replace them with the new one.jira: RHEL-65265