AAP-68113 [PENDING] Candle pin v2#363
Open
cshiels-ie wants to merge 22 commits intoansible:develfrom
Open
Conversation
- Added `cryptography` dependency to `pyproject.toml` and `uv.lock`. - Enhanced `PackageCRC` class to support mTLS authentication using Candlepin certificates. - Implemented methods to fetch certificates from the database and validate them. - Updated shipping logic to handle both mTLS and service account authentication modes. - Added logging for certificate loading and validation processes.
…ve tests - Updated the `is_shipping_configured` method in the `PackageCRC` class to correctly call the superclass method. - Introduced a new test suite for `PackageCRC`, including tests for certificate validation and shipping configuration logic. - Added tests to ensure proper behavior when certificates are valid, expired, or missing, as well as when the shipping configuration is checked under various conditions.
- Updated the `Collector` class to exclude sensitive billing keys from the configuration collection. - Improved error handling in the `PackageCRC` class to raise a `FailedToUploadPayload` exception when mTLS fails and no service account credentials are available. - Added tests to ensure proper behavior when service account credentials are missing or present during SSL errors.
- Introduced a new method `_cleanup_temp_pem_files` in the `PackageCRC` class to handle the closing of file descriptors and deletion of temporary PEM files. - Replaced the previous inline cleanup logic in the `ship` method with a call to the new cleanup method, enhancing maintainability and clarity.
- Introduced `CandlepinClient` for interacting with the Candlepin API, including methods for check-in and certificate regeneration. - Added lifecycle management functions to handle certificate parsing, renewal checks, and orchestration of check-in and renewal processes. - Implemented database functions to fetch and save Candlepin certificate data. - Created unit tests for the `CandlepinClient` and lifecycle management functions to ensure proper functionality and error handling. - Updated validation logic to integrate Candlepin lifecycle management into the existing workflow.
Contributor
|
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 mTLS support: a Candlepin REST client, certificate parsing/lifecycle and renewal, mTLS-capable CRC shipping with temporary cert files and auth-mode resolution, DB wiring for cert storage/registration, billing-param sanitization, tests, and a cryptography runtime dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant CRC as PackageCRC
participant Validation as Validation Handler
participant DB as Database
participant Client as CandlepinClient
participant Candlepin as Candlepin Server
CRC->>Validation: handle_crc_ship_target(params)
Validation->>DB: SELECT conf_setting for cert/key/uuid
DB-->>Validation: cert_pem, key_pem, consumer_uuid
alt cert present
Validation->>Validation: parse_cert() / is_cert_valid()
Validation->>Client: checkin(consumer_uuid, cert_pem, key_pem)
Client->>Candlepin: PUT /consumers/{uuid} (mTLS via temp cert files)
Candlepin-->>Client: 200/204
Client-->>Validation: success
alt needs renewal
Validation->>Client: regenerate_cert(consumer_uuid, cert_pem, key_pem)
Client->>Candlepin: POST /consumers/{uuid} (mTLS)
Candlepin-->>Client: {idCert: {cert, key}}
Client-->>Validation: new_cert_pem, new_key_pem
Validation->>DB: UPSERT new cert/key
end
Validation-->>CRC: billing_params w/ candlepin_cert_pem/key_pem
else no cert
Validation-->>CRC: billing_params (service-account mode)
end
CRC->>CRC: PackageCRC.ship() chooses auth mode (certs or service-account) and posts payload
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
…rtificate management - Replaced inline temporary file handling in the `ship` method with a context manager from `CandlepinClient` for improved clarity and resource management. - Introduced a new `is_cert_valid` function in the Candlepin lifecycle module to streamline certificate validation and logging. - Updated tests to reflect changes in certificate validation logic and ensure proper logging behavior for expired or invalid certificates.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
metrics_utility/test/library/test_candlepin_client.py (1)
114-118: Unused parameters in test method signature.The
cert_pemandkey_pemparameters have default values ofNonebut are immediately overwritten by_generate_cert_and_key(). These parameters appear to be leftover from refactoring.♻️ Suggested fix
- def test_files_have_mode_0600(self, cert_pem=None, key_pem=None): + def test_files_have_mode_0600(self): cert_pem, key_pem = _generate_cert_and_key()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/test/library/test_candlepin_client.py` around lines 114 - 118, The test method test_files_have_mode_0600 declares unused parameters cert_pem and key_pem which are immediately overwritten by _generate_cert_and_key(); remove the parameters from the signature so it becomes def test_files_have_mode_0600(self):, leaving the body unchanged (it still calls _generate_cert_and_key() and uses CandlepinClient._temp_cert_files to check file modes); run the test suite to ensure no other tests or fixtures depend on those parameters and adjust any callers if necessary.metrics_utility/management/validation.py (1)
257-296: Consider adding explicit placeholder UUID detection.The
if not consumer_uuid:check at line 267 is a truthiness check that will pass for any non-empty string. If the Docker dev seed inserts a placeholder UUID like"00000000-0000-0000-0000-000000000000", this will incorrectly trigger lifecycle operations. Consider an explicit check:🛡️ Suggested fix
+PLACEHOLDER_UUID = '00000000-0000-0000-0000-000000000000' + def _run_candlepin_lifecycle(cert_pem, key_pem, consumer_uuid): - if not consumer_uuid: + if not consumer_uuid or consumer_uuid == PLACEHOLDER_UUID: logger.warning(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/management/validation.py` around lines 257 - 296, The truthiness check in _run_candlepin_lifecycle for consumer_uuid allows placeholder UUIDs (e.g. "00000000-0000-0000-0000-000000000000") to proceed; update the guard to explicitly detect and skip placeholder values as well as empty/falsy ones by checking consumer_uuid against the known placeholder string (and/or any other configured sentinel) before proceeding with run_candlepin_lifecycle, and keep the existing warning message when skipping; reference consumer_uuid, _run_candlepin_lifecycle, and CANDLEPIN_CONSUMER_UUID when making the change.metrics_utility/test/validation/test_candlepin_validation.py (1)
119-202: Consider adding test coverage for lifecycle-enabled path.The tests cover credential injection and logging well, but don't exercise the
METRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLEDbranch inhandle_crc_ship_target. Consider adding a test that enables the flag and mocks_run_candlepin_lifecycleto verify the integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/test/validation/test_candlepin_validation.py` around lines 119 - 202, Tests are missing coverage for the METRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLED branch in handle_crc_ship_target; add a test in TestHandleCrcShipTargetCandlepin that sets monkeypatch.setenv('METRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLED','true'), patches django.db.connection.cursor as needed, and patches/Mocks metrics_utility.management.validation._run_candlepin_lifecycle to assert it is called with the expected params (and/or to return a controlled value), and also assert existing behavior (e.g., billing fields still present and cert injection/logging behavior) to verify integration.metrics_utility/library/candlepin/lifecycle.py (1)
42-43: Avoid accessing private_nameattribute; use public API instead.
attr.oid._nameis an internal attribute explicitly excluded from the public API (cryptography PR#5852rejected making it public). The current code risks breakage in future versions. For this use case, either use a static OID-to-name mapping or switch todotted_string(public API, though less human-readable).♻️ Suggested alternative
from cryptography.x509.oid import NameOID # Map known OIDs to friendly names (public API safe) OID_NAME_MAP = { NameOID.COMMON_NAME: 'commonName', NameOID.ORGANIZATION_NAME: 'organizationName', # Add additional OIDs as needed } subject = {OID_NAME_MAP.get(attr.oid, attr.oid.dotted_string): attr.value for attr in cert.subject} issuer = {OID_NAME_MAP.get(attr.oid, attr.oid.dotted_string): attr.value for attr in cert.issuer}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/library/candlepin/lifecycle.py` around lines 42 - 43, Replace the private attr.oid._name usage in the subject/issuer comprehensions with a public API approach: use attr.oid.dotted_string as the fallback and optionally map known cryptography.x509.oid.NameOID constants to friendly names (e.g., map NameOID.COMMON_NAME -> "commonName") before building the dicts; update the comprehensions that construct subject and issuer (which iterate cert.subject and cert.issuer and reference attr and attr.oid) to consult the OID_NAME_MAP.get(attr.oid, attr.oid.dotted_string) instead of attr.oid._name so the code uses public attributes and avoids private internals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metrics_utility/automation_controller_billing/package/package_crc.py`:
- Around line 19-29: The current _is_cert_valid(cert_pem: str) only checks parse
success and not_valid_after_utc; extend validation so shipping_auth_mode() will
not choose SHIPPING_AUTH_CERTIFICATES for unusable creds: update _is_cert_valid
to also check cert.not_valid_before_utc <= now (UTC) to reject not-yet-valid
certs, parse and load the corresponding private key (use the same PEM key input
used by shipping_auth_mode / the key field) to ensure it is a valid private key,
and verify the private key matches the certificate's public key (e.g., by
comparing public key parameters) before returning True; ensure
shipping_auth_mode's path that currently only checks non-empty key also calls
this enhanced _is_cert_valid and refrains from selecting
SHIPPING_AUTH_CERTIFICATES if any of these checks fail so cert/key mismatches,
malformed keys, or future-dated certs are rejected up front.
- Around line 118-132: The current ship() SSLError handler blindly retries the
POST and can duplicate uploads; change it to only retry when you can positively
detect a client-certificate rejection (inspect the SSLError exception
string/args for handshake/client-cert failure tokens such as
"CERTIFICATE_REQUIRED", "certificate", "handshake", "unknown ca", or similar)
and otherwise raise FailedToUploadPayload; if you cannot reliably detect such a
rejection, do not retry and instead raise the error. Use the same symbols shown
(_get_rh_user(), _get_rh_password(), _resolved_auth_mode,
SHIPPING_AUTH_SERVICE_ACCOUNT, _temp_cert_path/_temp_key_path and
super().ship()) to implement the conditional retry path, and if you must attempt
a retry ensure an idempotency guard is sent (e.g., set an Idempotency-Key
header) before calling super().ship() so the ingress cannot ingest duplicates.
In `@tools/docker/conf_setting.sql`:
- Around line 9-10: The seed data uses an all-zero UUID string which
deserializes to a non-empty Python string and bypasses the existing truthiness
check in validation.py; update the logic in _run_candlepin_lifecycle to
explicitly detect and treat the placeholder value
("00000000-0000-0000-0000-000000000000") as absent (i.e., skip the Candlepin
lifecycle) by checking consumer_uuid == "00000000-0000-0000-0000-000000000000"
or similar, or alternatively remove/replace the seed row with an actual empty
string (e.g., '""') so the existing if not consumer_uuid check works as
intended.
---
Nitpick comments:
In `@metrics_utility/library/candlepin/lifecycle.py`:
- Around line 42-43: Replace the private attr.oid._name usage in the
subject/issuer comprehensions with a public API approach: use
attr.oid.dotted_string as the fallback and optionally map known
cryptography.x509.oid.NameOID constants to friendly names (e.g., map
NameOID.COMMON_NAME -> "commonName") before building the dicts; update the
comprehensions that construct subject and issuer (which iterate cert.subject and
cert.issuer and reference attr and attr.oid) to consult the
OID_NAME_MAP.get(attr.oid, attr.oid.dotted_string) instead of attr.oid._name so
the code uses public attributes and avoids private internals.
In `@metrics_utility/management/validation.py`:
- Around line 257-296: The truthiness check in _run_candlepin_lifecycle for
consumer_uuid allows placeholder UUIDs (e.g.
"00000000-0000-0000-0000-000000000000") to proceed; update the guard to
explicitly detect and skip placeholder values as well as empty/falsy ones by
checking consumer_uuid against the known placeholder string (and/or any other
configured sentinel) before proceeding with run_candlepin_lifecycle, and keep
the existing warning message when skipping; reference consumer_uuid,
_run_candlepin_lifecycle, and CANDLEPIN_CONSUMER_UUID when making the change.
In `@metrics_utility/test/library/test_candlepin_client.py`:
- Around line 114-118: The test method test_files_have_mode_0600 declares unused
parameters cert_pem and key_pem which are immediately overwritten by
_generate_cert_and_key(); remove the parameters from the signature so it becomes
def test_files_have_mode_0600(self):, leaving the body unchanged (it still calls
_generate_cert_and_key() and uses CandlepinClient._temp_cert_files to check file
modes); run the test suite to ensure no other tests or fixtures depend on those
parameters and adjust any callers if necessary.
In `@metrics_utility/test/validation/test_candlepin_validation.py`:
- Around line 119-202: Tests are missing coverage for the
METRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLED branch in handle_crc_ship_target;
add a test in TestHandleCrcShipTargetCandlepin that sets
monkeypatch.setenv('METRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLED','true'),
patches django.db.connection.cursor as needed, and patches/Mocks
metrics_utility.management.validation._run_candlepin_lifecycle to assert it is
called with the expected params (and/or to return a controlled value), and also
assert existing behavior (e.g., billing fields still present and cert
injection/logging behavior) to verify integration.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: fe5b8deb-1f6d-4938-aaf5-3d59eb018c81
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
metrics_utility/automation_controller_billing/collector.pymetrics_utility/automation_controller_billing/package/package_crc.pymetrics_utility/library/candlepin/__init__.pymetrics_utility/library/candlepin/client.pymetrics_utility/library/candlepin/lifecycle.pymetrics_utility/management/validation.pymetrics_utility/test/gather/test_package_crc.pymetrics_utility/test/library/test_candlepin_client.pymetrics_utility/test/library/test_candlepin_lifecycle.pymetrics_utility/test/validation/test_candlepin_validation.pypyproject.tomltools/docker/conf_setting.sql
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metrics_utility/library/candlepin/lifecycle.py`:
- Around line 58-73: The is_cert_valid function currently only checks
parse_cert(...) and info['days_remaining'] < 0; update it to also verify the
certificate is not yet valid by checking info['not_valid_before_utc'] (from
parse_cert) against the current UTC time: if current time is before
not_valid_before_utc, log a warning (similar to the expired-path) mentioning the
not_valid_before_utc and return False so callers fall back to service-account
auth; keep existing expired and parse error handling unchanged and use the same
logger and message style as the existing checks.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: e042a952-a803-421b-bd18-0723c1e24bc7
📒 Files selected for processing (3)
metrics_utility/automation_controller_billing/package/package_crc.pymetrics_utility/library/candlepin/lifecycle.pymetrics_utility/test/gather/test_package_crc.py
✅ Files skipped from review due to trivial changes (1)
- metrics_utility/test/gather/test_package_crc.py
- Updated `is_cert_valid` function to check for certificates that are not yet valid, in addition to expired and unparseable certificates, improving logging for operator visibility. - Introduced a placeholder UUID constant to handle cases where a real consumer UUID is not registered, ensuring that lifecycle calls are skipped appropriately. - Refactored database functions to use transactions for saving Candlepin certificate data, enhancing data integrity. - Updated tests to reflect changes in lifecycle management, including handling of placeholder UUIDs and improved logging behavior.
- Changed the proxy URL in the `test_proxy_set` method from HTTP to HTTPS, ensuring secure connections are tested. - Updated the expected proxies dictionary to reflect the change in protocol.
- Implemented the `register_consumer` method in `CandlepinClient` to register a new AAP consumer with the Candlepin API, utilizing Red Hat subscription credentials. - Added helper functions to fetch registration credentials from the database and save the consumer registration details. - Integrated the registration process into the validation logic, allowing for automatic consumer registration when no identity certificate exists. - Enhanced unit tests for `CandlepinClient` and validation functions to cover the new registration functionality and ensure proper error handling.
- Updated the proxy configuration in the `CandlepinClient` constructor to normalize the proxy URL by stripping any existing scheme and prefixing it with the appropriate protocol. - Ensured that the proxies dictionary is correctly set up for both HTTP and HTTPS connections, improving the handling of proxy settings.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
metrics_utility/test/validation/test_candlepin_validation.py (1)
264-270: Minor duplication with_make_cursor_with_rowshelper.The
_make_cursor_rowshelper is nearly identical to_make_cursor_with_rowsdefined earlier (lines 26-33). Consider consolidating into a single helper function.♻️ Suggested consolidation
-def _make_cursor_rows(rows): - mock_cursor = MagicMock() - mock_cursor.fetchall.return_value = rows - mock_conn = MagicMock() - mock_conn.__enter__ = MagicMock(return_value=mock_cursor) - mock_conn.__exit__ = MagicMock(return_value=False) - return mock_connThen use
_make_cursor_with_rows(rows)[0]or adjust the original helper to return just the connection when cursor isn't needed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/test/validation/test_candlepin_validation.py` around lines 264 - 270, Consolidate the duplicate helpers by removing _make_cursor_rows and reusing the existing _make_cursor_with_rows: either call _make_cursor_with_rows(rows)[0] where the connection-only behavior is needed, or modify _make_cursor_with_rows to optionally return just the mock connection (e.g., via a parameter) and update call sites; replace all uses of _make_cursor_rows with the chosen approach and delete the redundant function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@metrics_utility/test/validation/test_candlepin_validation.py`:
- Around line 264-270: Consolidate the duplicate helpers by removing
_make_cursor_rows and reusing the existing _make_cursor_with_rows: either call
_make_cursor_with_rows(rows)[0] where the connection-only behavior is needed, or
modify _make_cursor_with_rows to optionally return just the mock connection
(e.g., via a parameter) and update call sites; replace all uses of
_make_cursor_rows with the chosen approach and delete the redundant function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 97648ca7-212f-4a3d-9cf2-3325647a5a0d
📒 Files selected for processing (5)
metrics_utility/library/candlepin/client.pymetrics_utility/library/candlepin/lifecycle.pymetrics_utility/management/validation.pymetrics_utility/test/library/test_candlepin_client.pymetrics_utility/test/validation/test_candlepin_validation.py
✅ Files skipped from review due to trivial changes (1)
- metrics_utility/test/library/test_candlepin_client.py
🚧 Files skipped from review as they are similar to previous changes (1)
- metrics_utility/library/candlepin/lifecycle.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metrics_utility/library/candlepin/client.py`:
- Around line 28-33: The proxy normalization currently strips the provided
scheme and forces per-protocol schemes which breaks HTTP CONNECT proxies; update
the logic in the client (the code that sets self.proxies in
metrics_utility/library/candlepin/client.py) to preserve an explicit scheme if
the user supplies one, otherwise default to "http://" and then assign the same
proxy URL for both 'http' and 'https' keys (i.e., do not create distinct
https/http schemes by reappending different prefixes); also update the test
assertion in test_candlepin_client.py (the test around lines 85-86) to expect
the same proxy URL for both protocols when a scheme is present or defaulted.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 687562c1-f58d-4cd3-9ada-bbf11ff74906
📒 Files selected for processing (1)
metrics_utility/library/candlepin/client.py
- Changed the coverage exclusions pattern from `metrics_utility/test/**` to `**/tests/**`, allowing for broader exclusion of test directories across the project.
- Introduced a constant for the default renewal days in the `lifecycle.py` module, improving code readability and maintainability. - Updated the `get_renewal_days` function to use the new constant, ensuring consistent default values and clearer logging for invalid environment variable cases. - Removed the redundant default value definition from the `validation.py` module, streamlining the codebase.
- Enhanced the proxy configuration logic in the `CandlepinClient` constructor to better handle HTTPS and HTTP proxy URLs. - Updated comments for clarity on how the proxy URLs are processed, ensuring correct usage of the intended scheme for both HTTP and HTTPS connections.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
metrics_utility/library/candlepin/client.py (1)
28-37:⚠️ Potential issue | 🟠 MajorProxy handling still risks broken HTTPS tunneling behavior
Line 36 keeps different proxy values for
httpsandhttp, and ifproxyis passed without a scheme, thehttpsvalue can remain scheme-less. This can breakrequestsproxy resolution and standard CONNECT proxy usage.Proposed fix
if proxy: - # Use the caller-supplied URL as-is for HTTPS targets (preserves the - # intended scheme — usually http:// so requests uses plain HTTP to reach - # the proxy and issues CONNECT for TLS tunneling, but https:// is also - # accepted for the rare case of an HTTPS-fronted proxy). - # The http:// key always uses plain HTTP since non-TLS traffic never - # needs TLS to the proxy itself. - host = proxy.split('://', 1)[-1] - self.proxies = {'https': proxy, 'http': f'http://{host}'} + if '://' not in proxy: + proxy = f'http://{proxy}' + self.proxies = {'https': proxy, 'http': proxy}#!/bin/bash set -euo pipefail # Verify current proxy normalization logic and related test expectations/docs. rg -n -C3 'if proxy:|self\.proxies|split\(.:\/\/|METRICS_UTILITY_PROXY_URL|proxy' \ metrics_utility/library/candlepin/client.py \ metrics_utility/management/validation.py \ metrics_utility/test/library/test_candlepin_client.pyAs per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/library/candlepin/client.py` around lines 28 - 37, The proxy handling may leave the 'https' proxy value without a scheme, breaking requests' proxy resolution; update the normalization so any incoming proxy string missing a scheme gets an explicit scheme (default to "http://"), then build self.proxies using that normalized proxy for the 'https' key and an explicit "http://{host}" for the 'http' key (use the normalized host computed from the normalized proxy); adjust the code around the proxy variable, host extraction, and self.proxies assignment to always supply scheme-qualified proxy URLs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@metrics_utility/library/candlepin/client.py`:
- Around line 25-27: The constructor currently sets self.verify = candlepin_ca
if candlepin_ca else False which makes TLS verification disabled by default;
change __init__ (referencing DEFAULT_CANDLEPIN_URL and self.verify) so verify
defaults to True when candlepin_ca is None (i.e., enable certificate
verification), and introduce an explicit opt-in flag (e.g., allow_insecure or
verify_tls=False parameter or env var check) that must be deliberately set to
disable verification; update register_consumer to continue using self.verify but
ensure any insecure path is only reachable when the new explicit opt-in is true
and document/guard that flag.
---
Duplicate comments:
In `@metrics_utility/library/candlepin/client.py`:
- Around line 28-37: The proxy handling may leave the 'https' proxy value
without a scheme, breaking requests' proxy resolution; update the normalization
so any incoming proxy string missing a scheme gets an explicit scheme (default
to "http://"), then build self.proxies using that normalized proxy for the
'https' key and an explicit "http://{host}" for the 'http' key (use the
normalized host computed from the normalized proxy); adjust the code around the
proxy variable, host extraction, and self.proxies assignment to always supply
scheme-qualified proxy URLs.
🪄 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: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 553698ad-1d2c-4d0f-ade9-2ecfd3d4f5e1
📒 Files selected for processing (4)
metrics_utility/library/candlepin/client.pymetrics_utility/library/candlepin/lifecycle.pymetrics_utility/management/validation.pysonar-project.properties
✅ Files skipped from review due to trivial changes (2)
- sonar-project.properties
- metrics_utility/management/validation.py
- Updated the `CandlepinClient` to enable TLS server verification by default, requiring explicit opt-in to disable it for controlled environments. - Refactored the database functions in `validation.py` to use a new `_upsert_conf_settings` method for better code reuse and clarity. - Improved unit tests to cover new TLS verification behavior and ensure correct handling of certificate and registration data in the database.
- Introduced a new management command `candlepin_manage` for handling Candlepin consumer registration and certificate lifecycle management. - Enhanced the `_upsert_conf_settings` function in `validation.py` to return success or failure status, improving error handling. - Updated logging to provide clearer feedback on the success of certificate and registration operations. - Added unit tests for the new management command to ensure proper functionality and error handling in various scenarios.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
metrics_utility/management/commands/candlepin_manage.py (1)
119-174: Consider extracting credential validation to reduce complexity.SonarCloud flagged cognitive complexity of 17 (threshold 15). The credential validation block (lines 136-146) could be extracted into a helper function.
Suggested refactor
+ def _validate_credentials(self, username, password, org): + """Return list of missing credential error messages, or empty list if all present.""" + missing = [] + if not username: + missing.append('username (pass --username or set SUBSCRIPTIONS_USERNAME in conf_setting)') + if not password: + missing.append('password (pass --password or set SUBSCRIPTIONS_PASSWORD in conf_setting)') + if not org: + missing.append('org (pass --org or ensure LICENSE.account_number is set in conf_setting)') + return missing + def _handle_register(self, options): # ... existing code ... - missing = [] - if not username: - missing.append('username (pass --username or set SUBSCRIPTIONS_USERNAME in conf_setting)') - if not password: - missing.append('password (pass --password or set SUBSCRIPTIONS_PASSWORD in conf_setting)') - if not org: - missing.append('org (pass --org or ensure LICENSE.account_number is set in conf_setting)') + missing = self._validate_credentials(username, password, org) if missing: for m in missing: self.stderr.write(f'Missing required value: {m}') return False🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/management/commands/candlepin_manage.py` around lines 119 - 174, Extract the credential resolution and validation block inside _handle_register into a helper function (e.g., _resolve_and_validate_credentials) that calls _fetch_registration_credentials_from_db, merges CLI options (options['username'], options['password'], options['org']) with DB values, and returns a tuple (username, password, org) or None/False on failure; move the missing-check logic (building the missing list and writing to self.stderr for each missing message) into that helper and have _handle_register call it and abort if it indicates failure, keeping all existing messages and behavior unchanged and preserving use of options keys and db_install_uuid handling.metrics_utility/test/library/test_candlepin_client.py (2)
122-126: Consider usingendswithfor file mode assertion.The string slice
[-3:]works but is less readable thanendswith. Static analysis flagged this.Suggested fix
def test_files_have_mode_0600(self, cert_pem=None, key_pem=None): cert_pem, key_pem = _generate_cert_and_key() with CandlepinClient._temp_cert_files(cert_pem, key_pem) as (cp, kp): - assert oct(os.stat(cp).st_mode)[-3:] == '600' - assert oct(os.stat(kp).st_mode)[-3:] == '600' + assert oct(os.stat(cp).st_mode).endswith('600') + assert oct(os.stat(kp).st_mode).endswith('600')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/test/library/test_candlepin_client.py` around lines 122 - 126, Replace the string-slice file mode assertions in test_files_have_mode_0600 with clearer endswith checks: inside the context manager using CandlepinClient._temp_cert_files(cert_pem, key_pem) change both occurrences that currently use oct(os.stat(...).st_mode)[-3:] == '600' to use oct(os.stat(...).st_mode).endswith('600') so the intent is more readable and satisfies static analysis.
122-122: Remove unused default parameters from test method signature.The method signature includes
cert_pem=None, key_pem=Nonebut immediately regenerates them inside. This appears to be a copy-paste artifact.Suggested fix
- def test_files_have_mode_0600(self, cert_pem=None, key_pem=None): + def test_files_have_mode_0600(self):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@metrics_utility/test/library/test_candlepin_client.py` at line 122, The test method test_files_have_mode_0600 includes unused default parameters cert_pem=None and key_pem=None which are immediately regenerated; remove these parameters from the method signature so it becomes def test_files_have_mode_0600(self): and update any local references if necessary (the body already regenerates cert_pem/key_pem), ensuring no external test harness expects those parameters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@metrics_utility/management/commands/candlepin_manage.py`:
- Around line 119-174: Extract the credential resolution and validation block
inside _handle_register into a helper function (e.g.,
_resolve_and_validate_credentials) that calls
_fetch_registration_credentials_from_db, merges CLI options
(options['username'], options['password'], options['org']) with DB values, and
returns a tuple (username, password, org) or None/False on failure; move the
missing-check logic (building the missing list and writing to self.stderr for
each missing message) into that helper and have _handle_register call it and
abort if it indicates failure, keeping all existing messages and behavior
unchanged and preserving use of options keys and db_install_uuid handling.
In `@metrics_utility/test/library/test_candlepin_client.py`:
- Around line 122-126: Replace the string-slice file mode assertions in
test_files_have_mode_0600 with clearer endswith checks: inside the context
manager using CandlepinClient._temp_cert_files(cert_pem, key_pem) change both
occurrences that currently use oct(os.stat(...).st_mode)[-3:] == '600' to use
oct(os.stat(...).st_mode).endswith('600') so the intent is more readable and
satisfies static analysis.
- Line 122: The test method test_files_have_mode_0600 includes unused default
parameters cert_pem=None and key_pem=None which are immediately regenerated;
remove these parameters from the method signature so it becomes def
test_files_have_mode_0600(self): and update any local references if necessary
(the body already regenerates cert_pem/key_pem), ensuring no external test
harness expects those parameters.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 96ae9b67-97c3-4d38-800e-a081f65c6d21
📒 Files selected for processing (9)
metrics_utility/library/candlepin/client.pymetrics_utility/library/candlepin/lifecycle.pymetrics_utility/management/commands/candlepin_manage.pymetrics_utility/management/validation.pymetrics_utility/test/library/test_candlepin_client.pymetrics_utility/test/management/__init__.pymetrics_utility/test/management/commands/__init__.pymetrics_utility/test/management/commands/test_candlepin_manage.pysonar-project.properties
🚧 Files skipped from review as they are similar to previous changes (1)
- sonar-project.properties
- Moved credential resolution and validation logic to a new method `_resolve_and_validate_credentials`, improving code organization and readability. - Updated `_handle_register` to utilize the new method for resolving credentials, ensuring that required fields are validated before proceeding with registration. - Enhanced error handling by providing clearer feedback when required values are missing.
|
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
1 participant
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.
Candlepin Integration
Overview
This branch adds Candlepin consumer identity certificate support to metrics-utility, enabling AAP controller instances to authenticate analytics uploads to
console.redhat.comusing mTLS instead of a service account. The certificate is stored in the AWXconf_settingdatabase table, making the solution topology-agnostic across RPM and containerised deployments.When a valid Candlepin certificate is present the upload path switches to mTLS automatically. If the certificate is absent, expired, or the TLS handshake fails, the system falls back to service-account authentication so existing deployments are never broken.
New modules
metrics_utility/library/candlepin/client.py—CandlepinClientThin REST client for the Candlepin API. All API calls that require authentication after initial registration use the consumer identity certificate (mTLS), matching the pattern used by
subscription-manager.register_consumerPOST /consumers?owner={org}checkinPUT /consumers/{uuid}regenerate_certPOST /consumers/{uuid}RuntimeErroron failureTLS verification defaults to
True(system CA store). Passcandlepin_cato verify against a specific CA bundle (e.g./etc/rhsm/ca/redhat-uep.pem). Verification can only be disabled by explicitly passingverify_tls=False; a warning is logged when this branch is reached.Proxy support: the supplied proxy URL is used as-is for the
httpskey and normalised tohttp://for thehttpkey, so plain-HTTP proxies with CONNECT tunnelling work correctly.Temporary PEM files written to disk for mTLS calls are created with
0o600permissions and unconditionally deleted on context-manager exit, even on exception.metrics_utility/library/candlepin/lifecycle.pyCertificate inspection and lifecycle orchestration helpers.
parse_cert(pem_text)is_cert_valid(cert_pem)Falseand logs a warning on any failure so callers fall back to service-account auth.needs_renewal(pem_text, days_before_expiry)Trueif the cert expires within the threshold (or is already expired).run_candlepin_lifecycle(...)(cert_pem, key_pem)— original or renewed.Config helpers (
get_candlepin_url,get_renewal_days,get_candlepin_ca) read from environment variables with sensible defaults.Changes to existing code
metrics_utility/management/validation.pyNew functions and constants added to
handle_crc_ship_targetand the surrounding helpers.Constants
Registration flow (
METRICS_UTILITY_CANDLEPIN_REGISTRATION_ENABLED)_fetch_registration_credentials_from_db()readsSUBSCRIPTIONS_USERNAME,SUBSCRIPTIONS_PASSWORD(set by AWX when the customer configures their Red Hat subscription), andLICENSE.account_number(the Candlepin org key) fromconf_settingin a single query._register_candlepin_consumer()callsCandlepinClient.register_consumerwith those credentials and persists the result via_save_candlepin_registration_to_db, which UPSERTs the cert PEM, key PEM, and consumer UUID toconf_settingin a single transaction.Lifecycle flow (
METRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLED)_fetch_candlepin_lifecycle_from_db()reads the stored cert, key, and consumer UUID._run_candlepin_lifecycle()callsrun_candlepin_lifecyclefrom the library module. If the cert is renewed,_save_candlepin_cert_to_dbUPSERTs the updated cert and key back toconf_setting.handle_crc_ship_targetexecution orderconf_setting.METRICS_UTILITY_CANDLEPIN_REGISTRATION_ENABLEDis set → attempt registration.METRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLEDis set → check-in + proactive renewal.candlepin_cert_pem/candlepin_key_pemintobilling_provider_paramsfor the upload step.All steps are best-effort: failures log a warning or error and fall through to service-account auth.
metrics_utility/automation_controller_billing/package/package_crc.pyshipping_auth_mode()now returnsSHIPPING_AUTH_CERTIFICATESwhen a valid Candlepin cert is available,SHIPPING_AUTH_SERVICE_ACCOUNTotherwise. The result is cached in_resolved_auth_mode.ship()overridden to manage the temp-file lifecycle for mTLS uploads viaCandlepinClient._temp_cert_files. OnSSLError, falls back to service-account auth if those credentials are configured; raisesFailedToUploadPayloadif not, so the operator gets an actionable error._send_data()gains aSHIPPING_AUTH_CERTIFICATESbranch that posts withsession.certand server-cert verification._gather_config()stripscandlepin_cert_pemandcandlepin_key_pemfrom thebilling_provider_paramsdict before writing it to theconfig.jsonpayload archive, preventing key material from being uploaded to the ingress endpoint.New CLI command
uv run ./manage.py candlepin_managecandlepin_manage registerRegisters this AAP instance with Candlepin and stores the resulting identity certificate, key, and consumer UUID in
conf_setting.Credentials are read from AWX
conf_settingby default (SUBSCRIPTIONS_USERNAME,SUBSCRIPTIONS_PASSWORD,LICENSE.account_number). CLI flags override the DB values.candlepin_manage renewReads the stored cert/key/UUID from
conf_setting, performs a check-in, and renews the certificate if it is within the renewal window (default: 30 days).New environment variables
METRICS_UTILITY_CANDLEPIN_URLhttps://subscription.rhsm.redhat.com/subscriptionMETRICS_UTILITY_CANDLEPIN_CAMETRICS_UTILITY_CANDLEPIN_RENEWAL_DAYS30METRICS_UTILITY_CANDLEPIN_REGISTRATION_ENABLEDMETRICS_UTILITY_CANDLEPIN_LIFECYCLE_ENABLEDDatabase storage
All Candlepin material is stored in the existing AWX
conf_settingtable using the standard UPSERT pattern already used by the rest of the codebase. No schema changes are required.CANDLEPIN_CONSUMER_CERTCANDLEPIN_CONSUMER_KEYCANDLEPIN_CONSUMER_UUIDThe AWX DB seed (
tools/docker/conf_setting.sql) seedsCANDLEPIN_CONSUMER_UUIDwith the all-zeros placeholder. The placeholder is treated the same as an absent value — no lifecycle or registration calls are made until a real UUID is written.Dependencies
cryptography>=42.0.0added topyproject.tomlfor X.509 certificate parsing.Test coverage
test/library/test_candlepin_client.pyCandlepinClient: construction, temp-file lifecycle,checkin,regenerate_cert,register_consumertest/library/test_candlepin_lifecycle.pyparse_cert,is_cert_valid,needs_renewal,run_candlepin_lifecycletest/gather/test_package_crc.pyPackageCRC: auth-mode selection, mTLS ship path, SSL fallback, temp-file cleanuptest/validation/test_candlepin_validation.py_run_candlepin_lifecycle,_register_candlepin_consumer,handle_crc_ship_targetintegrationtest/management/commands/test_candlepin_manage.pycandlepin_manage registerandrenewsubcommandsNote
High Risk
Touches analytics upload authentication and introduces certificate/key handling plus direct DB read/write of sensitive material, so misconfiguration or edge cases could break shipping or leak credentials if incorrect. Broad new surface area (Candlepin API calls, lifecycle automation, CLI) increases operational risk despite test coverage.
Overview
Adds Candlepin consumer identity certificate support so CRC billing uploads can authenticate via mTLS when a valid cert/key is available, with automatic fallback to service-account auth on invalid/expired certs or TLS handshake failures.
Introduces a new
metrics_utility.library.candlepinclient + lifecycle helpers, wires them intohandle_crc_ship_target()to optionally auto-register consumers and proactively check-in/renew certs using values stored inconf_setting, and adds acandlepin_managemanagement command to register/renew manually.Hardens security by stripping
candlepin_cert_pem/candlepin_key_pemfrom the archivedconfig.jsonpayload, uses secure temp files for mTLS keys during shipping, and addscryptographyplus extensive unit tests/seed updates for Candlepin settings.Reviewed by Cursor Bugbot for commit 58c971d. Bugbot is set up for automated code reviews on this repo. Configure here.