feat: byok-observability for aibridge by evgeniy-scherbina · Pull Request #239 · coder/aibridge

evgeniy-scherbina · 2026-03-30T15:54:32Z

Summary

Records credential metadata on each AI Bridge interception, enabling admins to see how requests were authenticated and identify which credential was used.

Each provider's CreateInterceptor now captures two new fields on the interceptor:

credential_kind: centralized, personal_api_key, or subscription
credential_hint: masked credential for audit (e.g. sk-a...(13)...efgh)

Changes

Extended Interceptor interface with SetCredential, CredentialKind, and CredentialHint, backed by an embeddable CredentialFields helper
Each provider sets credential metadata in CreateInterceptor:
- OpenAI: centralized (admin key) or personal (Bearer token)
- Anthropic: centralized, personal_api_key (X-Api-Key), or subscription (Bearer token)
- Copilot: always subscription
Improved MaskSecret to embed hidden character count (e.g. sk-a...(13)...efgh instead of sk-a*************efgh)

Relates to coder/coder#23808

intercept/credential.go

provider/anthropic.go

evgeniy-scherbina · 2026-03-31T19:27:38Z

provider/copilot.go

 		return nil, UnknownRoute
 	}

+	interceptor.SetCredential(intercept.CredentialKindSubscription, utils.MaskSecret(key))


CredentialKindSubscription makes sense for Copilot?

Was also thinking the same 🤔 I think subscription is not a really generic term and doesn't quite apply to copilot. Since for the header X-API-Key we use the term api_key, why not something like auth_token?

I think subscription is widely used term, e.g.:

https://docs.litellm.ai/docs/tutorials/claude_code_max_subscription

https://docs.litellm.ai/docs/providers/chatgpt

I heavily use it documentation PR: docs: add byok docs for aibridge coder#23922

Copilot also uses subscription term, see: https://github.com/settings/copilot/features

You currently have an active Copilot Pro subscription.

Now I started to think it's probably fine to use this term?

auth_token sounds good, but I think not everyone can link auth_token to Claude Pro/Max or ChatGPT Pro/Plus subscriptions. So it maybe a bit confusing for auditors?

I wouldn't rely on headers, X-API-Key is used by Anthropic. But OpenAI uses Authorization for everything.

evgeniy-scherbina · 2026-03-31T19:37:36Z

provider/openai.go

+	credKind := intercept.CredentialKindCentralized
 	if token := utils.ExtractBearerToken(r.Header.Get("Authorization")); token != "" {
 		cfg.Key = token
+		credKind = intercept.CredentialKindPersonalAPIKey


This is a bit tricky, it can be either CredentialKindPersonalAPIKey or CredentialKindSubscription.
We can rely on some hacks to determine it:

Presence of Chatgpt-Account-Id header

Check is it JWT Token or API KEY

Rely on URL (openai vs chatgpt)

See more details here: #227 (comment)

Alternatively we can introduce new enum value, smth like: CredentialKindPersonal

Why not make the modes header specific? If the Authorization header is used use mode auth_token, otherweise, if the X-API-Key header is used, use mode api_key? This would mean that for openai (default) and openai chatgpt both modes would be auth_token.

I think there are few problems with that:

I think headers are implementation details, auditors don't really know/care what headers are used?

Different headers are used by different providers. E.g. OpenAI doesn't use X-API-Key.

I think we want to differentiate between Personal API Key and ChatGPT Subscription for OpenAI provider?

@dannykopping The question is do we want to differentiate between Personal API Key and ChatGPT Subscription for OpenAI.

If the answer is yes - I think we should rely on Chatgpt-Account-Id header. That's what I recommend.
if the answer is no - we can either introduce new CredentialKindPersonal type which encompasses both:

CredentialKindPersonalAPIKey

CredentialKindSubscription

or go with Susana's approach (though I think it's less informative/obvious for users).

utils/mask.go

intercept/credential.go

intercept/interceptor.go

ssncferreira · 2026-04-01T10:52:14Z

provider/copilot.go

 		return nil, UnknownRoute
 	}

+	interceptor.SetCredential(intercept.CredentialKindSubscription, utils.MaskSecret(key))


Was also thinking the same 🤔 I think subscription is not a really generic term and doesn't quite apply to copilot. Since for the header X-API-Key we use the term api_key, why not something like auth_token?

ssncferreira · 2026-04-01T10:54:44Z

provider/openai.go

+	credKind := intercept.CredentialKindCentralized
 	if token := utils.ExtractBearerToken(r.Header.Get("Authorization")); token != "" {
 		cfg.Key = token
+		credKind = intercept.CredentialKindPersonalAPIKey


Why not make the modes header specific? If the Authorization header is used use mode auth_token, otherweise, if the X-API-Key header is used, use mode api_key? This would mean that for openai (default) and openai chatgpt both modes would be auth_token.

ssncferreira · 2026-04-01T10:58:10Z

provider/openai.go

 	//
 	// In BYOK mode the user's credential is in Authorization. Replace
 	// the centralized key with it so it is forwarded upstream.
+	credKind := intercept.CredentialKindCentralized


Isn't setting credHint missing here? Also, we should probably check if cfg.key is empty (chatgpt case), and error if the credential key is empty.

Isn't setting credHint missing here?

Actually - no, it's later used like this:

interceptor.SetCredential(credKind, utils.MaskSecret(cfg.Key))

It's easier for openai case, because authz header is always used.

if cfg.key is empty (chatgpt case), and error if the credential key is empty

Yeah, I remember about this. I can add in this PR.

@ssncferreira

if cfg.key is empty (chatgpt case), and error if the credential key is empty

We already know that claude (maybe other clients) send HEAD requests without authentication, maybe we don't need it?

Also I think it's not specific to chatgpt case.

ssncferreira · 2026-04-01T11:00:29Z

utils/mask.go

 	// If we'd reveal everything or more, mask it all.
 	if reveal*2 >= len(runes) {
-		return strings.Repeat("*", len(runes))
+		return fmt.Sprintf("...(%d)...", len(runes))


What is the benefit of doing this? This way we are explicitly saying the length of the key...which I think could be problematic if this is someway leaked?

It also adds confusion because the ... prefix and suffix don't actually count towards the length.
If we don't want to leak the length (which could be used by attackers which intercept these masked secrets to know how many chars to spoof to recreate the secret) then I'd suggest we just go with a fixed number of * chars, and not use ..

It's related to this discussion: #216 (comment)

I remember you wanted "Preserve the length" of the secret, and I'm fine with it.
But the issue with current approach is tokens can be very long, let's say 300 chars.
Do we really want to log and potentially store in Postgres 300 * chars? Feels like a waste of space?
Feels like current approach is most ineffective in storing length of the secret as you can imagine?

Clear > clever.

This current approach is not clear. ...(len)... leaves itself open to different interpretations.
Since this change isn't the focus of the PR, I'd suggest reverting it for now and opening an issue to address the size concerns.

I'd like to show the secret length, because it's diagnostically useful, but it has to be done in a way that is obvious.

Okay, I reverted it.

I’m fine with any format as long as it’s efficient, especially if we plan to store it in Postgres.

Another potential issue is the UI. If we decide to display a secret hint there, as Anthropic and OpenAI do, we’ll probably want to show a compact version anyway. That creates a slightly awkward implementation: we would log and store the long version of a hint, then shorten it at the API or frontend layer before displaying it.

Maybe good option is to have two fields:

secret_hint (short version)

secret_length

bridge.go

ssncferreira · 2026-04-01T11:17:57Z

I think we are missing adding the credential info to the logs, especially for error cases like 401/403/429

intercept/credential.go

intercept/interceptor.go

provider/anthropic.go

dannykopping · 2026-04-01T12:00:18Z

utils/mask.go

 	// If we'd reveal everything or more, mask it all.
 	if reveal*2 >= len(runes) {
-		return strings.Repeat("*", len(runes))
+		return fmt.Sprintf("...(%d)...", len(runes))


It also adds confusion because the ... prefix and suffix don't actually count towards the length.
If we don't want to leak the length (which could be used by attackers which intercept these masked secrets to know how many chars to spoof to recreate the secret) then I'd suggest we just go with a fixed number of * chars, and not use ..

dannykopping · 2026-04-01T12:01:09Z

utils/mask_test.go

-		{"long_api_key", "sk-ant-api03-abcdefgh", "sk-a*************efgh"},
-		{"unicode", "hélloworld🌍!", "hé********🌍!"},
-		{"github_token", "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefgh", "ghp_******************************efgh"},
+		{"short", "short", "...(5)..."},


This is pointless, TBH. If we're masking the full string then that defeats the original purpose of being able to identify them.

@dannykopping Logic didn't change? Previously we also masked strings if their length <= 10.
Previously it just looked like: ***** instead of ...(5)...

I must've missed that in the previous review.
Related to https://github.com/coder/aibridge/pull/239/changes#r3031158013, we can address this in a follow-up.

I reverted this change.

Co-authored-by: Susana Ferreira <susana@coder.com>

dannykopping · 2026-04-03T02:24:02Z

provider/anthropic_test.go


+			cred := interceptor.Credential()
+			assert.Equal(t, tc.wantCredentialKind, cred.Kind, "credential kind mismatch")
+			assert.NotEmpty(t, cred.Hint, "credential hint should not be empty")


This feels like a loose assertion here; NotEmpty doesn't tell us much.
Since our masking is deterministic, you could add the expected hint as the masked value and assert on that.

agree, done

dannykopping · 2026-04-03T02:29:10Z

utils/mask.go

 	// If we'd reveal everything or more, mask it all.
 	if reveal*2 >= len(runes) {
-		return strings.Repeat("*", len(runes))
+		return fmt.Sprintf("...(%d)...", len(runes))


Clear > clever.

This current approach is not clear. ...(len)... leaves itself open to different interpretations.
Since this change isn't the focus of the PR, I'd suggest reverting it for now and opening an issue to address the size concerns.

I'd like to show the secret length, because it's diagnostically useful, but it has to be done in a way that is obvious.

dannykopping · 2026-04-03T02:29:41Z

utils/mask_test.go

-		{"long_api_key", "sk-ant-api03-abcdefgh", "sk-a*************efgh"},
-		{"unicode", "hélloworld🌍!", "hé********🌍!"},
-		{"github_token", "ghp_ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefgh", "ghp_******************************efgh"},
+		{"short", "short", "...(5)..."},


I must've missed that in the previous review.
Related to https://github.com/coder/aibridge/pull/239/changes#r3031158013, we can address this in a follow-up.

evgeniy-scherbina · 2026-04-03T20:37:16Z

@ssncferreira

I think we are missing adding the credential info to the logs, especially for error cases like 401/403/429

I extended logger with credential info in newInterceptionProcessor where we call interceptor.ProcessRequest(rw, r).
It should capture all cases you mentioned.

evgeniy-scherbina commented Mar 30, 2026

View reviewed changes

intercept/credential.go Outdated Show resolved Hide resolved

evgeniy-scherbina commented Mar 30, 2026

View reviewed changes

provider/anthropic.go Outdated Show resolved Hide resolved

evgeniy-scherbina commented Mar 30, 2026

View reviewed changes

provider/anthropic.go Show resolved Hide resolved

evgeniy-scherbina force-pushed the yevhenii/byok-observability branch from 9bf93dd to c5dd566 Compare March 30, 2026 22:37

evgeniy-scherbina changed the base branch from yevhenii/aibridge-chatgpt-provider to ssncf/feat-validate-provider-names March 30, 2026 22:38

ssncferreira force-pushed the ssncf/feat-validate-provider-names branch 2 times, most recently from 71c69d3 to 30d306a Compare March 31, 2026 10:48

evgeniy-scherbina force-pushed the yevhenii/byok-observability branch from c5dd566 to d6e95ea Compare March 31, 2026 14:00

ssncferreira force-pushed the ssncf/feat-validate-provider-names branch from 7d0c198 to b1c6f3f Compare March 31, 2026 14:36

evgeniy-scherbina force-pushed the yevhenii/byok-observability branch from d6e95ea to b6a01e9 Compare March 31, 2026 16:47

evgeniy-scherbina changed the base branch from ssncf/feat-validate-provider-names to main March 31, 2026 16:47

evgeniy-scherbina force-pushed the yevhenii/byok-observability branch from e1e15df to b6a01e9 Compare March 31, 2026 17:38

evgeniy-scherbina commented Mar 31, 2026

View reviewed changes

provider/anthropic.go Outdated Show resolved Hide resolved

evgeniy-scherbina force-pushed the yevhenii/byok-observability branch from a610090 to d3e5994 Compare March 31, 2026 19:20

evgeniy-scherbina commented Mar 31, 2026

View reviewed changes

utils/mask.go Outdated Show resolved Hide resolved

feat: byok-observability for aibridge

ec6ae45

evgeniy-scherbina force-pushed the yevhenii/byok-observability branch from d3e5994 to ec6ae45 Compare March 31, 2026 19:59

evgeniy-scherbina mentioned this pull request Mar 31, 2026

feat: byok-observability for aibridge coder/coder#23808

Open

evgeniy-scherbina marked this pull request as ready for review March 31, 2026 23:36

ssncferreira reviewed Apr 1, 2026

View reviewed changes

ssncferreira requested a review from dannykopping April 1, 2026 11:02

dannykopping reviewed Apr 1, 2026

View reviewed changes

evgeniy-scherbina and others added 5 commits April 2, 2026 19:25

refactor: re-org credential configuring

8d7a6b6

Update intercept/credential.go

be0295d

Co-authored-by: Susana Ferreira <susana@coder.com>

refactor: minor refactor

08c98cb

refactor: make secret is always called

11709db

refactor: add CredentialKind custom type

74503bd

test: enhance tests

a2e538f

dannykopping reviewed Apr 3, 2026

View reviewed changes

evgeniy-scherbina added 5 commits April 3, 2026 14:05

test: enhance tests

c963324

ci: make fmt

3f63615

fix: revert MaskSecret changes

d6594a2

fix: add small comment to enum

43ee727

feat: log credential info on interception errors

9faf167

evgeniy-scherbina mentioned this pull request Apr 3, 2026

aibridge: Implement Bring Your Own Keys (BYOK) coder/internal#1401

Open

11 tasks

Conversation

evgeniy-scherbina commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

evgeniy-scherbina Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeniy-scherbina Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeniy-scherbina Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeniy-scherbina Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ssncferreira commented Apr 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evgeniy-scherbina commented Mar 30, 2026 •

edited

Loading

evgeniy-scherbina Mar 31, 2026 •

edited

Loading

evgeniy-scherbina Apr 1, 2026 •

edited

Loading

evgeniy-scherbina Apr 3, 2026 •

edited

Loading

evgeniy-scherbina Apr 2, 2026 •

edited

Loading

evgeniy-scherbina commented Apr 3, 2026 •

edited

Loading