add a ssl_context option to fingerprint - to allow the developer to override the automatically created ssl_context #2735

stephan48 · 2018-02-13T21:18:14Z

What do these changes do?

I faced the problem that with the aiohttp 3.0 ssl api changes i could no longer use fingerprint verification and client cert auth. As you could only specify one or the other. After a short talk with the developer we came to the conclusion that a way to solve the problem would be to add a ssl_context kwargs to the Fingerprint. This PR does exactly that.

Documentation is still missing.

Are there changes in behavior for the user?

As with the previous API aiohttp <=2 you can now use both methods again.

Related issue number

Nope

Checklist

will work on this tomorrow - its already late -
I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new news fragment into the CHANGES folder
- name it <issue_id>.<type> for example (588.bugfix)
- if you don't have an issue_id change it to the pr id after creating the pr
- ensure type is one of the following:
  - .feature: Signifying a new feature.
  - .bugfix: Signifying a bug fix.
  - .doc: Signifying a documentation improvement.
  - .removal: Signifying a deprecation or removal of public API.
  - .misc: A ticket has been closed, but it is not of interest to users.
- Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

…verride the automatically created ssl_context

webknjaz · 2018-02-13T21:33:57Z

aiohttp/connector.py

@@ -771,12 +778,18 @@ def _get_ssl_context(self, req):
            sslcontext = req.ssl
            if isinstance(sslcontext, ssl.SSLContext):
                return sslcontext
+            if not (not isinstance(sslcontext, Fingerprint) or not (sslcontext.ssl_context is not None) or not isinstance(


There's too many negations, which is hard to understand instantly and easy to misunderstand. I think, this can (and should) be simplified.

It seems to me that you want this check:

isinstance(sslcontext, Fingerprint) and isinstance(sslcontext.ssl_context, ssl.SSLContext)

webknjaz · 2018-02-13T21:49:18Z

aiohttp/connector.py

            if sslcontext is not None:
                # not verified or fingerprinted
                return self._make_ssl_context(False)
            sslcontext = self._ssl
            if isinstance(sslcontext, ssl.SSLContext):
                return sslcontext
+            if not (not isinstance(sslcontext, Fingerprint) or not (sslcontext.ssl_context is not None) or not isinstance(


This is a duplicate check. Consider code reuse, so that it won't result in diverse copy-paste in future.

…during the reformatting of the line

stephan48 · 2018-02-13T22:08:20Z

Hi, thank you for your comments I used some function in PyStorm to break the line - this apparently lead to it rewriting the expression without me checking. I reworked it - i would like to keep the "is not None" check as a precondition. I fixed the negation mess throu.

But I see that this check is not done in the other branches - is isinstance "None"-safe?

webknjaz · 2018-02-13T22:27:31Z

@stephan48 this is why I use Vim :)

isinstance is "None-safe": None is instance of the NoneType

stephan48 · 2018-02-13T22:34:35Z

yea :) for work stuff i tend to use windows & a IDE. for everything else vim on a ssh connection.

thanks for that info.

how about this:

            if ssl is None:  # pragma: no cover
                raise RuntimeError('SSL is not supported.')

            def check_ssl(ssl_ctx):
                if isinstance(ssl_ctx, ssl.SSLContext):
                    return ssl_ctx
                if isinstance(ssl_ctx, Fingerprint) and isinstance(ssl_ctx.ssl_context, ssl.SSLContext):
                    return ssl_ctx.ssl_context
                if ssl_ctx is not None:
                    # not verified or fingerprinted
                    return self._make_ssl_context(False)

            if req.ssl is not None:
                return check_ssl(req.ssl)

            if self._ssl is not None:
                return check_ssl(self._ssl)

            return self._make_ssl_context(True)

webknjaz · 2018-02-17T10:15:06Z

Your last if/return in check_ssl() is always true (I mean, if not needed there at all). But if looks unreliable to me in current form.

webknjaz · 2018-02-17T10:16:28Z

Oh, and please update code in branch, cause it's fairly complicated to comment on such snippet + it's ripped out of context, which makes it hard to imagine how it would play with other parts of code around it. TIA.

asvetlov · 2018-02-25T13:19:29Z

ping

…ontext

stephan48 · 2018-04-30T08:51:44Z

Hi,

sadly I got overwhelmed by private and work stuff and i totally forgot to check back in here :(

Would the changes/the version in the last commit(pre merge) be adequate?

-- stephan

codecov-io · 2018-04-30T09:40:32Z

Codecov Report

Merging #2735 into master will decrease coverage by 0.02%.
The diff coverage is 85.71%.

@@            Coverage Diff             @@
##           master    #2735      +/-   ##
==========================================
- Coverage   97.98%   97.95%   -0.03%     
==========================================
  Files          40       40              
  Lines        7592     7595       +3     
  Branches     1323     1323              
==========================================
+ Hits         7439     7440       +1     
- Misses         51       52       +1     
- Partials      102      103       +1

Impacted Files	Coverage Δ
aiohttp/client_reqrep.py	`97.36% <100%> (+0.01%)`	⬆️
aiohttp/connector.py	`96.5% <80%> (-0.37%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c4cbc61...abfd287. Read the comment docs.

stephan48 · 2018-04-30T09:47:37Z

Do I assume correctly, that the code coverage complains about the new check_ssl function being undocumented?

asvetlov

The design looks good, please fix a couple notes

asvetlov · 2018-04-30T14:43:41Z

aiohttp/connector.py

+            def check_ssl(ssl_ctx):
+                if isinstance(ssl_ctx, ssl.SSLContext):
+                    return ssl_ctx
+                if isinstance(ssl_ctx, Fingerprint) and \


Please use brackets:

if (isinstance(ssl_ctx, Fingerprint) and isinstance(ssl_ctx.ssl_context, ssl.SSLContext)): return ssl_ctx.ssl_context

asvetlov · 2018-04-30T14:44:01Z

aiohttp/connector.py

+                    return ssl_ctx
+                if isinstance(ssl_ctx, Fingerprint) and \
+                   isinstance(ssl_ctx.ssl_context, ssl.SSLContext):
+                    return ssl_ctx.ssl_context


The line is not covered by tests

asvetlov · 2018-04-30T14:46:33Z

aiohttp/connector.py

-                return sslcontext
-            if sslcontext is not None:
+
+            def check_ssl(ssl_ctx):


The function has no closures, please rename it into _check_ssl and move to the module's top-level namespace

…ontext

asvetlov · 2018-05-01T07:03:59Z

aiohttp/connector.py

+            return ssl_ctx
+        if (isinstance(ssl_ctx, Fingerprint) and
+                isinstance(ssl_ctx.ssl_context, ssl.SSLContext)):
+            return ssl_ctx.ssl_context


Please add a test for covering the line execution.

webknjaz · 2018-05-04T12:18:21Z

aiohttp/connector.py

-                # not verified or fingerprinted
-                return self._make_ssl_context(False)
+
+            if req.ssl is not None:


smth like this would look nicer that duplicate if-clauses:

_ssl = req.ssl or self._ssl if _ssl is not None: return self._check_ssl(_ssl)

asvetlov · 2020-11-21T15:28:09Z

I like the idea but sorry, I should close the PR.
It has no tests and documentation updates.
Please feel free to open a new PR that has all the required fixes.

add a ssl_context option to fingerprint - to allow the developer to o…

ea147b1

…verride the automatically created ssl_context

webknjaz requested changes Feb 13, 2018

View reviewed changes

greatly simplify the expression - i don't know what my IDE did there …

6c9a9f5

…during the reformatting of the line

asvetlov and others added 2 commits February 14, 2018 12:59

Merge branch 'master' into fingerprint-with-ssl-context

b4bcd89

Merge branch 'master' into fingerprint-with-ssl-context

a2fabeb

stephan48 added 2 commits April 30, 2018 10:46

simplify check logic in _get_ssl_context

f615480

Merge https://github.com/aio-libs/aiohttp into fingerprint-with-ssl-c…

59fe5c1

…ontext

stephan48 added 2 commits April 30, 2018 10:57

adjust line length in _get_ssl_context"

82309e2

correct syntax error occuring due to line breaking /o"

305f562

asvetlov requested changes Apr 30, 2018

View reviewed changes

stephan48 added 3 commits April 30, 2018 21:20

correct code according to guidelines

206eaef

Merge https://github.com/aio-libs/aiohttp into fingerprint-with-ssl-c…

427d7f4

…ontext

correct code according to styling guides

abfd287

asvetlov requested changes May 1, 2018

View reviewed changes

webknjaz reviewed May 4, 2018

View reviewed changes

webknjaz mentioned this pull request Aug 8, 2018

aiohttp ignoring SSL_CERT_DIR and or SSL_CERT_FILE environment vars. Results in [SSL: CERTIFICATE_VERIFY_FAILED] #3180

Closed

webknjaz and others added 2 commits April 24, 2020 23:45

Merge branch 'master' into fingerprint-with-ssl-context

9ce8d5d

Merge branch 'master' into fingerprint-with-ssl-context

b832f46

This comment has been minimized.

Sign in to view

asvetlov closed this Nov 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add a ssl_context option to fingerprint - to allow the developer to override the automatically created ssl_context #2735

add a ssl_context option to fingerprint - to allow the developer to override the automatically created ssl_context #2735

stephan48 commented Feb 13, 2018 •

edited

webknjaz Feb 13, 2018

webknjaz Feb 13, 2018

webknjaz Feb 13, 2018

stephan48 commented Feb 13, 2018

webknjaz commented Feb 13, 2018

stephan48 commented Feb 13, 2018 •

edited by webknjaz

webknjaz commented Feb 17, 2018

webknjaz commented Feb 17, 2018

asvetlov commented Feb 25, 2018

stephan48 commented Apr 30, 2018

codecov-io commented Apr 30, 2018 •

edited

stephan48 commented Apr 30, 2018

asvetlov left a comment

asvetlov Apr 30, 2018

asvetlov Apr 30, 2018

asvetlov Apr 30, 2018

asvetlov May 1, 2018

webknjaz May 4, 2018

This comment has been minimized.

asvetlov commented Nov 21, 2020

add a ssl_context option to fingerprint - to allow the developer to override the automatically created ssl_context #2735

add a ssl_context option to fingerprint - to allow the developer to override the automatically created ssl_context #2735

Conversation

stephan48 commented Feb 13, 2018 • edited

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephan48 commented Feb 13, 2018

webknjaz commented Feb 13, 2018

stephan48 commented Feb 13, 2018 • edited by webknjaz

webknjaz commented Feb 17, 2018

webknjaz commented Feb 17, 2018

asvetlov commented Feb 25, 2018

stephan48 commented Apr 30, 2018

codecov-io commented Apr 30, 2018 • edited

Codecov Report

stephan48 commented Apr 30, 2018

asvetlov 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

This comment has been minimized.

asvetlov commented Nov 21, 2020

stephan48 commented Feb 13, 2018 •

edited

stephan48 commented Feb 13, 2018 •

edited by webknjaz

codecov-io commented Apr 30, 2018 •

edited