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

Subgraph level APQ should execute before AWS Sigv4 signing #3608

Closed
Geal opened this issue Aug 18, 2023 · 5 comments · Fixed by #3735
Closed

Subgraph level APQ should execute before AWS Sigv4 signing #3608

Geal opened this issue Aug 18, 2023 · 5 comments · Fixed by #3735
Assignees

Comments

@Geal
Copy link
Contributor

Geal commented Aug 18, 2023

Right now SigV4 signing happens as a subgraph service plugin, so before going through rhai, coprocessor and traffic shaping (dedup, timeout, retry, rate limit), and then into the subgraph service. Inside the subgraph service, there is the APQ implementation.
APQ removes the query from the body and replaces it with a hash, and if APQ is disabled by the subgraph, or not yet available in the subgraph's APQ cache, then a new request with the query is sent. Both queries will modify the body that was signed by SigV4, so signature verification will fail.
And after APQ, the body is compressed.

Here is the order that would make sense:

  • deduplication
  • timeout: it must cover an entire subgraph call from the point of view of the execution service, even if that means multiple subgraph calls due to retries or APQ). It could be used before deduplication, but if deduplicated queries have the same timeout, then only the first one could reach the limit
  • APQ
  • compression (happens before retry: no need to do that work multiple times)
  • retry
  • rate limiting
  • AWS SigV4 (the signature holds the current time, so it must change on each retry)

Here we're reaching a design issue: we are missing a service between the subgraph service and the actual HTTP call. By separating in 2 services, we would be able to do:

  • subgraph service plugin:
    • deduplication
    • timeout
    • rhai
    • coprocessor
  • in subgraph service:
    • APQ
  • subgraph HTTP service plugin:
    • compression
    • retry
    • rate limiting
    • AWS SigV4
  • in subgraph HTTP service:
    • TLS
    • HTTP call

This would require splitting some parts of the traffic shaping plugin, but at this point I feel they would live better directly in the relevant service instead of all in a layer

@grnikhil
Copy link

Hi Team, I currently used the new plugin for subgraph authentication and facing the sign calculation mismatch errors in the response. I also tried to disable the apq but its still failing. Could you please suggest if yaml needs to some other configuration to make it work ?

@Geal
Copy link
Contributor Author

Geal commented Aug 29, 2023

what do you have in the subgraph configuration? APQ? Compression? Header manipulation?

@grnikhil
Copy link

Below is the yaml snippet. I'm doing the header manipulation in supergraph service to add custom header but nothing in subgraph service. I checked the source code and the authentication plugin is called immediately when request hits subgraph service and later apq/compression code is invoked. even if apq is disabled, compression still happens which mutates the request body. Service also add few headers. I think this is causing the signature mismatch when request hits aws api gateway. I think I agree that the signing should happen just inside call_http function just before do_fetch() is invoked so that no mutation happens after signing

apq:
enabled: false
subgraph:
all:
enabled: false

authentication:
subgraph:
subgraphs:
my_subgraph_name: # configuration that will apply to all subgraphs
aws_sig_v4:
default_chain:
region: "my_api_gateway_region"
service_name: "execute-api"
assume_role:
role_arn: "my_service_role_arn

@o0Ignition0o
Copy link
Contributor

Hey, thanks for the reply! I'll see if i can provide a fix.

@o0Ignition0o
Copy link
Contributor

@grnikhil #3735 is early wip but it should fix it once it lands.

o0Ignition0o added a commit that referenced this issue Sep 11, 2023
…ression and APQ (#3735)

Fix #3608

The router now adds SigningParams to the private context, which the
subgraph service can use to sign http calls before the HTTP fetch is
made (for websocket connection and regular http calls)
garypen pushed a commit that referenced this issue Sep 12, 2023
…ression and APQ (#3735)

Fix #3608

The router now adds SigningParams to the private context, which the
subgraph service can use to sign http calls before the HTTP fetch is
made (for websocket connection and regular http calls)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants