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

[OSSFScorecard] Create scorecard badge service #7687

Merged
merged 55 commits into from Apr 23, 2022

Conversation

rohankh532
Copy link
Contributor

Badge service to display scores from the OpenSSF Security Scorecard.

@shields-ci
Copy link

shields-ci commented Mar 6, 2022

Messages
📖 ✨ Thanks for your contribution to Shields, @rohankh532!

Generated by 🚫 dangerJS against 05d6e8f

@PyvesB
Copy link
Member

PyvesB commented Mar 6, 2022

Hello @rohankh532 ! 👋🏻

It would have been helpful to add some context about the Scorecard service, what these badges are trying to achieve, how they would be used, etc.

I've not looked at your implementation in details, but I did notice your score (valid) test which points to github/org/repo. The https://github.com/org/repo repository clearly isn't valid. The API seems to return sane-looking data regardless of what parameters are passed to it: https://api.securityscorecards.dev/projects/github/dqshsjqkhdqsjdhqsghd44/does-not-exist.json. I've also tried a bunch of repositories listed in https://github.com/ossf/scorecard/blob/main/cron/data/projects.csv, I've not yet managed to get any response that didn't have a score of 1, including for the Scorecard repository itself.

Is the API actually working? Is it documented anywhere?

@calebcartwright
Copy link
Member

Believe #5941 was a request along the same lines.

For what it's worth, I do think many of OSSF-based metrics would make for good badges, but also agree that we need to make sure the APIs we'd be depending on are in good order as P-Y noted

@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 March 6, 2022 17:50 Inactive
@rohankh532
Copy link
Contributor Author

rohankh532 commented Mar 6, 2022

Hello @rohankh532 ! 👋🏻

It would have been helpful to add some context about the Scorecard service, what these badges are trying to achieve, how they would be used, etc.

I've not looked at your implementation in details, but I did notice your score (valid) test which points to github/org/repo. The https://github.com/org/repo repository clearly isn't valid. The API seems to return sane-looking data regardless of what parameters are passed to it: https://api.securityscorecards.dev/projects/github/dqshsjqkhdqsjdhqsghd44/does-not-exist.json. I've also tried a bunch of repositories listed in https://github.com/ossf/scorecard/blob/main/cron/data/projects.csv, I've not yet managed to get any response that didn't have a score of 1, including for the Scorecard repository itself.

Is the API actually working? Is it documented anywhere?

Hey @PyvesB. Thanks for the comments.

Scorecards is a tool that assesses a repository with several security checks and provides a score for different categories. It is triggered via a GitHub workflow, which would run every time the project is updated. We are currently incorporating signatures on scorecard results so that their authenticity can be proved. This way, if someone claims their project has a certain score, it can actually be verified.

We are using badges to allow project owners to display their scorecard score so that other users can get a sense for how vulnerable the project is. The API currently just returns 1 for every repository as we are still working on a method for computing an aggregate score. So for now it is just a placeholder, but the format will still remain the same.

@PyvesB
Copy link
Member

PyvesB commented Mar 6, 2022

Thanks for sharing the extra details! If the API is still work in progress, I suggest we put this PR on hold for the time being, and circle back to it once things are in a more final state. How does that sound?

@rohankh532
Copy link
Contributor Author

Thanks for sharing the extra details! If the API is still work in progress, I suggest we put this PR on hold for the time being, and circle back to it once things are in a more final state. How does that sound?

Sounds good. I'll change this PR to a draft for now.

@rohankh532 rohankh532 marked this pull request as draft March 6, 2022 22:57
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 March 22, 2022 02:24 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 March 22, 2022 02:36 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 March 23, 2022 03:42 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 March 23, 2022 04:08 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 March 23, 2022 04:34 Inactive
@rohankh532
Copy link
Contributor Author

rohankh532 commented Mar 23, 2022

@rohankh532 rohankh532 marked this pull request as ready for review March 23, 2022 04:36
@calebcartwright
Copy link
Member

Great news! Could you please rebase and make sure the CI jobs are passing?

@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 March 25, 2022 03:48 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 April 6, 2022 02:47 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 April 6, 2022 02:53 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 April 6, 2022 03:51 Inactive
@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 April 6, 2022 04:05 Inactive
@rohankh532
Copy link
Contributor Author

@rohankh532 you should be able to rebase now to pull in the fix for the CI issue

Yup its fixed now, thanks!

@azeemshaikh38
Copy link

I'm not entirely sure I understood this part. Are you suggesting that our consumption of your API as part of our delivery of badges to your users is something that would be beneficial for you as part of testing/validation/feedback of your API?

We are looking to test/feedback both - API and badge. They are both new features rolling out to the public. The way this works is - when you "enable" Scorecard on a repository, you get an API which provides latest Scorecard result and the ability to add a Scorecard badge to your repo. So we'll be collecting feedback on user journey, badge design etc.

@calebcartwright
Copy link
Member

calebcartwright commented Apr 7, 2022

The way this works is - when you "enable" Scorecard on a repository, you get an API which provides latest Scorecard result and the ability to add a Scorecard badge to your repo. So we'll be collecting feedback on user journey, badge design etc.

I guess that's why this is returning an http 500 response

$ curl -i https://api.securityscorecards.dev/projects/github.com/badges/shields

As an early piece of feedback, I'll share that's semi-problematic for us, though not necessarily blocking. We typically try to map the http errors we receive when trying to fetch data from the upstream provider to something more contextual to our badge users. However, because this is returning a 500 response we have no way of distinguishing between whether there's a legitimate upstream service issue or the user hasn't enabled their repository or if they just had a typo in the badge route parameter or something else.

https://shields-staging-pr-7687.herokuapp.com/ossf-scorecard/github.com/badges/shields

@azeemshaikh38
Copy link

As an early piece of feedback, I'll share that's semi-problematic for us, though not necessarily blocking. We typically try to map the http errors we receive when trying to fetch data from the upstream provider to something more contextual to our badge users. However, because this is returning a 500 response we have no way of distinguishing between whether there's a legitimate upstream service issue or the user hasn't enabled their repository or if they just had a typo in the badge route parameter or something else.

Ah, that's a good point. I think returning 500 is a bug really. We should return a 404 in this case instead to indicate the resource was not found. Let me know what you think.

@rohankh532 could you update the API to ensure that we don't return a 500 error status here? If the project URL is valid and we don't have any data for it, we should return 404.

@shields-cd shields-cd temporarily deployed to shields-staging-pr-7687 April 7, 2022 19:56 Inactive
@rohankh532
Copy link
Contributor Author

As an early piece of feedback, I'll share that's semi-problematic for us, though not necessarily blocking. We typically try to map the http errors we receive when trying to fetch data from the upstream provider to something more contextual to our badge users. However, because this is returning a 500 response we have no way of distinguishing between whether there's a legitimate upstream service issue or the user hasn't enabled their repository or if they just had a typo in the badge route parameter or something else.

Ah, that's a good point. I think returning 500 is a bug really. We should return a 404 in this case instead to indicate the resource was not found. Let me know what you think.

@rohankh532 could you update the API to ensure that we don't return a 500 error status here? If the project URL is valid and we don't have any data for it, we should return 404.

Fixed this to differentiate between 404 and 500 errors.
Ex: https://shields-staging-pr-7687.herokuapp.com/ossf-scorecard/github.com/invalid/invalid

Copy link
Member

@calebcartwright calebcartwright 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 the dialog and collaboration! This looks to be in good order so going to merge 🚀

@repo-ranger repo-ranger bot merged commit 482be06 into badges:master Apr 23, 2022
@azeemshaikh38
Copy link

Thanks for the dialog and collaboration! This looks to be in good order so going to merge 🚀

Thanks a lot for all your help here @calebcartwright!

@rohankh532
Copy link
Contributor Author

Thanks for the dialog and collaboration! This looks to be in good order so going to merge 🚀

That's great--thank you so much for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
service-badge Accepted and actionable changes, features, and bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants