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

Provide TraceID in the logs #2269

Merged
merged 1 commit into from
Apr 20, 2023
Merged

Conversation

RomanZavodskikh
Copy link
Member

Signed-off-by: Roman Zavodskikh [email protected]

proxy/proxy.go Fixed Show fixed Hide fixed
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch from 48b53f9 to e8d3148 Compare April 14, 2023 18:19
@AlexanderYastrebov
Copy link
Member

Please describe the problem it solves. Is this open tracing relevant only? Do we need to log it for each log statement or only for selected logs?
See also https://github.com/sirupsen/logrus#fields

@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch 2 times, most recently from df429e2 to 5320626 Compare April 17, 2023 13:29
@RomanZavodskikh RomanZavodskikh marked this pull request as draft April 17, 2023 13:36
@RomanZavodskikh
Copy link
Member Author

@AlexanderYastrebov this is a draft PR for me to create a test image and use this image to check how will it work (and would it work at all) to provide traceID inside all logs. I just picked up some logs but hope this change to have an effect on all possible logs.

@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch 6 times, most recently from 4c899fb to ddf3927 Compare April 18, 2023 13:47
proxy/proxy.go Fixed Show fixed Hide fixed
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch from ddf3927 to e839801 Compare April 18, 2023 13:58
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch 3 times, most recently from 2c09001 to cb5aa90 Compare April 19, 2023 08:13
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch 4 times, most recently from 64882d5 to 9dcbdd6 Compare April 19, 2023 14:00
filters/diag/logheader.go Fixed Show fixed Hide fixed
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch from 89cfe2d to 378b919 Compare April 19, 2023 15:19
filters/diag/logheader.go Fixed Show fixed Hide fixed
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch 2 times, most recently from 09d44d0 to 5f6d7e2 Compare April 19, 2023 15:54
@@ -83,7 +82,7 @@ func (lh logHeader) Response(ctx filters.FilterContext) {
}
buf.WriteString("\r\n")

log.Println("Response for", buf.String())
ctx.Logger().Infof("Response for " + buf.String())
Copy link
Member

Choose a reason for hiding this comment

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

Format string should always be a constant. Imagine what happens if buf contains format specifier e.g. %:

package main

import "fmt"

func main() {
	buf := "%s"
	fmt.Printf("Response for " + buf)
}

outputs

Response for %!s(MISSING)

The proper way is ctx.Logger().Infof("Response for %s", buf).
The same applies to ctx.Logger().Errorf(err.Error()) as error message potentially may contain format specifiers.

See also https://owasp.org/www-community/attacks/Format_string_attack

@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch from 5f6d7e2 to c891f0e Compare April 20, 2023 07:36
@@ -116,5 +115,5 @@
}
buf.WriteString("\r\n")

log.Println(buf.String())
ctx.Logger().Infof("%s", buf.String())

Check failure

Code scanning / CodeQL

Clear-text logging of sensitive information

[Sensitive data returned by HTTP request headers](1) flows to a logging call.
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch 3 times, most recently from b5dd90e to 9e4bb39 Compare April 20, 2023 11:47
Signed-off-by: Roman Zavodskikh <[email protected]>
@RomanZavodskikh RomanZavodskikh force-pushed the traceIDToLogsAndMonitoring branch from 9e4bb39 to 4024ece Compare April 20, 2023 11:55
@AlexanderYastrebov
Copy link
Member

👍

1 similar comment
@RomanZavodskikh
Copy link
Member Author

👍

@RomanZavodskikh RomanZavodskikh merged commit d13bd6b into master Apr 20, 2023
@RomanZavodskikh RomanZavodskikh deleted the traceIDToLogsAndMonitoring branch April 20, 2023 12:43
AlexanderYastrebov added a commit that referenced this pull request Apr 20, 2023
Followup on #2269

Signed-off-by: Alexander Yastrebov <[email protected]>
AlexanderYastrebov added a commit that referenced this pull request Apr 20, 2023
Followup on #2269

Signed-off-by: Alexander Yastrebov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants