fix: avoid leak of tenant token on the logs #1620
Conversation
Changelog: None Ticket: None Signed-off-by: Luis Ramirez <[email protected]>
|
I am not a Go guy, so I am sure it can be made more elegant. Open to suggestions |
|
@MuchoLucho : Thanks for the change. From a security point of view a must have. It will be easier to share the debug logs. Very much appreciated! |
There was a problem hiding this comment.
Great first start in golang, @MuchoLucho ! See my comments below I think we can brush this up and merge it.
Also please add a good changelog entry, this is clearly user facing and we will release it.
| tentok4Log := tentok | ||
|
|
||
| // Truncates the tenant token for avoid any leak | ||
| if len(tentok) > 4 { | ||
| tentok4Log = tentok[0:2] + "..." + tentok[len(tentok)-2:] | ||
| } | ||
|
|
||
| log.Debugf("Tenant token: %s", tentok4Log) |
There was a problem hiding this comment.
With regards to the general logic. It won't happen in practice, but look at the following:
- do a copy of tenok to tenok4log
ifsomething something, then redact tenok4log for sercure logging- log unconditionally
See the issue? If the if is false then you log the original secret tenok.
There are many ways to solve this. One way:
| tentok4Log := tentok | |
| // Truncates the tenant token for avoid any leak | |
| if len(tentok) > 4 { | |
| tentok4Log = tentok[0:2] + "..." + tentok[len(tentok)-2:] | |
| } | |
| log.Debugf("Tenant token: %s", tentok4Log) | |
| tentok4Log := "" | |
| // Truncates the tenant token for avoid any leak | |
| if len(tentok) > 4 { | |
| tentok4Log = tentok[0:2] + "..." + tentok[len(tentok)-2:] | |
| } | |
| log.Debugf("Tenant token: %s", tentok4Log) |
and a simpler one:
| tentok4Log := tentok | |
| // Truncates the tenant token for avoid any leak | |
| if len(tentok) > 4 { | |
| tentok4Log = tentok[0:2] + "..." + tentok[len(tentok)-2:] | |
| } | |
| log.Debugf("Tenant token: %s", tentok4Log) | |
| // Truncates the tenant token for avoid any leak | |
| if len(tentok) > 4 { | |
| log.Debugf("Tenant token: %s..%s", tentok[0:2], tentok[len(tentok)-2:]) | |
| } |
| tentok4Log = tentok[0:2] + "..." + tentok[len(tentok)-2:] | ||
| } | ||
|
|
||
| log.Debugf("Tenant token: %s", tentok4Log) | ||
|
|
||
| // fill tenant token | ||
| authd.TenantToken = string(tentok) |
There was a problem hiding this comment.
Two lines below we have log.Debugf("Authorization data: %v", authd) which also will leak the token, right?
Changelog: None
Ticket: None
External Contributor Checklist
🚨 Please review the guidelines for contributing to this repository.
The majority of our contributions are fixes, which means your commit should have
the form below:
git --signoff. Also note that the signoff author must match the author of the commit.Description
Please describe your pull request.
Thank you!