Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

gh-127298: When in FIPS mode ensure builtin hashes check for usedforsecurity=False#127301

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
xnox wants to merge12 commits intopython:main
base:main
Choose a base branch
Loading
fromxnox:hashlib-builtin-fips-check

Conversation

xnox
Copy link

@xnoxxnox commentedNov 26, 2024
edited
Loading

When _hashlib/OpenSSL is available, and OpenSSL is in FIPS mode, ensure that builtin (fallback) hash implementations are wrapped with a check for usedforsecurity=False. It is likely that buitin implementations are FIPS unapproved (either algorithm disallowed; or the implementation not certified by NIST).

This enables strict approved-only compliance when usedforsecurity=True on FIPS systems only.

And yet it also enables fallback access with usedforsecurity=False for any missing (historical, disallowed or missing certified implementation) algorithms (i.e. blake2, md5, shake/sha3) depending on the runtime configuration of OpenSSL.

@xnox
Copy link
Author

FYI@stratakis

@xnox

This comment was marked as resolved.

@xnoxxnoxforce-pushed thehashlib-builtin-fips-check branch 3 times, most recently fromdb5c976 toad0fab8CompareNovember 26, 2024 18:20
…edforsecurity=FalseWhen _hashlib/OpenSSL is available, and OpenSSL is in FIPS mode,ensure that builtin (fallback) hash implementations are wrapped with acheck for usedforsecurity=False. It is likely that buitinimplementations are FIPS unapproved (either algorithm disallowed; orthe implementation not certified by NIST).This enables strict approved-only compliance when usedforsecurity=Trueon FIPS systems only.And yet it also enables fallback access with usedforsecurity=False forany missing (historical, disallowed or missing certifiedimplementation) algorithms (i.e. blake2, md5, shake/sha3) depending onthe runtime configuration of OpenSSL.
@xnoxxnoxforce-pushed thehashlib-builtin-fips-check branch fromad0fab8 to28d8035CompareNovember 26, 2024 18:32
@picnixz
Copy link
Member

picnixz commentedNov 27, 2024
edited
Loading

The built-in algorithms are based on HACL* and are therefore formally verified implementations. So I'm not entirely sure we can say that they are entirely FIPS unapproved (except that some algorithms, whatever their imementation is, might not be allowed on FIPS-only systems).

So, I would like to confirm with@msprotz whether their implementations of SHA and blake2 are FIPS-friendly or not (md5 is never FIPS-friendly I think?)

@xnox
Copy link
Author

xnox commentedNov 27, 2024
edited
Loading

The built-in algorithms are based on HACL* and are therefore formally verified implementations. So I'm not entirely sure we can say that they are entirely FIPS unapproved (except that some algorithms, whatever their imementation is, might not be allowed on FIPS-only systems).

If the binary build was not tested by a NIST accredited lab, and there is no CMVP certificate issued, it is not certified. HACL* formally verified implementation or not.

As in search onhttps://csrc.nist.gov/projects/cryptographic-module-validation-program/validated-modules/search should bring up an active certificate, for the binary build (not source-code) on a given supported operating environment (architecture & OS type).

So, I would like to confirm with@msprotz whether their implementations of SHA and blake2 are FIPS-friendly or not (md5 is never FIPS-friendly I think?)

It is irrelevant what is or isn't FIPS-friendly. In approved only mode, only certified implementation can be used, which has a CMVP certificate.

Meaning you cannot even use SHA2-256 HACL implementation, unless it was NIST certified. Despite it producing the same hexdigits for the same inputs =)

Hence here builtin implementations are only made available at runtime with a so called service indicator (ISO/FIPS term) when usedforsecurity=False is explicitly specified.

One cannot universally know what is or isn't allowed, as it depends on which module one is using at runtime, and what its security policy stated. There is no universal statement for FIPS as active modules last for 5 years, and historical modules continue to be in use, despite submission requirements changing.

Meaning an old 1.1.1 openssl fips module on a supported system can be missing shake/sha3, and allow md5. More recent 140-2 certificates may still allow MD5 as part of TLS MD5-SHA1 and HMAC, but not digest. Upcoming modules likely to not have MD5 at all (neither for TLS nor HMAC).

W.r.t. blake2 it is NIST disallowed for cryptographic protection of sensitive information (it lost to KECCAK in sha3 competition).

Note it is extremely useful to still have access to blake2 and md5 as "one-way non-cryptographic compression" functions. For example md5 will continue to be used to identify public cloud buckets; views in SQL databases; all US court submitted documents and so on. Thus access for non-security purposes is very relevant for users.

Example of blake2 usage for non-security purposes as a tuplehashhttps://github.com/aiidateam/aiida-core/blob/f74adb94cc1e8439c8076f563ec112466fdd174b/src/aiida/common/hashing.py#L78 which is not security relevant

Please also note diagrams & further commentry athttps://discuss.python.org/t/python-and-openssl-fips-mode/51389/12

The goal of this patch to ensure that no distro modifications are required of Python when operating on a system with OpenSSL in FIPS mode. This pull request will ensure that one can download python.org distributed or self-built cpython builds; and they just work correctly as expected by FIPS users; and matches behavior of all other open source interpreters. By default, they only offer approved & CMVP certified implementations. It is very consent based.

@picnixz
Copy link
Member

picnixz commentedNov 27, 2024
edited
Loading

In approved only mode, only certified implementation can be used, which has a CMVP certificate.

That's what I meant by FIPS-friendly. I wanted to confirm with the author whether this PR is needed or not. The reason is that you claimed that "It is likely that buitin implementations are FIPS unapproved" which I think should be verified by asking the relevant people first.

xnox reacted with heart emoji

FIPS mode is an OpenSSL feature and we don't require OpenSSL. So anyone wanting to rely on this will need to ensure their build includes Modules/_hashopenssl.c as `_hashlib` linked appropriately.
@xnox
Copy link
Author

xnox commentedNov 27, 2024
edited
Loading

@gpshead I have access to some 1.x based FIPS modules (Ubuntu & Azure Linux) and I think this will work correctly as is there too at runtime.

The unit test mock is 3+ specific, but runtime implementation is 1.x compatible. As _hashlib.get_fips_mode is implemented for both 1.x & 3 ABI.

Hence news entry should mention OpenSSL, but not the providers and/or major openssl version.

@gpsheadgpshead self-assigned thisNov 27, 2024
@gpsheadgpshead added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 27, 2024
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@gpshead for commitf8c5133 🤖

If you want to schedule another build, you need to add the🔨 test-with-buildbots label again.

xnox reacted with heart emoji

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 27, 2024
gpshead
gpshead previously approved these changesNov 27, 2024
Copy link
Member

@gpsheadgpshead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I pushed some changes based on what I saw in my review, there was some pedantic correctness stuff like get_fips_mode raising ValueError or returning values other than 0 or 1 to be handled. I moved the repeated logic into a single execution in the global scope instead of upon every constructor registration function call.

I also updated comment and news wording (good point about this not being 3 specific, I saw that in the code after my first news revision; fixed).

I'dlike it if we got rid of OpenSSL hashlib entirely in the long run, but that isn't likely for 3.14 and some vendors needing this bureaucratic oddity is a reason to keep that around anyways so long as it isn't a major burden.

I sent this to the buildbots out of curiosity to see if any have environments that trip the new hashlib_fips test up. But Ithink that is well enough written with the right conditionals that it'll properly skip as needed.

@gpshead
Copy link
Member

The buildbots are turning up some interesting failures. is the setUpClass assertion I added about _hashlib and _ssl not having been loaded accurate? It is possible to execute the test suite such that each test is not getting its own new process, whichmight be the source of that.

@gpsheadgpshead self-requested a reviewNovember 27, 2024 08:30
@xnox
Copy link
Author

I see what is happening with Blake tests on a RHEL in fips mode. I think we should adjust all Blake tests to always pass inusedforsecurity=False if we want all of those Blake tests to pass when the builder host is in fips mode.

Alternatively we can wrap OpenSSL C APIs to force or lift "fips=yes" property query string.

Btw the fact that Blake is accessible when host OS is in FIPS is one of the motivators for these patch series.

@xnox
Copy link
Author

I'dlike it if we got rid of OpenSSL hashlib entirely in the long run, but that isn't likely for 3.14 and some vendors needing this bureaucratic oddity is a reason to keep that around anyways so long as it isn't a major burden.

Majority of use cases should be using XXH3 from thehttps://xxhash.com/ family of hashes. xxhash3 is faster than md5/sha1/sha2/sha3. And secure hashes are often used out of convenience and availability, rather than matching intended purpose.

Creating and verifying signatures should be done with atomic operations which verify whole message and its signature in one shot; rather than computing/extracting digest separately and then separately signing/verifying it.

For internal only purposes, python could use xxhash3 or parallelhash-encoded-xxhash3 to create performant hashes of tuples.

Pushing users towards xxhash3 and pyca/cryptopgraphy is probably best in the long run. The issue is that it is impossible to tell if hashlib is or isn't being used for cryptographic security as defined by relevant legislation for a given user.

Having static builds of HACL implementations certified, and allowing to statically link those at build time would help too. But I am not aware of anyone willing to sponsor such certification work. Note that this approach is being taken with boringcrypto for golang; and aws-lc crypto for rust.

anyway, this is a sidetrack that can be discussed separately later.

gpshead reacted with thumbs down emojixnox reacted with laugh emoji

@gpshead
Copy link
Member

!buildbot RHEL

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@gpshead for commitdc964b9 🤖

The command will test the builders whose names match following regular expression:RHEL

The builders matched are:

  • AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR
  • s390x RHEL8 PR
  • s390x RHEL8 LTO PR
  • PPC64LE RHEL8 LTO + PGO PR
  • AMD64 RHEL8 FIPS No Builtin Hashes PR
  • AMD64 RHEL8 Refleaks PR
  • s390x RHEL9 PR
  • aarch64 RHEL8 LTO PR
  • s390x RHEL9 LTO PR
  • s390x RHEL8 Refleaks PR
  • PPC64LE RHEL8 PR
  • AMD64 RHEL8 PR
  • PPC64LE RHEL8 Refleaks PR
  • s390x RHEL9 Refleaks PR
  • PPC64LE RHEL8 LTO PR
  • s390x RHEL8 LTO + PGO PR
  • aarch64 RHEL8 Refleaks PR
  • s390x RHEL9 LTO + PGO PR
  • AMD64 RHEL8 LTO PR
  • aarch64 RHEL8 PR
  • AMD64 RHEL8 LTO + PGO PR
  • aarch64 RHEL8 LTO + PGO PR

@xnox
Copy link
Author

🤖 New build scheduled with the buildbot fleet by@gpshead for commitdc964b9 🤖

The command will test the builders whose names match following regular expression:RHEL

The builders matched are:

  • AMD64 RHEL8 FIPS Only Blake2 Builtin Hash PR

Will fail here:

h=hashlib.blake2s()

And in similar places, as on the above machine blake will now need "usedforsecurity=False" kwarg to be passed in.

See previous buildbot failure for all the functions where blake2 is attempted to be used, without specifying usedforsecurity=False which is now getting blocked by the new wrapper.

@gpsheadgpshead dismissed theirstale reviewNovember 27, 2024 11:02

FIPS buildbots (which aren't a supported platform) unhappy, one already broken on main FWIW

@xnox
Copy link
Author

xnox commentedDec 1, 2024

FIPS buildbots (which aren't a supported platform) unhappy, one already broken on main FWIW

@gpshead

Do you want me to help with this? I have managed to make testsuites pass on fips-enabled hosts before. And some of the changes here, will induce additional test failures on FIPS hosts. You were triggering build-bots which are not connected as github actions, thus should I simply run full testsuite on fips enabled host and propose fixes => would you then be able to trigger buildbot to verify / review improvements to get them back to green?

@gpshead
Copy link
Member

I believe#127467 for multiprocessing & concurrent.futures and the hashlib PR#127492 it is based on together take care of the bulk of the FIPS buildbot issues and help prepare for a PR such as this one.

I was able to get a similarly configured Python as those buildbots use to pass tests this way both with and without a restricted FIPS mode OpenSSL.cnf in#127467. (we'll see what the bot says now). I haven't tried a rebase of this on top of that yet.

@msprotz
Copy link
Contributor

So, I would like to confirm with@msprotz whether their implementations of SHA and blake2 are FIPS-friendly or not (md5 is never FIPS-friendly I think?)

Just getting back online after a week off. We do not have a FIPS certificate for HACL*. This is something we've been wanting for a long time, and I'm happy to work with a lab on this, but we would need someone to sponsor this work as getting a FIPS certificate from an accredited lab is not cheap.

This would nice for Python as it would allow running Python in FIPS mode without depending on OpenSSL. But right now, relying on OpenSSL when FIPS compliance is required is the only way.

Regarding the specific point of static linking: yes, this is the way to go, and I've had discussions with both NIST folks and aws-lc folks that this is a known pathway towards getting certification when static linking is needed (as is currently happening for the python build).

@picnixz let me know if you have any other questions, happy to elaborate, but hopefully I haven't missed anything on this thread

picnixz, gpshead, and xnox reacted with thumbs up emoji

xnox added a commit to xnox/os that referenced this pull requestDec 12, 2024
With these changes, blake will not be available in FIPS mode unlessusedforsecurity=False.Also see:-python/cpython#127301-python/cpython#127298
xnox added a commit to wolfi-dev/os that referenced this pull requestDec 12, 2024
With these changes, blake will not be available in FIPS mode unlessusedforsecurity=False.Also see:-python/cpython#127301-python/cpython#127298
@xnox
Copy link
Author

@gpshead hi, my PRs had comments that you are taking over with these changes instead, but they now seem to have stalled. Are you still actively working on this and related PRs?

techalchemy added a commit to techalchemy/os that referenced this pull requestApr 28, 2025
- fixes chainguard-dev/internal-dev/wolfi-dev#11886- backports fromwolfi-dev#36503 (seepython/cpython#127301)Signed-off-by: Dan Ryan <daniel.ryan@chainguard.dev>
techalchemy added a commit to techalchemy/os that referenced this pull requestApr 28, 2025
-fixeschainguard-dev/internal-dev#11886- backports fromwolfi-dev#36503 (seepython/cpython#127301)Signed-off-by: Dan Ryan <daniel.ryan@chainguard.dev>
techalchemy added a commit to techalchemy/os that referenced this pull requestApr 29, 2025
-fixeschainguard-dev/internal-dev#11886- backports fromwolfi-dev#36503 (seepython/cpython#127301)Signed-off-by: Dan Ryan <daniel.ryan@chainguard.dev>
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tirantiranAwaiting requested review from tirantiran is a code owner

@erlend-aaslanderlend-aaslandAwaiting requested review from erlend-aaslanderlend-aasland is a code owner

@gpsheadgpsheadAwaiting requested review from gpsheadgpshead is a code owner

Assignees

@gpsheadgpshead

Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

5 participants
@xnox@picnixz@bedevere-bot@gpshead@msprotz

[8]ページ先頭

©2009-2025 Movatter.jp