[py] Allows setting Remote webdriver ca_certs through REQUESTS_CA_BUNDLE env variable.

[miguelius]

Description

Allows to configure Remote Webdriver ca_cert through a common environment variable used to that kind of config: REQUESTS_CA_BUNDLE

Motivation and Context

We have a Selenium many grids exposed by https with their own CA and we need to override _ca_bundle through environment variable. Currently, it takes it from certifi.where(), but this method ignores REQUESTS_CA_BUNDLE.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• I have read the contributing document.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have added tests to cover my changes.
• All new and existing tests passed.

[CLAassistant]

All committers have signed the CLA.

[isaulv]

It might need to support SSL_CERT_FILE too depending on the OS.
Selenium uses urllib3 only though. Does it support REQUESTS_CA_BUNDLE?

[titusfortner]

I'm working on a config class for these. Can we integrate what you need into this? #12033

[isaulv]

It seems the reason why this fix is needed because some distros would override how certify.where() works where it's just hard coded. Makes it inflexible for custom self signed certs.

[miguelius]

Cool config class! Still we prefer this because it is easier with already existing tests without changing anything.

The variable is consistent with requests and allow us to prevent the issue certifi.

We are migrating to selenium grid behind https and we run containers. Adding just an environment variable would be least invassive solution to our legacy code.

We haven't tried SSL_CERT_FILE, but anyhow it uses certifi return value.

[titusfortner]

It is not good practice to set the internal state of a library class with an environment variable. It's somewhat similar to Java's (over-)use of System Properties in Service classes, but those are mostly there for use by the grid.

It is better practice to access configuration values/environment variables outside of the library in code you control and pass them in:

certificate_path = os.getenv("REQUESTS_CA_BUNDLE")
client_config = ClientConfig(certificate_path=certificate_path)
driver = webdriver.Chrome(client_config=client_config)

(note that this syntax is what would work once #12033 is merged)

The RemoteConnection() class does look to environment variables for the proxy information, but those environment variables are a convention used by multiple Python libraries. If we do decide to go this route, we should prepend SE_* if it is something only Selenium is looking for.

[codecov-commenter]

Codecov Report

Patch coverage: 18.18% and project coverage change: -0.33 ⚠️

Comparison is base (41744d9) 57.25% compared to head (01797de) 56.92%.

❗ Current head 01797de differs from pull request most recent head a986870. Consider uploading reports for the commit a986870 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #11957      +/-   ##
==========================================
- Coverage   57.25%   56.92%   -0.33%     
==========================================
  Files          86       86              
  Lines        5488     5423      -65     
  Branches      228      193      -35     
==========================================
- Hits         3142     3087      -55     
- Misses       2118     2143      +25     
+ Partials      228      193      -35     

Impacted Files	Coverage Δ
py/selenium/webdriver/ie/options.py	47.50% <15.62%> (-16.64%)	⬇️
py/selenium/webdriver/remote/remote_connection.py	54.58% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[miguelius]

Buenas!

It is not good practice to set the internal state of a library class with an environment variable. It's somewhat similar to Java's (over-)use of System Properties in Service classes, but those are mostly there for use by the grid.

yes, I understand, but since Selenium is overriding the use of requests env variable, I would like it to act the same way requests does. Specially because in case you need to check some stuff with remote rest APIs.

It is better practice to access configuration values/environment variables outside of the library in code you control and pass them in:

I don't know if this is so In containers environments. Additionally, you have HTTPS_PROXY env variable, which is used extensively by many libraries in many languages.
The snippet bellow still uses the env variable but with an object added with no behavior but holding data. This is common in java which has a clumsy interface to work with dictionaries but in python we have dicts. While not ideal, In java properties extends Dictionary but has convenient store and load methods, which i haven't seen in the configuration object on #12033. I guess this would add some cool value.

certificate_path = os.getenv("REQUESTS_CA_BUNDLE")
client_config = ClientConfig(certificate_path=certificate_path)
driver = webdriver.Chrome(client_config=client_config)

Something similar to the previous code could be achieved by (yes, it's class method but shouldn't be many certificate_bundles):

certificate_path = os.getenv("REQUESTS_CA_BUNDLE")
RemoteConnection.set_certificate_bundle_path(certificate_path)

(note that this syntax is what would work once #12033 is merged)

The RemoteConnection() class does look to environment variables for the proxy information, but those environment variables are a convention used by multiple Python libraries. If we do decide to go this route, we should prepend SE_* if it is something only Selenium is looking for.

I wouldn't add an environment variable that duplicates the responsibility of REQUESTS_CA_BUNDLE.

[titusfortner]

Oh this *is an env already being used elsewhere. That's different. I still don't love using ENV this way, but if other libraries support this particular ENV, then I'm ok with us doing it as well.

[miguelius]

I understand it may seem untidy, but it's a common practice environments and very useful on containers envs. BTW, we are not adding an extra one, but preventing the occlusion of a common one our use of the requests library.

May I ask you a favor. I'm getting an error on the linter with the comments. How can I reproduced it locally and fix it? Thank you very much in advance!

[titusfortner]

@diemol what did you do to get the linter working locally? Or doesn't work locally for me, either. @AutomatedTester do you know if there is an easy bazel rule we can drop in and use for this?

[diemol]

I installed the tox version configured in the GitHub workflow and then ran the same command from the step.

[titusfortner]

I did all of that, and made sure the versions all matched and it still didn't give me what I expected. I guess I'm on a different laptop now and should try again.

[titusfortner]

@miguelius sorry for not keeping on top of this one. thank you!

[miguelius]

Thank you, guys! This great for my team!!

Bientôt!