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

add support for password protected certificate files #143

Merged
merged 14 commits into from Oct 21, 2019

Conversation

@kkoshelev-g
Copy link
Contributor

@kkoshelev-g kkoshelev-g commented Sep 6, 2019

This should improve the security by protecting private cert key with password when using httplib2 with client certificates to initiate mutual TLS connection. See #142

This adds an optional "password" arg to add_certificate() function.
If "password" is provided then "key" points to password encrypted cert file.

This may break unit-tests due to changes to KeyCerts class. I'm still trying to figure out how to run tests properly. I will provide an update after I resolve those issues.

@codecov
Copy link

@codecov codecov bot commented Sep 6, 2019

Codecov Report

Merging #143 into master will increase coverage by 0.03%.
The diff coverage is 83.78%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #143      +/-   ##
==========================================
+ Coverage   75.03%   75.07%   +0.03%     
==========================================
  Files           8        8              
  Lines        2560     2588      +28     
==========================================
+ Hits         1921     1943      +22     
- Misses        639      645       +6
Impacted Files Coverage Δ
python3/httplib2/__init__.py 83.21% <100%> (+0.14%) ⬆️
python2/httplib2/__init__.py 82.25% <76%> (-0.25%) ⬇️

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 3a6d7cd...de8b785. Read the comment docs.

@temoto
Copy link
Member

@temoto temoto commented Sep 18, 2019

Thanks for work. Ways to improve:

  • hit external server ->tests.server_socket creates a local TCP listener socket which could be upgraded to HTTPS with context.wrap_socket
  • generate a dummy key/cert, put it into tests/, check if correct/wrong password makes any difference
  • we don't support 3.2 anyway, so no special code is required

@temoto
Copy link
Member

@temoto temoto commented Sep 18, 2019

I can do the local HTTPS server part maybe this weekend if you like.

@kkoshelev-g
Copy link
Contributor Author

@kkoshelev-g kkoshelev-g commented Sep 18, 2019

Done.

Not sure if we need to hit an external server or a local server. Need to make sure that password is passed through and verified when cert file is loaded. That happens when SSLContext.load_cert_chain() is called before connection is established.

I don't think I understand what you mean by no special code for 3.2 needed.

@temoto
Copy link
Member

@temoto temoto commented Sep 23, 2019

except IOError: # Skip on 3.2

@kkoshelev-g
Copy link
Contributor Author

@kkoshelev-g kkoshelev-g commented Sep 23, 2019

Hey, is there anything needed for this pull request to be merged? Please let me know.

@temoto
Copy link
Member

@temoto temoto commented Sep 24, 2019

Test without relying on external network.

@kkoshelev-g
Copy link
Contributor Author

@kkoshelev-g kkoshelev-g commented Sep 24, 2019

Done. Please let me know if anything else is missing.

return


class MockHttpServer():
Copy link
Member

@temoto temoto Sep 25, 2019

There already exists code that does same function, right below this class.

Copy link
Contributor Author

@kkoshelev-g kkoshelev-g Sep 25, 2019

I respectfully disagree with that statement.
The code below doesn't support SSL and has some hacky "with" implementation / object life time management. It's also impossible to get back client cert used by the server.

Copy link
Member

@temoto temoto Sep 26, 2019

Yeah, thanks for criticism, I very much agree that code turned out less simple than it should.

So if you disagree here, please wait for me to add https/cert support in existing server stub. There is no good reason to have two testing server implementations.

http = httplib2.Http(timeout=2)
http.add_certificate("akeyfile", "acertfile", "bitworking.org", "apassword")
try:
http.request("https://bitworking.org", "GET")
Copy link
Member

@temoto temoto Sep 25, 2019

Still trying to hit external website.

Copy link
Contributor Author

@kkoshelev-g kkoshelev-g Sep 25, 2019

Test without relying on external network.
The test without relying on external network is implemented - test_get_via_https_key_cert_password_with_pem_local_server.

Still trying to hit external website.
Just to clarify, do you expect all tests to use local server? There are quite a few tests which do that in test_external.py file. Or are you referring to newly added tests only?

Copy link
Member

@temoto temoto Sep 26, 2019

They are all transient, were created because local server stub was not implemented or not capable of required features at that time. See file docstring:

"""These tests rely on replies from public internet services

TODO: reimplement with local stubs
"""

@temoto
Copy link
Member

@temoto temoto commented Sep 26, 2019

@kkoshelev-g please have a look here #147
Does it satisfy your needs for testing this patch?

@temoto
Copy link
Member

@temoto temoto commented Oct 2, 2019

@kkoshelev-g
Copy link
Contributor Author

@kkoshelev-g kkoshelev-g commented Oct 2, 2019

@kkoshelev-g please have a look here #147
Does it satisfy your needs for testing this patch?

Sorry for the delay. I don't think so.

The proposed code does not allow testing mutual TLS. It is not possible to verify what was the certificate presented by the client from the server side. See cert serial number comparison in the test.

The test set up is also more complicated, one needs to pass a lot of args to set proper mutual TLS connection with cert/password.

The generation of certificates is fancy but it is not really necessary for the purpose of the testing.

@temoto
Copy link
Member

@temoto temoto commented Oct 15, 2019

@kkoshelev-g wow you are brilliant reviewer. Indeed one line was missing to check client certs in most common test server constructor, while present in rarely used one.

request.client_sock = sock

It's now possible to test client serial as in this test:

def test_client_cert_verified():
    cert_log = []

    def setup_tls(context, server, skip_errors):
        context.load_verify_locations(cafile=tests.CA_CERTS)
        context.verify_mode = ssl.CERT_REQUIRED
        return context.wrap_socket(server, server_side=True)

    def handler(request):
        cert_log.append(request.client_sock.getpeercert())
        return tests.http_response_bytes()

    http = httplib2.Http(ca_certs=tests.CA_CERTS)
    with tests.server_request(handler, tls=setup_tls) as uri:
        uri_parsed = urllib.parse.urlparse(uri)
        http.add_certificate(tests.CLIENT_PEM, tests.CLIENT_PEM, uri_parsed.netloc)
        http.request(uri)

    assert len(cert_log) == 1
    # TODO extract serial from tests.CLIENT_PEM
    assert cert_log[0]["serialNumber"] == "E2AA6A96D1BF1AEC"

@kkoshelev-g
Copy link
Contributor Author

@kkoshelev-g kkoshelev-g commented Oct 15, 2019

Thank you for the update. I guess I will have to wait until test framework changes are merged.

@temoto
Copy link
Member

@temoto temoto commented Oct 15, 2019

donzo

@kkoshelev-g
Copy link
Contributor Author

@kkoshelev-g kkoshelev-g commented Oct 15, 2019

I merged latest upstream changes into pull request branch. Please let me know if there is anything missing.

@@ -1958,6 +1983,7 @@ def request(
ca_certs=self.ca_certs,
disable_ssl_certificate_validation=self.disable_ssl_certificate_validation,
ssl_version=self.ssl_version,
key_password = certs[0][2],
Copy link
Member

@temoto temoto Oct 16, 2019

Just this minor spaces around '=', CI linter should've caught this.

Copy link
Contributor Author

@kkoshelev-g kkoshelev-g Oct 16, 2019

Done

temoto
temoto approved these changes Oct 17, 2019
Copy link
Member

@temoto temoto left a comment

LGTM, thank you.

@temoto temoto requested a review from cdent Oct 17, 2019
@temoto temoto merged commit 4009e8e into httplib2:master Oct 21, 2019
3 checks passed
@kkoshelev-g
Copy link
Contributor Author

@kkoshelev-g kkoshelev-g commented Oct 21, 2019

thank you

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jan 6, 2020
0.15.0

  python2: regression in connect() error handling
  httplib2/httplib2#150

  add support for password protected certificate files
  httplib2/httplib2#143

  feature: Http.close() to clean persistent connections and sensitive data
  httplib2/httplib2#149
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this issue Jan 14, 2020
0.15.0

  python2: regression in connect() error handling
  httplib2/httplib2#150

  add support for password protected certificate files
  httplib2/httplib2#143

  feature: Http.close() to clean persistent connections and sensitive data
  httplib2/httplib2#149
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants