Closed Bug 1619652 Opened 5 years ago Closed 4 years ago

Connect queries through web-server, microservices, and auth service with a requestId

Categories

(Taskcluster :: Services, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

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.

Brian, do you want to take a look at this and see what needs to be done in more detail?

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: nobody → bstack
Status: NEW → ASSIGNED

Any updates on this one? It came up again yesterday, so just wanted to check in quick.

This is in-progress now! Should be ready to land soon.

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.

Flags: needinfo?(abahnken)
Blocks: 1635571

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.

Flags: needinfo?(abahnken)

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.

Flags: needinfo?(abahnken)

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?

: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.

Flags: needinfo?(abahnken) → needinfo?(bstack)

(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.

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.

Flags: needinfo?(bstack)

per bstack's request, here is the PR to add this to taskcluster's nginx: https://github.com/mozilla-services/cloudops-infra/pull/2127

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!

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.