Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

build: Set security flags for release builds #6087

Merged
merged 3 commits into from
Jul 8, 2024

Conversation

Garfield96
Copy link
Contributor

@Garfield96 Garfield96 commented Sep 23, 2022

Signed-off-by: Christian Menges christian.menges@outlook.com

Set security flags for release builds using gcc or clang:

  • -Wl,-z,relro,-z,now Set Global Offset Table (GOT) to read-only. In theory, this increases startup times, but I couldn't observe a performance degradation.
  • -fstack-protector Protect against stack manipulation.
  • -D_FORTIFY_SOURCE=1 Replace certain functions with more secure alternatives. With level 1, most of the added security checks are optimized away.

Fixes #7315


Enter [N/A] in the box, if an item is not applicable to your change.

Testing
Before we can approve your change; please submit the following in a comment:

  • [N/A] Example configuration file for the change
  • [N/A] Debug log output from testing the change
  • [N/A] Attached Valgrind output that shows no leaks or memory corruption was found

If this is a change to packaging of containers or native binaries then please confirm it works for all targets.

Documentation

  • [N/A] Documentation required for this feature

Backporting

  • [N/A] Backport to latest stable release.

Fluent Bit is licensed under Apache 2.0, by submitting this pull request I understand that this code will be released under the terms of that license.

@patrick-stephens
Copy link
Contributor

We probably need a build of all targets to confirm this works ok on everything.

@Garfield96
Copy link
Contributor Author

@patrick-stephens I executed the package tests in my fork. Unfortunately, the timeout was reached before the tests completed. However, all executed tests were successful.
Build log:
fluent-bit-build-logs.txt

@Garfield96
Copy link
Contributor Author

@niedbalski Could you please review this PR? Thanks

@patrick-stephens
Copy link
Contributor

@Garfield96 thanks, we're both on PTO at the moment but I'll catch up with @niedbalski once he's back.

@Garfield96
Copy link
Contributor Author

Hi @patrick-stephens and @niedbalski,
this PR is already open for some time. Shall I change something or do you require additional information to review the PR?

@patrick-stephens
Copy link
Contributor

@Garfield96 I appreciate your time - unfortunately @niedbalski is off for an extended period.

@Garfield96
Copy link
Contributor Author

Hi @niedbalski,
I saw you are back. Could you please have a look at this PR? Thanks

@patrick-stephens patrick-stephens added the ok-package-test Run PR packaging tests label Apr 24, 2023
@patrick-stephens patrick-stephens temporarily deployed to unstable April 24, 2023 09:43 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:43 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr-package-test April 24, 2023 09:43 — with GitHub Actions Inactive
@patrick-stephens
Copy link
Contributor

@Garfield96 can you update the merge commit title (and ideally rebase)?

@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr-package-test April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
@patrick-stephens patrick-stephens temporarily deployed to pr April 24, 2023 09:44 — with GitHub Actions Inactive
Signed-off-by: Christian Menges <christian.menges@outlook.com>
Signed-off-by: Christian Menges <christian.menges@outlook.com>
@Garfield96
Copy link
Contributor Author

@patrick-stephens I rebased the PR. The test failures are unrelated to the change and are also present in several other recently opened PRs.

Signed-off-by: Christian Menges <christian.menges@outlook.com>
Copy link
Contributor

@patrick-stephens patrick-stephens left a comment

Choose a reason for hiding this comment

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

@leonardo-albertovich I think we were both ok with this?

@Garfield96
Copy link
Contributor Author

@leonardo-albertovich Can this PR get merged?

@rossigee
Copy link

rossigee commented Oct 2, 2023

Is this the cause of the following in my dmesg output?

[  365.250531] process 'opt/fluent-bit/bin/fluent-bit' started with executable stack

@patrick-stephens
Copy link
Contributor

Is this the cause of the following in my dmesg output?

[  365.250531] process 'opt/fluent-bit/bin/fluent-bit' started with executable stack

@rossigee I'm not sure what you're asking there - this PR is not merged so any changes in it are not in official releases.
As to why you're seeing that message, this really should be captured on an issue (if it is a problem) along with full details on the version of FB, OS, config, etc.?

@rossigee
Copy link

rossigee commented Oct 2, 2023

@patrick-stephens - sorry I wasn't clear. I saw this message while reviewing the logs on one of my hosts, and was alarmed. I wasn't sure if my 'fluent-bit' binary (official release build v2.1.9, deployed as a K8S DS) had somehow been tampered with, but on further investigation I came across this PR which addresses certain 'official release build' compile-time issues, one of which appears to be related to preventing the executable stack problem.

I wasn't able to find anyone else reporting the same issue by Googling the error message, and
I was curious as to whether or not this PR would solve my problem (once it's merged and released). Apologies for the noise.

@Garfield96
Copy link
Contributor Author

@rossigee I assume that this PR will remove the warning from your logs.

@Garfield96
Copy link
Contributor Author

@patrick-stephens I'm sorry for asking again, but can this PR be merged soon? I think a lot of users would benefit from it and as mentioned by @rossigee and #7315, some platforms even issue warnings for the current fluent-bit binary.

@patrick-stephens
Copy link
Contributor

@Garfield96 as I understand it I think we're happy with this - I'm not the best person to review the compilation flags themselves however @leonardo-albertovich indicated he was so I'll ping @edsiper to see if we can get it merged.

@Garfield96
Copy link
Contributor Author

Hi @leonardo-albertovich and @edsiper,
I'd highly appreciate if you could review and, ideally, merge this PR. Thanks

Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

Copy link
Contributor

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label May 14, 2024
@edsiper edsiper added this to the Fluent Bit v3.1.0 milestone May 14, 2024
@edsiper
Copy link
Member

edsiper commented May 14, 2024

added to Milestone v3.1.0

@edsiper edsiper merged commit 32cead0 into fluent:master Jul 8, 2024
18 of 21 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-package-test Run PR packaging tests packaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add missing .note.GNU-stack sections
5 participants