Issue #3483 Add cert retrieval for requests #3490

dusktreader · 2020-12-14T16:58:32Z

The authenticator.py code already retrieves credentials from the config
for every request based on url matching. This change makes the
authenticator also retrieve certs from the config for each request based
on url matching.

Also includes unit tests.

Pull Request Check List

Resolves: #3483

Added tests for changed code.
Updated documentation for changed code.

tbennett6421 · 2021-01-15T23:58:54Z

Forgive my ignorance. Would this work for poetry publish?

dusktreader · 2021-01-18T20:23:09Z

@tbennett6421 Yes, that's specifically what this is for.

GooseYArd · 2021-02-01T20:07:08Z

I just noticed that when cert_verify is called by _download_archive, cert's type is Path(), so the test

if not isinstance(cert, basestring):

yields False, and we mistakenly treat the cert as a list.

Oddly, when cert_verify winds up being called by legacy_repository.find_links_for_package, the certs name is passed as a string.

I'll see if I can figure out a fix

GooseYArd · 2021-02-02T14:40:18Z

This seems to do the trick (apologies for the edits, I found an issue with the previous patch)

diff --git a/poetry/installation/authenticator.py b/poetry/installation/authenticator.py
index 8b57c9c..f671809 100644
--- a/poetry/installation/authenticator.py
+++ b/poetry/installation/authenticator.py
@@ -75,6 +75,12 @@ class Authenticator(object):
         verify = kwargs.get("verify", certs.get("verify"))
         cert = kwargs.get("cert", certs.get("cert"))

+        if isinstance(cert, pathlib.PurePath):
+            cert = str(cert)
+
+        if isinstance(verify, pathlib.PurePath):
+            verify = str(verify)
+
         settings = session.merge_environment_settings(
             prepared_request.url, proxies, stream, verify, cert
         )

the requests module will not accept Path instances for either cert or verify, but it will accept cert as a tuple of (cert, key), in cases where the cert and key are not combined into a single file. I'm assuming that poetry expects the combined format since pip has the same expectation. I'm guessing that if poetry ever opts to support separate cert/key files, it would entail a different key in auth.yaml, so I doubt we'll need to check here whether cert is a tuple or not.

dusktreader · 2021-02-03T16:45:57Z

@GooseYArd it seems like you could just apply str() to any value of cert and verify, so maybe the if statements aren't necessary?

GooseYArd · 2021-02-03T17:05:03Z

@GooseYArd it seems like you could just apply str() to any value of cert and verify, so maybe the if statements aren't necessary?

hah funny you mention it- that was what I did in the initial patch without realizing that the values will be None if no cert/key or calist are specified, and request's cert verification was trying to locate the literal 'None' :)

dusktreader · 2021-02-03T17:14:44Z

@GooseYArd good point there. i'll apply this patch

BrutalSimplicity · 2021-02-26T03:27:49Z

Any chance this will be merged anytime soon? I'd love to promote Poetry as the default package manager, but work with enterprises that have an internal PyPi repository, and am unable to pull those packages without hacky workarounds.

astro-josh · 2021-04-13T18:24:13Z

The issue resolved here is also blocking adoption for our project

GooseYArd · 2021-04-13T18:27:44Z

@abn this was the second of the two private repo + cert related issues that were preventing us from using Artifactory pypi servers. I'm hoping it can be reviewed for 1.2, but am not sure who to ping. We've been using this PR internally for the past few months without issue, fwiw.

abn

Over all looks okay; can we get this rebased and compatibility code removed please?

abn · 2021-04-13T19:02:23Z

poetry/installation/authenticator.py

+            self._get_certs_for_netloc_from_config(netloc),
+        )
+
+    def _get_repository_netlocs(self):  # type: () -> Generator[[str, str], None, None]


We can drop compatibily type hints now.

abn · 2021-04-13T19:05:03Z

@GooseYArd @dusktreader apologies this slipped through the radar. If you can get it rebased and updated, I can get it reviewed and merged.

GooseYArd · 2021-04-13T21:22:21Z

@abn thanks a bunch, if @dusktreader doesn't beat me to it I'll make the updates you requested tomorrow AM

dusktreader · 2021-04-13T22:34:08Z

@abn Rebased and force-pushed! thanks

@GooseYArd got it covered, I think

abn

Minor suggestions

abn · 2021-04-14T23:57:37Z

poetry/installation/authenticator.py

+        verify = kwargs.get("verify", certs.get("verify"))
+        cert = kwargs.get("cert", certs.get("cert"))
+
+        if isinstance(cert, pathlib.PurePath):


PurePath might be problematic. Since the only case we want to prevent here is str(None) I'd probably suggest we update the check to be an is none check.

I changed this check to if cert: and things seem to work as expected for me on Ubuntu with only a client cert specified (verify should be None for me).

abn · 2021-04-14T23:59:24Z

poetry/installation/authenticator.py

-                    continue
+    def _get_certs_for_netloc_from_config(
+        self, netloc: str
+    ) -> Dict[str, pathlib.PosixPath]:


Should this be pathlib.Path?

I also made this change locally and also on line 167 for get_certs_for_url.

surry · 2021-04-27T14:38:27Z

@dusktreader thanks for this PR, also a blocker for us since we use a private repository with client certificates. I manually made these changes to my Poetry 1.1.6 install and my private repo is working now. Are you able to update and merge this soon? Would love to get this fix into a version I can update to with poetry self update.

dusktreader · 2021-04-28T22:51:50Z

@abn Applied suggested changes and pushed commit. Thanks!

surry

Minor suggestions along the lines of @abn's ones. Also, I quickly looked at why the Python 3.10 tests failed and it might be due to this:

pytest-dev/pytest#8539

Not sure what the fix is here, if any...

@abn would it be possible to merge this in for the upcoming 1.2.0a1 release?

poetry/installation/authenticator.py

abn

Functionally looks okay. Some minor suggestions added.

poetry/installation/authenticator.py

abn · 2021-05-05T23:25:41Z

tests/installation/test_authenticator.py

+def test_authenticator_uses_certs_from_config_if_not_provided(
+    config, mock_remote, http, mocker
+):
+    config.merge(
+        {
+            "repositories": {"foo": {"url": "https://foo.bar/simple/"}},
+            "http-basic": {"foo": {"username": "bar", "password": "baz"}},
+            "certificates": {
+                "foo": {"cert": "/path/to/cert", "client-cert": "/path/to/client-cert"}
+            },
+        }
+    )
+
+    authenticator = Authenticator(config, NullIO())
+    session_send = mocker.patch.object(authenticator.session, "send")
+    authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz")
+    kwargs = session_send.call_args[1]
+    assert pathlib.Path(kwargs["verify"]) == pathlib.Path("/path/to/cert")
+    assert pathlib.Path(kwargs["cert"]) == pathlib.Path("/path/to/client-cert")
+
+
+def test_authenticator_uses_provided_certs_instead_of_config_certs(
+    config, mock_remote, http, mocker
+):
+    config.merge(
+        {
+            "repositories": {"foo": {"url": "https://foo.bar/simple/"}},
+            "http-basic": {"foo": {"username": "bar", "password": "baz"}},
+            "certificates": {
+                "foo": {"cert": "/path/to/cert", "client-cert": "/path/to/client-cert"}
+            },
+        }
+    )
+
+    authenticator = Authenticator(config, NullIO())
+    session_send = mocker.patch.object(authenticator.session, "send")
+    authenticator.request(
+        "get",
+        "https://foo.bar/files/foo-0.1.0.tar.gz",
+        verify="/path/to/provided/cert",
+        cert="/path/to/provided/client-cert",
+    )
+    kwargs = session_send.call_args[1]
+    assert pathlib.Path(kwargs["verify"]) == pathlib.Path("/path/to/provided/cert")
+    assert pathlib.Path(kwargs["cert"]) == pathlib.Path("/path/to/provided/client-cert")


Suggested change

def test_authenticator_uses_certs_from_config_if_not_provided(

config, mock_remote, http, mocker

):

config.merge(

{

"repositories": {"foo": {"url": "https://foo.bar/simple/"}},

"http-basic": {"foo": {"username": "bar", "password": "baz"}},

"certificates": {

"foo": {"cert": "/path/to/cert", "client-cert": "/path/to/client-cert"}

},

}

)

authenticator = Authenticator(config, NullIO())

session_send = mocker.patch.object(authenticator.session, "send")

authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz")

kwargs = session_send.call_args[1]

assert pathlib.Path(kwargs["verify"]) == pathlib.Path("/path/to/cert")

