BotBuilder, as a library, should not pin 3rd party dependencies

[cognifloyd]

Version

4.11.0

Describe the bug

I'm trying to add BotBuilder to an app that already has ibm-watson as a dependency.
The conflict is due to botbuilder-core pinning a very old version of PyJWT.
ibm-watson needs ibm-cloud-sdk-core which needs PyJWT. ibm-cloud-sdk-core uses a range of versions, specifying PyJWT>=2.0.0a1,<3.0.0 in the last few versions, but for all older versions, it required PyJWT>=1.7.1.

botbuilder-python/libraries/botframework-connector/setup.py

Line 12 in 11b0877

"PyJWT==1.5.3",

BotBuilder is a library meant to be embedded in other applications, so pinning 3rd party deps is dangerous.
Looking through the dependencies in the various botbuilder libraries, it looks like several have dependencies that are pinned instead of specifying a range of valid versions. For the microsoft-provided deps, pinning makes perfect sense. For 3rd party deps, please do not pin the dep version (instead specify a range) and allow the application that is using the botbuilder framework to pin its own deps

To Reproduce

Steps to reproduce the behavior:

1. pip install --upgrade pip (in a virtualenv) to use the new resolver that refuses to install conflicting dependencies
2. pip install ibm-watson botbuilder-core==4.11.0
3. pip backtracks for awhile until it gets to some very old versions of ibm-watson that it can't process and dies.
4. even if pip could process the old versions, no version of the ibm lib ever supported version 1.5.3 of PyJWT that was released in 2018.

Expected behavior

For microsoft/azure libraries, go ahead and pin the deps. The botbuilder itself is versioned together, so pinning makes perfect sense.

If you want to pin the version 3rd party deps in requirements.txt, that also is fine so that you can say "This is the most tested/supported version". But please, in setup.py, specify a range of valid versions for 3rd party deps to ease integrating BotBuilder in existing applications.

Additional context

Here are the 3rd party dependency lines in setup.py that pin 3rd party deps in 4.11.0 (the deps are still pinned in main/4.12.0):

PyJWT, requests, cryptography

very common packages, likely to cause conflicts. Caused a conflict for me

botbuilder-python/libraries/botframework-connector/setup.py

Lines 10 to 12 in 11b0877

	"requests==2.23.0",
	"cryptography==2.8.0",
	"PyJWT==1.5.3",

aiohttp

very common package, likely to cause conflicts

botbuilder-python/libraries/botbuilder-integration-aiohttp/setup.py

Line 12 in 11b0877

"aiohttp==3.6.2",

botbuilder-python/libraries/botbuilder-integration-applicationinsights-aiohttp/setup.py

Line 9 in 11b0877

"aiohttp==3.6.2",

botbuilder-python/libraries/botbuilder-ai/setup.py

Line 11 in 11b0877

"aiohttp==3.6.2",

Unpinning aiohttp in botbuilder-aialso requires bumping the required version of aioresponses to at least 0.7.1 to get this fix pnuckowski/aioresponses#174 (adds support for aiohttp 3.7+):

botbuilder-python/libraries/botbuilder-ai/tests/requirements.txt

Line 1 in 11b0877

aioresponses==0.6.3

babel

common package, likely to cause conflicts

botbuilder-python/libraries/botbuilder-dialogs/setup.py

Line 14 in 11b0877

"babel==2.7.0",

jsonpickle

less common package, so I'm not as concerned.

botbuilder-python/libraries/botbuilder-core/setup.py

Line 11 in 11b0877

"jsonpickle==1.2",

botbuilder-python/libraries/botbuilder-azure/setup.py

Line 12 in 11b0877

"jsonpickle==1.2",

[cognifloyd]

Why is cryptography listed in the deps for botframework-connector? It is not imported anywhere by botframework-connector. And, these deps specify their own dependencies:

• adal==1.2.1 requires cryptography>=1.1.0
• msal==1.6.0 requires cryptography>=0.6,<4
• PyJWT>=1.5.3,<2.0.0 requires cryptography>=1.4.0,<4.0.0 (newer versions bump the min, but not the max)
• requests>=2.23.0,<3.0.0 requires cryptography>=1.3.4

I think cryptography should be removed from the requirements of botframework-connector.

[ghost]

You beat me to creating an issue about this. I am facing the same hurdle, my application uses latest pyjwt 2.0 whilst botframework uses the rather ancient 1.5.3.

[cognifloyd]

