Azure Credential to provide OIDC token for playbooks#193
Conversation
Requires Client Secret Credentials or a Host under Managed Identity.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds an Azure OIDC credential lookup plugin with required Azure inputs, service-principal or managed-identity selection, token retrieval and error mapping, plugin wiring, and entry-point discovery coverage. ChangesAzure OIDC credential flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant azure_oidc_backend
participant _initialize_credential
participant AzureCredential
Caller->>azure_oidc_backend: provide url and cloud_name
azure_oidc_backend->>_initialize_credential: initialize credential for selected cloud
_initialize_credential-->>azure_oidc_backend: ClientSecretCredential or ManagedIdentityCredential
azure_oidc_backend->>AzureCredential: get_token(url)
AzureCredential-->>azure_oidc_backend: token or Azure exception
azure_oidc_backend-->>Caller: token string or RuntimeError
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/awx_plugins/credentials/azure_token.py`:
- Around line 95-109: The credential initialization in _initialize_ currently
falls back to ManagedIdentityCredential whenever tenant, client, or secret is
missing, which can hide partially configured service-principal settings. Update
_initialize_ to explicitly detect when some but not all of tenant/client/secret
are provided and reject that configuration with an error instead of using
managed identity. Keep the existing ClientSecretCredential path for the fully
populated case, and only return ManagedIdentityCredential when none of the three
values are supplied.
- Around line 73-77: `cloud_name` is currently only exposed in the Azure
credential field definitions and is not propagated into the credential setup, so
the selected cloud never affects authentication. Update the Azure credential
flow by threading the chosen `cloud_name` from the credential constructors into
`_initialize_credential` (or through `azure_oidc_backend`), and use it to select
the correct Azure authority host instead of always defaulting to the public
cloud. Make sure the selection is wired through the relevant credential creation
path so `ClientSecretCredential` uses the matching cloud environment.
🪄 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: Enterprise
Run ID: 83cdb174-872d-423c-a128-9b3a027d7d9f
📒 Files selected for processing (1)
src/awx_plugins/credentials/azure_token.py
| def _initialize_credential( | ||
| tenant: str = '', | ||
| client: str = '', | ||
| secret: str = '', | ||
| ) -> TokenCredential: | ||
| explicit_credentials_provided = all((tenant, client, secret)) | ||
|
|
||
| if explicit_credentials_provided: | ||
| return ClientSecretCredential( | ||
| tenant_id=tenant, | ||
| client_id=client, | ||
| client_secret=secret, | ||
| ) | ||
|
|
||
| return ManagedIdentityCredential() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject partial service-principal configuration instead of falling back to managed identity.
Line 100 treats any missing tenant/client/secret as “use managed identity”. If a user partially configures a service principal, this can silently authenticate as the host identity instead.
Proposed fix
def _initialize_credential(
tenant: str = '',
client: str = '',
secret: str = '',
) -> TokenCredential:
+ if any((tenant, client, secret)) and not all((tenant, client, secret)):
+ raise RuntimeError(
+ 'Client ID, Client Secret, and Tenant ID must be provided together, '
+ 'or all left empty to use Managed Identity.',
+ )
+
explicit_credentials_provided = all((tenant, client, secret))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def _initialize_credential( | |
| tenant: str = '', | |
| client: str = '', | |
| secret: str = '', | |
| ) -> TokenCredential: | |
| explicit_credentials_provided = all((tenant, client, secret)) | |
| if explicit_credentials_provided: | |
| return ClientSecretCredential( | |
| tenant_id=tenant, | |
| client_id=client, | |
| client_secret=secret, | |
| ) | |
| return ManagedIdentityCredential() | |
| def _initialize_credential( | |
| tenant: str = '', | |
| client: str = '', | |
| secret: str = '', | |
| ) -> TokenCredential: | |
| if any((tenant, client, secret)) and not all((tenant, client, secret)): | |
| raise RuntimeError( | |
| 'Client ID, Client Secret, and Tenant ID must be provided together, ' | |
| 'or all left empty to use Managed Identity.', | |
| ) | |
| explicit_credentials_provided = all((tenant, client, secret)) | |
| if explicit_credentials_provided: | |
| return ClientSecretCredential( | |
| tenant_id=tenant, | |
| client_id=client, | |
| client_secret=secret, | |
| ) | |
| return ManagedIdentityCredential() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/awx_plugins/credentials/azure_token.py` around lines 95 - 109, The
credential initialization in _initialize_ currently falls back to
ManagedIdentityCredential whenever tenant, client, or secret is missing, which
can hide partially configured service-principal settings. Update _initialize_ to
explicitly detect when some but not all of tenant/client/secret are provided and
reject that configuration with an error instead of using managed identity. Keep
the existing ClientSecretCredential path for the fully populated case, and only
return ManagedIdentityCredential when none of the three values are supplied.
Also add test to validate azure_token will import.
The cloud_name logic was not wired up.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
163-168: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd optional dependencies feature-flag for
azure_token.The new
azure_tokencredential plugin usesazure-identityandmsrestazure, but a corresponding feature-flag dependency group (e.g.,credentials-azure-token) is missing inpyproject.toml. This prevents end-users from easily installing the required dependencies for this new plugin.💻 Proposed fix
Add the following under
[project.optional-dependencies]:credentials-azure-token = [ "awx_plugins.interfaces", "azure-identity", "msrestazure", ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pyproject.toml` around lines 163 - 168, Add a credentials-azure-token optional dependency group under [project.optional-dependencies] containing awx_plugins.interfaces, azure-identity, and msrestazure, matching the existing feature-flag dependency group structure.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/awx_plugins/credentials/azure_token.py`:
- Around line 98-118: Update _initialize_credential to reject partial
service-principal configuration instead of falling back to managed identity,
while preserving ClientSecretCredential for complete tenant, client, and secret
values. Construct ManagedIdentityCredential without the unsupported
cloud_environment argument, and pass client_id only when client is non-empty,
otherwise use None.
- Line 41: Update the default_cloud assignment to retain the
azure_cloud.AZURE_PUBLIC_CLOUD object instead of its .name string, so the
existing default_cloud.name accesses later in the plugin remain valid.
---
Outside diff comments:
In `@pyproject.toml`:
- Around line 163-168: Add a credentials-azure-token optional dependency group
under [project.optional-dependencies] containing awx_plugins.interfaces,
azure-identity, and msrestazure, matching the existing feature-flag dependency
group structure.
🪄 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: Enterprise
Run ID: acdc7ca2-3439-4668-af9f-7e2162c49205
📒 Files selected for processing (3)
pyproject.tomlsrc/awx_plugins/credentials/azure_token.pytests/importable_test.py
|
|
||
| # https://github.com/Azure/msrestazure-for-python/blob/master/msrestazure/azure_cloud.py | ||
| clouds = [x[1] for x in inspect.getmembers(azure_cloud) if isinstance(x[1], azure_cloud.Cloud)] | ||
| default_cloud = azure_cloud.AZURE_PUBLIC_CLOUD.name |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix AttributeError caused by accessing .name on a string.
At line 41, default_cloud is assigned the string value of azure_cloud.AZURE_PUBLIC_CLOUD.name. Later at lines 76 and 77, the code attempts to access default_cloud.name, which will raise an AttributeError: 'str' object has no attribute 'name' and crash the plugin upon import.
💻 Proposed fix
Change the assignment so default_cloud is the object itself:
-default_cloud = azure_cloud.AZURE_PUBLIC_CLOUD.name
+default_cloud = azure_cloud.AZURE_PUBLIC_CLOUD📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| default_cloud = azure_cloud.AZURE_PUBLIC_CLOUD.name | |
| default_cloud = azure_cloud.AZURE_PUBLIC_CLOUD |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/awx_plugins/credentials/azure_token.py` at line 41, Update the
default_cloud assignment to retain the azure_cloud.AZURE_PUBLIC_CLOUD object
instead of its .name string, so the existing default_cloud.name accesses later
in the plugin remain valid.
Update pyproject dependencies. Formatting changes. Fixes for default cloud. Fixes for ManagedAuthentication.
for more information, see https://pre-commit.ci
1 participant
Requires Client Secret Credentials or a Host under Managed Identity.
Summary by CodeRabbit