fix(sbi): enforce inbound OAuth2 on NEF service routes#23
Conversation
- Add NFContext interface and AuthorizationCheck method to NefContext, calling oauth.VerifyOAuth for inbound token validation - Add RouterAuthorizationCheck utility (util/util.go) and UtilLog - Refactor server.go: mount routes conditionally per ServiceList entry and attach per-group OAuth2 middleware; use correct ServiceName constant for each route group (NNEF_PFDMANAGEMENT, NNEF_OAM, 3GPP_TRAFFIC_INFLUENCE) - Extend ServiceList validation in config.go to include 3gpp-traffic-influence - Add 3gpp-traffic-influence to default serviceList in nefcfg.yaml - Remove four stale '// TODO: Authorize the AF' comments from pfd.go now that middleware handles authorization
solar224
force-pushed
the
fix/nef-oauth2-inbound-auth
branch
from
bbca77b to
b16537c
Compare
There was a problem hiding this comment.
Pull request overview
This PR hardens NEF’s SBI surface by enforcing inbound OAuth2 token validation on NEF service routes, while also improving Traffic Influence request validation and implementing authenticated forwarding of SMF notifications to the AF callback URI.
Changes:
- Add middleware-oriented authorization plumbing (NFContext interface, per-route-group OAuth2 checks, RouterAuthorizationCheck helper).
- Refactor route mounting to be serviceList-driven (plus protected callback group) and extend config defaults/validation for
3gpp-traffic-influenceandnnef-callback. - Validate
notificationDestinationfor Traffic Influence subscriptions and add callback forwarding logic with unit tests.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/factory/config.go | Extends serviceList validation to include traffic influence + callback services. |
| internal/util/util.go | Introduces RouterAuthorizationCheck helper used as OAuth2 middleware. |
| internal/sbi/server.go | Refactors route mounting per serviceList and attaches OAuth2 middleware per group. |
| internal/sbi/processor/ti.go | Adds notificationDestination presence + URL validation. |
| internal/sbi/processor/ti_test.go | Updates TI tests to include notificationDestination and adds missing-field cases. |
| internal/sbi/processor/pfd.go | Removes stale authorization TODO comments now handled by middleware. |
| internal/sbi/processor/callback.go | Implements forwarding SMF notifications to AF with outbound OAuth2 token binding. |
| internal/sbi/processor/callback_test.go | Adds unit tests for token binding and callback POST behavior. |
| internal/logger/logger.go | Adds a UtilLog logger category for util-layer logging. |
| internal/context/nef_context.go | Adds NFContext + AuthorizationCheck that calls oauth.VerifyOAuth. |
| config/nefcfg.yaml | Updates default serviceList to include traffic influence + callback services. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| callbackAuthCheck := nef_util.NewRouterAuthorizationCheck(models.ServiceName("nnef-callback")) | ||
| callbackGroup := s.router.Group(factory.NefCallbackResUriPrefix) | ||
| callbackGroup.Use(func(c *gin.Context) { | ||
| callbackAuthCheck.Check(c, s.Context()) | ||
| }) |
There was a problem hiding this comment.
The Gin middleware functions added via Group.Use don’t call c.Next() when authorization succeeds. In Gin, handlers must call Next() to continue the chain; otherwise the request will stop at the middleware and the route handlers won’t run (likely returning an empty 200). After Check passes (and c.IsAborted() is false), call c.Next() (or refactor Check to do so).
| "golang.org/x/oauth2" | ||
| ) | ||
|
|
||
| var afCallbackHTTPClient = &http.Client{} |
There was a problem hiding this comment.
afCallbackHTTPClient is a package-level http.Client with no timeout configured. For outbound callbacks this can hang indefinitely on network stalls and tie up goroutines. Consider setting a reasonable Timeout (or using a custom Transport with timeouts) and/or enforcing a deadline on requestCtx before issuing the request.
| func (rac *RouterAuthorizationCheck) Check(c *gin.Context, nefCtx nef_context.NFContext) { | ||
| token := c.Request.Header.Get("Authorization") | ||
| if err := nefCtx.AuthorizationCheck(token, rac.serviceName); err != nil { | ||
| logger.UtilLog.Debugf("RouterAuthorizationCheck::Check Unauthorized: %s", err.Error()) | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": err.Error()}) | ||
| c.Abort() | ||
| return |
There was a problem hiding this comment.
The OAuth2 middleware returns 401 with {"error": ...} but doesn’t set the metrics/sbi context key (sbi.IN_PB_DETAILS_CTX_STR) like the rest of the SBI handlers do for ProblemDetails responses. If you rely on that key for metrics/observability, consider setting it here as well (even if you keep the simplified 401 body).
Fixes: free5gc/free5gc#858 、 free5gc/free5gc#859 、 free5gc/free5gc#860 、 free5gc/free5gc#861 、 free5gc/free5gc#862