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

Lack of User-Agent header in document loader can cause http 403 errors with some domains #2781

Closed
gmulhearn-anonyome opened this issue Feb 13, 2024 · 9 comments

Comments

@gmulhearn-anonyome
Copy link
Contributor

Currently the Document loader in acapy does not include/overwrite the User-Agent header when making requests. The result of this is that a default User-Agent of python-requests/x.x.x (python-requests/2.31.0) is applied to the request. E.g. if i include a JSON-LD context which points to my local ngrok, we can inspect what acapy is sending:
image

We've found that this User-Agent, in combination with sending it from an AWS origin (i.e. acapy running inside AWS) can cause certain JSON-LD context documents to not fetch, and a 403 is returned. When the 403 is returned, the error is somewhat silent (except for in logs) and the result is usually verified=false for the JSON-LD document being verified.

This is most likely due to firewall configurations on those context document servers which flag the combination of User-Agent + IP address as unwanted. A similar issue had by someone online: adoptium/api.adoptium.net#580

The context which was throwing 403 for us was the DIF presentation exchange context: https://identity.foundation/presentation-exchange/submission/v1 . Which is fetched when acapy is verifying a well-formed DIF VP and needs to expand the document. Note that this error does not occur for ACApy <-> ACApy DIF VP presentations, as ACApy is missing those contexts when it acts as a holder: #2634

A patch we're experimenting with currently is adding in the User-Agent header before making the request. It seems to be working. See the diff here: https://github.com/anonyome/aries-cloudagent-python/pull/1/files . However ACApy might be more interested in a configurable way of setting the User-Agent?

@swcurran
Copy link
Contributor

@dbluhm @esune — any feedback on this one as the implementation is done? Thanks

@dbluhm
Copy link
Contributor

dbluhm commented Feb 13, 2024

The fix in the diff you linked seems like a reasonable approach to me!

@esune
Copy link
Member

esune commented Feb 13, 2024

If we agree on a user agent naming convention (I agree that what is in the linked diff seems reasonable) maybe we should use it as user agent for ALL requests made from ACA-Py for consistency?

@swcurran
Copy link
Contributor

swcurran commented Mar 5, 2024

@gmulhearn-anonyome you mentioned the following. Do you have a PR you could offer for this? We'd like to get this addressed. Thanks!

A patch we're experimenting with currently is adding in the User-Agent header before making the request. It seems to be working. See the diff here: https://github.com/anonyome/aries-cloudagent-python/pull/1/files . However ACApy might be more interested in a configurable way of setting the User-Agent?

@gmulhearn-anonyome
Copy link
Contributor Author

gmulhearn-anonyome commented Mar 5, 2024

hey @swcurran , have made a PR with the patch #2824 . Though I do agree that improvements generally could be made as per @esune 's comment. e.g.:

  • globally use this user agent header
  • perhaps configurable user agent? or atleast extensible?

@esune
Copy link
Member

esune commented Mar 6, 2024

@gmulhearn-anonyome what do you mean by "extensible" exactly?

My only concern with applying a patch for this endpoint only is that we won;t be using the user agent elsewhere and it will cause technical debt. If we are fine with that for now, we should at least make sure to log an issue to tackle the "global" setting in the near future.

@gmulhearn-anonyome
Copy link
Contributor Author

gmulhearn-anonyome commented Mar 6, 2024

@esune it was just a random idea. e.g. by extensible, maybe we lock in the base User Agent as something like ACApy/x.x.x, and then the consumer of acapy could add some config to extend it with their own app-specific suffix. e.g.

ACApy/x.x.x MyCustomControllerProduct/1.0.0.

Anyway, not my call; not 100% sure what devs want here.

@esune
Copy link
Member

esune commented Mar 7, 2024

Thanks for the clarification @gmulhearn-anonyome , it makes sense now 🙂

dbluhm added a commit that referenced this issue Mar 12, 2024
patch for #2781: User Agent header in doc loader
@dbluhm
Copy link
Contributor

dbluhm commented Mar 12, 2024

Resolved by #2824; further discussion on #2834 for the general case of setting User-Agent header and whether we want to make the value used configurable.

@dbluhm dbluhm closed this as completed Mar 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants