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

Fix legacy relative paths including ../ #327

Merged

Conversation

johanwiren
Copy link

@johanwiren johanwiren commented Jun 28, 2021

Artifactory's pypi repositories creates links from the index pages that look something like this: ../../$package-name/$version/$artifact.$ext. This breaks since urllib does not normalize the ../ parts from the links and Artifactory does not resolve them when fetching the artifacts, leading to an unexpected 404.

This PR adds support for resolving the ../ parts of the URL.
It also adds the cacert derivation into the build to allow SSL certificate verification.

Nix is kind of new to me and I struggled really hard trying to write a sane test for this and failed so I'm posting this PR without tests and am open to write tests if someone can hint me how to set that up for testing this.

@adisbladis
Copy link
Member

The SSL cert verification code needs to be the way it is for reasons explained in #299 (comment).

It cannot be "fixed" by adding root certs.

@adisbladis
Copy link
Member

Apart from that I can't really judge the code here. CC @nlewo

@adisbladis adisbladis requested a review from nlewo June 29, 2021 02:57
@johanwiren
Copy link
Author

johanwiren commented Jun 30, 2021

I read #299 now and the comments there. The reason why adding cacert is necessary is because it is currently not working the way the code portrays it should. It does try to validate certificates, hence the ssl.SSLCertVerificationError that is mentioned in that same thread. I encountered it too.

Passing ssl.CERT_NONE as context into urllib.request.urlopen is not even valid since context needs to be a valid SSLContext.

From the documentation: "If context is specified, it must be a ssl.SSLContext instance describing the various SSL options. See HTTPSConnection for more details."

I think what the code was meant to do was

context = ssl.create_default_context()
context.verify_mode = ssl.CERT_NONE

@johanwiren
Copy link
Author

Can we perhaps make verifying certificates an opt-in (or opt-out) so that the user can control it? Shall I open a separate issue for that maybe?

For my use case it is very important that certificates are verified.

@nlewo
Copy link
Member

nlewo commented Jun 30, 2021

@johanwiren Regarding certificate, i'm surprised you need to verified them. It currently works like that:

  1. Poetry reads the index with the ssl verification enabled to generate the poetry.lock file
  2. The lock file then contains the hash (sha256) of the package archive
  3. Poetry2nix reads the poetry.lock and generates a fetchurl expression which contains the hash from the poetry.lock file
  4. At build time, Nix downloads the package archive and check the hash of the downloaded artifact correspond to the hash specified in the poetry.lock file (because it is a fetchurl parameter)

So, i don't see where the threat could be: we use SSL to get the hash of the file and Nix validates this hash. Even if Nix downloads the artifact from a compromised source, the hash verification will then fails.

Also, I submitted #329 to fix the SSL error you encountered (my bad, sorry).

(I will also take a look at your initial issue.)

@johanwiren
Copy link
Author

I removed the cacert fix from this PR. It only addresses handling urls containing ../ now.

@johanwiren
Copy link
Author

@nlewo Thanks for explaining the security mechanisms in play here. As I said, I'm new to Nix, and I guess this is related to always having "reproducible builds" and I can understand the tradeoffs being made here.

@adisbladis
Copy link
Member

Since I've merged @nlewo's latest TLS fixes this needs rebasing.

@johanwiren
Copy link
Author

Rebased on master

@adisbladis
Copy link
Member

LGTM!

@adisbladis adisbladis merged commit e40e8ed into nix-community:master Jul 2, 2021
@johanwiren johanwiren deleted the fix-legacy-relative-path branch July 2, 2021 16:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants