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

Adds OpenSSF Scorecard documentation to Security.md. #14319

Closed
wants to merge 3 commits into from

Conversation

vpetersson
Copy link
Contributor

As per our conversation today @bagder - adding the much deserved OpenSSF badge :)

@vpetersson
Copy link
Contributor Author

Linting error appears to be unrelated to the change.

@bagder
Copy link
Member

bagder commented Jul 30, 2024

We removed the plethora of badges back in 7399fa5. I'm not sure we are ready to restart this.

I think just providing the info in words and a prominent link might be sufficient.

@danielgustafsson
Copy link
Member

We removed the plethora of badges back in 7399fa5. I'm not sure we are ready to restart this.

No thanks.

I think just providing the info in words and a prominent link might be sufficient.

👍

@vszakats
Copy link
Member

Well, we still explicitly suppress -Wsign-conversion warnings :(

@bagder
Copy link
Member

bagder commented Jul 30, 2024

Well, we still explicitly suppress -Wsign-conversion warnings :(

That's not a requirement for getting a gold for best-practices.

@vszakats
Copy link
Member

The OpenSSF "Best Practices Working Group" still lists it in their guide:
https://best.openssf.org/Compiler-Hardening-Guides/Compiler-Options-Hardening-Guide-for-C-and-C++.html

Maybe there is a separate guide about those best practices that can be safely ignored.

@bagder
Copy link
Member

bagder commented Jul 30, 2024

The badge is about the OpenSSF Best Practices Badge Program: https://www.bestpractices.dev/en/projects/63

@bagder
Copy link
Member

bagder commented Jul 30, 2024

I would be deeply upset if anyone would insist on -Wsign-conversion for real, because it is not intelligent.

@vszakats
Copy link
Member

vszakats commented Jul 30, 2024

Oh, thanks @bagder!

So maybe those compiler folks who decided to enable it via -Wconversion are also unintelligent, as well as the working group.

@bagder
Copy link
Member

bagder commented Jul 30, 2024

Warnings are helpful, but they are toggles for a reason. I'm sure this option works great in some environments and I don't understand what you're after here but you cannot deny that enforcing this option on a code base like curl is invasive to a degree that does not improve the source code.

That's why saying thou must use this option is a bad idea.

@vszakats
Copy link
Member

vszakats commented Jul 30, 2024

I'm not after anything, just spent a couple of full weeks trying to fix that in curl, only to throw away most of it (even some of the good parts unfortunately, in lib). To this day I'm confused as to why such a bad advice can make it into a prominent best-practice guide, and remain there. And wonder what makes one codebase fit for that advice and unfit for others. Such discussion would have been enlightening, to at least learn something out of this.

At the end nobody learned anything (except that you think I'm unintelligent, which makes it for a rather fantastic ending of a long day).

@bagder
Copy link
Member

bagder commented Jul 31, 2024

Viktor, I don't think you are unintelligent and I never said so.

Read what I wrote again; I said that it is wrong to insist on -Wsign-conversion for real, because it is not intelligent.

First, we need to separate persons from actions. I never said anyone person was stupid, but such an act would be silly. Then, I have not seen you insist on this so this does not even point to an action done by you.

Your PR trying to enable this option was a great effort. Unfortunately it could not be merged in the end. It's sad when we spend time and effort on something which does not end up getting used, but it's not entirely pointless because we learned a thing or two.

To this day I'm confused as to why such a bad advice can make it into a prominent best-practice guide

Me too, but as I was partially involved in discussing details in that guide with OpenSSF I know that it was made by just a random bunch of people. It was not done by carefully asking around old open source C projects or even long-time C programmers. Actually, simply given a question about this I would probably also easily have said that the option is a good idea. Before we tried hard to actually enforce it and saw what kind of workarounds we would have to do in order to satisfy it.

@vszakats
Copy link
Member

vszakats commented Jul 31, 2024

@bagder: Thanks for clarifying, it's somewhat of a relief. Any kind of feedback, especially positive is rather hard to come by in volunteer work, and there're those that are difficult to assess or easy to mis-read. Yesterday I was also laughted at by a vcpkg maintainer over here in curl, and bashed by another person; for work taking 2-shift days and sometimes weeks. It truly makes me second-guess my own intelligence.

Back to the warning, besides the controversial best-practice promoted by OpenSSF, what also keeps me wondering is that this warning comes by default with the otherwise useful and widely used -Wconversion option 1. Meaning, it's enabled by default in possibly quite a lot of projects. Unless they also manually suppress it like curl does (which is of course possible, I haven't made statistics, after realizing/remembering the default status too late into the task).

Thirdly, in curl there is a long-time warning suppression effort called warnless. I may be totally mistaken by it's purpose or goal, but judging by the name and what it does, it seems like something similar to what my dropped/rejected effort was trying to do. If so, it either could be extended onto other cases, or dropped, if it's just unwanted noise or obsolete. Either way, it'd be useful (IMO) to touch this briefly to learn what its goal is and how does it (or doesn't) relate to sign conversions.

Footnotes

  1. https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html#index-Wconversion

@bagder
Copy link
Member

bagder commented Jul 31, 2024

Meaning, it's enabled by default in possibly quite a lot of projects

Maybe it is. Maybe it isn't. I'm not aware of anyone ever have tried to investigate what warning options that are in effective use in widely portable C programs. Also: I'm not sure it would change our situation even if it turned out to be used a lot.

there is a long-time warning suppression effort called warnless. I may be totally mistaken by it's purpose or goal, but judging by the name and what it does, it seems like something similar to what my dropped/rejected effort was trying to do.

Yes I believe so. That effort was spear-headed by long-time absent Yang Tse.

If so, it either could be extended onto other cases, or dropped, if it's just unwanted noise or obsolete.

Like everything else in the source I'm sure it can be enhanced or change into something else, sure. I suspect dropping it completely would just introduce a lot of typecasts instead and I don't think that's an improvement.

it'd be useful (IMO) to touch this briefly to learn what its goal is and how does it (or doesn't) relate to sign conversions.

warnless.h provides functions and macros for type conversions (that should not cause warnings).

@vpetersson vpetersson changed the title Adds well deserved OpenSSF Gold Badge Adds OpenSSF Scorecard documentation to Security.md. Jul 31, 2024
@vpetersson
Copy link
Contributor Author

We removed the plethora of badges back in 7399fa5. I'm not sure we are ready to restart this.

I think just providing the info in words and a prominent link might be sufficient.

Fair fair! Didn't know about the badge removal.

I've added it in text to SECURITY.md instead which might be more appropriate.

SECURITY.md Outdated Show resolved Hide resolved
@bagder bagder closed this in 6fc66e1 Aug 17, 2024
@bagder
Copy link
Member

bagder commented Aug 17, 2024

Thanks!

cpswan added a commit to cpswan/curl that referenced this pull request Aug 21, 2024
that actually documents OpenSSF Best Practices. Scorecard [0] is a
different OpenSSF project, that incorporates Best Practices, but is
distinct in its objectives and how it achieves them.
This change clarifies the terminology, and also removes any
implication that Gold Best Practices is an award rather than a self
certification programme.
As curl was a leader in implementing Best Practices some folk may be
more familiar with the earlier Core Infrastructure Initiative (CII)
naming, so a reference to that has been added.

[0] https://scorecard.dev/

Signed-off-by: Chris Swan <478926+cpswan@users.noreply.github.com>
Ref: curl#14319
cpswan added a commit to cpswan/curl that referenced this pull request Aug 21, 2024
SECURITY.md has a recently added section titled OpenSSF Scorecard
that actually documents OpenSSF Best Practices. Scorecard [0] is a
different OpenSSF project, that incorporates Best Practices, but is
distinct in its objectives and how it achieves them.
This change clarifies the terminology, and also removes any
implication that Gold Best Practices is an award rather than a self
certification programme.
As curl was a leader in implementing Best Practices some folk may be
more familiar with the earlier Core Infrastructure Initiative (CII)
naming, so a reference to that has been added.

[0] https://scorecard.dev/

Signed-off-by: Chris Swan <478926+cpswan@users.noreply.github.com>
Ref: curl#14319
cpswan added a commit to cpswan/curl that referenced this pull request Aug 21, 2024
SECURITY.md has a recently added section titled OpenSSF Scorecard
that actually documents OpenSSF Best Practices. Scorecard [0] is a
different OpenSSF project, that incorporates Best Practices, but is
distinct in its objectives and how it achieves them.
This change clarifies the terminology, and also removes any
implication that Gold Best Practices is an award rather than a self
certification programme.
As curl was a leader in implementing Best Practices some folk may be
more familiar with the earlier Core Infrastructure Initiative (CII)
naming, so a reference to that has been added.

[0] https://scorecard.dev/

Signed-off-by: Chris Swan <478926+cpswan@users.noreply.github.com>
Ref: curl#14319
bagder pushed a commit that referenced this pull request Aug 22, 2024
SECURITY.md has a recently added section titled OpenSSF Scorecard
that actually documents OpenSSF Best Practices. Scorecard [0] is a
different OpenSSF project, that incorporates Best Practices, but is
distinct in its objectives and how it achieves them.
This change clarifies the terminology, and also removes any
implication that Gold Best Practices is an award rather than a self
certification programme.
As curl was a leader in implementing Best Practices some folk may be
more familiar with the earlier Core Infrastructure Initiative (CII)
naming, so a reference to that has been added.

[0] https://scorecard.dev/

Signed-off-by: Chris Swan <478926+cpswan@users.noreply.github.com>
Ref: #14319
Closes #14635
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants