Wire AWS STS middleware into the runner middleware chain #3770
Conversation
jhrozek
requested review from
ChrisJBurns,
JAORMX,
amirejaz,
eleftherias,
lujunsan and
yrobla
as code owners
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3770 +/- ##
==========================================
- Coverage 66.78% 66.70% -0.08%
==========================================
Files 437 437
Lines 43002 43025 +23
==========================================
- Hits 28718 28701 -17
- Misses 12078 12118 +40
Partials 2206 2206 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
jhrozek
force-pushed
the
aws_sts-pr-5b-runner-wiring
branch
from
7e7727d to
1764a75
Compare
github-actions
bot
added
size/M
| UpstreamSwapConfig *upstreamswap.Config `json:"upstream_swap_config,omitempty" yaml:"upstream_swap_config,omitempty"` | ||
|
|
||
| // AWSStsConfig contains AWS STS token exchange configuration for accessing AWS services | ||
| AWSStsConfig *awssts.Config `json:"aws_sts_config,omitempty" yaml:"aws_sts_config,omitempty"` |
There was a problem hiding this comment.
is this meant to be middleware configuration? should it go with other middleware? cc @blkt
There was a problem hiding this comment.
fwiw I modeled this after the token exchange and header injection code. If there's other pattern I should be using, I'll be happy to do that.
There was a problem hiding this comment.
I don't know how valuable is it to enforce that distinction anymore. The main problem is that RunConfig represents both the MCP server's config and the Proxy's config, and doubts regarding where to put any bit of configuration stem out of that.
I think it's sensible to have each configuration parameter as a top level field of a struct, it's much easier to know where to find things, and have two separate structs for MCP server and Proxy configurations.
Finally, I would leave to the code under pkg/runner/runner.go to determine the correct order of middlewares.
This is easier said than done because this requires a data migration. This is one of the reasons why I believe having run configs on database would be a boon, because data migrations are an established practice.
I would not bang the head to hard on this and leave it where it's easy to understand. Having middleware configurations in a list rather than in top level fields gives little advantage.
|
|
||
| // addAWSStsMiddleware adds AWS STS middleware if configured | ||
| func addAWSStsMiddleware(middlewares []types.MiddlewareConfig, config *RunConfig) ([]types.MiddlewareConfig, error) { | ||
| if config.AWSStsConfig == nil { |
There was a problem hiding this comment.
Do we want to check if the config.RemoteURL is empty here too? just to avoid the middleware signing against the wrong thing?
jhrozek
force-pushed
the
aws_sts-pr-5b-runner-wiring
branch
from
1764a75 to
ac1c550
Compare
github-actions
bot
added
size/M
Register the AWS STS middleware in the runner so it can be activated via RunConfig. Add AWSStsConfig to RunConfig, a WithAWSStsConfig builder option, and factory registration in GetSupportedMiddlewareFactories. Place the middleware in PopulateMiddlewareConfigs after audit/authz but before header forwarding — only authorized requests trigger credential exchange and SigV4 signing happens as late as possible. Regenerate Swagger docs to include the new awssts.Config and awssts.RoleMapping schemas. Consolidate the three GetSupportedMiddlewareFactories tests into one table-driven test and extract the duplicated createMinimalAuthServerConfig helper to package level. Related: #3569
jhrozek
force-pushed
the
aws_sts-pr-5b-runner-wiring
branch
from
ac1c550 to
d1414c0
Compare
github-actions
bot
added
size/M
4 participants
Register the AWS STS middleware in the runner so it can be activated via RunConfig.
Add AWSStsConfig to RunConfig, a WithAWSStsConfig builder option, and factory registration in GetSupportedMiddlewareFactories. Place the middleware in PopulateMiddlewareConfigs after audit/authz but before header forwarding — only authorized requests trigger credential exchange and SigV4 signing happens as late as possible.
Regenerate Swagger docs to include the new awssts.Config and awssts.RoleMapping schemas.
Consolidate the three GetSupportedMiddlewareFactories tests into one table-driven test and extract the duplicated createMinimalAuthServerConfig helper to package level.
Related: #3569