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

refactor(gateway): authentication middlewares and context information #3736

Merged
merged 5 commits into from
Aug 17, 2023

Conversation

atzoum
Copy link
Contributor

@atzoum atzoum commented Aug 10, 2023

Description

  • Introducing authentication middlewares in gateway and propagating source authentication information in the request's context for subsequent handlers to use.
    • writeKeyAuth: authenticates writeKey in the Authorization header and validates that the source is enabled.
    • webhookAuth: authenticates writeKey in the Authorization header or as a query parameter and validates that the source is enabled and is of type webhook.
    • sourceIDAuth: authenticates sourceID in the X-Rudder-Source-Id header and validates that the source is enabled.
  • Using http server interceptors for special scenarios:
    • beaconInterceptor: reads the writeKey from the query params and sets it in the request's Authorization header
    • pixelInterceptor: reads information from the query parameters to fill in the request's body and authorization header before passing it to the next handler and finally writes a pixel response to the client regardless of the next handler's response
  • Simplifying gateway's backend configuration state synchronisation which now only needs to maintain 2 maps so that it can retrieve source configuration during runtime from either a sourceID or a writeKey.
  • Removing admin handler from gateway, since it is not used.
  • Adding more test scenarios in gateway.

Linear Ticket

Link

Security

  • The code changed/added as part of this pull request won't create any security issues with how the software is being used.

@atzoum atzoum changed the title [WIP] refactor(gateway): authentication middleware [WIP] refactor(gateway): authentication middlewares and authentication information Aug 10, 2023
@atzoum atzoum changed the title [WIP] refactor(gateway): authentication middlewares and authentication information [WIP] refactor(gateway): authentication middlewares and context information Aug 10, 2023
@codecov
Copy link

codecov bot commented Aug 10, 2023

Codecov Report

Patch coverage: 86.08% and project coverage change: +0.20% 🎉

Comparison is base (8ab2ac2) 68.44% compared to head (f43600c) 68.64%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3736      +/-   ##
==========================================
+ Coverage   68.44%   68.64%   +0.20%     
==========================================
  Files         337      344       +7     
  Lines       51602    51565      -37     
==========================================
+ Hits        35320    35398      +78     
+ Misses      14004    13885     -119     
- Partials     2278     2282       +4     
Files Changed Coverage Δ
app/apphandlers/embeddedAppHandler.go 75.95% <ø> (-0.28%) ⬇️
app/apphandlers/gatewayAppHandler.go 13.72% <ø> (+0.39%) ⬆️
gateway/response/response.go 100.00% <ø> (ø)
utils/misc/misc.go 60.66% <ø> (-0.06%) ⬇️
gateway/handle_diagnostics.go 46.15% <46.15%> (ø)
gateway/handle_http_pixel.go 70.58% <70.58%> (ø)
gateway/webhook/webhook.go 60.47% <76.00%> (+4.36%) ⬆️
gateway/handle_webhook.go 80.00% <80.00%> (ø)
gateway/handle_http.go 83.33% <81.08%> (+24.14%) ⬆️
gateway/import_handler.go 87.50% <82.85%> (-12.50%) ⬇️
... and 10 more

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@atzoum atzoum force-pushed the chore.gatewayAuth branch 12 times, most recently from b9f9c12 to 8529b33 Compare August 11, 2023 10:20
@atzoum atzoum changed the title [WIP] refactor(gateway): authentication middlewares and context information refactor(gateway): authentication middlewares and context information Aug 11, 2023
@atzoum atzoum marked this pull request as draft August 11, 2023 10:35
@atzoum atzoum marked this pull request as ready for review August 11, 2023 12:22
gateway/handle_observability.go Outdated Show resolved Hide resolved
@@ -124,42 +113,12 @@ func (webhook *HandleT) failRequest(w http.ResponseWriter, r *http.Request, reas
}

func (webhook *HandleT) RequestHandler(w http.ResponseWriter, r *http.Request) {
reqType := r.Context().Value(gwtypes.CtxParamCallType).(string)
Copy link
Member

Choose a reason for hiding this comment

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

I am bit concerned with the usage of context values. Because of their impact in readability and losing compile time checks.

I need to dice deeper into the code to understand if we are able to avoid them and at what costs.

However, if we keep them I would suggest creating a function that takes a request as parameter and returns reqType.

Copy link
Contributor Author

@atzoum atzoum Aug 14, 2023

Choose a reason for hiding this comment

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

If it helps, all code paths which are using context values are covered with tests and if improper usage was introduced, casting errors would be recorded.

gateway/handle_http_auth.go Outdated Show resolved Hide resolved
gateway/handle_http_auth.go Show resolved Hide resolved
@atzoum atzoum force-pushed the chore.gatewayAuth branch 2 times, most recently from 6baeca0 to a030a3e Compare August 16, 2023 10:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants