Connect queries through web-server, microservices, and auth service with a requestId
Categories
(Taskcluster :: Services, enhancement)
Tracking
(Not tracked)
People
(Reporter: dustin, Assigned: bstack)
References
(Blocks 1 open bug)
Details
For some requests, there are three services involved:
- browser makes GraphQL POST to web-server
- web-server calls REST API on another service (e.g., secrets.get)
- that service calls auth.authenticateHawk
Bug 1547731 covers logging the requests in the first case.
But, correlating all of these would be much easier if they all logged the same requestId. Then for example if we saw a log of an unexpected secret fetch in the secrets service, we could use the requestId to correlate that with the authenticateHawk call and with the GraphQL query that caused it, and trace that to its originating IP.
Reporter | ||
Comment 1•5 years ago
|
||
Brian, do you want to take a look at this and see what needs to be done in more detail?
Reporter | ||
Comment 2•5 years ago
|
||
Bug 1547731 being complete, I think what remains is:
- configure web-server to pass the requestId through to all REST API methods (may require client changes)
- configure API methods to accept a requestId in a header (may already be the case)
- configure API method logging to include the requestId (I think this is already the case)
Assignee | ||
Updated•5 years ago
|
Comment 3•5 years ago
|
||
Any updates on this one? It came up again yesterday, so just wanted to check in quick.
Assignee | ||
Comment 4•5 years ago
|
||
This is in-progress now! Should be ready to land soon.
Assignee | ||
Comment 5•5 years ago
|
||
Discussing some bits of this with bpitts on Monday w.r.t. the trace headers we get forwarded to us by nginx so that we can hopefully correlate these logs with nginx logs and even gclb as well.
Other than that we have a mostly done implantation handling the interesting bits of threading traceIds around the tc services themselves.
Also are/were worried about how to handle incoming requests from untrusted sources with a trace header already specified. Discussed it a bit with ajvb in slack and NI'ing here to continue the discussion. tl;dr is that due to how tc works with inter-service routing, there's no real concept of "internal" vs "external" requests so we don't know if a trace header is legit.
tc team has some ideas about how we could go about this but want to get your input first before we do anything else.
Assignee | ||
Comment 6•5 years ago
|
||
https://github.com/taskcluster/taskcluster/pull/2774 is up for review now.
Comment 7•5 years ago
|
||
Brian and I met and discussed the security concern. We decided that nginx will set the taskcluster trace header and that will be forwarded along. Internal traffic within Taskcluster won't go through nginx again, therefore the header will be kept in tact.
Assignee | ||
Comment 8•4 years ago
|
||
We've updated the tc side to only work with x-taskcluster-trace-id
header. Next step is to get nginx to set this header. bpitts has helped me start with
set_by_lua_block $trace_id { return "however you generate a random string in lua" }
proxy_set_header x-taskcluster-trace-id $trace_id;
But I'd like some input on the "however you generate a random string in lua"
step. There appear to be libraries like this one that will do it for us but I don't know how we feel about adding things like that.
Ideally we'd end up with a uuid. I can do the implementation once we decide how we'd like to go about it.
Assignee | ||
Comment 9•4 years ago
|
||
bpitts found that we already have https://openresty.org/en/lua-resty-string-library.html and it supports random string generation.
Should we just use that?
Comment 10•4 years ago
|
||
:bstack - Just tried the lua-resty-string lib out within iprepd-nginx and seems like it may be what you are looking for. I'm good with either of those libraries and they are both in opm, so let me know which you'd prefer and I can get it added to the base iprepd-nginx image.
Comment 11•4 years ago
|
||
(In reply to AJ Bahnken [:ajvb] from comment #10)
:bstack - Just tried the lua-resty-string lib out within iprepd-nginx and seems like it may be what you are looking for. I'm good with either of those libraries and they are both in opm, so let me know which you'd prefer and I can get it added to the base iprepd-nginx image.
In current state iprepd-nginx acts as a middleware that can apply security policies and can be swapped out as needed, but it sounds like with this change it will become a dependency to maintain the traceability in the application audit trail.
Not a problem or anything; discussed with :bpitts as well and he is OK with this but I am wondering if we should consider setting these headers in the application if they do not exist from upstream (as a follow up item, not necessarily right away). The reason being avoiding a scenario where iprepd-nginx is disabled/replaced/etc and we end up losing the request audit trail as a secondary effect.
Assignee | ||
Comment 12•4 years ago
|
||
Ah, the application will set them if they're not defined currently. Just means that it doesn't correlatete application logs with nginx logs if the header is not there.
Comment 13•4 years ago
|
||
per bstack's request, here is the PR to add this to taskcluster's nginx: https://github.com/mozilla-services/cloudops-infra/pull/2127
Assignee | ||
Comment 14•4 years ago
|
||
This is landed and deployed. bpitts is going to do followup work of having nginx generate this traceId and forward it along. Marking this bit of implementation as complete!
Description
•