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

@nohlson

#112301 has good intentions to make CPython more safe.
But we can not satisfy all platforms.

There are 2 issues with openssf compiler options.

Performance degradation, there is no measurable impact, but it does not mean that everyone wants this.
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

@kulikjak

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

pythongh-121996: Introduce configure --disable-openssf-guide

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.

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.

Solaris: gh-112301: -fstack-protector-strong is also needed in LDFLAGS on Solaris/Illumos #121979
Compiler issue that didn't catch at GA: Consider applying flags for warnings about potential security issues #112301 (comment)

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,

By default: ./configure == ./configure --disable-openssf-policy=no for everywhere.
If user meet issues for their platform that was not watched by our CI system: use ./configure --disable-openssf-policy if there is no way to build.

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..)

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).

Okay, so as the middle ground.

./configure --disable-safety: Disable no performance impact compiler options. (CI will not enable this configuration by default)
./configure --enable-slower-safety: Enable performance impact compiler options. (We should update the Github Action configuration too)

Does it seem fine?

pythongh-121996: Introduce --disable-safty and --enable-slower-safty

Merge remote-tracking branch 'upstream/main' into pythongh-121996-two…

@kulikjak

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.

@kulikjak

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.

gh-121996: Introduce --disable-safety and --enable-slower-safety opt…

@nohlson

@nohlson cc @mdboom

I merged the PR #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)

--enable-slower-safety: As @mdboom said, we need to enable this option as optional because we already observed the performance impact from here: Consider applying flags for warnings about potential security issues #112301 (comment)
--disable-safety: We already spotted the build failure issue from the last several PRs, and we can fix them while we are developing the dev version, but once the CPython is released there is no way to people fix the flag(it's a catastrophe and you can not prevent them if the platform is out of our buildbot and Github Action systems), so this flag will only be used for such situation.

@corona10

@corona10
Something to note is that the benchmark #112301 (comment) 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.

@nohlson

@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.

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.

@nohlson

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

@encukou

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

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:

not add this kind of flags, or make them optional. (But of course there are other redistributors who might not care as much, so it's better to have good defaults with an opt-out.)
document the recommendations, so they could be considered (for Python builds or system-wide).
test that the build works with the recommended flags :)

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.

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.

pythongh-121996: Fix --disable-safety and --enable-slower-safety options

gh-121996: Fix --disable-safety and --enable-slower-safety options (g…

@encukou

@encukou cc @nohlson

With 046670c 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 #112301, which is the original GSoC issue!

Navigation Menu

Repository navigation

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

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

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

Description

Linked PRs

Activity

corona10 commented on Jul 19, 2024

mdboom commented on Jul 19, 2024

corona10 commented on Jul 19, 2024

corona10 commented on Jul 19, 2024

mdboom commented on Jul 19, 2024

corona10 commented on Jul 19, 2024

kulikjak commented on Jul 22, 2024

nohlson commented on Jul 22, 2024

corona10 commented on Jul 23, 2024

nohlson commented on Jul 23, 2024

corona10 commented on Jul 23, 2024

encukou commented on Jul 23, 2024

corona10 commented on Jul 23, 2024

nohlson commented on Jul 23, 2024

encukou commented on Jul 24, 2024

eli-schwartz commented on Jul 28, 2024

corona10 commented on Jul 30, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions