Add verify and proxies to context #142

lmazuel · 2018-05-07T19:51:45Z

Hi,

I work with @yugangw-msft in the SDK/CLI team. I'm migrating the Autorest internal authentication system to use 100% of ADAL, so all SDK/CLI queries pass through it.
Part of my migration, I realized that my current implementation support verify/proxies as context parameters, where ADAL does not. This PR is just passing through requests verify/proxies. No breaking changes, just let the options pass to requests. Documentation strings are taken from requests directly.

Reference:
http://docs.python-requests.org/en/master/user/advanced/#proxies

Let me know if you have questions (internal email or here)
Thank you!

Laurent

rayluo · 2018-05-07T22:11:51Z

Thanks Laurent! We will take a look into it soon! Stay tuned.

rayluo · 2018-05-08T00:41:38Z

adal/authentication_context.py

+        :param verify_ssl: (optional) requests verify. Either a boolean, in which case it 
+            controls whether we verify the server's TLS certificate, or a string, in which 
+            case it must be a path to a CA bundle to use. This value is ignored if env 
+            variable ADAL_PYTHON_SSL_NO_VERIFY is used.


Would it be more natural that we do the other way around, i.e. env var will be ignored if function argument is explicitly provided? I know there might not be an industry standard on this behavior but, it happens that the Python standard lib urllib uses the same approach as what I suggested here:

Alternatively, the optional proxies argument may be used to explicitly specify proxies. It must be a dictionary mapping scheme names to proxy URLs, where an empty dictionary causes no proxies to be used, and None (the default value) causes environmental proxy settings to be used as discussed above. For example: ...

I agree with @rayluo on this

Ok, in progress

rayluo · 2018-05-08T00:51:25Z

adal/authentication_context.py

+            case it must be a path to a CA bundle to use. This value is ignored if env 
+            variable ADAL_PYTHON_SSL_NO_VERIFY is used.
+        :param proxies: (optional) requests proxies. Dictionary mapping protocol to the URL 
+            of the proxy.


It is true that this is the same concise docstring used by requests library. But they also provide intensive documentation to provide the examples on what "dictionary mapping" actually looks like. Since we don't currently have those documentation effort, I would suggest we at least add one more sentence here. Either explicitly mentions an example:

Its content will look like proxies = { 'http': 'http://10.10.1.10:3128', 'https': 'http://10.10.1.10:1080', }

Or simply add a link to its official documentation in requests (since we are exposing this requests behavior anyway).

I prefer external link, scale better in the long term IMHO :)

yugangw-msft · 2018-05-08T05:17:30Z

Agree with existing code review comments. LGTM on the rest

yugangw-msft · 2018-05-08T05:33:39Z

adal/authentication_context.py

@@ -95,6 +101,7 @@ def __init__(
            'options': GLOBAL_ADAL_OPTIONS,
            'api_version': api_version,
            'verify_ssl': None if env_value is None else not env_value, # mainly for tracing through proxy


Also, this line will need to be updated; otherwise the argument value will not be propagated particularly when it is a string to a bundle

Actually, the argument value was passed as default parameter in the os.environ, so yes, it was propagated, but I will simplify this line

lmazuel · 2018-05-08T18:33:41Z

@rayluo @yugangw-msft I think the logic is better now, let me know.

Also, I "fixed" a weird scenario. The way the code was written, an empty ADAL_PYTHON_SSL_NO_VERIFY was considered as verify (since bool(not '') == True). Everything else is still considered as "no verify" (even export ADAL_PYTHON_SSL_NO_VERIFY=no). Will be too dangerous IMHO to change the logic to actually look at what is inside the string.

rayluo

I like that you called out the way we use ADAL_PYTHON_SSL_NO_VERIFY is its presence only. In such case, I made further minor suggestion in the code. Please take a look and let me know.

Other things look good.

rayluo · 2018-05-08T19:51:51Z

adal/authentication_context.py

        '''
        self.authority = Authority(authority, validate_authority is None or validate_authority)
        self._oauth2client = None
        self.correlation_id = None
-        env_value = os.environ.get('ADAL_PYTHON_SSL_NO_VERIFY')
+        env_verify = False if 'ADAL_PYTHON_SSL_NO_VERIFY' in os.environ else None


How about env_verify = 'ADAL_PYTHON_SSL_NO_VERIFY' not in os.version? Because None is conceptually NOT an expected value by requests' verify anyway.

None is the actual default value of requests actually, if you look at the method signature:
https://github.com/requests/requests/blob/1e8a3142d2376e86d8f923840c0b8d361af7786b/requests/sessions.py#L449

This is done that way in the operation signature, so a not-None value override the verify static value in the Session object (which this one is True by default: https://github.com/requests/requests/blob/1e8a3142d2376e86d8f923840c0b8d361af7786b/requests/sessions.py#L373). Since you don't use a Session, that would be equivalent, but None is accurate as well in this context. As long as you don't decide to use a common Session in the context, our two codes are indeed equivalent :).
Tell me what you prefer

Yes I know that as a Python convention, many function technically uses None as default value. When I said "None is CONCEPTUALLY not an expected value" in this case, I mean the underlying documentation saying that:

"verify can either be a boolean or a string";

From a document perspective, it does not explicitly mention None, it just says "by default it will be set to True"; (By the way, if I were writing that function, I would probably actually define True as its default value: request(..., verify=True, ...)

Back to our case. Any future maintainer reading this line would think "why we differenciate False and None here? Do they make a difference?" and then they would need to dive into requests' doc. By explicitly coding a True or False as the value we care and use, we can avoid that mental exercise, if nothing else.

Fine with that, doing the change.

rayluo · 2018-05-08T20:00:08Z

adal/authentication_context.py

+        :param verify_ssl: (optional) requests verify. Either a boolean, in which case it 
+            controls whether we verify the server's TLS certificate, or a string, in which 
+            case it must be a path to a CA bundle to use. This value overrides env 
+            variable ADAL_PYTHON_SSL_NO_VERIFY.


Suggest to change "This value overrides env variable ADAL_..." to "If this value is None, we will use the presence of env variable ADAL_PYTHON_SSL_NO_VERIFY to turn off SSL verification."

You can help to proofread it. The goal is to also explicitly document the usage of ADAL_PYTHON_SSL_NO_VERIFY as using its presence (or not) as a boolean; and we are NOT using ADAL_PYTHON_SSL_NO_VERIFY as a string path to a CA bundle.

What about:

If this value is not provided, and ADAL_PYTHON_SSL_NO_VERIFY env varaible is set, behavior is equivalent to verify_ssl=False

That sounds good! :-)

rayluo

Add verify and proxies to context

35f9e00

lmazuel mentioned this pull request May 7, 2018

Switch to pure ADAL Azure/msrestazure-for-python#94

Merged

rayluo reviewed May 8, 2018

View reviewed changes

rayluo requested a review from yugangw-msft May 8, 2018 00:51

yugangw-msft reviewed May 8, 2018

View reviewed changes

Fix verify logic

56bbb8f

yugangw-msft approved these changes May 8, 2018

View reviewed changes

rayluo reviewed May 8, 2018

View reviewed changes

Review

87f029a

rayluo approved these changes May 8, 2018

View reviewed changes

rayluo merged commit 3765b34 into AzureAD:dev May 15, 2018

abhidnya13 mentioned this pull request May 15, 2018

Release 0.6.0 #147

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add verify and proxies to context #142

Add verify and proxies to context #142

lmazuel commented May 7, 2018

rayluo commented May 7, 2018

rayluo May 8, 2018

yugangw-msft May 8, 2018

lmazuel May 8, 2018

rayluo May 8, 2018 •

edited

Loading

lmazuel May 8, 2018

yugangw-msft commented May 8, 2018 •

edited

Loading

yugangw-msft May 8, 2018 •

edited

Loading

lmazuel May 8, 2018 •

edited

Loading

lmazuel commented May 8, 2018 •

edited

Loading

rayluo left a comment

rayluo May 8, 2018

lmazuel May 8, 2018

rayluo May 8, 2018

lmazuel May 8, 2018

rayluo May 8, 2018

lmazuel May 8, 2018 •

edited

Loading

rayluo May 8, 2018

rayluo left a comment

Add verify and proxies to context #142

Add verify and proxies to context #142

Conversation

lmazuel commented May 7, 2018

rayluo commented May 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayluo May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yugangw-msft commented May 8, 2018 • edited Loading

yugangw-msft May 8, 2018 • edited Loading

Choose a reason for hiding this comment

lmazuel May 8, 2018 • edited Loading

Choose a reason for hiding this comment

lmazuel commented May 8, 2018 • edited Loading

rayluo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lmazuel May 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rayluo left a comment

Choose a reason for hiding this comment

rayluo May 8, 2018 •

edited

Loading

yugangw-msft commented May 8, 2018 •

edited

Loading

yugangw-msft May 8, 2018 •

edited

Loading

lmazuel May 8, 2018 •

edited

Loading

lmazuel commented May 8, 2018 •

edited

Loading

lmazuel May 8, 2018 •

edited

Loading