fix(docker): update OIDC Azure AD configuration and metadata URL handling#785
Conversation
|
@edmondas can you merge this or do I need to update something ? Thanks |
@jozefrebjak I have added some comments, so I'm not sure if you can see those :) |
I can't see any comments |
edmondas
left a comment
There was a problem hiding this comment.
Please take a look at my comments
@jozefrebjak What about now? I think I forgot to submit my review... It's quite unusual that there's an additional step - I expected my comments to be visible instantly. |
| 'tenant' => '${PA_OIDC_AZURE_TENANT:-common}', | ||
| 'auto_discovery' => ${oidc_azure_auto_discovery}, | ||
| 'metadata_url' => '${PA_OIDC_AZURE_METADATA_URL:-https://login.microsoftonline.com/{tenant}/v2.0/.well-known/openid-configuration}', | ||
| 'metadata_url' => '${azure_metadata_url}', |
There was a problem hiding this comment.
I made some improvements to URL handling and added special placeholders (see commit 641d7b9), so in this case I feel that this URL handling is not required—just use the URL like this:
https://login.microsoftonline.com/{tenant}/v2.0/.well-known/openid-configuration
| | `PA_OIDC_AZURE_TENANT` | Tenant ID, domain, or 'common' for multi-tenant | `common` | No | | ||
| | `PA_OIDC_AZURE_AUTO_DISCOVERY` | Use auto-discovery | `true` | No | | ||
| | `PA_OIDC_AZURE_METADATA_URL` | Metadata URL for discovery | Azure's standard URL | No | | ||
| | `PA_OIDC_AZURE_METADATA_URL` | Custom metadata URL (auto-generated if not provided) | `https://login.microsoftonline.com/{tenant}/v2.0/.well-known/openid-configuration` | No | |
There was a problem hiding this comment.
Not sure if it's needed, as this is controlled through PA_OIDC_AZURE_TENANT
There was a problem hiding this comment.
@edmondas If it's done within backend and we only need to set tenant then it's not needed.
There was a problem hiding this comment.
@jozefrebjak Could you clean this up? I feel this parameter is not required because it will be built based on the PA_OIDC_AZURE_TENANT value by the backend. Thanks.
|
|
||
| # Add Azure configuration if enabled | ||
| if [ "${oidc_azure_enabled}" = "true" ]; then | ||
| # Build the metadata URL with the actual tenant value |
There was a problem hiding this comment.
This is now done on the backend side, so not sure if it's still relevant
|
@jozefrebjak Thanks for your contribution and sorry for bothering you with the changes! I went ahead and did some of the cleanups we discussed in the review: removed the manual URL building logic in the Docker entrypoint script and now use the simple template URL with {tenant} placeholder, also removed the PA_OIDC_AZURE_METADATA_URL environment variable since Azure uses a predictable standard endpoint. The implementation now properly uses the existing URL template processing instead of duplicating logic in the shell script. |
This pull request updates the OIDC Azure AD integration documentation and configuration logic to clarify tenant and metadata URL usage, improve configuration flexibility, and enhance user mapping. It also removes some legacy permission template mapping logic.
OIDC Azure AD Configuration Improvements:
DOCKER.mddocumentation forPA_OIDC_AZURE_TENANTto clarify that it accepts tenant ID, domain, or 'common', and forPA_OIDC_AZURE_METADATA_URLto specify that it is auto-generated if not provided. Added detailed configuration notes and example tenant values for better user guidance.docker-entrypoint.sh, the Azure metadata URL is now dynamically constructed using the actual tenant value if not explicitly set, ensuring correct endpoint configuration.User Mapping and Permission Template Changes:
emailfor the username and added thesubject(sub) field for improved identity tracking.permission_template_mappingand the environment variable override fordefault_permission_template, simplifying the permission template logic. [1] [2]