belgif / rest-guide

REST Guidelines of Belgian government institutions
https://www.belgif.be/specification/rest/api-guide/
Apache License 2.0
25 stars 4 forks source link

Require access token for /health endpoint? #107

Closed jpraet closed 9 months ago

jpraet commented 1 year ago

The REST Guide states on https://www.belgif.be/specification/rest/api-guide/#health that "When invoked without any access token, the resource simply returns its status."

Is it really recommended to allow unprotected access to /health endpoint without access token? It seems to unnecessarily increase the attack surface.

This is also contradicted by

https://github.com/belgif/openapi-common/blob/1af5851d093ec6142052e9b5c876194d6914c615/src/main/openapi/common/v1/common-v1.yaml#L12

pvdbosch commented 1 year ago

This reminds me of similar discussion #96 on access to the /doc resources.

The description in the OpenAPI "This resource is only available to supervision users" seems to be Smals-specific and an old remnant; should be updated.

I think we can at least change the guide's requirement to "Access to this /health resource SHOULD be granted to any user of the API.", or maybe leave the decision entirely up to the API provider.

It might be better if an API provider offers one central API that returns the health of all its APIs. This API could call internally all /health endpoints of individual web services.

Advantages:

pvdbosch commented 1 year ago

I removed the Smals-specific operation description in https://github.com/belgif/openapi-common/pull/5, remainder of this issue still to be discussed

pvdbosch commented 11 months ago

Conclusion of the WG:

weaken restriction to: -> "Access to this /health resource SHOULD be granted to any client of the API."

This includes the case that if the API allows non-logged in users access to its operations, the health check should be accessible by non-logged in users as well.

Also mention attention point: denial of service - caching

PR will be prepared for next meeting

pvdbosch commented 11 months ago

PR is ready for review.

I rewrote the health check section as an explicit rule.

Also added :

Clients SHOULD interpret any other HTTP 5xx status, or lack of HTTP response, as the API being unreachable.

Note the "unreachable" instead of "down/unavailable", bc middle boxes, connectivity issues, ...

pvdbosch commented 9 months ago

PR was merged, will be part of next release