assert pathlib.Path(kwargs["cert"]) == pathlib.Path("/path/to/client-cert")

def test_authenticator_uses_provided_certs_instead_of_config_certs(

config, mock_remote, http, mocker

):

config.merge(

{

"repositories": {"foo": {"url": "https://foo.bar/simple/"}},

"http-basic": {"foo": {"username": "bar", "password": "baz"}},

"certificates": {

"foo": {"cert": "/path/to/cert", "client-cert": "/path/to/client-cert"}

},

}

)

authenticator = Authenticator(config, NullIO())

session_send = mocker.patch.object(authenticator.session, "send")

authenticator.request(

"get",

"https://foo.bar/files/foo-0.1.0.tar.gz",

verify="/path/to/provided/cert",

cert="/path/to/provided/client-cert",

)

kwargs = session_send.call_args[1]

assert pathlib.Path(kwargs["verify"]) == pathlib.Path("/path/to/provided/cert")

assert pathlib.Path(kwargs["cert"]) == pathlib.Path("/path/to/provided/client-cert")

@pytest.mark.parametrize("cert,client_cert", [

(None, None),

("/path/to/provided/cert", "/path/to/provided/client-cert")

])

def test_authenticator_uses_certs_from_config_if_not_provided(

cert, client_cert, config, mock_remote, http, mocker

):

configured_cert = "/path/to/cert"

configured_client_cert = "/path/to/client-cert"

config.merge(

{

"repositories": {"foo": {"url": "https://foo.bar/simple/"}},

"http-basic": {"foo": {"username": "bar", "password": "baz"}},

"certificates": {

"foo": {"cert": configured_cert, "client-cert": configured_client_cert}

},

}

)

authenticator = Authenticator(config, NullIO())

session_send = mocker.patch.object(authenticator.session, "send")

authenticator.request("get", "https://foo.bar/files/foo-0.1.0.tar.gz", verify=cert, cert=client_cert)

kwargs = session_send.call_args[1]

assert Path(kwargs["verify"]) == Path(cert or configured_cert)

assert Path(kwargs["cert"]) == Path(client_cert or configured_client_cert)

Nothing wroing with the implementation, just wanted to avoid some duplicated code.

I think it makes sense to have the tests separated since the test name clearly defines the conditions under test. In this suggestion, the parameterization makes the test cases less clear.

I am not wedded to parametrised tests. But I am much more keen on adding to the already large sets of duplicated code in our test suites. It is becoming harder to maintain. Additionally you can add a comment next to the parameters if further clarity is required. Personally, I do think the parameters are clear enough.

OK. Will update

This has been updated as requested

abn · 2021-05-05T23:26:47Z

tests/installation/test_authenticator.py

@@ -1,3 +1,4 @@
+import pathlib


Suggested change

import pathlib

from pathlib import Path

There's a few reasons to prefer import pathlib to from pathlib import Path. The primary one is that it follows the import pattern used in this module already with requests, pytest, and other. Secondly, pathlib.Path is more clear and explicit the type of Path object being used since the name Path is common and can be overloaded. The last reason, though it isn't really relevant here, is that using the import path style makes it much easier to mock internal functions in pathlib if necessary.

I would say those reasons are rather subjective and not necessarily universal. Further, I do not think we use anything other than pathlib.Path as Path through the code base. I recommended the change as this is how Path is import through the rest of the code. As for mocking, I suspect we will have to patch anyway within poetry.* in most cases.

OK. Will update

Since 1.2.0 looks imminent I thought I'd bump this thread- this is the last cert-related issue that is blocking my company from being able to use poetry generally for our internal packaging, and since I've already converted a number of our packages (and have been using a private fork with a couple of cert fixes) I'm desperate to get this change merged. Is there a compromise we could reach here?

I will try to get some cycles to work on this today. The latest requests for changes broke a bunch of unit tests, so I will have to figure out what happened there. Sorry it's been slow to merge.

Updates pushed. Tests passed locally; will see what the pipeline does when it's greenlit to run

The python3.6 tests on FreeBSD failed with a strange (or missing) error: https://github.com/python-poetry/poetry/pull/3490/checks?check_run_id=2657871763

Maybe that test just needs to be re-run and the other workflows could be approved by a maintainer see if there are other issues this PR needs to address? @abn

