feat: support azure entra id service principal authentication #1133
Conversation
📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds Azure Entra ID (service principal) support by introducing ClientID/ClientSecret/TenantID fields, centralizing Azure auth to prefer service-principal tokens over context tokens and API keys, persisting credentials via DB migration/storage, adding redaction/env-var handling, and exposing UI/documentation for the new fields. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Handler as Request Handler
participant Provider as AzureProvider
participant Store as Config Store / DB
participant AzureSDK as Azure Identity SDK
Handler->>Provider: Incoming request referencing a key
Provider->>Store: Load key (may include client_id/secret/tenant or api key)
alt Service Principal configured
Provider->>AzureSDK: getOrCreateAuth(clientID, clientSecret, tenantID)
AzureSDK-->>Provider: TokenCredential / AccessToken
Provider->>Handler: Attach "Authorization: Bearer <token>"
else Context token available
Provider->>Handler: Attach "Authorization: Bearer <context token>"
else Fallback to API key
Provider->>Handler: Attach "api-key" / "x-api-key" header with key value
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Areas needing extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
🧪 Test Suite AvailableThis PR can be tested by a repository admin. |
TejasGhatte
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
231-243: Consider using password input type for Client Secret field.For improved security posture, the Client Secret field could use
type="password"to mask the input value while typing. This would be consistent with security best practices for credential inputs, though I note other secret fields in this form also use plain text inputs.🔎 Suggested change
<FormControl> - <Input placeholder="your-client-secret or env.AZURE_CLIENT_SECRET" {...field} value={field.value ?? ""} /> + <Input placeholder="your-client-secret or env.AZURE_CLIENT_SECRET" type="password" {...field} value={field.value ?? ""} /> </FormControl>transports/bifrost-http/handlers/providers.go (1)
767-791: LGTM! Redaction logic correctly preserves raw Azure credentials.The implementation properly handles the new Service Principal fields (ClientID, ClientSecret, TenantID) by preserving the original raw values when the incoming update contains redacted values that match the old redacted values. This follows the same pattern used for
APIVersionabove.🔎 Optional: Minor redundant check cleanup
The
oldRawKey.AzureKeyConfig != nilcheck within each inner block (lines 770, 778, 786) is redundant since the outer block at line 754 already verifies this condition. You could simplify by removing these redundant checks for cleaner code:if updateKey.AzureKeyConfig.ClientID != nil && - oldRedactedKey.AzureKeyConfig.ClientID != nil && - oldRawKey.AzureKeyConfig != nil { + oldRedactedKey.AzureKeyConfig.ClientID != nil { if lib.IsRedacted(*updateKey.AzureKeyConfig.ClientID) &&This is purely cosmetic and matches the existing style, so feel free to keep as-is.
ui/lib/schemas/providerForm.ts (1)
76-79: LGTM! Schema correctly defines optional Azure credential fields.The fields are properly defined as optional strings, consistent with the Go backend's pointer types (
*string).🔎 Optional: Consider validation for Service Principal completeness
For Service Principal authentication, all three fields (ClientID, ClientSecret, TenantID) are typically required together. You could add a refinement to validate that either all three are provided or none:
const AzureKeyConfigSchema = z.object({ endpoint: z.string().min(1, "Endpoint is required for Azure keys"), deployments: z.union([z.record(z.string(), z.string()), z.string()]).optional() .refine((value) => !value || isValidDeployments(value), { message: "..." }), api_version: z.string().optional(), client_id: z.string().optional(), client_secret: z.string().optional(), tenant_id: z.string().optional(), }).refine((data) => { const hasClientId = !!data.client_id?.trim(); const hasClientSecret = !!data.client_secret?.trim(); const hasTenantId = !!data.tenant_id?.trim(); const allProvided = hasClientId && hasClientSecret && hasTenantId; const noneProvided = !hasClientId && !hasClientSecret && !hasTenantId; return allProvided || noneProvided; }, { message: "For Service Principal auth, provide all of Client ID, Client Secret, and Tenant ID, or leave all empty", path: ["client_id"], });This is optional since the backend likely handles this validation, but it would provide better UX feedback.
framework/configstore/migrations.go (1)
1833-1874: LGTM! Migration correctly adds Azure credential columns.The migration follows established patterns in this file:
- Uses
HasColumnchecks beforeAddColumnfor idempotency- Proper error wrapping with descriptive messages
- Clean rollback implementation
🔎 Optional: Defensive rollback checks
For consistency with some other migrations (e.g.,
migrationAddEnabledColumnToKeyTableat line 1134), you could addHasColumnchecks in the rollback to make it more defensive:Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() + if migrator.HasColumn(&tables.TableKey{}, "azure_client_id") { if err := migrator.DropColumn(&tables.TableKey{}, "azure_client_id"); err != nil { return fmt.Errorf("failed to drop azure_client_id column: %w", err) } + } // ... similar for other columnsThis is optional since the current pattern matches other simpler migrations in this file.
transports/bifrost-http/lib/config.go (1)
3439-3495: Env‑var processing for new Azure fields is correct; small redundancy nit.
processAzureKeyConfigEnvVarscorrectly:
- Resolves
client_id,client_secret, andtenant_idviaprocessEnvValue.- Tracks
EnvKeyInfoentries with fully qualifiedConfigPathmatching redaction paths.Minor nit:
azureConfig.ClientSecretis assigned&processedClientSecrettwice (before and after theenvVarblock). You can drop one of the assignments to avoid confusion; behavior is currently correct.core/providers/azure/files.go (1)
8-8: setAzureAuth behavior matches new auth priority; consider reusing getAzureAuthHeaders.Service principal → context token → API key ordering and header mangling (Authorization vs
api-key) look correct and consistent with the centralizedgetAzureAuthHeaderslogic.To reduce drift between code paths, you could implement
setAzureAuthby delegating togetAzureAuthHeaders(ctx, key, false)and then applying the returned map toreq.Header, instead of duplicating the token acquisition logic.Also applies to: 15-56
core/providers/azure/azure.go (1)
32-44: Centralized Azure auth and token cache look good; cache key should include clientSecret to respect rotations.The new pieces:
DefaultAzureScopeandgetOrCreateAuth(tenantID, clientID, clientSecret)usingazidentity.NewClientSecretCredential.credentials sync.MaponAzureProvider.getAzureAuthHeadersimplementing the auth priority (service principal → context token → API key, withx-api-keyfor Anthropic).are all aligned with the intended behavior.
One gotcha: the credentials cache key is
tenantID + ":" + clientID, so if the client secret is rotated for a given (tenant, client),getOrCreateAuthwill continue to reuse the oldTokenCredentialand never see the new secret until the process restarts or the map is cleared. To support secret rotation, consider including the secret (or a hash of it) in the cache key, or explicitly clearing the corresponding entry when provider config changes.Also applies to: 46-69, 71-114
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
core/go.mod(4 hunks)core/internal/testutil/account.go(1 hunks)core/providers/azure/azure.go(19 hunks)core/providers/azure/files.go(1 hunks)core/schemas/account.go(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(2 hunks)framework/configstore/tables/key.go(5 hunks)framework/configstore/tables/virtualkey.go(1 hunks)transports/bifrost-http/handlers/providers.go(1 hunks)transports/bifrost-http/lib/config.go(3 hunks)ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx(1 hunks)ui/lib/schemas/providerForm.ts(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
ui/lib/schemas/providerForm.tscore/schemas/account.gotransports/bifrost-http/handlers/providers.goframework/configstore/tables/virtualkey.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsxdocs/quickstart/go-sdk/provider-configuration.mdxcore/providers/azure/files.gotransports/bifrost-http/lib/config.gocore/providers/azure/azure.goui/lib/types/config.tsui/lib/types/schemas.tscore/go.modcore/internal/testutil/account.goframework/configstore/tables/key.goframework/configstore/migrations.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
core/schemas/account.gotransports/bifrost-http/handlers/providers.goframework/configstore/tables/virtualkey.gocore/providers/azure/files.gotransports/bifrost-http/lib/config.gocore/providers/azure/azure.gocore/internal/testutil/account.goframework/configstore/tables/key.goframework/configstore/migrations.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.go
🧬 Code graph analysis (5)
core/providers/azure/azure.go (5)
core/schemas/provider.go (1)
NetworkConfig(48-56)core/schemas/account.go (2)
Key(8-19)AzureKeyConfig(23-31)core/providers/utils/utils.go (1)
NewBifrostOperationError(733-744)core/utils.go (1)
Ptr(56-58)core/providers/azure/types.go (1)
AzureAPIVersionDefault(4-4)
ui/lib/types/config.ts (1)
core/schemas/account.go (1)
AzureKeyConfig(23-31)
core/internal/testutil/account.go (2)
core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(56-58)
framework/configstore/tables/key.go (2)
core/schemas/account.go (1)
AzureKeyConfig(23-31)ui/lib/types/config.ts (1)
AzureKeyConfig(23-31)
framework/configstore/migrations.go (2)
framework/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables/key.go (2)
TableKey(13-61)TableKey(64-64)
🪛 GitHub Actions: PR #1133
framework/configstore/tables/key.go
[error] 90-90: k.AzureKeyConfig.ClientID undefined (type *schemas.AzureKeyConfig has no field or method ClientID)
🔇 Additional comments (14)
core/go.mod (3)
5-23: Dependency additions align well with Azure Entra ID feature.The changes add the necessary Azure SDK modules (azcore, azidentity) and transitive dependencies (MSAL, JWT, crypto utilities) to support Service Principal authentication. The additions are minimal and focused. Ensure the build verification passed in the CI/CD pipeline and that all new dependencies integrate without conflicts.
Verify that:
go mod downloadandgo buildsucceed with these new dependencies.- No conflicting versions between direct and transitive dependencies exist.
- The codebase is using these new modules as expected (should be confirmed by the broader PR changes).
Also applies to: 25-70
49-49: Consider upgrading golang-jwt/jwt/v5 to the latest stable release.The golang.org/x/crypto v0.45.0 dependency correctly addresses two security vulnerabilities in the SSH package and SSH Agent package. However, a security advisory exists for the golang-jwt/jwt/v5 line regarding excessive memory allocation during header parsing. Verify that v5.3.0 includes the fix for this advisory or upgrade to the latest available v5.x release.
6-7: The Azure SDK versions specified (azcore v1.20.0 and azidentity v1.13.1) are both stable and recently released (November 2025). azcore v1.20.0 was published on November 6, 2025, and azidentity v1.13.1 was released on November 10, 2025. Both versions are stable as they have reached major version v1. No known security advisories were found for these versions. The timing of their releases (within a few days) suggests compatibility testing has been performed. These versions are appropriate for Azure Entra ID (Service Principal) authentication support.docs/quickstart/go-sdk/provider-configuration.mdx (1)
349-412: LGTM! Clear documentation for the dual authentication methods.The documentation correctly explains both authentication paths with appropriate code examples. The note at lines 410-412 clearly indicates the authentication priority logic.
framework/configstore/tables/virtualkey.go (1)
56-58: LGTM! Consistent sensitive data redaction for new Azure credentials.The new Azure credential fields (
AzureClientID,AzureClientSecret,AzureTenantID) are correctly cleared in theAfterFindhook, following the established pattern for other sensitive fields. This prevents credential leakage when querying virtual key provider configurations.core/schemas/account.go (1)
27-30: LGTM! Clean schema additions for Azure Service Principal credentials.The new fields follow established patterns: optional pointer types (
*string), proper JSON tags withomitempty, and clear documentation comments. The naming is consistent with other credential fields in the codebase.core/internal/testutil/account.go (1)
222-225: LGTM!The new Azure Service Principal credentials are correctly wired from environment variables using the established
bifrost.Ptr()pattern, consistent with how other optional fields (e.g.,SessionToken,Region) are handled elsewhere in this file.ui/lib/types/config.ts (2)
37-41: Verify default values align with backend expectations.The defaults initialize
scopeto an empty array[]. If this field is not supported by the backend, sending an empty array could cause validation issues or be silently ignored. The other fields (client_id,client_secret,tenant_id) defaulting to empty strings is consistent with optional credential fields.
27-31: Mismatch:scopefield exists in TypeScript but not in Go backend schema.The TypeScript
AzureKeyConfiginterface includes an optionalscope: string[]field, but the Go backendAzureKeyConfigstruct (core/schemas/account.go lines 23-31) does not have a correspondingScopefield. The backend internally uses a hardcodedDefaultAzureScopeconstant instead. This means the UI will send a scope value that the backend cannot deserialize or use, creating a silent data loss scenario.Either remove the
scopefield from the TypeScript interface if it's not needed, or add support for it in the Go backend schema if it's intended for future use.framework/configstore/migrations.go (1)
113-115: Migration integration looks correct.The new migration is properly placed at the end of the
triggerMigrationsfunction, maintaining the sequential migration order.transports/bifrost-http/lib/config.go (1)
3294-3305: Azure env‑var diffing for client_id / client_secret / tenant_id looks consistent.Extending
getFieldValueto surfaceclient_id,client_secret, andtenant_idforisEnvVarReferenceChangingkeeps Azure env‑tracking aligned with existing Endpoint/APIVersion handling; no issues spotted.framework/configstore/tables/key.go (1)
45-52: Bedrock JSON field rename and hooks wiring look correct.Renaming to
BedrockBatchS3ConfigJSONand usingsonic.Marshal/json.UnmarshalinBeforeSave/AfterFindis consistent with existingBedrockDeploymentsJSONhandling. No functional issues spotted.Also applies to: 151-193, 269-304
core/providers/azure/azure.go (2)
171-183: Usage of getAzureAuthHeaders across completion / list / streaming paths is consistent.All the updated call sites (
completeRequest,listModelsByKey, text/chat/responses streams,SpeechStream) now:
- Call
getAzureAuthHeaders(ctx, key, isAnthropicModel)with the correctisAnthropicModelflag.- Propagate
*schemas.BifrostErrordirectly on failure.- Apply the returned header map to the outgoing request or pass it to shared OpenAI/Anthropic streaming helpers.
This unifies auth behavior across Azure endpoints and correctly handles
Authorizationvsapi-key/x-api-keybased on model type.Also applies to: 256-263, 408-412, 549-552, 596-599, 736-739, 769-772, 915-918
1308-1310: File and batch APIs now return Bifrost errors from setAzureAuth; behavior is coherent.The various file/batch methods (
FileUpload,FileList,FileRetrieve,FileDelete,FileContent,BatchCreate,BatchList,BatchRetrieve,BatchCancel) now:
- Use
setAzureAuth(ctx, req, key)and propagate*schemas.BifrostErroron failure.- Continue to build URLs and handle responses as before.
This keeps auth handling for these endpoints aligned with the new service principal / context token / API key priority without changing their external behavior.
Also applies to: 1415-1417, 1521-1526, 1617-1622, 1744-1749, 1865-1867, 1986-1988, 2087-2092, 2185-2190
TejasGhatte
force-pushed
the
12-16-feat_support_client_id_and_client_secret_in_azure
branch
from
d8e54ff to
3dd5373
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
214-216: Minor grammar issue in alert text.The word "both" implies two items, but three fields are listed (Client ID, Client Secret, and Tenant ID).
🔎 Suggested fix
<AlertDescription> - Leave both Client ID, Client Secret and Tenant ID empty to use Azure Entra ID attached to your environment. + Leave Client ID, Client Secret, and Tenant ID empty to use Azure Entra ID attached to your environment. </AlertDescription>ui/lib/types/schemas.ts (1)
21-24: Scope field in TypeScript schema is not reflected in Go backend or UI form.The
scopefield (z.array(z.string()).optional()) exists here but is missing from:
- Go
AzureKeyConfigstruct incore/schemas/account.go- UI form fields in
apiKeysFormFragment.tsxThe Azure provider uses a hardcoded default scope (
https://cognitiveservices.azure.com/.default). If this field is reserved for future customization, add a comment. Otherwise, remove it to maintain consistency.
🧹 Nitpick comments (7)
transports/bifrost-http/handlers/providers.go (1)
753-792: Azure SP field merge behavior looks correct; optional small tidy‑up.The preservation logic for
ClientID,ClientSecret, andTenantIDwhen the UI sends back redacted/env values matches the existing pattern forValueandAPIVersion, and should correctly keep raw creds on no‑change updates. The extraoldRawKey.AzureKeyConfig != nilchecks inside each field block are redundant given the outer guard, but that’s cosmetic and can be left as‑is or simplified later.docs/quickstart/go-sdk/provider-configuration.mdx (1)
349-413: Azure auth docs align with implementation; consider aligning API version with default (optional).The Service Principal vs direct key flow and env‑var usage look consistent with the backend changes. If you want to avoid confusion later, you might update the example APIVersion to match the default noted in
schemas.AzureKeyConfig’s comment, but this is purely a docs polish.transports/bifrost-http/lib/config.go (2)
3294-3305: getFieldValue Azure extensions correctly support env‑key cleanup.Adding
client_id,client_secret, andtenant_idhere allowsisEnvVarReferenceChangingto treat the new Azure fields like the existing endpoint/api_version/Vertex/Bedrock fields, so env‑key entries are cleaned up when these references change. Logic and field names line up with the config paths set inprocessAzureKeyConfigEnvVars.
3439-3495: Env‑var processing for Azure SP fields is correct; only minor duplication nit.
processAzureKeyConfigEnvVarsnow processesClientID,ClientSecret, andTenantIDviaprocessEnvValueand tracks them as"azure_config"EnvKeyInfos with preciseConfigPaths, which is whatGetProviderConfigRedactedand cleanup helpers expect. The double assignment toazureConfig.ClientSecretis harmless but could be collapsed to a single assignment after the env‑tracking block for clarity.framework/configstore/migrations.go (1)
1858-1870: Rollback doesn't guard column existence before dropping.The
Migratefunction checksHasColumnbefore adding, but theRollbackunconditionally drops columns. If migration partially failed (e.g., onlyazure_client_idwas added), rolling back will error on non-existent columns. Consider adding guards for consistency with the migrate path.🔎 Suggested improvement
Rollback: func(tx *gorm.DB) error { tx = tx.WithContext(ctx) migrator := tx.Migrator() - if err := migrator.DropColumn(&tables.TableKey{}, "azure_client_id"); err != nil { - return fmt.Errorf("failed to drop azure_client_id column: %w", err) + if migrator.HasColumn(&tables.TableKey{}, "azure_client_id") { + if err := migrator.DropColumn(&tables.TableKey{}, "azure_client_id"); err != nil { + return fmt.Errorf("failed to drop azure_client_id column: %w", err) + } } - if err := migrator.DropColumn(&tables.TableKey{}, "azure_client_secret"); err != nil { - return fmt.Errorf("failed to drop azure_client_secret column: %w", err) + if migrator.HasColumn(&tables.TableKey{}, "azure_client_secret") { + if err := migrator.DropColumn(&tables.TableKey{}, "azure_client_secret"); err != nil { + return fmt.Errorf("failed to drop azure_client_secret column: %w", err) + } } - if err := migrator.DropColumn(&tables.TableKey{}, "azure_tenant_id"); err != nil { - return fmt.Errorf("failed to drop azure_tenant_id column: %w", err) + if migrator.HasColumn(&tables.TableKey{}, "azure_tenant_id") { + if err := migrator.DropColumn(&tables.TableKey{}, "azure_tenant_id"); err != nil { + return fmt.Errorf("failed to drop azure_tenant_id column: %w", err) + } } return nil },ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
231-243: Consider usingtype="password"for the Client Secret field.The Client Secret is a sensitive credential. Using
type="password"would mask the input, consistent with security best practices and similar to how API keys are typically handled in forms.🔎 Suggested fix
<FormField control={control} name={`key.azure_key_config.client_secret`} render={({ field }) => ( <FormItem> <FormLabel>Client Secret (Optional)</FormLabel> <FormControl> - <Input placeholder="your-client-secret or env.AZURE_CLIENT_SECRET" {...field} value={field.value ?? ""} /> + <Input type="password" placeholder="your-client-secret or env.AZURE_CLIENT_SECRET" {...field} value={field.value ?? ""} /> </FormControl> <FormMessage /> </FormItem> )} />core/providers/azure/azure.go (1)
46-69: Credential caching doesn't account for secret rotation.The cache key is
tenantID:clientID, so if a user rotatesclientSecretwithout changing the other values, the stale credential remains cached indefinitely. Consider either:
- Including a hash of
clientSecretin the cache key- Adding a TTL or cache invalidation mechanism
- Documenting that service restart is required after secret rotation
🔎 Option 1: Include secret in cache key
func (p *AzureProvider) getOrCreateAuth( tenantID, clientID, clientSecret string, ) (azcore.TokenCredential, error) { - key := tenantID + ":" + clientID + key := tenantID + ":" + clientID + ":" + clientSecret // Fast path if val, ok := p.credentials.Load(key); ok { return val.(azcore.TokenCredential), nil }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
core/go.mod(4 hunks)core/internal/testutil/account.go(1 hunks)core/providers/azure/azure.go(19 hunks)core/providers/azure/files.go(1 hunks)core/schemas/account.go(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(2 hunks)framework/configstore/tables/key.go(5 hunks)framework/configstore/tables/virtualkey.go(1 hunks)transports/bifrost-http/handlers/providers.go(1 hunks)transports/bifrost-http/lib/config.go(3 hunks)ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx(1 hunks)ui/lib/schemas/providerForm.ts(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- core/schemas/account.go
- ui/lib/schemas/providerForm.ts
- ui/lib/types/config.ts
- core/go.mod
- framework/configstore/tables/virtualkey.go
- core/internal/testutil/account.go
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/providers.goframework/configstore/tables/key.goframework/configstore/migrations.gocore/providers/azure/azure.goui/lib/types/schemas.tsdocs/quickstart/go-sdk/provider-configuration.mdxcore/providers/azure/files.goui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/providers.goframework/configstore/tables/key.goframework/configstore/migrations.gocore/providers/azure/azure.gocore/providers/azure/files.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/lib/config.gotransports/bifrost-http/handlers/providers.go
🧬 Code graph analysis (6)
transports/bifrost-http/lib/config.go (4)
core/schemas/account.go (1)
AzureKeyConfig(23-31)core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(56-58)framework/configstore/clientconfig.go (1)
EnvKeyInfo(26-32)
transports/bifrost-http/handlers/providers.go (3)
core/schemas/account.go (1)
AzureKeyConfig(23-31)ui/lib/types/config.ts (1)
AzureKeyConfig(23-30)transports/bifrost-http/lib/config.go (1)
IsRedacted(3030-3053)
framework/configstore/tables/key.go (2)
core/schemas/account.go (1)
AzureKeyConfig(23-31)ui/lib/types/config.ts (1)
AzureKeyConfig(23-30)
framework/configstore/migrations.go (2)
framework/migrator/migrator.go (3)
New(131-149)DefaultOptions(100-106)Migration(62-69)framework/configstore/tables/key.go (2)
TableKey(13-61)TableKey(64-64)
core/providers/azure/azure.go (5)
core/schemas/provider.go (1)
NetworkConfig(48-56)core/schemas/account.go (2)
Key(8-19)AzureKeyConfig(23-31)core/providers/utils/utils.go (1)
NewBifrostOperationError(733-744)core/schemas/utils.go (2)
IsAnthropicModel(1048-1050)Ptr(16-18)core/utils.go (1)
Ptr(56-58)
core/providers/azure/files.go (5)
core/providers/azure/azure.go (2)
DefaultAzureScope(33-33)AzureAuthorizationTokenKey(30-30)core/schemas/account.go (2)
Key(8-19)AzureKeyConfig(23-31)ui/lib/types/config.ts (1)
AzureKeyConfig(23-30)core/providers/utils/utils.go (1)
NewBifrostOperationError(733-744)core/schemas/bifrost.go (1)
Azure(36-36)
🪛 GitHub Actions: PR #1133
framework/configstore/tables/key.go
[error] 90-90: AzureKeyConfig: 'ClientID' is not a field of *schemas.AzureKeyConfig. (undefined field or method 'ClientID')
🔇 Additional comments (13)
transports/bifrost-http/lib/config.go (1)
2169-2196: Azure credential redaction behavior is solid and consistent.The redaction for
ClientID,ClientSecret, andTenantIDmirrors the existing pattern: env‑backed values becomeenv.VAR_NAME, and non‑env secrets are masked withRedactKey. This closes the earlier clear‑textClientSecretgap and keeps all three fields out of API responses while still preserving editability via redacted markers.framework/configstore/tables/key.go (4)
29-36: TableKey Azure credential columns are well‑shaped and match schema.The added
AzureClientID,AzureClientSecret, andAzureTenantIDfields give a clear, 1:1 mapping toschemas.AzureKeyConfigand use appropriate DB types (text for secrets, varchar for IDs). This is consistent with how Vertex/Bedrock fields are modeled.
45-51: Bedrock JSON column naming/usage is coherent.The explicit
BedrockBatchS3ConfigJSONcolumn alongsideBedrockDeploymentsJSONkeeps JSON blobs isolated and matches how you marshal/unmarshal these structures later inBeforeSave/AfterFind. No issues here.
83-110: Azure BeforeSave mapping correctly persists new SP fields.When
AzureKeyConfigis present, all three SP fields are copied to their dedicated DB columns and fully cleared when the config is nil, which keeps the row state consistent with the virtual struct. This matches the existing endpoint/api_version/deployments handling.
211-219: Azure AfterFind reconstruction matches the stored schema and avoids leaks.Rebuilding
schemas.AzureKeyConfigfromAzureEndpoint,AzureAPIVersion, and the newAzureClient*/AzureTenantIDcolumns is straightforward and symmetrical withBeforeSave. Since redaction happens inGetProviderConfigRedacted, returning raw values here is appropriate for internal use.framework/configstore/migrations.go (1)
1833-1874: Migration implementation looks good.The migration follows the established patterns in the codebase: proper migration ID,
HasColumnguards on the migrate path, context propagation, and error wrapping.ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx (1)
209-256: New Azure Entra ID authentication UI section looks well-structured.The form fields are properly wired to the schema, follow the existing patterns in the file, and provide clear guidance to users through the Alert component.
ui/lib/types/schemas.ts (1)
21-23: Client ID, Client Secret, and Tenant ID fields are correctly added.These three fields align with the Go backend schema and the UI form implementation.
core/providers/azure/files.go (1)
15-56: Authentication implementation follows correct priority and handles all cases.The three-tier authentication priority (Service Principal → Context token → API key) is implemented correctly with proper error handling. The header management (setting Authorization and removing api-key, or vice versa) ensures clean authentication state.
core/providers/azure/azure.go (4)
71-114: Centralized authentication header logic is well-designed.The
getAzureAuthHeadersfunction cleanly encapsulates the three-tier authentication priority and correctly handles the header differences between Anthropic models (x-api-key) and standard Azure OpenAI models (api-key).
170-182: Auth header application in completeRequest is correct.Headers are retrieved via the centralized method and properly applied to the request before sending.
549-599: Streaming methods consistently use centralized auth.Both Anthropic and non-Anthropic code paths correctly call
getAzureAuthHeaderswith the appropriateisAnthropicModelflag.
1308-1310: File and batch operations correctly handle auth errors.The
setAzureAuthreturn value is now checked at all call sites, and errors are properly propagated or accumulated in the retry loops.Also applies to: 1415-1417, 1521-1526, 1617-1622, 1744-1749, 1865-1867, 1986-1988, 2087-2092, 2185-2190
TejasGhatte
force-pushed
the
12-16-feat_support_client_id_and_client_secret_in_azure
branch
from
3dd5373 to
be1b879
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
transports/bifrost-http/lib/config.go (1)
3294-3305: Env-var handling for new Azure fields is wired correctly (minor cleanup possible)
getFieldValueandprocessAzureKeyConfigEnvVarsnow understandclient_id,client_secret, andtenant_id, so env-key tracking and cleanup work for these fields just like for endpoint/api_version and other provider configs. The pointer checks andprocessEnvValueusage are safe.One small nit:
azureConfig.ClientSecretis assigned&processedClientSecrettwice in theclient_secretblock; you can drop the earlier assignment without changing behavior to keep the function slightly cleaner.Also applies to: 3440-3495
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
core/go.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
core/go.mod(4 hunks)core/internal/testutil/account.go(1 hunks)core/providers/azure/azure.go(19 hunks)core/providers/azure/files.go(1 hunks)core/schemas/account.go(1 hunks)docs/quickstart/go-sdk/provider-configuration.mdx(2 hunks)framework/configstore/migrations.go(2 hunks)framework/configstore/tables/key.go(5 hunks)framework/configstore/tables/virtualkey.go(1 hunks)transports/bifrost-http/handlers/providers.go(1 hunks)transports/bifrost-http/lib/config.go(3 hunks)ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx(1 hunks)ui/lib/schemas/providerForm.ts(1 hunks)ui/lib/types/config.ts(1 hunks)ui/lib/types/schemas.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- core/internal/testutil/account.go
- ui/lib/types/config.ts
- core/go.mod
- ui/lib/types/schemas.ts
- framework/configstore/migrations.go
- framework/configstore/tables/virtualkey.go
- ui/lib/schemas/providerForm.ts
- ui/app/workspace/providers/fragments/apiKeysFormFragment.tsx
- docs/quickstart/go-sdk/provider-configuration.mdx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
always check the stack if there is one for the current PR. do not give localized reviews for the PR, always see all changes in the light of the whole stack of PRs (if there is a stack, if there is no stack you can continue to make localized suggestions/reviews)
Files:
transports/bifrost-http/handlers/providers.gocore/providers/azure/files.gocore/schemas/account.gotransports/bifrost-http/lib/config.goframework/configstore/tables/key.gocore/providers/azure/azure.go
🧠 Learnings (2)
📚 Learning: 2025-12-09T17:07:42.007Z
Learnt from: qwerty-dvorak
Repo: maximhq/bifrost PR: 1006
File: core/schemas/account.go:9-18
Timestamp: 2025-12-09T17:07:42.007Z
Learning: In core/schemas/account.go, the HuggingFaceKeyConfig field within the Key struct is currently unused and reserved for future Hugging Face inference endpoint deployments. Do not flag this field as missing from OpenAPI documentation or require its presence in the API spec until the feature is actively implemented and used. When the feature is added, update the OpenAPI docs accordingly; otherwise, treat this field as non-breaking and not part of the current API surface.
Applied to files:
transports/bifrost-http/handlers/providers.gocore/providers/azure/files.gocore/schemas/account.gotransports/bifrost-http/lib/config.goframework/configstore/tables/key.gocore/providers/azure/azure.go
📚 Learning: 2025-12-12T08:25:02.629Z
Learnt from: Pratham-Mishra04
Repo: maximhq/bifrost PR: 1000
File: transports/bifrost-http/integrations/router.go:709-712
Timestamp: 2025-12-12T08:25:02.629Z
Learning: In transports/bifrost-http/**/*.go, update streaming response handling to align with OpenAI Responses API: use typed SSE events such as response.created, response.output_text.delta, response.done, etc., and do not rely on the legacy data: [DONE] termination marker. Note that data: [DONE] is only used by the older Chat Completions and Text Completions streaming APIs. Ensure parsers, writers, and tests distinguish SSE events from the [DONE] sentinel and handle each event type accordingly for correct stream termination and progress updates.
Applied to files:
transports/bifrost-http/handlers/providers.gotransports/bifrost-http/lib/config.go
🧬 Code graph analysis (4)
transports/bifrost-http/handlers/providers.go (3)
core/schemas/account.go (1)
AzureKeyConfig(23-31)ui/lib/types/config.ts (1)
AzureKeyConfig(23-30)transports/bifrost-http/lib/config.go (1)
IsRedacted(3030-3053)
transports/bifrost-http/lib/config.go (5)
core/schemas/account.go (1)
AzureKeyConfig(23-31)ui/lib/types/config.ts (1)
AzureKeyConfig(23-30)core/schemas/utils.go (1)
Ptr(16-18)core/utils.go (1)
Ptr(56-58)framework/configstore/clientconfig.go (1)
EnvKeyInfo(26-32)
framework/configstore/tables/key.go (2)
core/schemas/account.go (1)
AzureKeyConfig(23-31)ui/lib/types/config.ts (1)
AzureKeyConfig(23-30)
core/providers/azure/azure.go (6)
core/schemas/account.go (2)
Key(8-19)AzureKeyConfig(23-31)core/schemas/bifrost.go (2)
BifrostError(462-471)Azure(36-36)core/providers/utils/utils.go (1)
NewBifrostOperationError(733-744)core/schemas/utils.go (2)
IsAnthropicModel(1048-1050)Ptr(16-18)core/utils.go (1)
Ptr(56-58)core/providers/azure/types.go (1)
AzureAPIVersionDefault(4-4)
🪛 GitHub Actions: PR #1133
framework/configstore/tables/key.go
[error] 90-90: AzureKeyConfig: undefined: ClientID (type *schemas.AzureKeyConfig has no field or method ClientID).
🔇 Additional comments (9)
transports/bifrost-http/handlers/providers.go (1)
767-791: AzureKeyConfig redaction merge for ClientID/ClientSecret/TenantID is consistent and safeThe new preserve-on-redaction logic for
client_id,client_secret, andtenant_idaligns with the existing Azureapi_versionhandling: pointer checks are correct, comparisons use the redacted value, and restoring fromoldRawKey.AzureKeyConfigavoids losing the original secrets when the UI sends masked values. I don’t see correctness or safety issues here.core/schemas/account.go (1)
28-30: AzureKeyConfig schema extensions look correct
ClientID,ClientSecret, andTenantIDare added as optional pointers with the expected JSON field names and comments, matching the rest of the stack (DB, config, UI). This will serialize cleanly and supports redaction.framework/configstore/tables/key.go (1)
33-35: DB wiring for Azure and Bedrock key configs is consistent and symmetricThe added Azure columns (
AzureClientID,AzureClientSecret,AzureTenantID) and Bedrock JSON columns are correctly mapped:
BeforeSavecopies from the in-memory config into the DB fields with appropriate nil checks.AfterFindreconstructsAzureKeyConfigandBedrockKeyConfigonly when relevant columns are populated, mirroring the existing endpoint/api_version/deployments patterns.This keeps the table representation and schema structs in sync without obvious nil or marshaling pitfalls.
Also applies to: 45-52, 83-110, 151-193, 211-219
transports/bifrost-http/lib/config.go (1)
2169-2196: Azure credential redaction now properly covers ClientID/ClientSecret/TenantIDThe Azure redaction block correctly:
- Emits
env.VARwhen an env mapping exists forclient_id,client_secret, ortenant_id.- Otherwise masks values via
RedactKey, so raw client secrets and IDs never leave the backend.This brings Azure into line with Vertex/Bedrock redaction behavior and fixes the earlier clear‑text
ClientSecretexposure.core/providers/azure/files.go (1)
8-13: setAzureAuth correctly implements prioritized Azure auth with error propagationThis helper now:
- Prefers service principal credentials via
getOrCreateAuthandGetToken(DefaultAzureScope), falling back to a context token and then toapi-key.- Returns a
*schemas.BifrostErroron failures rather than silently mutating headers, letting callers abort file/batch flows cleanly.The header mutations (mutually exclusive
Authorizationvsapi-key) and nil-token check look sound.Please run an end‑to‑end test against Azure with: (1) only client_id/client_secret/tenant_id set, (2) only context token set, and (3) only api key set, to confirm the three paths behave as expected for file/batch APIs.
Also applies to: 15-56
core/providers/azure/azure.go (4)
14-20: Centralized Azure service principal support and credential caching look solidThe additions of
DefaultAzureScope, thecredentials sync.Map,getOrCreateAuth, andgetAzureAuthHeaderscleanly encapsulate Azure auth:
- Credentials are cached per
tenantID:clientID, avoiding redundantNewClientSecretCredentialcalls.getAzureAuthHeadersuniformly enforces the priority order (service principal → context token → API key) and returns typed*BifrostErroron failure.This is a good foundation for all Azure paths to share.
It would be worth validating against the Azure docs for client‑credentials auth to confirm
DefaultAzureScopeandClientSecretCredentialare the right choices for all supported Azure OpenAI endpoints.Also applies to: 32-44, 46-114
170-183: completeRequest and listModelsByKey now align with centralized authBoth
completeRequestandlistModelsByKeynow:
- Compute
isAnthropicModelwhere relevant and obtain headers fromgetAzureAuthHeaders, so service principal / context / API key flows are shared.- Apply the returned header map directly to
fasthttp.Requestbefore building the Azure URL.This removes inlined auth logic and keeps the surface consistent across text/chat/embedding and model-list operations.
Also applies to: 256-263
408-413: Streaming endpoints correctly adopt getAzureAuthHeaders
TextCompletionStream,ChatCompletionStream,ResponsesStream, andSpeechStreamnow all callgetAzureAuthHeaders:
- Anthropic paths pass
isAnthropicModel=trueso API-key fallback usesx-api-key, then addanthropic-version.- OpenAI-compatible Azure paths pass
falseand getapi-keyorAuthorizationas appropriate.- Each function short‑circuits and returns the
*BifrostErrorfromgetAzureAuthHeaderson auth failure.This keeps streaming auth behavior in sync with non‑streaming calls and with the service principal priority rules.
Also applies to: 548-552, 596-599, 736-739, 769-772, 914-918
1308-1310: File and batch APIs now surface Azure auth failures explicitlyAll file/batch methods (
FileUpload/List/Retrieve/Delete/ContentandBatchCreate/List/Retrieve/Cancel) now:
- Call the updated
setAzureAuthand check its*schemas.BifrostErrorreturn.- Abort early on auth failures (including token acquisition problems) instead of sending a malformed unauthenticated request.
The resource cleanup paths (releasing requests/responses) remain correct around these new checks.
Also applies to: 1415-1417, 1521-1526, 1617-1622, 1744-1749, 1865-1867, 1986-1988, 2087-2092, 2185-2190
Summary
Added Azure Entra ID (Service Principal) authentication support to the Azure provider, allowing for more secure token-based authentication with automatic token management.
Changes
ClientID,ClientSecret, andTenantIDcredentialsType of change
Affected areas
How to test
go test ./providers/azure/...Breaking changes
Related issues
Enhances Azure authentication options for enterprise deployments.
Security considerations
Checklist