python / cpython

The Python programming language
https://www.python.org
Other
62.66k stars 30.05k forks source link

Introduce configure --disable-openssf-guide or --enable-openssf-guide #121996

Closed corona10 closed 2 months ago

corona10 commented 2 months ago

https://github.com/python/cpython/issues/112301 has good intentions to make CPython more safe. But we can not satisfy all platforms.

There are 2 issues with openssf compiler options.

  1. Performance degradation, there is no measurable impact, but it does not mean that everyone wants this.
  2. Build failure, which is not managed by our tier system.

To solve this issue, I would like to recommend to provide ./configure --disable-openssf-guide or ./configure --enable-openssf-guide as optional.

I am not sure which option would be better.

If we choose ./configure --disable-openssf-guide than people should use this flag if the compiler flags does not support their systems or make any issue for their system. If we choose ./configure --enable-openssf-guide, we will enable these options only for our tier 1 system or only for the CI.

WDYT? @nohlson @vstinner @mdboom

Linked PRs

corona10 commented 2 months ago

cc @kulikjak I prefer this option rather than workaround approach for solaris systems.

mdboom commented 2 months ago

I worry about any more flags that add to the overall CI testing matrix (which already has holes). I'd prefer the approach Victor suggested elsewhere to add configure checks for each of the flags, so that platforms that support them (mainly the Tier 1 platforms) will automatically apply and test them whereever possible.

I think I'd prefer to reserve any configure flags for any extra checks that are known to have a performance impact, rather than ones that should be no cost and work for most people. That suggests a different name, something like --enable-slower-safety or something.

corona10 commented 2 months ago

I worry about any more flags that add to the overall CI testing matrix (which already has holes).

Oh, I am just thinking about applying OpenSSF policy by default from tier 1 to tier 3 by default. So I think that there will be no hole if we can catch them from GA to the build bot because we will not apply the new flag to the any of CIs.

The main problem is that we can not catch the issue outside of our tier1 to 3 platforms and it already happens.

I know that we can fix them before we officially release the CPython but the problem is that we can not fix the version that we already released, but there are a lot of people who build the CPython from the officially released source code so IMO, so we have to provide a way to solve the issue easily.

So my suggestion is to provide ./configure --disable-openssf-guide and use the flag if the platform does not support our OpenSSF compiler options while they wait for the fixed version.

In short,

corona10 commented 2 months ago

That suggests a different name, something like --enable-slower-safety or something

It is possible, but I think we have to define the ground rule about compiler safety options, whether they should be applied by default or optional. (TBH, by default is ideal..)

mdboom commented 2 months ago

So my suggestion is to provide ./configure --disable-openssf-guide and use the flag if the platform does not support our OpenSSF compiler options while they wait for the fixed version.

Ah, I see. Thanks for the clarification. This seems fine, then.

It is possible, but I think we have to define the ground rule about compiler safety options, whether they should be applied by default or optional. (TBH, by default is ideal..)

I have a more nuanced view. I think by default only if we know there is no runtime performance impact. For those with performance impact, we should hold off on adding them to the defaults until we can eliminate or counteract the performance impact. (There may also be a grey area where we are willing to accept a small performance impact).

corona10 commented 2 months ago

Okay, so as the middle ground.

Does it seem fine?

kulikjak commented 2 months ago

cc @kulikjak I prefer this option rather than workaround approach for solaris systems.

I agree with what was said here, but it's a different issue from #121979. When you move the check behind --disable-safety (well, behind not --disable-safety), it doesn't fix what my PR is trying to solve - that is, Solaris/Illumos linker needs -fstack-protector-strong set in LDFLAGS set as well.

I am happy to rebase it onto this though when it gets into main.

nohlson commented 2 months ago

I have a more nuanced view. I think by default only if we know there is no runtime performance impact. For those with performance impact, we should hold off on adding them to the defaults until we can eliminate or counteract the performance impact. (There may also be a grey area where we are willing to accept a small performance impact).

I agree with this idea. For a majority of options the autoconf check flag should keep builds from failing if the option is not available, and there might be some nuances as pointed out by @kulikjak above.

The PR for -D_FORTIFY_SOURCE=3 option did merge and I can create a PR to treat warnings as errors for that check which I didn't include but was brought up in the discussion. This was an option that does theoretically introduce a performance impact, however we saw little regression in benchmarks, so this would be in that grey area.

I like the idea of a single flag to --enable-slower-safety for future options which we see have a more significant impact on benchmarks.

corona10 commented 2 months ago

@nohlson cc @mdboom

I merged the PR https://github.com/python/cpython/pull/122054 because we need 2 options for the following reasons. (I think that we can modify documentation or naming before 3.14 is officially released)

nohlson commented 2 months ago

@corona10 Something to note is that the benchmark https://github.com/python/cpython/issues/112301#issuecomment-2168663553 was ran on my own machine and is certainly unstable. All subsequent benchmarks have been ran by @mdboom on dedicated benchmarking machines mentioned in that thread and they show that for the options already enabled there is no discernible impact on performance.

We can decide for each option going forward which configure option it falls under, but my idea was that in pursuit of #112301 if we benchmarked an option that had significant performance impacts that we would not enable it at all, even in slower-safety unless it was a security improvement that could not be overlooked. The options suggested in the current OpenSSF guidance don't seem to fit that description based on the current benchmarks we have ran.

corona10 commented 2 months ago

@nohlson IMO, this topic is in favor of each person's perspective. As the core dev, I just want to provide the opt-out option if the user can not build CPython with newly introduced options. For those reasons, we still provide LTO compiler option and BOLT option as the optional option rather than default option for the same reason. (See: --with-lto, --enable-bolt, those options are performance related option not security option but same perspective from the build failure issue)

So if you think that D_FORTIFY_SOURCE should be the default option than just move it to the --disable-safety and remove the --enable-slower-safety. I am fine with it.

encukou commented 2 months ago

To repeat, BASECFLAGS is not just about building CPython. These flags are also used to build third-party extensions (might have conflicting requirements). It's not easy for extension authors to separate these from truly essential options like the Python include paths.

Is this the intent? If so, it should indeed be documented well.

corona10 commented 2 months ago

@nohlson By the way who is the mentor of GSoC project?

nohlson commented 2 months ago

So if you think that D_FORTIFY_SOURCE should be the default option than just move it to the --disable-safety and remove the --enable-slower-safety. I am fine with it.

I think future options enabled should be minimally invasive and we can give the user an opt-out, rather than many performance impacting options that are opt-in, so that if an option does significantly impact benchmarks it would not be included at least for this initial effort. Limiting the options to CPython as @encukou suggests can be implemented and seems to be more in line with the initial goals of #112301. @corona10 the GSoC project mentor is @sethmlarson

encukou commented 2 months ago

AFAIK: Traditionally on Linux, this kind of options is set by the distro, rather than in the upstream project. The one I'm familiar with is Fedora, which has guidelines and defaults that aren't too different from what OpenSSF in spirit, with people always pushing for more safety if it doesn't break things. Fedora also has a split between what the distro itself builds and what's exported for users building their own extensions (if I recall correctly, adding that was driven by Python, back in the day).

(Click to see them if you're curious) ``` $ cat /etc/os-release | grep PRETTY_NAME PRETTY_NAME="Fedora Linux 40 (Workstation Edition)" $ rpm --eval '%{build_cflags}' -O2 -flto=auto -ffat-lto-objects -fexceptions -g -grecord-gcc-switches -pipe -Wall -Werror=format-security -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -m64 -march=x86-64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protection -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer $ rpm --eval '%{build_ldflags}' -Wl,-z,relro -Wl,--as-needed -Wl,-z,pack-relative-relocs -Wl,-z,now -specs=/usr/lib/rpm/redhat/redhat-hardened-ld-errors -specs=/usr/lib/rpm/redhat/redhat-hardened-ld -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -Wl,--build-id=sha1 $ rpm --eval '%{extension_cflags}' -fexceptions -fcf-protection $ rpm --eval '%{extension_ldflags}' ```

Such a redistributor will want CPython to:

On Windows and Mac, CPython itself is the builder/redistributor, so it's much more important to set the right flags there. It seems Windows is not in scope here, since MSVC uses different flags.

When documenting the options, perhaps it would be best to frame these not as “what OpenSSF recommends”, but as “what CPython uses for the python.org releases” and “what we recommend and test for slower but safer builds”.


Then there is the of third-party extensions. Good defaults with easy opt-in would probably involve python3-config command and the sysconfig module on the CPython side, and much more work in setuptools and/or packaging standards. So, yes, for GSOC, it's probably best to limit the options to CPython only.

eli-schwartz commented 2 months ago

As an extension packaging tool, we strictly avoid python3-config since it provides no way to avoid BASECFLAGS. If we don't have access to python3.pc, we do a rather painful hacky rummaging-around through the undocumented internals of sysconfig.get_config_vars() to retrieve the minimal incdir flags. :)

In fact, python-config also includes whatever $CFLAGS were exported at the time CPython was built. It's totally unusable. There is no point worrying what additional changes the OpenSSF flavored flags are going to have on it.

corona10 commented 2 months ago

@encukou cc @nohlson

With https://github.com/python/cpython/commit/046670c3a0480560b6bfa06727fd7aa6a1798614 now new options only applied to CFLAGS_NODIST. By the way, since this issue only covers how to disable default safety options and enable slower safety option and the issue itself, if we need to discuss the compiler option itself, let's discuss it at https://github.com/python/cpython/issues/112301, which is the original GSoC issue!