Yes, that failure seems to be unrelated to my change. I will try to act on any more feedback on this quickly, since folks are waiting on this feature.

This has been updated as requested.

abn · 2021-05-05T23:27:42Z

poetry/installation/authenticator.py

@@ -1,9 +1,12 @@
 import logging
+import pathlib


Suggested change

import pathlib

from pathlib import Path

This has been adjusted as requested

dusktreader · 2021-05-27T15:42:37Z

@abn Can you trigger the pipelines, please? Also, it seems like the "Tests / FreeBSD / PYTHON:python3.6" is failing due to an unrelated issue. Also, I've addressed your comments above

dusktreader · 2021-06-07T16:29:47Z

Can we get a maintainer to look at this please? Just waiting for pipelines

surry · 2021-06-08T14:48:16Z

I pinged some of the devs on Discord to get the pipelines going, so you might be able to get more traction there.

@abn would you be able to take another look now that the checks pass? This PR would really help users that rely on client certificates to access private Pypi repositories.

astro-josh · 2021-08-23T15:21:39Z

Any idea when this would be implemented? Project stuck at 1.0.10 until this is fixed for use of our private repo.

jakeri · 2021-10-04T07:22:45Z

Any updates on this one?

We wanted to use poetry for our internal build but we need the client cert functional. No we have to do a really convoluted build step with extracting requirements.txt, etc.

dusktreader · 2021-10-04T23:48:57Z

Rebased to fix merge conflict.

dusktreader · 2021-12-14T17:33:16Z

Rebased and force pushed again.

@abn or anyone else: does this have any traction?

LarsStegman · 2022-01-18T13:59:37Z

@abn @finswimmer @neersighted @sdispater @stephsamson Could one of the maintainers please look at this? I'm running into problems with multiple of our projects due to being stuck at an old version of Poetry.

neersighted · 2022-01-18T16:47:35Z

@abn @finswimmer @neersighted @sdispater @stephsamson Could one of the maintainers please look at this? I'm running into problems with multiple of our projects due to being stuck at an old version of Poetry.

This needs another rebase from @dusktreader, but I am happy to land it once it's rebased onto current master and gets another review pass.

dusktreader · 2022-01-18T16:52:02Z

@abn @finswimmer @neersighted @sdispater @stephsamson Could one of the maintainers please look at this? I'm running into problems with multiple of our projects due to being stuck at an old version of Poetry.

This needs another rebase from @dusktreader, but I am happy to land it once it's rebased onto current master and gets another review pass.

Yes, I noticed some of the underlying logic changed a bit. I'm going to work on getting it rebased eventually, but I've been busy lately. Also, it seemed like wasted effort rebasing over and over. Now that a maintainer is looking at it, I'll try to get it done.

BrutalSimplicity · 2022-02-27T05:32:33Z

The completion of this PR is holding up full adoption for 100s of developers at my company. It's been in-progress for over a year. Any chance we can see this completed soon?

Is there some method to the prioritization of these requests? It seems like being able to use a certificate for requests made by Poetry would be a major blocker to widespread adoption for many professional developers.

ITProKyle · 2022-03-15T12:32:55Z

Would it be possible to release this as a patch to 1.1 to get it out the door faster? It is a major blocker for many and is desperately needed ASAP.

dusktreader · 2022-03-16T20:34:35Z

I simply don't have time to do this right now, and it's not in my current critical path. If someone else has time to pick up the mantle and rebase it, I (and apparently many others) would greatly appreciate it.

chriswhite199 · 2022-03-16T20:58:52Z

For those blocked by this, can you not just set CURL_CA_BUNDLE env variable before running poetry to your CA bundle file?

This is what I'm doing for hosted Artifactory pypi mirror and private repos.

ITProKyle · 2022-03-23T14:06:13Z

@chriswhite199 - yes and no.

Yes, that workaround works for me as someone that is familiar with poetry and its quirks.

No, that workaround will not work for the >100 developers I work with that have never used poetry.

chriswhite199 · 2022-03-23T22:10:56Z

@ITProKyle - I hear you, and we just add a poetry wrapper script in the base of our repositories to do this - It handles detection of whether poetry is installed globally (and if not pip installs it into a .poetry-venv folder). Developers are taught to use it (in the same way they might use a maven or gradle wrapper if they were coding in Java).

radoering · 2022-04-14T14:30:37Z

Superseded by: #5320

papb · 2022-08-01T03:26:21Z

For those blocked by this, can you not just set CURL_CA_BUNDLE env variable before running poetry to your CA bundle file?

This is what I'm doing for hosted Artifactory pypi mirror and private repos.

Oohhhhhhh thank you for teaching me this. Where did you find about this CURL_CA_BUNDLE env variable? Where is this documented? @chriswhite199

github-actions · 2024-02-29T10:07:16Z

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

janheindejong mentioned this pull request Feb 23, 2021

Poetry uses default repository instead of private repository when both repo's have same package #3719

Closed

3 tasks

GooseYArd mentioned this pull request Mar 28, 2021

Handle 403 response from secondary repos #3608

Closed

2 tasks

abn self-requested a review April 13, 2021 18:42

abn requested changes Apr 13, 2021

View reviewed changes

abn added this to the 1.2 milestone Apr 13, 2021

abn added the kind/bug Something isn't working as expected label Apr 13, 2021

abn requested changes Apr 19, 2021

View reviewed changes

surry reviewed May 4, 2021

View reviewed changes

poetry/installation/authenticator.py Outdated Show resolved Hide resolved

poetry/installation/authenticator.py Outdated Show resolved Hide resolved

abn requested changes May 5, 2021

View reviewed changes

Tucker Beck and others added 12 commits December 14, 2021 09:29

Issue #3483 Fixed broken unit test for windows

9d51dd0

Issue #3483 Fixing path issue in unit tests for windows

2b78bde

Issue #3483: Fixed cert handling for pathlib paths

7dcb682

Issue #3483: Style change to make the linter happy

fddf76b

Issue #3483: Rebased and fixed type hint

d5bb81a

Issue #3483: Fixed a style issue

038ed31

Issue #3483: Applied updates from code review

6fc640c

Issue #3483: more revisions based on code review feedback

864d677

Issue #3483: Some revisions based on code review

7de523a

Issue #3483: Applied code review suggestions and fixed unit tests

4a6bf9b

Issue #3483 Fixed broken code from rebase

451568c

Issue #3483: Applied fix for linting issue and improved test coverage

1d9132c

Issue #3483 Fixed issues with linting and type-checking

45bb9d1

neersighted self-assigned this Jan 20, 2022

ITProKyle mentioned this pull request Mar 15, 2022

Issue #3483 Add cert retrieval for requests #5320

Merged

2 tasks

radoering closed this Apr 14, 2022

github-actions bot locked as resolved and limited conversation to collaborators Feb 29, 2024

Issue #3483 Add cert retrieval for requests #3490

Issue #3483 Add cert retrieval for requests #3490

Conversation

dusktreader commented Dec 14, 2020

Pull Request Check List

tbennett6421 commented Jan 15, 2021

dusktreader commented Jan 18, 2021

GooseYArd commented Feb 1, 2021

GooseYArd commented Feb 2, 2021 • edited Loading

dusktreader commented Feb 3, 2021

GooseYArd commented Feb 3, 2021

dusktreader commented Feb 3, 2021

BrutalSimplicity commented Feb 26, 2021

astro-josh commented Apr 13, 2021

GooseYArd commented Apr 13, 2021

abn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abn commented Apr 13, 2021

GooseYArd commented Apr 13, 2021

dusktreader commented Apr 13, 2021

abn 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

surry commented Apr 27, 2021

dusktreader commented Apr 28, 2021

surry left a comment

Choose a reason for hiding this comment

abn 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

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

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

dusktreader commented May 27, 2021

dusktreader commented Jun 7, 2021

surry commented Jun 8, 2021

astro-josh commented Aug 23, 2021

jakeri commented Oct 4, 2021

dusktreader commented Oct 4, 2021

dusktreader commented Dec 14, 2021

LarsStegman commented Jan 18, 2022

neersighted commented Jan 18, 2022

dusktreader commented Jan 18, 2022

BrutalSimplicity commented Feb 27, 2022

ITProKyle commented Mar 15, 2022

dusktreader commented Mar 16, 2022

chriswhite199 commented Mar 16, 2022

ITProKyle commented Mar 23, 2022

chriswhite199 commented Mar 23, 2022

radoering commented Apr 14, 2022

papb commented Aug 1, 2022

github-actions bot commented Feb 29, 2024

GooseYArd commented Feb 2, 2021 •

edited

Loading