search

ga4gh / workflow-execution-service-schemas

The WES API is a standard way to run and manage portable workflows.
Apache License 2.0
82 stars 38 forks source link

Throttling access to WES #54

Open rishidev opened 6 years ago

rishidev commented 6 years ago

Steps can be taken to help prevent a Denial of Service attack

denis-yuen commented 6 years ago

FYI @apmasell

geoffjentry commented 6 years ago

This seems like an implementation detail and not something which should be in the spec

apmasell commented 6 years ago

When to throttle is implementation-specific, but how the server tells the client that it is in an overload state needs to be part of the specification so that clients know they must handle that case and so there is a standard signalling mechanism.

geoffjentry commented 6 years ago

@apmasell Ah, I see. You're looking to standardize on something like Twitter's HTTP 420? That makes sense.

Apologies as I wasn't on the call, but before we go too far down this path though, which driver project generated this feature request? It'd be good to keep non-driver project driven features to a minimum until post-Basel

apmasell commented 6 years ago

This came out of the security discussion rather than a driver project need.

rishidev commented 6 years ago

The suggestion was for the spec to specify a HTTP 409 for Try Again Later, but in the definitions it looks like this is for CONFLICT, which might not match the use case.

briandoconnor commented 6 years ago

Jeff points out this should be 429... correct?

tetron commented 6 years ago

Yes 429 is right.

ruchim commented 4 years ago

Questions about how and where this would live in the spec -- is this basically an example response code across all the endpoints?

it feels like returning 429s is just a best practice for any service implementation -- be it WES, TES, TRS, DRS etc -- or even the other streams. I feel that this is better as documentation that anything specific to WES implementors. Curious for your thoughts on that @rishidev

patmagee commented 2 years ago

I take the stance of @ruchim here, i do not really think its the role of WES to specify exactly what a response to throttling should be. For example In some scenarios I know certain WAF's will respond with 403 without the request ever reaching the WES API. We can suggest it, but IMO it should not be required in the spec

uniqueg commented 2 years ago

The way that error responses are described in the various specs, including even just the Cloud APIs, is quite inconsistent. Several of them define 400, 401, 403, 404 and 500 responses, some have a generic error model. And generally it's not clear what the expectation is for the implementation of various errors - are the responses expected to be implemented in all cases? Should the exact wording be used? What about custom errors? This makes it difficult on clients, interop and compliance testing.

I think a general strategy here would be useful. Personally, I would favor a general error response model that is consistent with https://www.rfc-editor.org/rfc/rfc7807, and then only define concrete responses if the API absolutely mandates their implementation (and then consider only including the minimal information, e.g., the code and a default message that can, however, be modified by implementers in a spec-conformant manner). Any other errors I would leave up to implementers to handle, as long as they follow the general error response model.