Include http request context in decision logs #6750
Conversation
ashutosh-narkar
force-pushed
the
update-dec-log
branch
from
7442a04
to
3c0db3d
Compare
ashutosh-narkar
changed the title
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| @@ -60,6 +60,7 @@ func (h *LoggingHandler) loggingEnabled(level logging.Level) bool { | |||
| func (h *LoggingHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) { | |||
| var rctx logging.RequestContext | |||
| rctx.ReqID = atomic.AddUint64(&h.requestID, uint64(1)) | |||
| rctx.HTTPRequestContext = logging.HTTPRequestContext{Header: r.Header.Clone()} | |||
There was a problem hiding this comment.
Similar to the requested_by field in the decision log, the request context would be available in the log when the log-level is info or higher.
| | `[_].input` | `any` | Input data provided in the policy query. | | ||
| | `[_].result` | `any` | Policy decision returned to the client, e.g., `true` or `false`. | | ||
| | `[_].requested_by` | `string` | Identifier for client that executed policy query, e.g., the client address. | | ||
| | `[_].request_context.http.headers` | `object` | Set of key-value pairs describing HTTP headers and their corresponding values. The header keys in this object are specified by the user as part of the decision log configuration. | |
There was a problem hiding this comment.
Should we further clarify to the user to expect a list of values for each header?
| for _, h := range rctx.HTTPRequest.Headers { | ||
| values := decision.HTTPRequestContext.Header.Values(h) | ||
| if len(values) > 0 { | ||
| headers[h] = decision.HTTPRequestContext.Header.Values(h) |
There was a problem hiding this comment.
Should we be sanitizing these values so a client can't do log injection attacks? Or do we perform global sanitizing somewhere before emitting data to the log?
There was a problem hiding this comment.
We don't do any sanitizing afaict. Anything particular you had in mind?
There was a problem hiding this comment.
Nothing in particular. In general though, this is a whole category of XSS attacks that target the consumer of the log. In part, this is a consideration for the consumer-side, but it could be argued that OPA, which is generating the log, should take some of the burden to sanitize the content; especially as some of the values are created wholesale by the requesting client, and not OPA.
| decisionInfo: &server.Info{Bundles: map[string]server.BundleInfo{"b1": {Revision: "A"}}, HTTPRequestContext: logging.HTTPRequestContext{Header: h2}}, | ||
| expected: &RequestContext{HTTPRequest: &HTTPRequestContext{Headers: map[string][]string{"foo": []string{"bar"}}}}, | ||
| }, | ||
| } |
There was a problem hiding this comment.
How about test cases for when there are incoming headers but the config has
- an empty
headerslist, - no
headerlist but ahttpentry, - no
httpentry?
There was a problem hiding this comment.
Added these test cases.
It would be useful if users had the ability to enhance the decision log with info from the incoming HTTP request such as headers. This change allows users to configure headers whose values if present in the incoming HTTP request would be surfaced via the decision log. This can be extended in the future to include more context from the request. Fixes: open-policy-agent#6693 Signed-off-by: Ashutosh Narkar <anarkar4387@gmail.com>
ashutosh-narkar
force-pushed
the
update-dec-log
branch
from
3c0db3d
to
90adba3
Compare
It would be useful if users had the ability to enhance the
decision log with info from the incoming HTTP request such as
headers. This change allows users to configure headers whose
values if present in the incoming HTTP request would be
surfaced via the decision log. This can be extended in the
future to include more context from the request.
Fixes: #6693