Add app auth config sessions service, and implement create app session with JWT tokens by gabrielcorado · Pull Request #62324 · gravitational/teleport

gabrielcorado · 2025-12-17T02:09:44Z

Brief context for reviewers on RFD 0030e: It allows Teleport to verify JWT tokens issued by external services (such as an IdP) with an audience set to Teleport (as specified by configuration). To enable this, we need to start sessions based on these JWT tokens, and this sessions service handles that process.

As per RFD, this service is responsible for receiving the JWT (external) sent and performing:

Token verification. Validate the values based on the config, and token freshness (rejecting tokens issued after a predefined time).
If verification is good, it will create a new app access session to the target application.

Note: This service will be used by Teleport Proxy to generate the app sessions. This will be done in a separate PR and will include integration tests.

greedy52

first round 🎉

greedy52 · 2025-12-17T21:18:12Z

+  // SessionId is ID the new app session will use.
+  string session_id = 2;


it might be safer for auth to generate this instead of inputting it from proxy.

Agreed. The thing is, we still require the proxy to generate the session ID (otherwise, we would need an auth server request for every MCP/APP request).

What about moving this logic into a shared package (maybe api/ or something else) and having the auth not generate it but validate it instead?

personally i would prefer lib/services over api/ but either is fine. or a new common package.

that said, i wonder if we can do in a way that Proxy does not need to guess. we can brainstorm some ideas offline.

I've moved this logic into a shared package that can be used by auth and proxy. So now, the auth will generate the session ID, and the proxy will also try to generate it when retrieving the session.

We can consider doing something different here later, like not using this hashed JWT as the session ID (use another field to store it) and making the proxy capable of searching by that field. For now, we'll keep it this way, and if we find out that this needs to change, we can update it (there are some backward compatibility requirements, but it's all internal, so we should be fine).

cthach · 2025-12-18T19:28:41Z

Sorry for the delay. Reviewing now.

cthach · 2025-12-18T19:39:32Z

+  // SessionId is ID the new app session will use.
+  string session_id = 2;


cthach · 2025-12-18T19:40:23Z

+  // ClientAddr is a client (user's) address.
+  string client_addr = 5;


Can we please be more specific about what this is? Is it the client source IP address?

Yes, it is the client's remote IP address (LoginIP). I've renamed it to RemoteAddr to match the name used elsewhere.

That is fine, but just please add IP so when we consume the string we know exactly what to parse it as

// RemoteAddr is a client (user's) IP address.

I know it seems like a minor detail, but in my networking mind "remote address" can sometimes contain the source port or can mean other things depending on the context

…tion

cthach · 2026-01-08T16:07:08Z

Reviewing now

cthach

Looks good overall. Left mostly nit suggestions. Have one blocking comment regarding removing legacy dependency if possible since we're introducing a new service.

cthach · 2026-01-08T16:19:21Z

+// Response for CreateAppSessionWithJWT.
+message CreateAppSessionWithJWTResponse {
+  // Session is the app session.
+  types.WebSessionV2 session = 1;


We had a ton of issues with gogoproto and have been trying to move away from it.

Is there anyway possible that we can avoid importing teleport/legacy/types/types.proto, even if that means having to define a new package with what we need?

If it is too much of a lift, I understand, but this would really help with our long-term transition away from gogoproto and legacy code 🙏🏾

100% we should make this change. But I don't think this is the right PR to do it (given that we'll need to propose a new type, and ideally get feedback on it), and here is my suggestion:

We go with this PR as it is (using legacy types.WebSessionV2) and the following PR that will use this service (changes at lib/web/app).

I can open a separate PR that has no dependency on either of those two with the new app session type and helper/conversion functions for the legacy type. We can review it and make it ready for usage.

After 2 is done, I can go back to this and update. We could assume this won't be released yet, so there are no issues with backward compatibility.

What do you think?

Also, @greedy52, for visibility on this.

That sounds good to me. Thank you!!

cthach

Great work.

Please address the authz issue before merging.

cthach · 2026-01-12T21:09:42Z

+	sid := services.GenerateAppSessionIDFromJWT(req.Jwt)
+	defer func() {
+		if emitErr := s.emitter.EmitAuditEvent(ctx, newVerifyJWTAuditEvent(ctx, req, "", err)); emitErr != nil {
+			s.logger.ErrorContext(ctx, "failed to emit jwt verification audit event", "error", emitErr)
+		}
+	}()
+
+	if err := validateCreateAppSessionWithJWTRequest(req); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	authCtx, err := s.authorizer.Authorize(ctx)
+	if err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	if !authz.HasBuiltinRole(*authCtx, string(types.RoleProxy)) {
+		return nil, trace.AccessDenied("this request can be only executed by a proxy")
+	}


Suggested change

sid := services.GenerateAppSessionIDFromJWT(req.Jwt)

defer func() {

if emitErr := s.emitter.EmitAuditEvent(ctx, newVerifyJWTAuditEvent(ctx, req, "", err)); emitErr != nil {

s.logger.ErrorContext(ctx, "failed to emit jwt verification audit event", "error", emitErr)

}

}()

if err := validateCreateAppSessionWithJWTRequest(req); err != nil {

return nil, trace.Wrap(err)

}

authCtx, err := s.authorizer.Authorize(ctx)

if err != nil {

return nil, trace.Wrap(err)

}

if !authz.HasBuiltinRole(*authCtx, string(types.RoleProxy)) {

return nil, trace.AccessDenied("this request can be only executed by a proxy")

}

