Implement Candlepin certificate integration#16388
Open
jessicamack wants to merge 12 commits intoansible:develfrom
Open
Implement Candlepin certificate integration#16388jessicamack wants to merge 12 commits intoansible:develfrom
jessicamack wants to merge 12 commits intoansible:develfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Candlepin consumer registration and mTLS certificate lifecycle: a management command, REST client, lifecycle helpers, DB model and migration, analytics API endpoints and views, licensing integration, analytics upload fallback to mTLS, settings, and unit tests. Changes
Sequence DiagramssequenceDiagram
participant CLI as CLI (register)
participant DB as Database
participant CC as CandlepinClient
participant CP as Candlepin Server
CLI->>DB: Check existing cert/key
alt No cert or --force
CLI->>DB: Fetch stored credentials
CLI->>CC: Instantiate with URL/CA/proxy
CC->>CP: POST /consumers?owner={org}
CP-->>CC: JSON {uuid, idCert}
CC-->>CLI: (cert_pem, key_pem, consumer_uuid)
CLI->>CLI: Parse & display cert details
alt Not --dry-run
CLI->>DB: Save cert/key/UUID
end
else Cert exists and no force
CLI->>CLI: Exit (no-op)
end
sequenceDiagram
participant CLI as CLI (renew)
participant DB as Database
participant CC as CandlepinClient
participant CP as Candlepin Server
CLI->>DB: Fetch stored cert/key/UUID
alt UUID missing or placeholder
CLI->>CLI: Fail with error
else Valid UUID
CLI->>CLI: Parse current cert
CLI->>CC: Instantiate client
CC->>CP: PUT /consumers/{uuid} (checkin)
CP-->>CC: 200/204
CLI->>CLI: Evaluate needs_renewal (unless --force)
alt Renewal needed or --force
CLI->>CC: POST /consumers/{uuid} (regenerate)
CC->>CP: POST /consumers/{uuid}
CP-->>CC: JSON {idCert}
CC-->>CLI: (new_cert_pem, new_key_pem)
CLI->>CLI: Display old/new cert details
alt Not --dry-run
CLI->>DB: Save renewed cert/key
end
else No renewal needed
CLI->>CLI: Exit (no changes)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/management/commands/candlepin_manage.py`:
- Around line 79-80: Do not accept the subscription password as a command-line
argument; update the argument handling around reg.add_argument('--password',
...) to prevent exposing the secret in shell history/process listings by
replacing the current --password argv option with a secure input method: either
remove the --password flag and call getpass.getpass() (importing getpass)
wherever the command builds credentials (e.g., in the management command's
argument parsing/handle logic), or add a --password-stdin boolean flag and
implement reading the password from stdin when that flag is set; ensure the code
that previously read args.password now reads the secure source and no longer
registers a plain --password argument.
- Around line 220-226: The manual renew command is ignoring the result of
client.checkin, allowing failures to be masked; update the code around
client.checkin(consumer_uuid, cert_pem, key_pem) in candlepin_manage.py to
capture its return or handle exceptions (e.g., checkin_result =
client.checkin(...)), verify success before proceeding to compute
renewal_needed, and if checkin fails raise a CommandError or return a
non-zero/failure value so the CLI exits with an error instead of printing "No
renewal needed"; ensure any exceptions from client.checkin are caught and
surfaced to the operator rather than being discarded.
In `@awx/main/tests/unit/utils/test_candlepin_licensing.py`:
- Around line 35-36: Replace the mock.patch target from the non-existent module
attribute to the real Django DB connection: change
mock.patch('awx.main.utils.licensing.connection') to
mock.patch('django.db.connection') in the affected tests (e.g.,
test_fetch_candlepin_lifecycle_from_db and the other two tests on the same
file), so the functions that import connection locally will receive the mocked
django.db.connection.
In `@awx/main/utils/licensing.py`:
- Around line 651-658: The current call to _upsert_conf_settings followed by
logger.info treats a failed DB upsert as success; update the callers (the block
using _upsert_conf_settings that logs "Renewed Candlepin cert and key saved to
conf_setting." and the other similar sites around the file) to check the boolean
result and stop/propagate the failure instead of continuing. Specifically, if
_upsert_conf_settings(...) returns False, either raise an exception or return an
error from the surrounding function (or perform compensating action such as
deleting or rolling back the remote Candlepin consumer/credential you just
created via the consumer POST) so that the remote mutation is not left live
while local state remains stale; ensure this behavior is applied to the
occurrences referencing _upsert_conf_settings, the logger.info lines, and their
surrounding registration/renewal flows.
- Around line 593-601: The raw SQL read/write in licensing.py (the blocks using
django.db.connection.cursor() and json.loads/json.dumps) bypasses Setting
encryption hooks and causes SUBSCRIPTIONS_PASSWORD (and SUBSCRIPTIONS_USERNAME)
to be stored/read incorrectly; replace those raw SQL accesses with the Setting
model or awx.conf.settings accessor (e.g., use Setting.get() /
Setting.objects.get()/awx.conf.settings[...] as appropriate) for keys registered
with encrypted=True so values are encrypted on write and decrypted on read,
while keeping raw SQL only for unregistered keys like
CANDLEPIN_CONSUMER_CERT/KEY/UUID; ensure the code paths that currently build
placeholders and call cursor.execute() are updated to use the Setting API for
the subscription keys and that json.dumps/json.loads usage is removed or applied
through the Setting API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6f24b248-43ab-4201-b26d-b81336830a51
📒 Files selected for processing (8)
awx/main/management/commands/candlepin_manage.pyawx/main/tests/unit/utils/test_candlepin_client.pyawx/main/tests/unit/utils/test_candlepin_licensing.pyawx/main/tests/unit/utils/test_candlepin_lifecycle.pyawx/main/utils/candlepin/__init__.pyawx/main/utils/candlepin/client.pyawx/main/utils/candlepin/lifecycle.pyawx/main/utils/licensing.py
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
awx/main/tests/unit/utils/test_candlepin_licensing.py (1)
70-80:⚠️ Potential issue | 🔴 CriticalPatch
django.db.connection, not a missing module attribute.
_fetch_registration_credentials_from_db()importsconnectioninside the function, so@mock.patch('awx.main.utils.licensing.connection')will raise before the test runs. Patchdjango.db.connectioninstead so the local import binds to the mock.Run this to confirm the helper uses a function-local import while the test patches a nonexistent module attribute:
#!/bin/bash set -euo pipefail echo "Helper implementation:" rg -n -C2 "def _fetch_registration_credentials_from_db|from django\\.db import connection" awx/main/utils/licensing.py echo echo "Current test patch target:" rg -n -C1 "@mock\\.patch\\('awx\\.main\\.utils\\.licensing\\.connection'\\)" awx/main/tests/unit/utils/test_candlepin_licensing.py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/unit/utils/test_candlepin_licensing.py` around lines 70 - 80, The test is patching awx.main.utils.licensing.connection but _fetch_registration_credentials_from_db imports connection locally, so the patch never applies; update the test decorator to patch django.db.connection instead (change the `@mock.patch` target used on the test_fetch_registration_credentials_from_db function) so the local import inside _fetch_registration_credentials_from_db binds to the mock_connection used in the test.awx/main/management/commands/candlepin_manage.py (2)
81-82:⚠️ Potential issue | 🟠 MajorDon't accept the subscription password on argv.
Line 82 still exposes the RHSM secret via shell history and process listings. Please switch this to an interactive prompt or a
--password-stdinstyle flow instead of a plain--passwordoption.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/management/commands/candlepin_manage.py` around lines 81 - 82, The command currently adds a plain-text password flag via reg.add_argument('--password') which exposes RHSM secrets; change the implementation in the candlepin_manage management command to remove the '--password' CLI option and instead support a secure input flow: accept either a '--password-stdin' flag that reads the password from STDIN or, when no password provided and a terminal is attached, prompt interactively (e.g., using getpass.getpass) before using the value; update any code paths that reference the parsed 'password' argument accordingly and preserve the existing '--username' behavior.
224-225:⚠️ Potential issue | 🟠 MajorFail the manual
renewcommand when check-in fails.
CandlepinClient.checkin()returnsFalseon network/API failure, but Line 225 discards the result and can still fall through to "No renewal needed" with exit code 0. This should surface as command failure instead of silently succeeding.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/management/commands/candlepin_manage.py` around lines 224 - 225, The checkin result from CandlepinClient.checkin is ignored; change the call in the command's handle method so you capture its return value (e.g. result = client.checkin(consumer_uuid, cert_pem, key_pem)) and if result is False treat it as a failure: write an error to stderr and fail the management command (raise django.core.management.base.CommandError or exit non‑zero) instead of continuing to the "No renewal needed" path. Ensure you keep the existing self.stdout.write message and reference consumer_uuid/cert_pem/key_pem when logging.
🧹 Nitpick comments (1)
awx/main/analytics/core.py (1)
414-414: Raisingrequests.RequestExceptionwith a string message is unconventional.
requests.RequestExceptiontypically wraps actual request errors. Using it as a control flow mechanism with a custom message string may cause confusion. Consider using a different approach such as a simple flag variable or a custom exception.♻️ Alternative using a flag variable
+ use_basic_auth = False + if cert_path and key_path: logger.debug("Using certificate-based authentication for analytics upload") try: response = s.post(url, files=files, cert=(cert_path, key_path), verify=settings.INSIGHTS_CERT_PATH, headers=s.headers, timeout=(31, 31)) except requests.RequestException as e: logger.warning(f"Certificate-based authentication failed: {e}, falling back to basic auth") - raise + use_basic_auth = True else: # No certificate available, skip to basic auth logger.debug("No Candlepin certificate available, falling back to basic authentication") - raise requests.RequestException("No certificate available") + use_basic_auth = True - except requests.RequestException: + if use_basic_auth: # Second attempt: Basic authentication try:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/analytics/core.py` at line 414, The code currently raises requests.RequestException with a custom message via the statement `raise requests.RequestException("No certificate available")`; replace this unconventional use with either a simple control-flow flag/return or a dedicated exception type: define a local custom exception (e.g., NoCertificateError) or use a built-in like ValueError/RuntimeError, then raise that instead with contextual details, or refactor the surrounding function to return a boolean/None to indicate the missing certificate; update any callers that expect the old RequestException to handle the new return/exception accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/api/views/analytics.py`:
- Line 414: The view references AWX_ANALYTICS_CERTIFICATE_AUTH_ENABLED via
getattr(settings, ...); add an explicit default constant named
AWX_ANALYTICS_CERTIFICATE_AUTH_ENABLED = True in the project's defaults settings
module (next to the other Candlepin/analytics settings) with a short comment
like "Enable certificate-based authentication for analytics" so the setting is
documented and consistent with other defaults; no changes needed to the view
code.
- Line 417: The has_redhat_credentials flag only checks REDHAT_USERNAME or
SUBSCRIPTIONS_USERNAME; update the expression that sets has_redhat_credentials
to also return True when both service account settings are present by checking
getattr(settings, 'SUBSCRIPTIONS_CLIENT_ID', None) and getattr(settings,
'SUBSCRIPTIONS_CLIENT_SECRET', None) (i.e., OR the existing basic-auth check
with a check that both client id and client secret exist). Locate the assignment
for has_redhat_credentials in analytics.py and modify it to include this
additional service-account check.
In `@awx/main/analytics/core.py`:
- Around line 406-410: The exception handler currently sets cert_path and
key_path to None before re-raising (in the except requests.RequestException as e
block), which prevents the finally cleanup from deleting the temporary files;
revert that change so the original cert_path and key_path remain intact for
cleanup: either remove the assignment cert_path, key_path = None from the except
block or capture original paths into temp variables (e.g., orig_cert_path,
orig_key_path) before trying mTLS and use those for cleanup in the finally,
ensuring the exception is still re-raised without losing the file path
references used by the finally block.
In `@awx/main/management/commands/candlepin_manage.py`:
- Around line 178-183: The parse_cert() call used solely for display may raise
ValueError and currently aborts the command before the new cert/consumer is
persisted; change the flow in the management command (candlepin_manage.Command
methods where parse_cert is called) to persist/save the certificate/consumer
first and then attempt to parse for operator output, wrapping parse_cert(...) in
a try/except ValueError so parsing failures are best-effort: on success print
the existing formatted lines (consumer_uuid, info["serial"], info["cn"],
info["not_after"], info["days_remaining"]); on failure still report "Registered
successfully" and consumer_uuid and emit a concise fallback message like
"Certificate metadata unavailable: <error message>" to stdout/stderr. Apply the
same try/except pattern to the other block around the parse_cert call referenced
at lines 240-244.
- Around line 188-189: The CLI prints "saved to database" unconditionally
despite `_save_candlepin_registration_to_db` and `_save_candlepin_cert_to_db`
swallowing errors; change the flow so the command only reports success when a
save actually succeeded: modify the two utility functions to either raise on
failure or return a boolean/explicit status (e.g., True/False or a saved
object), then update the management command calls in `candlepin_manage.py` (the
places that call `_save_candlepin_registration_to_db(cert_pem, key_pem,
consumer_uuid)` and the `_save_candlepin_cert_to_db(...)` call around lines with
the stdout.write success messages) to check the returned status or catch
exceptions and only write the "saved to database" message on success, otherwise
propagate or print a clear failure status. Ensure you update both call sites.
In `@awx/main/models/candlepin.py`:
- Around line 31-35: The model currently relies on consumer_uuid.unique which
doesn't prevent multiple rows; change the model to enforce a DB-level singleton
(e.g., add a fixed primary key or a singleton_guard field with unique=True) and
update the singleton accessors to use that key explicitly. Concretely, add a
non-changeable singleton field (for example an Integer/PositiveSmallInteger
primary key with default=1 and editable=False or a Boolean/Char field defaulting
to a constant with unique=True) in the CandlepinCertificate model, and modify
get_instance() and get_or_create_instance() to call get()/get_or_create() using
that primary key or singleton field (e.g., pk=1 or singleton_guard=True) so the
DB will only ever allow one row and the accessors always target that single
record; apply the same change pattern to the other model code referenced around
lines 65-87.
- Around line 23-47: regenerate_secret_key.Command.handle is missing support for
re-encrypting CandlepinCertificate.PASSWORD_FIELDS so existing cert_pem/key_pem
remain encrypted with the old SECRET_KEY; add a private method
_candlepin_certificates() to the Command class following the same pattern used
for _unified_job and _credential (iterate CandlepinCertificate objects, decrypt
then re-encrypt each field in CandlepinCertificate.PASSWORD_FIELDS and save),
and call this method from handle() alongside the UnifiedJob and Credential calls
to ensure cert_pem and key_pem are rotated.
In `@awx/main/tests/unit/utils/test_candlepin_licensing.py`:
- Around line 96-109: The test stubs for parse_cert are returning 'expires_at'
but the code under test reads 'not_after', so update the mocks in
test_candlepin_licensing.py (the parse_cert return value used by tests that call
_save_candlepin_cert_to_db and _save_candlepin_registration_to_db) to include a
'not_after' key with the ISO timestamp (or include both 'not_after' and
'expires_at' to be explicit), ensuring the expiry parsing path is exercised and
the assertions that 'expires_at' (or parsed expiry) is persisted are meaningful.
In `@awx/main/utils/candlepin/lifecycle.py`:
- Around line 81-88: The renewal threshold is off-by-one in needs_renewal:
change the comparison so certificates with exactly days_before_expiry remaining
are considered needing renewal; update the return in needs_renewal (which calls
parse_cert) to use <= instead of < so it matches the docstring and the existing
logic in get_certificate_info.
---
Duplicate comments:
In `@awx/main/management/commands/candlepin_manage.py`:
- Around line 81-82: The command currently adds a plain-text password flag via
reg.add_argument('--password') which exposes RHSM secrets; change the
implementation in the candlepin_manage management command to remove the
'--password' CLI option and instead support a secure input flow: accept either a
'--password-stdin' flag that reads the password from STDIN or, when no password
provided and a terminal is attached, prompt interactively (e.g., using
getpass.getpass) before using the value; update any code paths that reference
the parsed 'password' argument accordingly and preserve the existing
'--username' behavior.
- Around line 224-225: The checkin result from CandlepinClient.checkin is
ignored; change the call in the command's handle method so you capture its
return value (e.g. result = client.checkin(consumer_uuid, cert_pem, key_pem))
and if result is False treat it as a failure: write an error to stderr and fail
the management command (raise django.core.management.base.CommandError or exit
non‑zero) instead of continuing to the "No renewal needed" path. Ensure you keep
the existing self.stdout.write message and reference
consumer_uuid/cert_pem/key_pem when logging.
In `@awx/main/tests/unit/utils/test_candlepin_licensing.py`:
- Around line 70-80: The test is patching awx.main.utils.licensing.connection
but _fetch_registration_credentials_from_db imports connection locally, so the
patch never applies; update the test decorator to patch django.db.connection
instead (change the `@mock.patch` target used on the
test_fetch_registration_credentials_from_db function) so the local import inside
_fetch_registration_credentials_from_db binds to the mock_connection used in the
test.
---
Nitpick comments:
In `@awx/main/analytics/core.py`:
- Line 414: The code currently raises requests.RequestException with a custom
message via the statement `raise requests.RequestException("No certificate
available")`; replace this unconventional use with either a simple control-flow
flag/return or a dedicated exception type: define a local custom exception
(e.g., NoCertificateError) or use a built-in like ValueError/RuntimeError, then
raise that instead with contextual details, or refactor the surrounding function
to return a boolean/None to indicate the missing certificate; update any callers
that expect the old RequestException to handle the new return/exception
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d03e6907-713c-41fe-8f95-6c06fc79bb03
📒 Files selected for processing (15)
awx/api/urls/analytics.pyawx/api/views/analytics.pyawx/main/analytics/core.pyawx/main/management/commands/candlepin_manage.pyawx/main/migrations/0206_candlepin_certificate.pyawx/main/models/__init__.pyawx/main/models/candlepin.pyawx/main/tests/unit/utils/test_candlepin_client.pyawx/main/tests/unit/utils/test_candlepin_licensing.pyawx/main/tests/unit/utils/test_candlepin_lifecycle.pyawx/main/utils/candlepin/__init__.pyawx/main/utils/candlepin/client.pyawx/main/utils/candlepin/lifecycle.pyawx/main/utils/licensing.pyawx/settings/defaults.py
✅ Files skipped from review due to trivial changes (1)
- awx/main/utils/candlepin/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- awx/main/utils/licensing.py
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
awx/main/tests/unit/utils/test_candlepin_licensing.py (1)
94-97:⚠️ Potential issue | 🟠 MajorMock return value uses incorrect key — expiry persistence is untested.
Per a prior review, the implementation reads
parse_cert(...)[\"not_after\"]to extract the expiry date, but these mocks returnexpires_atinstead. The tests pass but do not verify that expiry is correctly persisted.Proposed fix
mock_parse_cert.return_value = { 'serial': '123456', - 'expires_at': '2027-01-01T00:00:00+00:00', + 'not_after': '2027-01-01T00:00:00+00:00', }And similarly for line 116-119:
mock_parse_cert.return_value = { 'serial': '789012', - 'expires_at': '2027-01-01T00:00:00+00:00', + 'not_after': '2027-01-01T00:00:00+00:00', }Also applies to: 116-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/tests/unit/utils/test_candlepin_licensing.py` around lines 94 - 97, The unit tests mock parse_cert to return {'serial': '123456', 'expires_at': ...} but the code under test reads parse_cert(... )["not_after"], so update the mock return values in test_candlepin_licensing (the mock named mock_parse_cert) to use the 'not_after' key with the same timestamp string (and replicate the same change for the second mock instance), ensuring the expiry field is correctly exercised and persisted by the code paths that read parse_cert()["not_after"].
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/tests/unit/utils/test_candlepin_lifecycle.py`:
- Around line 32-39: The test assumes get_candlepin_url() returns the production
URL by default but the implementation of get_candlepin_url() (function
get_candlepin_url) returns None when the AWX_ANALYTICS_CANDLEPIN_URL setting is
absent; update the test in test_candlepin_lifecycle.py to assert None for the
default case (or alternatively set mock_settings.AWX_ANALYTICS_CANDLEPIN_URL to
the production URL before calling get_candlepin_url()), referencing
get_candlepin_url to locate the behavior to match.
---
Duplicate comments:
In `@awx/main/tests/unit/utils/test_candlepin_licensing.py`:
- Around line 94-97: The unit tests mock parse_cert to return {'serial':
'123456', 'expires_at': ...} but the code under test reads parse_cert(...
)["not_after"], so update the mock return values in test_candlepin_licensing
(the mock named mock_parse_cert) to use the 'not_after' key with the same
timestamp string (and replicate the same change for the second mock instance),
ensuring the expiry field is correctly exercised and persisted by the code paths
that read parse_cert()["not_after"].
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: c9bf36f6-58e7-43d1-8924-6ea9ededc437
📒 Files selected for processing (3)
awx/main/tests/unit/utils/test_candlepin_client.pyawx/main/tests/unit/utils/test_candlepin_licensing.pyawx/main/tests/unit/utils/test_candlepin_lifecycle.py
jessicamack
force-pushed
the
candlepin-integration
branch
from
d6f5287 to
4258d86
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/analytics/core.py`:
- Around line 421-425: In the basic-auth fallback except block inside ship(),
wrap the OIDCClient(rh_id, rh_secret).make_request call in its own try/except so
any exceptions from client.make_request are caught; on exception log the failure
(including exception details) and ensure the code sets response to a falsy value
(or returns False) so ship() does not raise but returns the failure result to
the gather path. Reference OIDCClient, client.make_request, and the existing
except requests.RequestException as e block when adding the inner try/except and
logging.
- Around line 394-425: The multipart upload reuses the same open file handle in
the files payload so retries send an empty stream; before each alternative POST
(the certificate-based attempt in the cert_path branch, the basic-auth s.post
call, and the OIDCClient.make_request call) rewind or recreate the files
payload: for any open file objects referenced in files (used by the s.post calls
and OIDCClient.make_request), call .seek(0) on each file descriptor or rebuild
the files tuple/list from fresh open file handles so the multipart encoder
re-reads the archive; update the blocks around
get_or_generate_candlepin_certificate, the basic auth s.post, and the
OIDCClient(...) call to reset the files stream before each request.
In `@awx/main/utils/licensing.py`:
- Around line 592-597: The code returns encrypted blobs for
CandlepinCertificate.cert_pem and key_pem; instead, call decrypt_field on both
before returning so callers get raw PEM bytes/strings: fetch instance via
CandlepinCertificate.get_instance(), validate as before, then replace returning
instance.cert_pem and instance.key_pem with decrypt_field(instance, "cert_pem")
and decrypt_field(instance, "key_pem") (keep returning instance.consumer_uuid
unchanged); also add the import for decrypt_field from awx.main.utils.encryption
near the other imports.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f2eb09d-8483-4e16-8c18-151c8fed5522
📒 Files selected for processing (15)
awx/api/urls/analytics.pyawx/api/views/analytics.pyawx/main/analytics/core.pyawx/main/management/commands/candlepin_manage.pyawx/main/migrations/0206_candlepin_certificate.pyawx/main/models/__init__.pyawx/main/models/candlepin.pyawx/main/tests/unit/utils/test_candlepin_client.pyawx/main/tests/unit/utils/test_candlepin_licensing.pyawx/main/tests/unit/utils/test_candlepin_lifecycle.pyawx/main/utils/candlepin/__init__.pyawx/main/utils/candlepin/client.pyawx/main/utils/candlepin/lifecycle.pyawx/main/utils/licensing.pyawx/settings/defaults.py
✅ Files skipped from review due to trivial changes (4)
- awx/main/models/init.py
- awx/api/urls/analytics.py
- awx/main/utils/candlepin/init.py
- awx/main/tests/unit/utils/test_candlepin_lifecycle.py
|
tvo318
reviewed
Apr 16, 2026
Member
tvo318
left a comment
tvo318
left a comment
There was a problem hiding this comment.
Code Review
This PR adds Candlepin mTLS certificate integration for analytics authentication — a significant feature with solid test coverage and clean module organization. Found one HIGH issue (temporary certificate file leak on mTLS failure in core.py), two MEDIUM issues (misleading docstring about TLS verification defaults, race condition in singleton creation), and one LOW style nit. The temp file leak is the most important to fix before merge as it can leave private key material on disk.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
awx/main/management/commands/candlepin_manage.py (2)
220-249:⚠️ Potential issue | 🟠 MajorKeep renew robust when current cert parsing fails.
After a
parse_cert()failure,infois undefined, but later paths still use it for the no-renewal message and old serial output. Also,needs_renewal()can raise the sameValueErroroutside the earlier best-effort block.Proposed fix
+ info = None try: info = parse_cert(cert_pem) self.stdout.write('Current certificate:') @@ - renewal_needed = force or needs_renewal(cert_pem, renewal_days) + try: + renewal_needed = force or needs_renewal(cert_pem, renewal_days) + except ValueError as e: + self.stderr.write(f'Could not determine renewal status from stored certificate: {e}') + return False if not renewal_needed: - self.stdout.write(f'Certificate has {info["days_remaining"]} days remaining (renewal threshold: {renewal_days} days). No renewal needed.') + self.stdout.write(f'Certificate has {info["days_remaining"]} days remaining (renewal threshold: {renewal_days} days). No renewal needed.') return True @@ try: new_info = parse_cert(new_cert_pem) - self.stdout.write(f' Old serial : {info["serial"]}') + self.stdout.write(f' Old serial : {info["serial"] if info else "unknown"}') self.stdout.write(f' New serial : {new_info["serial"]}') self.stdout.write(f' Valid until : {new_info["not_after"]} ({new_info["days_remaining"]} days remaining)')Also applies to: 272-279
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/management/commands/candlepin_manage.py` around lines 220 - 249, parse_cert() failures leave info undefined and later code (the "no renewal needed" message and the old-serial output) and the needs_renewal() call assume info exists or can re-raise; fix by initializing info to a safe default dict (e.g., keys "serial", "cn", "not_after", "days_remaining" with fallback values like "unknown" or 0) before the try/except around parse_cert(), and change the needs_renewal(cert_pem, renewal_days) call to be wrapped in a try/except ValueError that treats parsing errors as "renewal needed" (or logs and forces renewal when --force is not set); update any uses of info (e.g., the no-renewal message and the earlier old serial output) to reference the safe defaults so they never reference an undefined variable; apply the same pattern to the other occurrence that uses parse_cert/needs_renewal (the later block around consumer_uuid/CandlepinClient.checkin).
50-52:⚠️ Potential issue | 🟠 MajorDon’t accept the subscription password on argv.
--passwordexposes the Red Hat subscription secret through shell history and process listings. Use stdin or an interactive prompt instead.Proposed fix using stdin
- ' LICENSE.account_number). Pass --username / --password / ', + ' LICENSE.account_number). Pass --username / --password-stdin /', ' --org to override.', @@ - reg.add_argument('--password', help='Red Hat subscription password (overrides REDHAT_PASSWORD from database)') + reg.add_argument('--password-stdin', dest='password_stdin', action='store_true', help='Read Red Hat subscription password from stdin') @@ - password = options.get('password') or db_password + password = sys.stdin.readline().rstrip('\n') if options.get('password_stdin') else db_password @@ - missing.append('password (pass --password or set REDHAT_PASSWORD in database)') + missing.append('password (pass --password-stdin or set REDHAT_PASSWORD in database)')Also applies to: 81-83, 131-139
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/management/commands/candlepin_manage.py` around lines 50 - 52, The command currently accepts the subscription secret via the --password CLI option (and reads options['password']) which leaks secrets in process listings; remove or stop using the --password argv value and instead obtain the password via a secure prompt or stdin: stop adding/using the --password argument (or treat it as deprecated) and, where the code references options.get('password') / args.password (e.g., in the candlepin management command handler and any helper methods that consume password), replace that usage with a secure read: if stdin is not a tty read the password bytes from sys.stdin (strip newline), otherwise call getpass.getpass(prompt) to interactively prompt; apply this change consistently for all occurrences that currently accept --password (the blocks around the current --password handling and subsequent uses).
🧹 Nitpick comments (2)
awx/main/utils/candlepin/client.py (1)
212-230: Minor: redundantchmod(tempfile.mkstemp already creates the file with mode 0600).
tempfile.mkstemp()on POSIX already creates the file with permissions0o600owned by the current user, so the explicitos.chmod(path, 0o600)is a no-op. Leaving it is harmless, but the ordering (chmod before write) is slightly misleading. Also note theos.close(fd)in theexceptpath can double-close if thewith os.fdopen(fd, 'w')context opened successfully and thenwrite()raised — thewithalready closedfd— but theOSErroris caught so no functional bug. Optional cleanup only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/utils/candlepin/client.py` around lines 212 - 230, The _write_temp_pem function has a redundant os.chmod call and a potential double-close of the raw fd: remove the os.chmod(path, 0o600) (tempfile.mkstemp already sets 0o600) and ensure the raw fd is not closed twice by setting fd = None as soon as os.fdopen(fd, 'w') successfully takes ownership (or otherwise only calling os.close(fd) in the except branch when the fd still refers to an open descriptor). Update the code in _write_temp_pem to eliminate os.chmod and adjust the exception handling around os.fdopen / os.close / os.unlink so you only close the raw fd if it was never wrapped by os.fdopen.awx/main/analytics/core.py (1)
35-67: Duplicate temp-cert helper — consolidate withCandlepinClient._temp_cert_files.
awx/main/utils/candlepin/client.pyalready defines an equivalent_temp_cert_filescontext manager with the same contract (write PEM cert+key to secure temp files,0o600on the key, cleanup on exit). Duplicating it here doubles the surface for future cert-handling changes (permissions, error handling, disk cleanup on crash). Consider importing the existing helper or extracting a single shared utility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@awx/main/analytics/core.py` around lines 35 - 67, The file defines a duplicate _temp_cert_files context manager; replace this local duplicate by reusing the existing helper from CandlepinClient._temp_cert_files (or extract a shared utility) so there is a single implementation to maintain permissions, writing and cleanup behavior; update imports to import the helper from awx.main.utils.candlepin.client (or move both implementations into a new shared module and import it from both places), then remove the local _temp_cert_files definition in awx/main/analytics/core.py and ensure callers now use the imported/shared _temp_cert_files.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@awx/main/analytics/core.py`:
- Around line 429-460: When the certificate-based POST via s.post in the block
that uses get_or_generate_candlepin_certificate and _temp_cert_files returns a
non-success HTTP status, add a logger.warning that includes the
response.status_code and response.text so cert-present-but-rejected cases are
visible (i.e., after the if response.status_code < 300: return True add an else:
logger.warning(...)). Do the same for the basic-auth s.post call (the block that
calls s.post with auth=(rh_id, rh_secret)): if response.status_code >= 300 log a
warning with status and body before falling through to OIDC; keep existing
exception handlers unchanged.
In `@awx/main/management/commands/candlepin_manage.py`:
- Around line 90-104: The --dry-run flag currently still performs non-idempotent
Candlepin actions; update the command handlers so the dry-run check happens
before any Candlepin write operations (specifically ensure you exit early when
options.dry_run is true prior to calling register_consumer() in the register
flow and prior to calling regenerate_cert() (or whatever function rotates the
server cert) in the renew flow), or alternatively rename the flag to --no-save
and document it as unsafe; make the change in the command logic that processes
the 'register' and 'renew' subparsers so no external Candlepin writes occur when
the flag is set.
In `@awx/main/utils/candlepin/lifecycle.py`:
- Around line 225-246: get_certificate_info currently only considers expiry and
can label future-not_before certificates as "valid"; update the logic (after
parse_cert and before expiry checks in get_certificate_info) to detect
certificates that are not yet valid (use cert_info['not_before'] or a
days-until-valid value from parse_cert, or call is_cert_valid(cert_pem) to reuse
existing validation) and mark them unhealthy: set info['status'] =
'not_yet_valid' (or 'invalid'), info['needs_renewal'] = True and provide an
explanatory info['message'] like "Certificate not valid until <not_before>".
Ensure this check runs prior to the existing days_until_expiry /
renewal_threshold branch so future-dated certs are not reported as 'valid'.
In `@awx/main/utils/licensing.py`:
- Around line 778-820: get_or_generate_candlepin_certificate accepts
username/password but never uses them; wire them through to registration so
caller-supplied credentials are honored. Change the call to
_register_candlepin_consumer(username, password) in
get_or_generate_candlepin_certificate, update _register_candlepin_consumer to
accept optional username/password parameters and pass them into
client.register_consumer(username, password, org, install_uuid) (falling back to
the existing settings/db lookup when None), and update docstring/usage to
reflect that caller credentials will be used for Candlepin POST /consumers.
---
Duplicate comments:
In `@awx/main/management/commands/candlepin_manage.py`:
- Around line 220-249: parse_cert() failures leave info undefined and later code
(the "no renewal needed" message and the old-serial output) and the
needs_renewal() call assume info exists or can re-raise; fix by initializing
info to a safe default dict (e.g., keys "serial", "cn", "not_after",
"days_remaining" with fallback values like "unknown" or 0) before the try/except
around parse_cert(), and change the needs_renewal(cert_pem, renewal_days) call
to be wrapped in a try/except ValueError that treats parsing errors as "renewal
needed" (or logs and forces renewal when --force is not set); update any uses of
info (e.g., the no-renewal message and the earlier old serial output) to
reference the safe defaults so they never reference an undefined variable; apply
the same pattern to the other occurrence that uses parse_cert/needs_renewal (the
later block around consumer_uuid/CandlepinClient.checkin).
- Around line 50-52: The command currently accepts the subscription secret via
the --password CLI option (and reads options['password']) which leaks secrets in
process listings; remove or stop using the --password argv value and instead
obtain the password via a secure prompt or stdin: stop adding/using the
--password argument (or treat it as deprecated) and, where the code references
options.get('password') / args.password (e.g., in the candlepin management
command handler and any helper methods that consume password), replace that
usage with a secure read: if stdin is not a tty read the password bytes from
sys.stdin (strip newline), otherwise call getpass.getpass(prompt) to
interactively prompt; apply this change consistently for all occurrences that
currently accept --password (the blocks around the current --password handling
and subsequent uses).
---
Nitpick comments:
In `@awx/main/analytics/core.py`:
- Around line 35-67: The file defines a duplicate _temp_cert_files context
manager; replace this local duplicate by reusing the existing helper from
CandlepinClient._temp_cert_files (or extract a shared utility) so there is a
single implementation to maintain permissions, writing and cleanup behavior;
update imports to import the helper from awx.main.utils.candlepin.client (or
move both implementations into a new shared module and import it from both
places), then remove the local _temp_cert_files definition in
awx/main/analytics/core.py and ensure callers now use the imported/shared
_temp_cert_files.
In `@awx/main/utils/candlepin/client.py`:
- Around line 212-230: The _write_temp_pem function has a redundant os.chmod
call and a potential double-close of the raw fd: remove the os.chmod(path,
0o600) (tempfile.mkstemp already sets 0o600) and ensure the raw fd is not closed
twice by setting fd = None as soon as os.fdopen(fd, 'w') successfully takes
ownership (or otherwise only calling os.close(fd) in the except branch when the
fd still refers to an open descriptor). Update the code in _write_temp_pem to
eliminate os.chmod and adjust the exception handling around os.fdopen / os.close
/ os.unlink so you only close the raw fd if it was never wrapped by os.fdopen.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 398f6649-1d6e-4b78-ab28-6ba16a5e2037
📒 Files selected for processing (12)
awx/api/views/analytics.pyawx/main/analytics/core.pyawx/main/management/commands/candlepin_manage.pyawx/main/migrations/0206_candlepin_certificate.pyawx/main/models/candlepin.pyawx/main/tests/unit/utils/test_candlepin_client.pyawx/main/tests/unit/utils/test_candlepin_licensing.pyawx/main/tests/unit/utils/test_candlepin_lifecycle.pyawx/main/utils/candlepin/client.pyawx/main/utils/candlepin/lifecycle.pyawx/main/utils/licensing.pyawx/settings/defaults.py
✅ Files skipped from review due to trivial changes (2)
- awx/settings/defaults.py
- awx/main/tests/unit/utils/test_candlepin_lifecycle.py
fosterseth
reviewed
Apr 21, 2026
fosterseth
reviewed
Apr 22, 2026
fosterseth
reviewed
Apr 22, 2026
fosterseth
reviewed
Apr 22, 2026
prat98
reviewed
Apr 23, 2026
8 tasks
fosterseth
reviewed
Apr 27, 2026
| ) | ||
|
|
||
| register( | ||
| 'CANDLEPIN_EXPIRES_AT', |
Member
There was a problem hiding this comment.
does this need to be its own setting or can this trivially be obtained by parsing the certificate?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SUMMARY
Integrate with Candlepin to automatically obtain and maintain an identity certificate for analytics authentication when customers use basic auth (username/password) during manifest renewal. This PR is meant to bring the work from the metrics-utility into AWX.
ISSUE TYPE
COMPONENT NAME
STEPS TO REPRODUCE AND EXTRA INFO
Summary by CodeRabbit
New Features
Tests
Settings