allow appending deployment key to username (#2062)

[finswimmer]

This PR allows appending deployment keys to usernames as used by gitlab. According to gitlab's docs a + is allowed in usernames. This is fixed as well.

Fixes: #2062

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

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

[ryancausey]

Found a case where this still failed.

[ryancausey]

I found a case in my usage where this doesn't appear to be enough. The deploy key for gitlab appears to allow - characters, and the regex portion [\w\d]+ doesn't match when a - is present.

Changing the [\w\d]+ to [\w\d-]+ got it working for me.

[finswimmer]

Thanks for the feedback. Is there any source where it is described how the deployment key looks like? I wasn't able to find one :(

[ryancausey]

I haven't found any. The only suggestion I can make is to perhaps make the portion after the : much less restrictive since it looks like the end of that match will be bounded by @ in all the patterns below.

I don't know what value there is in trying to restrict what can go after the : unless there's a need for it to make the matching work properly.

[betaboon]

hello, i have been running into the same problem. using the changes proposed in this PR solved the problem for me. can this get merged ?

[ryancausey]

Is there something blocking this PR from being reviewed and/or merged?

[abn]

#2062 (comment)

TLDR; we should parse for both user and password seperately. And possibly log a warning that this would mean credentials get stored as plain text value in both the lockfile and pyproject.toml.

This will require rework.

[webyneter]

When shall we expect the rework to be finished?

[finswimmer]

@webyneter: @abn brings up a very good point I haven't care about when writing this PR: security. When we allow the schema user:deploymentkey/user:password these things will be included in the pyproject.toml and in sdist/wheel when the package get's build. This is a bit risky as one shouldn't like to share these information when deploying a package.

So, we have to discuss whether we like to support this at all and if yes, if a warning about the security concerns are enough.

fin swimmer

[webyneter]

Having this in poetry would make a lot of difference, for now I have to stick with 1.0.2 just to be able to install internal dependencies...

[ryancausey]

The warning and log seems a reasonable approach for now. I would consider this a first pass.

I wouldn't know how else people are storing their deploy tokens beyond it sitting in their requirements.txt file. Unless pip already has a way to do environment variable substitution into a requirement string?

[abn]

In the end, it falls to the project maintainers to decide how they want to go about it for their projects. Personally, having deployment keys in metadata and/or plain text git files are a no go. But ultimately, poetry is a tool and so we should allow valid configuration (ie. urls in the user:pass format). However, in the case where these get included as plain text in config/metadata we could require the user to maybe provide an override flag, something along the lines of --allow-insecure-metadata.

If the package is public, you are better off not needing a deployment key anyway /shrug. Eventually, there is a possibility that we can extend how poetry handles index credentials to also allow for repo credentials. However, there is still the issue of these being available in a distribution metadata.

@webyneter is the use of git credentials not a viable solution in your case? (see #2062) Considering the context, I am guessing by "internal dependencies" you mean from SCM and not private index.

[webyneter]

@abn yes, exactly: I'm installing a python package from a private gitlab repo within my gitlab org, the project has got setup.py and stuff.
I didn't think about git credentials, will give it a try, thanks.

[ryancausey]

AFAIK the pip ecosystem doesn't have a nice way to hide this information either, and defaults to "use a private PYPI". If this workflow was to be supported and truly not store the credentials in metadata or files, I'm guessing it would be a lot of work.

The thought I had about it would be the same way I would end up solving it for requirements.txt files: using jinja templating + environment variables to perform substitution at parse time. However, that doesn't necessarily ensure that the values don't get written back out into a lock file and other metadata files. Extra care would have to be taken to support that.

But at the end of the day, pip lets you pass requirements formatted as such and install them so I personally don't think poetry should completely prevent it. I agree with @abn that it's not good practice but that it also shouldn't be entirely prevented. An extra option to force override and allow it seems like a reasonable middle ground between a full blown secure implementation and fully locking off the ability to use a pip feature.

[teto]

why check urls at all ? It's not like you can prevent using unreachable urls in the first place (server an go down etc), it can just introduce bugs. anyway, looking forward to a fix :)

[finswimmer]

@teto: We need to parse the url to be able to identify the several parts like the source itself, the protocol used, the revision/tag/branch. The information must be stored in the correct format in the pyproject.toml. Also we must be able to create a valid PEP 508 string out of it.

Anyway, the corresponding code has move to poetry-core. So I will move the PR over to it in the next days.

[setu4993]

Do the changes in this PR also address #2348?

[setu4993]

@finswimmer : Have you had a chance to create an equivalent PR for these changes on poetry-core? I looked but didn't see anything there, but maybe I missed something.

If not, what's the ETA on getting this merged?

[finswimmer]

@setu4993 It's over here: python-poetry/poetry-core#96

But I plan to include some kind of warning as suggested by @abn in poetry itself. That's why it is a draft PR.

[github-actions]

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.