Issue #2068 boolean flag change

[johnekent]

No description provided.

[johnekent]

Updated the pre-commit as well, since flake8 location changed. That's why multiple commits.

[ianco]

This returns 401 Unauthorized is an admin api key is enabled, which is probably not what you want.

E.g aca-py as ./bin/aca-py start --endpoint 8888 --no-ledger -it http 0.0.0.0 8887 -ot http --admin 0.0.0.0 8889 --admin-api-key 666 and then in another shell curl -I http://localhost:8889/

There are a few url's that are unprotected (even when there is an api key), for example you can still curl http://localhost:8889/api/doc (as well as the status url's that Wade mentioned)

[johnekent]

@ianco - This change was not initially intended to impact security behavior, but you make a good point. With allow_head = True or False, the server returns a 401 error with admin api key enabled. With the api key enabled the development proxy would not work which is currently OK, but this may have gaps in the future.
This feature only currently considers HEAD calls with admin-insecure-mode, so that the default route returns a 302 instead of a 405 with the corresponding error in the server log. In my development proxy situation, I can't configure it to use any routes other than the default.
The security risk of adding this default route to the unprotected set isn't clear to me.

Do you think it would be better to first handle the initial use case (insecure mode) now, and then consider the admin api key scenario in the future? Or, should more thought be given to adding the default route to the unprotected set (or similar)? If you provide direction on where in the code base the unprotected set is configured, I can also try to see how that works across the use case variations.

[WadeBarnes]

I don't think it would be advisable to add the default route to the set of unprotected routes.

@johnekent, Are you able to provide more details on why your setup requires allow_head = True? Perhaps there is another way to accomplish the same thing. Would there be an issue in you're scenario if the admin api key were enabled?

[ianco]

@johnekent the list of unprotected paths is defined here: https://github.com/hyperledger/aries-cloudagent-python/blob/main/aries_cloudagent/admin/server.py#L300

These include the /api/doc endpoint (so you can connect to the swagger page and authenticate) and the status endpoints that we use for our deployments. (All of our deployments are secured by an api key, but we don't require the api key access the status endpoints.)

It's recommended to enable the api key for all deployments (other than local dev). Could your do a HEAD query against one of our existing status endpoints?

[johnekent]

@WadeBarnes I am using the Coder development environment which provides a Dev URLs feature for ports (admin, endpoints, etc.) to be available as a reverse proxy - e.g. for accessing the swagger docs from a browser, or for a controller UI to connect with an agent. The majority of work will be on developing controllers, which shouldn't require admin api keys. I.e., this is for controller development, not for server administration/configuration.

@ianco the setup described above would be considered local dev, and would only be used for those purposes. The dev urls configuration can't specify any endpoints, only server and port.

If this feature can be merged, then Coder can be used for controller development. That seems like a benefit to the aries ecosystem that doesn't cause any harm and also doesn't change the current security. However, I am open to exploring alternatives.

[ianco]

OK, I'm ok with this as long as @WadeBarnes is ok with it lol

It won't cause any hard because the HEAD method can't be used if the admin api key is enabled, so it's only available in a non-secure environment.

(Btw the admin api key is definitely intended for use with controllers, it's all that's protecting aca-py's admin api from malicious use. If you deploy aca-py into a prod environment with no api key and someone gains access to the admin url they can issue themselves credentials under your identity.)

[johnekent]

Thanks @ianco . I will make sure the overall development cycle for controllers includes usage of the api keys; thanks for the additional consideration.

[johnekent]

@ianco and @WadeBarnes what about this addition - adding HEAD to the existing OPTIONS check. This allows both use cases while extending the existing security for method exceptions.

[WadeBarnes]

Allowing HEAD requests to bypass security when the admin api key is enabled would falsely indicate the endpoints were available when in fact they would not respond to any other request. It also opens the door for the API to be more easily scanned when it's supposed to be secured.

Allowing HEAD under the regular security constraints would be fine. Meaning HEAD requests could be used in insecure mode, or secure mode provided the api key was also supplied in the request.

[WadeBarnes]

Please revert this change.

[johnekent]

Reverted. Put it for review because it was another approach, similar to OPTIONS. Thanks for reviewing.

[WadeBarnes]

@johnekent, Looks good to me. Could you please squash and rebase your changes into a single commit and add DCO sign-off please?

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
No Duplication information

[johnekent]

Appreciate the patience and feedback here @WadeBarnes and @ianco . Squashed back to what's ultimately the initial commit, and I believe the DCO's on that one. Will check back after the scans. Thanks.

[WadeBarnes]

@ianco, ready for your review and approval.