Closed PabloAlexis611 closed 5 months ago
abn
commented
6 months ago Isn't this an expected error for FIPS enabled system? As in "do not rely on md5 as a secure hash"? The error message could be better and I guess we could add an option to reduce the strictness here.
dimbleby
commented
6 months ago yeah, setting usedforsecurity=False is certainly the wrong thing to do here.
The hash absolutely is being used for security - its purpose is to prove that the package that you are installing is the same package that was uploaded to the repository.
Of course md5 is not a secure hash: but then the FIPS-enabled system is just doing what it should and refusing to allow you to rely on that hash.
People who are running FIPS-enabled systems surely care even more than the rest of us that they are getting the packages that they think they are getting: disabling that check would be the opposite of what the FIPS-enabled crowd should want to do.
Update your private mirror to provide secure hashes.
PabloAlexis611
commented
6 months ago Makes sense. To provide context, the private mirror is hosted by a Sonatype Nexus instance - it should provide sha256 hashes as of version 3.41. I'm starting to think then this is not a poetry issue and indeed what's happening is correct behavior. We might close this ticket.
@abn @dimbleby is there a way though I can configure poetry to not use MD5 at all, or to force using SHA256? Or is it fully reliant on the repository to provide the sha256 in the resolved URLs?
In this case, the tomli package already has a defined sha256 in the poetry.lock file, so I'd have thought if the sha256 is not available from the repo, poetry could calculate this hash instead of falling back to the md5 provided by the request to the private repository - making sure it matches what already exists in the poetry.lock file.
abn
commented
6 months ago The sha256 calculation was introduced in #2958. The use of sha256 will be required if known_hash is None.
So, something like this might improve the error handling better.
known_hash = None
with contextlib.suppress(ValueError, AttributeError):
# we handle ValueError here as well since under FIPS environments this is what is raised
known_hash = getattr(hashlib, hash_name)() if hash_name else None@abn I like that solution better since it doesn't involve lying to the FIPS system (as my unsecure workaround was doing, which of course I only did for testing purposes)
I tested this patch you put above as follows:
# ...
from contextlib import contextmanager, suppress # <- added suppress here
# ...
known_hash = None
with suppress(ValueError, AttributeError):
# we handle ValueError here as well since under FIPS environments this is what is raised
known_hash = getattr(hashlib, hash_name)() if hash_name else NoneAnd it works in my FIPS-enabled system, no more [digital envelope routines: EVP_DigestInit_ex] disabled for FIPS errors due to a repository request providing me with MD5 hashes in the URL. This indeed improves the error handling for FIPS-enabled systems.
Could we get a PR with your changes in? Would love to have this in a future poetry release our team could update to. I'll keep the patch in our build-process in the meantime. If you're too busy I'm willing to contribute this update as well.
abn
commented
6 months ago Happy to have you get a PR up for review if you can. Help is always welcome. The only thing I'd say we'd need is a test case that validates the scenario better to ensure we do not regress and that we do indeed validate the computed hashes.
Also there is a bit of nuance here regarding security. If your index does not provide sha256, you are in effect trusting the local computed hash of whosoever generated the lockfile. It does weaken your security posture, but not a lot
mnunna-broadcom
commented
6 months ago @PabloAlexis611 I am very much interested in this change too. Are you planning on creating that PR?
PabloAlexis611
commented
6 months ago @mnunna-vmware yes, got the changes locally and on my fork, working on the unit tests before opening up the PR
github-actions[bot]
commented
4 months ago This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Description
Using custom sources for a private repo (private mirror of PyPI), we need to perform a
poetry lock --no-updatecommand so that thepoetry.lockfile gets regenerated with the new source URL data. This stems from internet-enabled development, but air-gapped builds.There might be the case that one of the dependencies pulled from this private mirror of PyPI only contains an
md5known hash.This then attempts to generate an MD5 hash on a FIPS-enabled system, causing failure.
Workarounds
To manually patch the
src/poetry/repositories/http_repository.pyfile at https://github.com/python-poetry/poetry/blob/28d5c007d8a73fac466deedd3b691fbf14bd9fff/src/poetry/repositories/http_repository.py#L377with the following content:
Which is not what I'd want to do in a FIPS-enabled system.
Poetry Installation Method
pipx
Operating System
RedHat 8.9
Poetry Version
Poetry (version 1.8.1)
Poetry Configuration
Python Sysconfig
Example pyproject.toml
Poetry Runtime Logs