Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mask the value of the Authorization header if debug is enabled #501

Merged
merged 4 commits into from
May 14, 2024

Conversation

rosskirkpat
Copy link
Contributor

📝 Description

What does this PR do and why is this change necessary?

If LINODE_DEBUG is enabled, the resty debug returns the plain-text Authorization header value from the request. This PR ensures that the Authorization header value will be sanitized/masked if debug mode is enabled.

I also added a logger to the internal testutil package that is compliant with the resty.Logger interface.

✔️ How to Test

What are the steps to reproduce the issue or verify the changes?

Reproduce the issue: set LINODE_DEBUG when using linodego and observe a plain-text token in the debug output from resty ie. Authorization: Bearer <LINODE_TOKEN_PLAIN_TEXT>

Verify the changes: set LINODE_DEBUG when using linodego and observe a masked token in the debug output from resty (see below).

2024/05/07 12:58:21.997176 DEBUG RESTY 
==============================================================================
~~~ REQUEST ~~~
GET  /v4/linode/instances  HTTP/1.1
HOST   : api.linode.com
HEADERS:
	Accept: application/json
	Authorization: Bearer *******************************
	Content-Type: application/json
	User-Agent: linodego/dev https://github.com/linode/linodego
BODY   :
***** NO CONTENT *****
------------------------------------------------------------------------------
~~~ RESPONSE ~~~
STATUS       : 200
PROTO        : 
RECEIVED AT  : 2024-05-07T12:58:21.997154-04:00
TIME DURATION: 47.792µs
HEADERS      :
	Content-Type: application/json
BODY         :
{
   "id": 100,
   "region": "test-central",
   "label": "this-is-a-test-linode"
}
==============================================================================

How do I run the relevant unit/integration tests?

I added a new test TestDebugLogSanitization relating to these changes.

Signed-off-by: Ross Kirkpatrick <rkirkpat@akamai.com>
Signed-off-by: rosskirkpat <rosskirkpat@outlook.com>
@rosskirkpat rosskirkpat requested a review from a team as a code owner May 7, 2024 17:52
@rosskirkpat rosskirkpat requested review from jriddle-linode and yec-akamai and removed request for a team May 7, 2024 17:52
Signed-off-by: Ross Kirkpatrick <rosskirkpat@outlook.com>
@jriddle-linode jriddle-linode added the improvement for improvements in existing functionality in the changelog. label May 8, 2024
Copy link
Contributor

@jriddle-linode jriddle-linode left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch thank you.

client.go Outdated Show resolved Hide resolved
Copy link
Contributor

@yec-akamai yec-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! Everything looks great aside from @lgarber-akamai 's comment 👍

Copy link
Contributor

@lgarber-akamai lgarber-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be out Thursday and Friday so I'll give this my preemptive approval for once my comment has been addressed. Thanks for the contribution!

@rosskirkpat
Copy link
Contributor Author

I'll be out Thursday and Friday so I'll give this my preemptive approval for once my comment has been addressed. Thanks for the contribution!

@lgarber-akamai When you have a minute, would I be able ant to get your sign-off on the latest commit? I believe this PR is ready to be merged now.

Copy link
Contributor

@lgarber-akamai lgarber-akamai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks perfect, thank you!

@lgarber-akamai lgarber-akamai merged commit 25fe7d3 into linode:main May 14, 2024
4 checks passed
@rosskirkpat rosskirkpat deleted the sanitize-debug-header branch May 14, 2024 15:05
renovate bot added a commit to anza-labs/lke-operator that referenced this pull request May 23, 2024
[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [github.com/linode/linodego](https://github.com/linode/linodego) |
`v1.33.1` -> `v1.34.0` |
[![age](https://developer.mend.io/api/mc/badges/age/go/github.com%2flinode%2flinodego/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/go/github.com%2flinode%2flinodego/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/go/github.com%2flinode%2flinodego/v1.33.1/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/go/github.com%2flinode%2flinodego/v1.33.1/v1.34.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

### Release Notes

<details>
<summary>linode/linodego (github.com/linode/linodego)</summary>

###
[`v1.34.0`](https://github.com/linode/linodego/releases/tag/v1.34.0)

[Compare
Source](https://github.com/linode/linodego/compare/v1.33.1...v1.34.0)

#### What's Changed

##### ⚠️  Breaking Change

- \[BREAKING] Add support for LKE Control Plane ACL by
[@&#8203;lgarber-akamai](https://github.com/lgarber-akamai) in
[linode/linodego#495

##### 🐛 Bug Fixes

- Prevent unexpected warning from Resty when calling
`Client.SetDebug(false)` by
[@&#8203;lgarber-akamai](https://github.com/lgarber-akamai) in
[linode/linodego#508

##### 💡 Improvements

- Mask the value of the Authorization header if debug is enabled by
[@&#8203;rosskirkpat](https://github.com/rosskirkpat) in
[linode/linodego#501
- Expose region capabilities enum by
[@&#8203;yec-akamai](https://github.com/yec-akamai) in
[linode/linodego#507

##### ⚙️  Repo/CI Improvements

- replace test execution handler with conditional by
[@&#8203;ykim-1](https://github.com/ykim-1) in
[linode/linodego#502

##### 📦 Dependency Updates

- bump golang.org/x/net from 0.24.0 to 0.25.0 by
[@&#8203;dependabot](https://github.com/dependabot) in
[linode/linodego#500
- bump github.com/go-resty/resty/v2 from 2.12.0 to 2.13.1 by
[@&#8203;dependabot](https://github.com/dependabot) in
[linode/linodego#505
- bump golangci/golangci-lint-action from 5 to 6 by
[@&#8203;dependabot](https://github.com/dependabot) in
[linode/linodego#506

#### New Contributors

- [@&#8203;rosskirkpat](https://github.com/rosskirkpat) made their
first contribution in
[linode/linodego#501

**Full Changelog**:
linode/linodego@v1.33.1...v1.34.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Enabled.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/anza-labs/lke-operator).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy4zNjguMTAiLCJ1cGRhdGVkSW5WZXIiOiIzNy4zNjguMTAiLCJ0YXJnZXRCcmFuY2giOiJtYWluIiwibGFiZWxzIjpbImFyZWEvZGVwZW5kZW5jeSJdfQ==-->

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement for improvements in existing functionality in the changelog.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants