[SQSERVICES-1980] Guest Links Password Retry Limit #3202

battermann · 2023-03-29T13:03:00Z

https://wearezeta.atlassian.net/browse/SQSERVICES-1980

/conversations/join should have the same rate limiting settings as /login.

Checklist

Add a new entry in an appropriate subdirectory of changelog.d
Read and follow the PR guidelines

battermann · 2023-03-29T13:03:41Z

This renders like this:

location ~* ^(/v[0-9]+)?/conversations/join {
    limit_req zone=reqs_per_user burst=20;
    limit_req zone=reqs_per_addr burst=10 nodelay;
}

jschaul · 2023-03-29T13:08:42Z

what are you trying to achieve here? Missing context, please link jira issue or describe problem you're trying to solve.

By default the endpoint is behind authorization so only the per_user rate limit applies, the per_ip does not, or is not really so relevant. Do you expect people to be able to call this endpoint without being logged in to Wire?

jschaul · 2023-03-29T13:09:19Z

This renders like this:

location ~* ^(/v[0-9]+)?/conversations/join {
    limit_req zone=reqs_per_user burst=20;
    limit_req zone=reqs_per_addr burst=10 nodelay;
}

please also show the rendered diff to what it would render normally; without the rate limit changes you added.

battermann · 2023-03-29T13:16:31Z

3256a3257,3297
>         location ~* ^(/v[0-9]+)?/conversations/join {
>     
>             # remove access_token from logs, see 'Note sanitized_request' above.
>             set $sanitized_request $request;
>             if ($sanitized_request ~ (.*)access_token=[^&\s]*(.*)) {
>                 set $sanitized_request $1access_token=****$2;
>             }
>             limit_req zone=reqs_per_user burst=20;
>             limit_req zone=reqs_per_addr burst=10 nodelay;
>     
>             if ($request_method = 'OPTIONS') {
>                 add_header 'Access-Control-Allow-Methods' "GET, POST, PUT, DELETE, OPTIONS";
>                 add_header 'Access-Control-Allow-Headers' "$http_access_control_request_headers, DNT,X-Mx-ReqToken,Keep-Alive,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type";
>                 add_header 'Content-Type' 'text/plain; charset=UTF-8';
>                 add_header 'Content-Length' 0;
>                 return 204;
>             }
>     
>             proxy_pass         http://galley;
>             proxy_http_version 1.1;
>             client_max_body_size 256k;
>     
>                 
>             proxy_set_header   Connection     "";
>                 
>             proxy_set_header   Authorization  "";
>     
>             proxy_set_header   Z-Type         $zauth_type;
>             proxy_set_header   Z-User         $zauth_user;
>             proxy_set_header   Z-Client       $zauth_client;
>             proxy_set_header   Z-Connection   $zauth_connection;
>             proxy_set_header   Z-Provider     $zauth_provider;
>             proxy_set_header   Z-Bot          $zauth_bot;
>             proxy_set_header   Z-Conversation $zauth_conversation;
>             proxy_set_header   Request-Id     $request_id;more_set_headers 'Access-Control-Allow-Origin: $cors_header';
>     
>             more_set_headers 'Access-Control-Expose-Headers: Request-Id, Location';
>             more_set_headers 'Request-Id: $request_id';
>             more_set_headers 'Strict-Transport-Security: max-age=31536000; preload';
>         }
>     
4421c4462
<         checksum/configmap: 7e6b878a5e95c15643e503b60fcb3b2cc8d46a32fad034bab1d09aa93d47e0b7
---
>         checksum/configmap: 03e8bb513fc307f2e7cd4f2cf4262248277b635c95d8072e23b0c2e928d85a37

battermann · 2023-03-29T13:19:44Z

3256a3257,3297
>         location ~* ^(/v[0-9]+)?/conversations/join {
>     
>             # remove access_token from logs, see 'Note sanitized_request' above.
>             set $sanitized_request $request;
>             if ($sanitized_request ~ (.*)access_token=[^&\s]*(.*)) {
>                 set $sanitized_request $1access_token=****$2;
>             }
>             limit_req zone=reqs_per_user burst=20;
>             limit_req zone=reqs_per_addr burst=10 nodelay;
>     
>             if ($request_method = 'OPTIONS') {
>                 add_header 'Access-Control-Allow-Methods' "GET, POST, PUT, DELETE, OPTIONS";
>                 add_header 'Access-Control-Allow-Headers' "$http_access_control_request_headers, DNT,X-Mx-ReqToken,Keep-Alive,User-Agent,X-Requested-With,If-Modified-Since,Cache-Control,Content-Type";
>                 add_header 'Content-Type' 'text/plain; charset=UTF-8';
>                 add_header 'Content-Length' 0;
>                 return 204;
>             }
>     
>             proxy_pass         http://galley;
>             proxy_http_version 1.1;
>             client_max_body_size 256k;
>     
>                 
>             proxy_set_header   Connection     "";
>                 
>             proxy_set_header   Authorization  "";
>     
>             proxy_set_header   Z-Type         $zauth_type;
>             proxy_set_header   Z-User         $zauth_user;
>             proxy_set_header   Z-Client       $zauth_client;
>             proxy_set_header   Z-Connection   $zauth_connection;
>             proxy_set_header   Z-Provider     $zauth_provider;
>             proxy_set_header   Z-Bot          $zauth_bot;
>             proxy_set_header   Z-Conversation $zauth_conversation;
>             proxy_set_header   Request-Id     $request_id;more_set_headers 'Access-Control-Allow-Origin: $cors_header';
>     
>             more_set_headers 'Access-Control-Expose-Headers: Request-Id, Location';
>             more_set_headers 'Request-Id: $request_id';
>             more_set_headers 'Strict-Transport-Security: max-age=31536000; preload';
>         }
>     
4421c4462
<         checksum/configmap: 7e6b878a5e95c15643e503b60fcb3b2cc8d46a32fad034bab1d09aa93d47e0b7
---
>         checksum/configmap: 03e8bb513fc307f2e7cd4f2cf4262248277b635c95d8072e23b0c2e928d85a37

The result is basically that conversations/join has now additionally a rate limiting per IP config additionally to the per-user config:

limit_req zone=reqs_per_addr burst=10 nodelay;

jschaul · 2023-03-29T15:20:20Z

changelog.d/2-features/pr-3202

@@ -0,0 +1 @@
+`conversations/join` endpoint rate limited per IP address


I suppose this introduces the conversations/join endpoint in the public API. The rate limit is secondary. Can you reformulate and move this to API changes subfolder?

jschaul · 2023-03-29T15:26:50Z

charts/nginz/values.yaml

@@ -444,6 +444,11 @@ nginx_conf:
      - all
      doc: true
      oauth_scope: conversations_code
+    - path: /conversations/join


Do I understand correctly that this is an authenticated-endpoint-only?

I don't have enough context here to actually approve this PR. The rate limit bit is fine; but this adds a new public endpoint which I don't know exactly how it works what it does and how product wise it's supposed to be used.

But the existing guest links use /conversations/code (not /join). So shouldn't this modify the existing API's rate limiting and not introduce this new endpoint? Do client teams switch over to this other endpoint? Where was this implemented to start with?

Do I understand correctly that this is an authenticated-endpoint-only?

I don't have enough context here to actually approve this PR. The rate limit bit is fine; but this adds a new public endpoint which I don't know exactly how it works what it does and how product wise it's supposed to be used.

But the existing guest links use /conversations/code (not /join). So shouldn't this modify the existing API's rate limiting and not introduce this new endpoint? Do client teams switch over to this other endpoint? Where was this implemented to start with?

This PR does not introduce a new endpoint. It was already there before and did match the following directive:

- path: /conversations envs: - all

I just created a new entry, so that the configuration is only applied to the path /conversations/join.

Ah I see. Okay. That's fine. Thanks for the context.

jschaul · 2023-03-29T16:04:42Z

This PR also needs a rebase on develop.

Also, what is the relation here with /conversations/code-check? (which already has a normal, per-ip rate limit applied)?

battermann · 2023-03-30T06:27:51Z

This PR also needs a rebase on develop.

Also, what is the relation here with /conversations/code-check? (which already has a normal, per-ip rate limit applied)?

Here is a list of conversation endpoints that should give an overview over the relations between them:

POST /conversations/:cid/code
- Gets existing conversation code
GET /conversations/:cid/code
- Creates a conversation code
POST /conversations/code-check
- Checks the validity of a conversation code
GET /conversations/join
- Gets limited conversation information by key/code pair
POST /conversations/join
- Joins a conversation using a reusable code

The last one is the endpoint in question.

…-guest-links-password-retry-limit-3

jschaul

Thank you for all the context. LGTM.

jschaul · 2023-04-03T09:58:04Z

charts/nginz/values.yaml

@@ -444,6 +444,11 @@ nginx_conf:
      - all
      doc: true
      oauth_scope: conversations_code
+    - path: /conversations/join


Ah I see. Okay. That's fine. Thanks for the context.

rate limit per ip for /conversations/join

af18bad

zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Mar 29, 2023

battermann changed the title ~~rate limit per ip for /conversations/join~~ [SQSERVICES-1980] Guest Links Password Retry Limit Mar 29, 2023

battermann marked this pull request as ready for review March 29, 2023 13:29

changelog

28233c0

battermann requested a review from jschaul March 29, 2023 13:38

jschaul reviewed Mar 29, 2023

View reviewed changes

Merge remote-tracking branch 'origin/develop' into SQSERVICES-1980-be…

46b135c

…-guest-links-password-retry-limit-3

jschaul approved these changes Apr 3, 2023

View reviewed changes

battermann merged commit 3a8a19e into develop Apr 3, 2023

battermann deleted the SQSERVICES-1980-be-guest-links-password-retry-limit-3 branch April 3, 2023 09:59

battermann mentioned this pull request Apr 3, 2023

[SQSERVICES-1980] Guest links password retry limit #3178

Closed

2 tasks

zebot mentioned this pull request Apr 17, 2023

Release 2023-04-17 - (expected chart version 4.35.0) #3230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[SQSERVICES-1980] Guest Links Password Retry Limit #3202

[SQSERVICES-1980] Guest Links Password Retry Limit #3202

Uh oh!

battermann commented Mar 29, 2023 •

edited

Loading

Uh oh!

battermann commented Mar 29, 2023

Uh oh!

jschaul commented Mar 29, 2023

Uh oh!

jschaul commented Mar 29, 2023

Uh oh!

battermann commented Mar 29, 2023

Uh oh!

battermann commented Mar 29, 2023

Uh oh!

jschaul Mar 29, 2023

Uh oh!

jschaul Mar 29, 2023

Uh oh!

battermann Mar 30, 2023

Uh oh!

jschaul Apr 3, 2023

Uh oh!

jschaul commented Mar 29, 2023 •

edited

Loading

Uh oh!

battermann commented Mar 30, 2023

Uh oh!

jschaul left a comment

Uh oh!

jschaul Apr 3, 2023

Uh oh!

Uh oh!

		@@ -0,0 +1 @@
		`conversations/join` endpoint rate limited per IP address

[SQSERVICES-1980] Guest Links Password Retry Limit #3202

[SQSERVICES-1980] Guest Links Password Retry Limit #3202

Uh oh!

Conversation

battermann commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

battermann commented Mar 29, 2023

Uh oh!

jschaul commented Mar 29, 2023

Uh oh!

jschaul commented Mar 29, 2023

Uh oh!

battermann commented Mar 29, 2023

Uh oh!

battermann commented Mar 29, 2023

Uh oh!

jschaul Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

jschaul Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

battermann Mar 30, 2023

Choose a reason for hiding this comment

Uh oh!

jschaul Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

jschaul commented Mar 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

battermann commented Mar 30, 2023

Uh oh!

jschaul left a comment

Choose a reason for hiding this comment

Uh oh!

jschaul Apr 3, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

battermann commented Mar 29, 2023 •

edited

Loading

jschaul commented Mar 29, 2023 •

edited

Loading