Skip to content

fix: Create consumer cert & key, when chown failed #3466

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jirihnidek
Copy link
Contributor

  • When it wasn't possible to change group of consumer key.pem, due to missing SELinux rule, then consumer cert.pem was not created. rhsm.service should write only error log message to rhsm.log in this case

@cnsnyder cnsnyder requested review from a team and ptoscano and removed request for a team October 18, 2024 10:37
Copy link

github-actions bot commented Oct 18, 2024

Coverage

Coverage (computed on Fedora latest) •
FileStmtsMissCoverMissing
subscription_manager
   identity.py1557253%31, 57–61, 69–76, 79, 82–83, 86–87, 91, 95, 98, 106–107, 115, 117–121, 123, 125–126, 129, 133–137, 139–140, 143–148, 151–156, 159–161, 164, 187–190, 194–195, 197, 212, 221–223, 238–240, 247
   managerlib.py40611671%61–67, 98–104, 109, 113, 116, 142–147, 149–154, 212–216, 278, 280–284, 400, 408, 431, 465, 566–567, 569–573, 576–577, 581–582, 588–590, 592, 596, 598–601, 623, 625, 658, 691–693, 697–699, 702–703, 706, 729–731, 739–742, 744–747, 759–761, 763, 774, 799, 804–809, 811, 813–816, 822–823, 825, 828, 831–832, 838–839, 849–852
TOTAL17373448374% 

Tests Skipped Failures Errors Time
2413 14 💤 0 ❌ 0 🔥 30.726s ⏱️

Copy link
Contributor

@ptoscano ptoscano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the same checks be added to managerlib.check_identity_cert_perms() as well?

@DuckBoss DuckBoss self-requested a review April 30, 2025 20:42
Copy link
Contributor

@DuckBoss DuckBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jirihnidek I have the same question as Pino.
I see there's cert group permissions being set in check_identity_cert_perms function.

statinfo: os.stat_result = os.stat(cert)
if statinfo[stat.ST_UID] != 0 or statinfo[stat.ST_GID] != cert_guid:
os.chown(cert, 0, cert_guid)
log.warning("Corrected incorrect ownership of %s." % cert)
mode: int = stat.S_IMODE(statinfo[stat.ST_MODE])
if mode != ID_CERT_PERMS:
os.chmod(cert, ID_CERT_PERMS)
log.warning("Corrected incorrect permissions on %s." % cert)

Is this something that needs to also be updated? Otherwise, LGTM.

@jirihnidek jirihnidek force-pushed the jhnidek/rhsm_group_fix branch from f5ded2e to a8acf29 Compare May 6, 2025 09:12
@jirihnidek
Copy link
Contributor Author

@jirihnidek I have the same question as Pino. I see there's cert group permissions being set in check_identity_cert_perms function.

The permission of cert.pem and key.pem have always been set in ConsumerIdentity.write() and check_identity_cert_perms() regardless who is group owner of these files. I do not see any reason, why we should change this behavior now. The group should be able to only read the cert.pem and key.pem and it does not matter who is group owner.

statinfo: os.stat_result = os.stat(cert)
if statinfo[stat.ST_UID] != 0 or statinfo[stat.ST_GID] != cert_guid:
os.chown(cert, 0, cert_guid)
log.warning("Corrected incorrect ownership of %s." % cert)
mode: int = stat.S_IMODE(statinfo[stat.ST_MODE])
if mode != ID_CERT_PERMS:
os.chmod(cert, ID_CERT_PERMS)
log.warning("Corrected incorrect permissions on %s." % cert)

Is this something that needs to also be updated? Otherwise, LGTM.

You are right that we should try to catch exception in managerlib.py. Good catch. Thanks. PR updated.

* When it wasn't possible to change group of consumer `key.pem`,
  due to missing SELinux rule, then consumer `cert.pem` was not
  created. `rhsm.service` should write only error log message
  to `rhsm.log` in this case
* When it is not possible to change ownership of `key.pem` or
  `cert.pem` later using `check_identity_cert_perms()`, then
  write only error message to log file
@jirihnidek jirihnidek force-pushed the jhnidek/rhsm_group_fix branch from a8acf29 to 7d26a91 Compare May 6, 2025 09:25
Copy link
Contributor

@DuckBoss DuckBoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants