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

📖 Mention 2FA relevance although not checked by Scorecard #2528

Merged
merged 2 commits into from
Dec 8, 2022
Merged

📖 Mention 2FA relevance although not checked by Scorecard #2528

merged 2 commits into from
Dec 8, 2022

Conversation

joycebrum
Copy link
Contributor

What kind of change does this PR introduce?

(Is it a bug fix, feature, docs update, something else?)

docs update

Which issue(s) this PR fixes

Fixes #2526

Special notes for your reviewer

Not sure about the title, let me know what you think

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

NONE

Signed-off-by: Joyce Brum <joycebrum@google.com>
@joycebrum joycebrum temporarily deployed to integration-test December 8, 2022 14:11 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Dec 8, 2022

Codecov Report

Merging #2528 (43c91c9) into main (fa0592f) will not change coverage.
The diff coverage is n/a.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2528   +/-   ##
=======================================
  Coverage   40.77%   40.77%           
=======================================
  Files         115      115           
  Lines        9604     9604           
=======================================
  Hits         3916     3916           
  Misses       5409     5409           
  Partials      279      279           

@github-actions
Copy link

github-actions bot commented Dec 8, 2022

Integration tests success for
[1013a3e]
(https://github.com/ossf/scorecard/actions/runs/3649049293)

Copy link
Contributor

@olivekl olivekl left a comment

Choose a reason for hiding this comment

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

Thanks for this useful addition! I left some comments.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Member

@naveensrinivasan naveensrinivasan left a comment

Choose a reason for hiding this comment

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

I'm undecided about this. Do we want to be the source of security recommendations? How important are they? What should we recommend, and what shouldn't we?

@olivekl
Copy link
Contributor

olivekl commented Dec 8, 2022

I'm undecided about this. Do we want to be the source of security recommendations? How important are they? What should we recommend, and what shouldn't we?

Good questions. If we're positioning Scorecard as a checklist of actions to take to improve the security of your project, then neglecting any mention of 2FA seems like a big gap. But where to draw the line? I would be fine with making this exception for 2FA only.

Definitely would like to hear other thoughts on this. @azeemshaikh38 @laurentsimon

@laurentsimon
Copy link
Contributor

laurentsimon commented Dec 8, 2022

I'm undecided about this. Do we want to be the source of security recommendations? How important are they? What should we recommend, and what shouldn't we?

I don't have a clear answer either. But I suppose when it's related to important GH settings, we could consider it. Maybe we could also have a paragraph explaining "what scorecard is not". In a "what scorecard is not" section, we could say things like: we don't check packages, we don't check for malicious code, etc. It is not currently obvious from the "What is scorecard" section that we only look at source repository (not packages); we look at best practices in your settings and code, etc. We could make this a little clearer up front, since some people get lost in the myriad of tools for supply-chain: example, we could explain with diagrams where Scorecard stands in the entire supply-chain story.

Good questions. If we're positioning Scorecard as a checklist of actions to take to improve the security of your project, then neglecting any mention of 2FA seems like a big gap. But where to draw the line? I would be fine with making this exception for 2FA only.

Definitely would like to hear other thoughts on this. @azeemshaikh38 @laurentsimon

I'd be fine having a section about "Limitations and complementary tools" with pointers for users. For 2FA, we could point to using AllStar in conjunction with Scorecard - something we're trying to improve.

Beyond that, we may need a dedicated website: where do I start to secure my supply-chain. I think OSSF / LF have been thinking about this, IIUC

Just some ideas...

@joycebrum
Copy link
Contributor Author

In my opinion, Scorecard already works with Github Best Practices recommendation and 2FA would certainly be a check if possible.

Considering the objective of improving the supply-chain security, every project that decides to adopt Scorecard would see this 2FA part and would consider implement it, which would increase the supply-chain security by extension.

And I agree with @olivekl that we can make some exceptions consider the relevance of 2FA. Besides, this "what scorecard is not" would be great to also deal with #2474.

@laurentsimon
Copy link
Contributor

In my opinion, Scorecard already works with Github Best Practices recommendation and 2FA would certainly be a check if possible.

Considering the objective of improving the supply-chain security, every project that decides to adopt Scorecard would see this 2FA part and would consider implement it, which would increase the supply-chain security by extension.

Good point. We did not consider it before because it requires an admin token from org maintainers. But we already have a check like WebHook which only runs with special tokens too.. so we could add a check for it. It just won't be available to many users.. but maybe with fine-grained tokens on their way it would work...

Signed-off-by: Joyce Brum <joycebrum@google.com>
@joycebrum joycebrum temporarily deployed to integration-test December 8, 2022 18:37 — with GitHub Actions Inactive
@github-actions
Copy link

github-actions bot commented Dec 8, 2022

Integration tests success for
[43c91c9]
(https://github.com/ossf/scorecard/actions/runs/3651109243)

@naveensrinivasan
Copy link
Member

I'm undecided about this. Do we want to be the source of security recommendations? How important are they? What should we recommend, and what shouldn't we?

Good questions. If we're positioning Scorecard as a checklist of actions to take to improve the security of your project, then neglecting any mention of 2FA seems like a big gap. But where to draw the line? I would be fine with making this exception for 2FA only.

Definitely would like to hear other thoughts on this. @azeemshaikh38 @laurentsimon

Sounds good to me! Thanks

@varunsh-coder
Copy link
Contributor

In my opinion, Scorecard already works with Github Best Practices recommendation and 2FA would certainly be a check if possible.
Considering the objective of improving the supply-chain security, every project that decides to adopt Scorecard would see this 2FA part and would consider implement it, which would increase the supply-chain security by extension.

Good point. We did not consider it before because it requires an admin token from org maintainers. But we already have a check like WebHook which only runs with special tokens too.. so we could add a check for it. It just won't be available to many users.. but maybe with fine-grained tokens on their way it would work...

Would be great if Scorecard had this 2FA check. I am not sure what GitHub APIs need to be called to confirm if all maintainers of a project have 2FA enabled. Does anyone know?

@olivekl
Copy link
Contributor

olivekl commented Dec 8, 2022

Would be great if Scorecard had this 2FA check. I am not sure what GitHub APIs need to be called to confirm if all maintainers of a project have 2FA enabled. Does anyone know?

Erring on the side of caution, would we want to surface that information (assuming it's possible)? Would visibility be limited only to project owners and not be made available the general public? It seems risky to make 2FA status generally known.

@varunsh-coder
Copy link
Contributor

Would be great if Scorecard had this 2FA check. I am not sure what GitHub APIs need to be called to confirm if all maintainers of a project have 2FA enabled. Does anyone know?

Erring on the side of caution, would we want to surface that information (assuming it's possible)? Would visibility be limited only to project owners and not be made available the general public? It seems risky to make 2FA status generally known.

Thats a good point. One way to think about it is to only surface info to the public when it is available without authentication/ authorization. If GitHub API needs a token to provide the info, e.g. in case of 2FA, then Scorecard should not make the info public. Just make it available as output of the CLI.

@laurentsimon
Copy link
Contributor

Would be great if Scorecard had this 2FA check. I am not sure what GitHub APIs need to be called to confirm if all maintainers of a project have 2FA enabled. Does anyone know?

Erring on the side of caution, would we want to surface that information (assuming it's possible)? Would visibility be limited only to project owners and not be made available the general public? It seems risky to make 2FA status generally known.

Thats a good point. One way to think about it is to only surface info to the public when it is available without authentication/ authorization. If GitHub API needs a token to provide the info, e.g. in case of 2FA, then Scorecard should not make the info public. Just make it available as output of the CLI.

+1, this would only be used in the CLI.

Copy link
Contributor

@laurentsimon laurentsimon left a comment

Choose a reason for hiding this comment

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

OK. Let's merge this in given that 2FA is important. It'd be good to be able to link to another doc / website / project for 2FA and the difference between SMS vs OTP-based, etc.

@laurentsimon laurentsimon merged commit e8b0223 into ossf:main Dec 8, 2022
raghavkaul pushed a commit to raghavkaul/scorecard that referenced this pull request Feb 9, 2023
* feat: add information about two factor authentication

Signed-off-by: Joyce Brum <joycebrum@google.com>

* fix: descriptiton of 2FA to be more complete

Signed-off-by: Joyce Brum <joycebrum@google.com>

Signed-off-by: Joyce Brum <joycebrum@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mention 2FA relevance although not checked by Scorecard
5 participants