@sferdi0 Can you check out where botframework-connector is using PyJWT to see what it would take to upgrade to 2.0? It is safe to upgrade to <2.0 without code changes (based on reading the changelog), but several things changed in 2.0.

[ghost]

@cognifloyd I can't validate incoming messages for my Bot, e.g: my route is /api/messages and Microsoft sends me, for example, a conversationUpdate (when an user adds my app/bot to their Teams) - and this code tries to validate the incoming request using it's Authorization header:

botbuilder-python/libraries/botframework-connector/botframework/connector/auth/skill_validation.py

Line 58 in c491636

token = jwt.decode(bearer_token, verify=False)

In the new(er) PyJWT 2.0 that should be this instead:

token = jwt.decode(token, options={"verify_signature": False})

There are more places where JWT is used throughout the code but I don't think a migration would be complicated at all.

[cognifloyd]

OK. I tested with the following dependency updates and all tests pass without any code changes.

requests, crptography, PyJWT

• libraries/botframework-connector/setup.py#L10-L11

Note that this drops the explicit cryptography requirement, as botframework-connector does not use it directly.

-    "requests==2.23.0",
+    "requests>=2.23.0,<2.26",

-    "cryptography==3.2",

-    "PyJWT==1.5.3",
+    "PyJWT>=1.5.3,<2.0.0",

aiohttp

• libraries/botbuilder-ai/setup.py#L11
• libraries/botbuilder-integration-aiohttp/setup.py#L12
• libraries/botbuilder-integration-applicationinsights-aiohttp/setup.py#L9

-    "aiohttp==3.6.2",
+    "aiohttp>=3.6.2,<3.8.0",

aioresponses

• libraries/botbuilder-ai/tests/requirements.txt#L1

-aioresponses==0.6.3
+aioresponses>=0.7.1

jsonpickle

• libraries/botbuilder-azure/setup.py#L12
• libraries/botbuilder-core/setup.py#L11

-    "jsonpickle==1.2",
+    "jsonpickle>=1.2,<1.5",

babel

I didn't test updating babel.

[cognifloyd]

At a glance, bumping to PyJWT 2.0 will also require bumping msal and possibly others, as that also requires <2.0.

I would be okay with any updated PyJWT requirement (whether or not that includes v2.0) as long as it allows installing PyJWT>=1.7.1.

Please and thank you!

[cognifloyd]

Also, if anyone else can submit a PR, that would be awesome. Sorry, I'm unable to contribute a PR myself, but I hope I've left enough breadcrumbs for others to get this updated.

[axelsrz]

Hello @cognifloyd thanks for bringing this up to our attention. We have decided in the past for a pinning strategy due to breaking changes by dependency updates appearing on our daily/PR builds.
With the arguments and situation presented in this thread we're considering to move to a "version range" policy, but we would have to draft a process for continuous development.

We will be updating this thread with any decisions taken in the following weeks, with solutions potentially similar to the one you proposed.

Let us know if you have further comments.

[compulim]

I think even though we move forward on the "version range" policy. After every release of the SDK, we should bump every dependencies to their latest version.

It's good for hygiene, progression on technical debts, and reducing chances of vulnerabilities.

@axelsrz could you bring this up in R13 planning?

[davidfishlock]

Is there any progress on this? I'm also running into this with the requests library.

If an exploit was to be found in requests then all applications using this SDK are required to remain vulnerable until botbuilder update their pinned dependency. Requests is currently pinned to a version from over a year ago.

All other SDKs that are used by applications that also declare this dependency are also being forced into specifiying a low range by this library.

If the policy is to continue pinning, then the dependencies must be tested and frequently updated as a priority. But I've not experienced an instance of requests making a breaking change without incrementing the major version, I don't really see a justification for pinning it to a minor.

[carlosscastro]

Thanks for all the work and investigation, @cognifloyd and other contributors here, it's fantastic to receive such detailed feedback.

[cas--]

Why is cryptography listed in the deps for botframework-connector? It is not imported anywhere by botframework-connector. And, these deps specify their own dependencies:

* `adal==1.2.1` requires `cryptography>=1.1.0`

* `msal==1.6.0` requires `cryptography>=0.6,<4`

* `PyJWT>=1.5.3,<2.0.0` requires `cryptography>=1.4.0,<4.0.0` (newer versions bump the min, but not the max)

* `requests>=2.23.0,<3.0.0` requires `cryptography>=1.3.4`

I think cryptography should be removed from the requirements of botframework-connector.

I'm sure adal should also be removed as it is unused: #1620