authCtx, err := s.authorizer.Authorize(ctx)

if err != nil {

return nil, trace.Wrap(err)

}

if !authz.HasBuiltinRole(*authCtx, string(types.RoleProxy)) {

return nil, trace.AccessDenied("this request can be only executed by a proxy")

}

sid := services.GenerateAppSessionIDFromJWT(req.Jwt)

defer func() {

if emitErr := s.emitter.EmitAuditEvent(ctx, newVerifyJWTAuditEvent(ctx, req, "", err)); emitErr != nil {

s.logger.ErrorContext(ctx, "failed to emit jwt verification audit event", "error", emitErr)

}

}()

if err := validateCreateAppSessionWithJWTRequest(req); err != nil {

return nil, trace.Wrap(err)

}

Shouldn't we do our authz checks first?

Sorry, I missed this in the first pass.

Nice catch. I've moved the authz to the top, but I kept the defer function before, so we can still generate audit logs when authz fails as well.

generate audit logs when authz fails as well

I'm a relatively new dev at Teleport. Is this a common pattern?

Personally I would be concerned about malicious unauthorized users spamming this endpoint, possibly causing a DoS via the side effect that each failure, even from unauthorized user, creates an audit event and thereby consuming resources.

Then again, I'm especially paranoid. If this potential attack vector is something we accepted in our threat model, I'm good with it, just wanted us to acknowledge.

I'm a relatively new dev at Teleport. Is this a common pattern?

(I had to double-check on this) It seems we only log access denied for some resource access. For example, if you try to access a database using a database user you don't have access to. With that said, I'll move the emit to occur after authz, as there is not much value in having it (see below for more).

Personally I would be concerned about malicious unauthorized users spamming this endpoint, possibly causing a DoS via the side effect that each failure, even from unauthorized user, creates an audit event and thereby consuming resources.

Only issuing an audit after the authorization is performed won't affect the concern you raised, since it only verifies that the request was made by an internal component (Proxy in this case). No user permissions are currently verified at this step. The user's authorization to access X occurs later, when we generate the session, and already generates an audit event.

To reduce the risk, as you said, we'll impose a rate limit on callers to this method (which will be publicly accessible and requested by users). I couldn't find any place where we enforce this measure on the auth server methods.

Thanks for hearing me out.

I forgot that there was an authentication check prior to this logic. I was mostly concerned with us triggering events for unauthenticated and unauthorized requests.

I chatted with @rosstimothy and he mentioned that we have started to move towards emitting audit events for successful and failed authorizations. For example, see #62384.

So I believe what you had earlier where we emit the event even on failed authorization was good and inline with this pattern.

Apologies for the back and forth!

…performed

…uthz is performed"

add app auth config sessions service

19474ec

gabrielcorado requested a review from greedy52 December 17, 2025 02:09

gabrielcorado self-assigned this Dec 17, 2025

gabrielcorado added the no-changelog Indicates that a PR does not require a changelog entry label Dec 17, 2025

github-actions bot requested review from cthach and probakowski December 17, 2025 02:10

github-actions bot added the size/lg label Dec 17, 2025

fix: lint

2687521

greedy52 reviewed Dec 17, 2025

View reviewed changes

cthach reviewed Dec 18, 2025

View reviewed changes

gabrielcorado added 2 commits December 19, 2025 01:23

refactor: review suggestions

68cd67d

refactor(appauthconfig): check for cast error and fix test

db6ec96

gabrielcorado requested review from cthach and greedy52 December 19, 2025 04:30

refactor(appauthconfig): move params struct and app session id genera…

b3cd344

…tion

greedy52 approved these changes Jan 8, 2026

View reviewed changes

Comment thread lib/auth/sessions.go

cthach reviewed Jan 8, 2026

View reviewed changes

chore: code review suggestions

f193c33

gabrielcorado requested a review from cthach January 9, 2026 01:55

cthach approved these changes Jan 12, 2026

View reviewed changes

public-teleport-github-review-bot bot removed the request for review from probakowski January 12, 2026 21:15

gabrielcorado added 4 commits January 12, 2026 19:36

refactor(appauthconfigv1): perform authz before validating request

8d5ef88

refactor(appauthconfigv1): emit audit events after internal authz is …

2fbfcd6

…performed

test(appauthconfigv1): update test

cd4299a

Revert "refactor(appauthconfigv1): emit audit events after internal a…

443bff3

…uthz is performed"

gabrielcorado enabled auto-merge January 13, 2026 19:31

gabrielcorado added this pull request to the merge queue Jan 13, 2026

Merged via the queue into master with commit a16ac96 Jan 13, 2026
45 checks passed

gabrielcorado deleted the gabrielcorado/push-tkslwnulnspx branch January 13, 2026 20:12

gabrielcorado mentioned this pull request Jan 20, 2026

Add streamable HTTP MCP handler with app auth config #62975

Draft

9 tasks

		// SessionId is ID the new app session will use.
		string session_id = 2;

		// ClientAddr is a client (user's) address.
		string client_addr = 5;

Conversation

gabrielcorado commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greedy52 left a comment • 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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

cthach commented Dec 18, 2025

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cthach commented Jan 8, 2026

Uh oh!

cthach left a comment

Choose a reason for hiding this comment

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!

cthach left a comment

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!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

gabrielcorado commented Dec 17, 2025 •

edited

Loading

greedy52 left a comment •

edited

Loading