8000 Introduce configure --disable-openssf-guide or --enable-openssf-guide · Issue #121996 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Closed
corona10 opened this issue Jul 19, 2024 · 17 comments
Closed

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

corona10 opened this issue Jul 19, 2024 · 17 comments
Assignees
@corona10
Labels
build The build process and cross-build type-feature A feature request or enhancement

Comments

@corona10
Copy link
Member
corona10 commented Jul 19, 2024 •
edited by bedevere-app bot
Loading

#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 corona10 self-assigned this Jul 19, 2024
@corona10
Copy link
Member Author

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

@corona10 corona10 mentioned this issue Jul 19, 2024
Open
@corona10 corona10 added type-feature A feature request or enhancement build The build process and cross-build labels Jul 19, 2024
corona10 added a commit to corona10/cpython that referenced this issue Jul 19, 2024
@corona10
pythongh-121996: Introduce configure --disable-openssf-guide
70bba41
@bedevere-app bedevere-app bot mentioned this issue Jul 19, 2024
Closed
corona10 added a commit to corona10/cpython that referenced this issue Jul 19, 2024
@corona10
pythongh-121996: Introduce configure --disable-openssf-guide
53c28e5
@mdboom
Copy link
Contributor
mdboom commented Jul 19, 2024

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
Copy link
Member Author
corona10 commented Jul 19, 2024 •
edited
Loading

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,

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

@corona10
Copy link
Member Author

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
Copy link
Contributor
mdboom commented Jul 19, 2024

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
Copy link
Member Author
corona10 commented Jul 19, 2024 •
edited
Loading

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?

corona10 added a commit to corona10/cpython that referenced this issue Jul 20, 2024
@corona10
pythongh-121996: Introduce --disable-safty and --enable-slower-safty
8455a0f
@bedevere-app bedevere-app bot mentioned this issue Jul 20, 2024
Merged
corona10 added a commit to corona10/cpython that referenced this issue Jul 22, 2024
@corona10
Merge remote-tracking branch 'upstream/main' into pythongh-121996-two…
764873a 
…-options
@kulikjak
Copy link
Contributor

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
Copy link
Contributor
nohlson commented Jul 22, 2024

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 8000 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 added a commit that referenced this issue Jul 23, 2024
@corona10
gh-121996: Introduce --disable-safety and --enable-slower-safety opt…
a9bb3c7 
…ions (#122054)

* gh-121996: Introduce --disable-safty and --enable-slower-safty

* Update GA

* fix

* Address code review

* Update CI
@corona10
Copy link
Member Author
corona10 commented Jul 23, 2024 •
edited
Loading

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

@nohlson
Copy link
Contributor
nohlson commented Jul 23, 2024

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

@corona10
Copy link
Member Author
corona10 commented Jul 23, 2024 •
edited
Loading

@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
Copy link
Member
encukou commented Jul 23, 2024

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
Copy link
Member Author

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

@nohlson
Copy link
Contributor
nohlson commented Jul 23, 2024

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
Copy link
Member
encukou commented Jul 24, 2024

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.

@eli-schwartz
Copy link
Contributor

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 added a commit to corona10/cpython that referenced this issue Jul 29, 2024
@corona10
pythongh-121996: Fix --disable-safety and --enable-slower-safety options
145a085
@bedevere-app bedevere-app bot mentioned this issue Jul 29, 2024
Merged
corona10 added a commit that referenced this issue Jul 29, 2024
@corona10
gh-121996: Fix --disable-safety and --enable-slower-safety options (g…
046670c 
…h-122414)
@corona10
Copy link
Member Author
corona10 commented Jul 30, 2024 •
edited
Loading

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

@sethmlarson sethmlarson mentioned this issue Aug 2, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@corona10 corona10

Labels
build The build process and cross-build type-feature A feature request or enhancement
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@mdboom @encukou @corona10 @nohlson @eli-schwartz @kulikjak
0