Skip to content

Commit

Permalink
[TT-12741] Looped ap is wrongfully inherit the caller's authenticatio…
Browse files Browse the repository at this point in the history
…n key when using url rewrite (#6778)

### **User description**
<details open>
<summary><a href="https://tyktech.atlassian.net/browse/TT-12741"
title="TT-12741" target="_blank">TT-12741</a></summary>
  <br />
  <table>
    <tr>
      <th>Summary</th>
<td>Looped APIs wrongfully inherit the caller's Authentication key when
using URL rewrite</td>
    </tr>
    <tr>
      <th>Type</th>
      <td>
<img alt="Bug"
src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10303?size=medium"
/>
        Bug
      </td>
    </tr>
    <tr>
      <th>Status</th>
      <td>In Dev</td>
    </tr>
    <tr>
      <th>Points</th>
      <td>N/A</td>
    </tr>
    <tr>
      <th>Labels</th>
<td><a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20'24Bugsmash%20ORDER%20BY%20created%20DESC"
title="'24Bugsmash">'24Bugsmash</a>, <a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20customer_bug%20ORDER%20BY%20created%20DESC"
title="customer_bug">customer_bug</a>, <a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20jira_escalated%20ORDER%20BY%20created%20DESC"
title="jira_escalated">jira_escalated</a></td>
    </tr>
  </table>
</details>
<!--
  do not remove this marker as it will break jira-lint's functionality.
  added_by_jira_lint
-->

---

PR to see CI/CD result, please don't merge it.


___

### **PR Type**
Bug fix, Tests


___

### **Description**
- Introduced a new context constant `SelfLooping` and methods
`ctxSetSelfLooping` and `ctxSelfLooping` to manage self-looping state in
requests.
- Updated `ctxCheckLimits` to bypass rate limits and quotas for
self-looping requests.
- Modified API loader to set self-looping state for self-referencing
requests.
- Enhanced the test `TestQuotaNotAppliedWithURLRewrite` to include
scenarios for self-looping and URL rewrite, ensuring proper behavior.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>ctx.go</strong><dd><code>Add support for managing
self-looping state in context</code>&nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

ctx/ctx.go

<li>Added a new constant <code>SelfLooping</code> to the context.<br>
<li> Introduced new methods <code>ctxSetSelfLooping</code> and
<code>ctxSelfLooping</code> for <br>managing self-looping state in
requests.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6778/files#diff-600f5f552779994b15324fda108549eec7e7be30b1d8a1a16ee8344243e0cbc7">+1/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></td></tr><tr><td><strong>Bug fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>api.go</strong><dd><code>Update rate limit and quota
checks for self-looping requests</code></dd></summary>
<hr>

gateway/api.go

<li>Modified <code>ctxCheckLimits</code> to skip rate limits and quotas
for <br>self-looping requests.<br> <li> Added logic to check and set
self-looping state in requests.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6778/files#diff-644cda3aeb4ac7f325359e85fcddb810f100dd5e6fa480b0d9f9363a743c4e05">+20/-1</a>&nbsp;
&nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>api_loader.go</strong><dd><code>Set self-looping state
for self-referencing requests</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
</dd></summary>
<hr>

gateway/api_loader.go

- Added logic to set self-looping state when the hostname is "self".



</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6778/files#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68">+1/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></td></tr><tr><td><strong>Tests</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>middleware_test.go</strong><dd><code>Enhance tests to
cover self-looping and URL rewrite scenarios</code></dd></summary>
<hr>

gateway/middleware_test.go

<li>Updated <code>TestQuotaNotAppliedWithURLRewrite</code> to include
extended paths <br>and self-looping scenarios.<br> <li> Added a loader
to create a merged API spec for testing.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6778/files#diff-6a09a08e3f82cc5e9d8c6b5c8426d75ea1e5d85e15ab008fca1f512e7c49c1e6">+7/-1</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>
</table></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

---------

Co-authored-by: Tit Petric <[email protected]>
Co-authored-by: Tit Petric <[email protected]>
  • Loading branch information
3 people authored Dec 19, 2024
1 parent 51e50c3 commit d59ae8c
Show file tree
Hide file tree
Showing 9 changed files with 151 additions and 8 deletions.
1 change: 1 addition & 0 deletions ctx/ctx.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ const (
// CacheOptions holds cache options required for cache writer middleware.
CacheOptions
OASDefinition
SelfLooping
)

func ctxSetSession(r *http.Request, s *user.SessionState, scheduleUpdate bool, hashKey bool) {
Expand Down
Binary file added docs/diagrams/middleware-looping.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
7 changes: 7 additions & 0 deletions gateway/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ import (
"sync"
"time"

"github.com/TykTechnologies/tyk/internal/httpctx"

gqlv2 "github.com/TykTechnologies/graphql-go-tools/v2/pkg/graphql"

"github.com/getkin/kin-openapi/openapi3"
Expand Down Expand Up @@ -3176,6 +3178,11 @@ func ctxSetCheckLoopLimits(r *http.Request, b bool) {

// Should we check Rate limits and Quotas?
func ctxCheckLimits(r *http.Request) bool {
// If this is a self loop, do not need to check the limits and quotas.
if httpctx.IsSelfLooping(r) {
return false
}

// If looping disabled, allow all
if !ctxLoopingEnabled(r) {
return true
Expand Down
2 changes: 2 additions & 0 deletions gateway/api_loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"github.com/TykTechnologies/tyk/storage"
"github.com/TykTechnologies/tyk/trace"

"github.com/TykTechnologies/tyk/internal/httpctx"
"github.com/TykTechnologies/tyk/internal/httputil"
"github.com/TykTechnologies/tyk/internal/otel"
)
Expand Down Expand Up @@ -599,6 +600,7 @@ func (d *DummyProxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {

var handler http.Handler
if r.URL.Hostname() == "self" {
httpctx.SetSelfLooping(r, true)
if h, found := d.Gw.apisHandlesByID.Load(d.SH.Spec.APIID); found {
if chain, ok := h.(*ChainObject); ok {
handler = chain.ThisHandler
Expand Down
95 changes: 95 additions & 0 deletions gateway/looping_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package gateway

import (
"encoding/json"
"net/http"
"sync"
"testing"

Expand Down Expand Up @@ -324,7 +325,101 @@ func TestLooping(t *testing.T) {
{Path: "/external/", Code: 200},
}...)
})
}

func TestLooping_AnotherAPIWithAuthTokens(t *testing.T) {
ts := StartTest(nil)
defer ts.Close()

// Looping to another api with auth tokens
specs := ts.Gw.BuildAndLoadAPI(func(spec *APISpec) {
spec.APIDefinition.APIID = "apia"
spec.APIDefinition.Name = "ApiA"
spec.APIDefinition.Proxy.ListenPath = "/apia"
spec.APIDefinition.UseKeylessAccess = false
spec.APIDefinition.AuthConfigs = map[string]apidef.AuthConfig{
"authToken": {
AuthHeaderName: "Authorization",
},
}

UpdateAPIVersion(spec, "v1", func(v *apidef.VersionInfo) {
v.UseExtendedPaths = true
v.ExtendedPaths.URLRewrite = []apidef.URLRewriteMeta{{
Path: "/",
Method: http.MethodGet,
MatchPattern: ".*",
RewriteTo: "tyk://apib",
}}
})
}, func(spec *APISpec) {
spec.APIDefinition.APIID = "apib"
spec.APIDefinition.Name = "ApiB"
spec.APIDefinition.Proxy.ListenPath = "/apib"
spec.APIDefinition.UseKeylessAccess = false
spec.APIDefinition.AuthConfigs = map[string]apidef.AuthConfig{
"authToken": {
AuthHeaderName: "X-Api-Key",
},
}
})
specApiA := specs[0]
specApiB := specs[1]

_, authKeyForApiA := ts.CreateSession(func(s *user.SessionState) {
s.AccessRights = map[string]user.AccessDefinition{
specApiA.APIDefinition.APIID: {
APIName: specApiA.APIDefinition.Name,
APIID: specApiA.APIDefinition.APIID,
Versions: []string{"default"},
AllowanceScope: specApiA.APIDefinition.APIID,
},
}
s.OrgID = specApiA.APIDefinition.OrgID
})

_, authKeyForApiB := ts.CreateSession(func(s *user.SessionState) {
s.AccessRights = map[string]user.AccessDefinition{
specApiB.APIDefinition.APIID: {
APIName: specApiB.APIDefinition.Name,
APIID: specApiB.APIDefinition.APIID,
Versions: []string{"default"},
AllowanceScope: specApiB.APIDefinition.APIID,
},
}
s.OrgID = specApiB.APIDefinition.OrgID
})

headersWithApiBToken := map[string]string{
"Authorization": authKeyForApiA,
"X-Api-Key": authKeyForApiB,
}
headersWithoutApiBToken := map[string]string{
"Authorization": authKeyForApiA,
"X-Api-Key": "some-string",
}
headersWithOnlyApiAToken := map[string]string{
"Authorization": authKeyForApiA,
}
_, _ = ts.Run(t, []test.TestCase{
{
Headers: headersWithApiBToken,
Path: "/apia",
Code: http.StatusOK,
},
{
Headers: headersWithoutApiBToken,
Path: "/apia",
Code: http.StatusForbidden,
BodyMatch: "Access to this API has been disallowed",
},
{
Headers: headersWithOnlyApiAToken,
Path: "/apia",
Code: http.StatusUnauthorized,
BodyMatch: "Authorization field missing",
},
}...)
}

func TestConcurrencyReloads(t *testing.T) {
Expand Down
3 changes: 2 additions & 1 deletion gateway/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,8 @@ func TestQuotaNotAppliedWithURLRewrite(t *testing.T) {
spec := ts.Gw.BuildAndLoadAPI(func(spec *APISpec) {
spec.Proxy.ListenPath = "/quota-test"
spec.UseKeylessAccess = false
UpdateAPIVersion(spec, "Default", func(v *apidef.VersionInfo) {
UpdateAPIVersion(spec, "v1", func(v *apidef.VersionInfo) {
v.UseExtendedPaths = true
v.ExtendedPaths.URLRewrite = []apidef.URLRewriteMeta{{
Path: "/abc",
Method: http.MethodGet,
Expand Down
13 changes: 6 additions & 7 deletions gateway/mw_auth_key.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,16 +6,15 @@ import (
"strings"
"time"

"github.com/TykTechnologies/tyk/internal/crypto"
"github.com/TykTechnologies/tyk/internal/otel"
"github.com/TykTechnologies/tyk/storage"

"github.com/TykTechnologies/tyk/user"

"github.com/TykTechnologies/tyk/apidef"
"github.com/TykTechnologies/tyk/config"
"github.com/TykTechnologies/tyk/internal/crypto"
"github.com/TykTechnologies/tyk/internal/httpctx"
"github.com/TykTechnologies/tyk/internal/otel"
"github.com/TykTechnologies/tyk/request"
signaturevalidator "github.com/TykTechnologies/tyk/signature_validator"
"github.com/TykTechnologies/tyk/storage"
"github.com/TykTechnologies/tyk/user"
)

const (
Expand Down Expand Up @@ -95,7 +94,7 @@ func (k *AuthKey) ProcessRequest(_ http.ResponseWriter, r *http.Request, _ inter
}

// skip auth key check if the request is looped.
if ses := ctxGetSession(r); ses != nil && !ctxCheckLimits(r) {
if ses := ctxGetSession(r); ses != nil && httpctx.IsSelfLooping(r) {
return nil, http.StatusOK
}

Expand Down
19 changes: 19 additions & 0 deletions internal/httpctx/looping.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package httpctx

import (
"net/http"

"github.com/TykTechnologies/tyk/ctx"
)

var selfLoopingValue = NewValue[bool](ctx.SelfLooping)

// SetSelfLooping updates the request context with a boolean value indicating whether the request is in a self-looping state.
func SetSelfLooping(r *http.Request, value bool) {
selfLoopingValue.Set(r, value)
}

// IsSelfLooping returns true if the request is flagged as self-looping, indicating it originates and targets the same service.
func IsSelfLooping(r *http.Request) bool {
return selfLoopingValue.Get(r)
}
19 changes: 19 additions & 0 deletions internal/httpctx/looping_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
package httpctx_test

import (
"net/http/httptest"
"testing"

"github.com/stretchr/testify/assert"

"github.com/TykTechnologies/tyk/internal/httpctx"
)

func TestSetSelfLooping(t *testing.T) {
req := httptest.NewRequest("GET", "/", nil)
assert.False(t, httpctx.IsSelfLooping(req))
httpctx.SetSelfLooping(req, true)
assert.True(t, httpctx.IsSelfLooping(req))
httpctx.SetSelfLooping(req, false)
assert.False(t, httpctx.IsSelfLooping(req))
}

0 comments on commit d59ae8c

Please sign in to comment.