[TT-12095] Fixing unhashed API keys exposed in OTEL spans by mativm02 · Pull Request #6296 · TykTechnologies/tyk

mativm02 · 2024-05-21T19:42:46Z

User description

Description

tyk.api.apikey and tyk.api.oauthid attributes were exposing keys when OTEL was enabled. Now, the value of this attribute is the hashed key instead.
Can be checked when creating a key:

Now, the attribute must show the hashed key, not the actual key.

Related Issue

https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9

Motivation and Context

https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Refactoring or add test (improvements in base code or adds test coverage to functionality)

Checklist

I ensured that the documentation is up to date
I explained why this PR updates go.mod in detail with reasoning why it's required
I would like a code coverage CI quality gate exception and have explained why

PR Type

Bug fix

Description

Updated gateway/mw_auth_key.go to use the hashed API key in OTEL span attributes instead of the plain key.
Updated gateway/mw_oauth2_key_exists.go to use the hashed OAuth client ID in OTEL span attributes.

Changes walkthrough 📝

Relevant files

Bug fix

mw_auth_key.go `Use hashed API key in OTEL span attributes` gateway/mw_auth_key.go Added `hashedKey` variable to store the hashed key from the session. Updated OTEL span attribute to use the hashed key instead of the plain key.	+2/-1
mw_oauth2_key_exists.go `Use hashed OAuth client ID in OTEL span attributes` gateway/mw_oauth2_key_exists.go Updated OTEL span attribute to use the hashed key for OAuth client ID.	+1/-1

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

github-actions · 2024-05-21T19:43:29Z

API Changes

no api changes detected

github-actions · 2024-05-21T19:43:52Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward and localized to specific functions related to authentication and telemetry. The logic is simple and the impact of the changes is clear, focusing on replacing plain text keys with hashed keys in telemetry data.
🧪 Relevant tests	No
⚡ Possible issues	Possible Bug: If `session.KeyHash()` or `session.KeyID` methods are not handling cases where session might be nil or not properly initialized, this could lead to runtime panics. It's important to ensure that these methods are safe to call in all circumstances that `ProcessRequest` might encounter.
🔒 Security concerns	No

Code feedback:

relevant file	gateway/mw_auth_key.go
suggestion	Consider adding error handling for `session.KeyHash()` to gracefully handle any potential issues with session state or hash computation. This could prevent runtime errors in cases where the session might be incomplete or corrupted. [important]
relevant line	hashedKey := session.KeyHash()

relevant file	gateway/mw_auth_key.go
suggestion	Ensure that `session.KeyHash()` method includes safety checks for nil session objects to avoid potential nil pointer dereferences. [important]
relevant line	hashedKey := session.KeyHash()

relevant file	gateway/mw_oauth2_key_exists.go
suggestion	Verify that replacing `session.OauthClientID` with `session.KeyHash()` in `ctxSetSpanAttributes` does not affect other systems or logging mechanisms that might expect the original OAuth client ID. Consider maintaining both if needed. [medium]
relevant line	ctxSetSpanAttributes(r, k.Name(), otel.OAuthClientIDAttribute(session.KeyHash()))

relevant file	gateway/mw_auth_key.go
suggestion	Add logging around the key hashing process to aid in debugging and to ensure that key transformations are occurring as expected. This can be particularly useful in tracing issues related to session management and key handling. [medium]
relevant line	hashedKey := session.KeyHash()

github-actions · 2024-05-21T19:44:31Z

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Prevent potential nil pointer dereference by checking if key exists before accessing session properties Ensure that `session.KeyHash()` is only called when `keyExists` is true to avoid potential nil pointer dereference if the session is not properly initialized. gateway/mw_auth_key.go [125] -hashedKey := session.KeyHash() +if keyExists { + hashedKey := session.KeyHash() +} Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug that could lead to a nil pointer dereference, which is a critical issue. Ensuring that `session.KeyHash()` is only called when `keyExists` is true improves the robustness of the code.	9
	Prevent potential nil pointer dereference by checking session validity before accessing its properties Ensure that `session.KeyHash()` is only called when the session is valid to avoid potential nil pointer dereference. gateway/mw_oauth2_key_exists.go [133] -ctxSetSpanAttributes(r, k.Name(), otel.OAuthClientIDAttribute(session.KeyHash())) +if session != nil { + ctxSetSpanAttributes(r, k.Name(), otel.OAuthClientIDAttribute(session.KeyHash())) +} Suggestion importance[1-10]: 9 Why: This suggestion addresses a potential bug that could lead to a nil pointer dereference, which is a critical issue. Ensuring that `session.KeyHash()` is only called when the session is valid improves the robustness of the code.	9
	Add nil checks for session to prevent runtime errors To ensure consistency and avoid errors, check if `session` is nil before accessing `session.KeyHash()` and `session.Alias`. gateway/mw_auth_key.go [125-157] -hashedKey := session.KeyHash() -otel.APIKeyAliasAttribute(session.Alias) +if session != nil { + hashedKey := session.KeyHash() + otel.APIKeyAliasAttribute(session.Alias) +} Suggestion importance[1-10]: 8 Why: Adding nil checks for `session` before accessing its properties helps prevent runtime errors, improving the stability and reliability of the code. This is a significant improvement, though slightly less critical than the first suggestion.	8
Maintainability	Improve code readability by using more descriptive variable names Consider using a more descriptive variable name than `hashedKey` to clarify that this represents a hash of the API key. gateway/mw_auth_key.go [125] -hashedKey := session.KeyHash() +apiKeyHash := session.KeyHash() Suggestion importance[1-10]: 6 Why: Using a more descriptive variable name improves code readability and maintainability. However, this is a minor improvement compared to functional or security-related changes.	6

github-actions · 2024-05-21T19:49:20Z

💥 CI tests failed 🙈

git-state

all ok

Please look at the run or in the Checks tab.

sredxny · 2024-05-27T14:07:18Z

+		attributes := []otel.SpanAttribute{otel.APIKeyAliasAttribute(session.Alias)}
+
+		if hashKeys {
+			attributes = append(attributes, otel.APIKeyAttribute(session.KeyHash()))
+		}


what about moving this into its own function so you can add uni test easily?

@sredxny wouldn't that make it less readable? 🤔

sonarqubecloud · 2024-05-28T12:40:39Z

Please retry analysis of this Pull-Request directly on SonarCloud

sonarqubecloud · 2024-05-28T12:40:39Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

mativm02 · 2024-05-28T12:59:11Z

/release to release-5.3.2

tykbot · 2024-05-28T12:59:22Z

Working on it! Note that it can take a few minutes.

### **User description**  ## Description `tyk.api.apikey` and `tyk.api.oauthid` attributes were exposing keys when OTEL was enabled. Now, the value of this attribute is the hashed key instead. Can be checked when creating a key: <img width="543" alt="image" src="https://github.com/TykTechnologies/tyk/assets/83959431/167e18b8-234f-4b2e-af19-35aa75bb3339"> Now, the attribute must show the hashed key, not the actual key.  ## Related Issue https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9     ## Motivation and Context https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix ___ ### **Description** - Updated `gateway/mw_auth_key.go` to use the hashed API key in OTEL span attributes instead of the plain key. - Updated `gateway/mw_oauth2_key_exists.go` to use the hashed OAuth client ID in OTEL span attributes. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>mw_auth_key.go</strong><dd><code>Use hashed API key in OTEL span attributes</code>                              </dd></summary> <hr> gateway/mw_auth_key.go <li>Added <code>hashedKey</code> variable to store the hashed key from the session.<br> <li> Updated OTEL span attribute to use the hashed key instead of the plain <br>key.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6296/files#diff-aeba053023a54c723dd9f83837e29ca0b2d9a212bc98fa6ad4bbb062669a1cf0">+2/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>mw_oauth2_key_exists.go</strong><dd><code>Use hashed OAuth client ID in OTEL span attributes</code>              </dd></summary> <hr> gateway/mw_oauth2_key_exists.go <li>Updated OTEL span attribute to use the hashed key for OAuth client ID.<br> <br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6296/files#diff-c50b0affcb835d15ff9bd169b21775b8023ebb0558a0c59d6c26cb821db544e9">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Sredny M <[email protected]> (cherry picked from commit 46ca642)

tykbot · 2024-05-28T12:59:51Z

@mativm02 Succesfully merged PR

… in OTEL spans (#6296) [TT-12095] Fixing unhashed API keys exposed in OTEL spans (#6296) ### **User description**  ## Description `tyk.api.apikey` and `tyk.api.oauthid` attributes were exposing keys when OTEL was enabled. Now, the value of this attribute is the hashed key instead. Can be checked when creating a key: <img width="543" alt="image" src="https://github.com/TykTechnologies/tyk/assets/83959431/167e18b8-234f-4b2e-af19-35aa75bb3339"> Now, the attribute must show the hashed key, not the actual key.  ## Related Issue https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9     ## Motivation and Context https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix ___ ### **Description** - Updated `gateway/mw_auth_key.go` to use the hashed API key in OTEL span attributes instead of the plain key. - Updated `gateway/mw_oauth2_key_exists.go` to use the hashed OAuth client ID in OTEL span attributes. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>mw_auth_key.go</strong><dd><code>Use hashed API key in OTEL span attributes</code>                              </dd></summary> <hr> gateway/mw_auth_key.go <li>Added <code>hashedKey</code> variable to store the hashed key from the session.<br> <li> Updated OTEL span attribute to use the hashed key instead of the plain <br>key.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6296/files#diff-aeba053023a54c723dd9f83837e29ca0b2d9a212bc98fa6ad4bbb062669a1cf0">+2/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>mw_oauth2_key_exists.go</strong><dd><code>Use hashed OAuth client ID in OTEL span attributes</code>              </dd></summary> <hr> gateway/mw_oauth2_key_exists.go <li>Updated OTEL span attribute to use the hashed key for OAuth client ID.<br> <br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6296/files#diff-c50b0affcb835d15ff9bd169b21775b8023ebb0558a0c59d6c26cb821db544e9">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Sredny M <[email protected]>

### **User description**  ## Description `tyk.api.apikey` and `tyk.api.oauthid` attributes were exposing keys when OTEL was enabled. Now, the value of this attribute is the hashed key instead. Can be checked when creating a key: <img width="543" alt="image" src="https://github.com/TykTechnologies/tyk/assets/83959431/167e18b8-234f-4b2e-af19-35aa75bb3339"> Now, the attribute must show the hashed key, not the actual key.  ## Related Issue https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9     ## Motivation and Context https://tyktech.atlassian.net/browse/TT-12095?atlOrigin=eyJpIjoiMGJkNjAxOTI1MWQ5NGE0ZWIyNDgyNzlmMDcxMTM4YmEiLCJwIjoiaiJ9  ## How This Has Been Tested     ## Screenshots (if appropriate) ## Types of changes  - [ ] Bug fix (non-breaking change which fixes an issue) - [ ] New feature (non-breaking change which adds functionality) - [ ] Breaking change (fix or feature that would cause existing functionality to change) - [ ] Refactoring or add test (improvements in base code or adds test coverage to functionality) ## Checklist    - [ ] I ensured that the documentation is up to date - [ ] I explained why this PR updates go.mod in detail with reasoning why it's required - [ ] I would like a code coverage CI quality gate exception and have explained why ___ ### **PR Type** Bug fix ___ ### **Description** - Updated `gateway/mw_auth_key.go` to use the hashed API key in OTEL span attributes instead of the plain key. - Updated `gateway/mw_oauth2_key_exists.go` to use the hashed OAuth client ID in OTEL span attributes. ___ ### **Changes walkthrough** 📝 <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td><strong>Bug fix </strong></td><td><table> <tr> <td> <details> <summary><strong>mw_auth_key.go</strong><dd><code>Use hashed API key in OTEL span attributes</code>                              </dd></summary> <hr> gateway/mw_auth_key.go <li>Added <code>hashedKey</code> variable to store the hashed key from the session.<br> <li> Updated OTEL span attribute to use the hashed key instead of the plain <br>key.<br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6296/files#diff-aeba053023a54c723dd9f83837e29ca0b2d9a212bc98fa6ad4bbb062669a1cf0">+2/-1</a>      </td> </tr> <tr> <td> <details> <summary><strong>mw_oauth2_key_exists.go</strong><dd><code>Use hashed OAuth client ID in OTEL span attributes</code>              </dd></summary> <hr> gateway/mw_oauth2_key_exists.go <li>Updated OTEL span attribute to use the hashed key for OAuth client ID.<br> <br> </details> </td> <td><a href="https://github.com/TykTechnologies/tyk/pull/6296/files#diff-c50b0affcb835d15ff9bd169b21775b8023ebb0558a0c59d6c26cb821db544e9">+1/-1</a>      </td> </tr> </table></td></tr></tr></tbody></table> ___ > 💡 **PR-Agent usage**: >Comment `/help` on the PR to get a list of all available PR-Agent tools and their descriptions --------- Co-authored-by: Sredny M <[email protected]>

fixing unhashed API keys exposed in OTEL spans

0f0290a

github-actions Bot added the Bug fix label May 21, 2024

github-actions Bot added the Review effort [1-5]: 2 label May 21, 2024

checking if hashing is enabled before creating span

c09286f

mativm02 requested review from padiazg and sredxny May 27, 2024 12:22

mativm02 and others added 2 commits May 27, 2024 09:25

Merge branch 'master' into TT-12095

13b7b86

Merge branch 'master' into TT-12095

4ced777

sredxny requested changes May 27, 2024

View reviewed changes

mativm02 requested a review from sredxny May 27, 2024 19:13

sredxny approved these changes May 28, 2024

View reviewed changes

Merge branch 'master' into TT-12095

e81f58e

mativm02 enabled auto-merge (squash) May 28, 2024 12:26

mativm02 disabled auto-merge May 28, 2024 12:42

mativm02 merged commit 46ca642 into master May 28, 2024

mativm02 deleted the TT-12095 branch May 28, 2024 12:58

buger mentioned this pull request May 28, 2024

Merging to release-5.3.2: [TT-12095] Fixing unhashed API keys exposed in OTEL spans (#6296) #6314

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[TT-12095] Fixing unhashed API keys exposed in OTEL spans#6296

[TT-12095] Fixing unhashed API keys exposed in OTEL spans#6296
mativm02 merged 5 commits into
masterfrom
TT-12095

mativm02 commented May 21, 2024 •

edited by github-actions Bot

Loading

Uh oh!

github-actions Bot commented May 21, 2024 •

edited

Loading

Uh oh!

github-actions Bot commented May 21, 2024

Uh oh!

github-actions Bot commented May 21, 2024

Uh oh!

github-actions Bot commented May 21, 2024

Uh oh!

sredxny May 27, 2024

Uh oh!

mativm02 May 27, 2024

Uh oh!

sonarqubecloud Bot commented May 28, 2024

Uh oh!

sonarqubecloud Bot commented May 28, 2024

Uh oh!

mativm02 commented May 28, 2024

Uh oh!

tykbot Bot commented May 28, 2024

Uh oh!

tykbot Bot commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

mativm02 commented May 21, 2024 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Related Issue

Motivation and Context

How This Has Been Tested

Screenshots (if appropriate)

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

github-actions Bot commented May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 21, 2024

PR Review 🔍

Uh oh!

github-actions Bot commented May 21, 2024

PR Code Suggestions ✨

Uh oh!

github-actions Bot commented May 21, 2024

💥 CI tests failed 🙈

git-state

Uh oh!

sredxny May 27, 2024

Choose a reason for hiding this comment

Uh oh!

mativm02 May 27, 2024

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 28, 2024

Uh oh!

sonarqubecloud Bot commented May 28, 2024

Quality Gate passed

Uh oh!

mativm02 commented May 28, 2024

Uh oh!

tykbot Bot commented May 28, 2024

Uh oh!

tykbot Bot commented May 28, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mativm02 commented May 21, 2024 •

edited by github-actions Bot

Loading

github-actions Bot commented May 21, 2024 •

edited

Loading