docs icon indicating copy to clipboard operation
docs copied to clipboard

Add python example for signature validation on secret scanning notifications

Open ckuethe opened this issue 2 years ago • 20 comments

Why:

The secret scanning workflow documentation (https://docs.github.com/en/enterprise-cloud@latest/developers/overview/secret-scanning-partner-program#implement-signature-verification-in-your-secret-alert-service) doesn't have example validation in python. I found at least two other people asking about how to do this in python with no answers.

Fixes #18778

What's being changed:

Added an example of signature validation using python-ecdsa (https://github.com/tlsfuzzer/python-ecdsa)

Check off the following:

  • [X] I have reviewed my changes in staging (look for "Automatically generated comment" and click Modified to view your latest changes).
  • [X] For content changes, I have completed the self-review checklist.

Writer impact (This section is for GitHub staff members only):

  • [ ] This pull request impacts the contribution experience
    • [ ] I have added the 'writer impact' label
    • [ ] I have added a description and/or a video demo of the changes below (e.g. a "before and after video")

ckuethe avatar Jun 22 '22 22:06 ckuethe

Thanks for opening this pull request! A GitHub docs team member should be by to give feedback soon. In the meantime, please check out the contributing guidelines.

welcome[bot] avatar Jun 22 '22 22:06 welcome[bot]

Automatically generated comment ℹ️

This comment is automatically generated and will be overwritten every time changes are committed to this branch.

The table contains an overview of files in the content directory that have been changed in this pull request. It's provided to make it easy to review your changes on the staging site. Please note that changes to the data directory will not show up in this table.


Content directory changes

You may find it useful to copy this table into the pull request summary. There you can edit it to share links to important articles or changes and to give a high-level overview of how the changes in your pull request support the overall goals of the pull request.

Source Preview Production What Changed
developers/overview/secret-scanning-partner-program.md fpt
ghec
fpt
ghec

fpt: Free, Pro, Team ghec: GitHub Enterprise Cloud ghes: GitHub Enterprise Server ghae: GitHub AE

github-actions[bot] avatar Jun 22 '22 22:06 github-actions[bot]

@ckuethe Thanks so much for opening a PR! I'll get this triaged for review ✨

janiceilene avatar Jun 28 '22 12:06 janiceilene

Thanks for opening a pull request! We've triaged this issue for technical review by a subject matter expert :eyes:

github-actions[bot] avatar Jul 21 '22 09:07 github-actions[bot]

This is a gentle bump for the docs team that this PR is waiting for technical review.

github-actions[bot] avatar Jul 28 '22 20:07 github-actions[bot]

This is a gentle bump for the docs team that this PR is waiting for technical review.

github-actions[bot] avatar Aug 05 '22 20:08 github-actions[bot]

Thx

JosephAvery avatar Aug 11 '22 03:08 JosephAvery

Hello @janiceilene, @felicitymay, and @cmwilson21 - do you know if the appropriate folks have had a chance to look at this issue?

ckuethe avatar Aug 29 '22 17:08 ckuethe

@ckuethe Thanks for checking in! The PR is on our board waiting for review. A writer will get eyes on it soon. 👀

Thank you for your patience as we work through our backlog 💛

cmwilson21 avatar Aug 29 '22 21:08 cmwilson21

Hi @ckuethe - thanks for your patience 🙏🏻

We've checked with the relevant team and we are happy to accept your contribution ✨

However, could you remove the caching of our public key, and provide one example only?

In the long term, we ideally want to link to an examples repository, which would be easier for everyone to maintain. In the mean time, we understand that examples are helpful to our users, but we don't want to have too many of these (maintainability issue).

Thanks in advance for your understanding. I am happy to review your PR once you've made these changes 😃

mchammer01 avatar Sep 22 '22 09:09 mchammer01

Hello @mchammer01 - Updated to address your feedback. Let me know if there are other changes you'd like.

ckuethe avatar Oct 04 '22 18:10 ckuethe

Thank you @ckuethe - I've asked one of the team for a final technical review. Thanks for your patience 🙂 🙏🏻

mchammer01 avatar Oct 06 '22 10:10 mchammer01

@marzvrover - would you mind reviewing this from a technical perspective? (As a member of the Docs team, I did an editorial review, and this LGTM). Thanks in advance for your help with this 🙇🏻 😃

mchammer01 avatar Oct 07 '22 14:10 mchammer01

Assuming you have python-ecdsa installed, you should be able to simply paste the example into a python 3 interpreter. I'm a fan of examples that have cut-paste-and-do-what-i-mean functionality :)

ckuethe avatar Oct 10 '22 19:10 ckuethe

Hello @mchammer01 @marzvrover - anything else you would like me to update in this example? I just updated the comment to accurately reflect that this PR now contains only a single implementation of validation using python-ecdsa

ckuethe avatar Nov 02 '22 19:11 ckuethe

@marzvrover - gentle bump on the request above 💜

mchammer01 avatar Nov 03 '22 20:11 mchammer01

@marzvrover Can I push back on that request? I'd like this example to be completely standalone.

For the purposes of this demonstration it's not important exactly how the key gets into memory, it's not important how the message to be validated to gets into memory. What is important is that we have a message, a public key, and a simple example of using that key to determine message validity.

Fetching the key from the API would require additional logic to handle signing key changes by checking the key ID, and

ckuethe avatar Nov 04 '22 18:11 ckuethe

@ckuethe it helps enforce the best practice of pulling in our active keys rather than relying on a local copy. This would keep the Python sample inline with our Go, Ruby, and JavaScript samples.

marzvrover avatar Nov 04 '22 19:11 marzvrover

@ckuethe - can you follow @marzvrover's recommendation above for consistency? I am happy to merge this once you remove the hardcoded public key.

mchammer01 avatar Nov 07 '22 07:11 mchammer01

@ckuethe Just checking in again on this one. Have you had a chance to consider the recommendation above?

cmwilson21 avatar Dec 05 '22 16:12 cmwilson21

👋 Just checking in. It looks like it's been a while since anything was updated on this one. I'm going to go ahead and close it now, but feel free to ping me or reopen if you'd like to resume work on it. 💛

cmwilson21 avatar Dec 21 '22 21:12 cmwilson21

Hi @ckuethe, I just wanted to let you know that we used your contributions and added you as coauthor for the commit https://github.com/github-technology-partners/signature-verification/commit/85ca4c59b2223ab2461181d94d021ff65f612e0f.

marzvrover avatar Mar 05 '24 22:03 marzvrover

Hey baby girl what are you doing today

On Tue, Mar 5, 2024 at 2:30 PM marz @.***> wrote:

Hi @ckuethe https://github.com/ckuethe, I just wanted to let you know that we used your contributions and added you as coauthor for the commit @.*** https://github.com/github-technology-partners/signature-verification/commit/85ca4c59b2223ab2461181d94d021ff65f612e0f .

— Reply to this email directly, view it on GitHub https://github.com/github/docs/pull/18776#issuecomment-1979746671, or unsubscribe https://github.com/notifications/unsubscribe-auth/AWGTV2O2CKZ65NNW5MXMRBTYWZBRFAVCNFSM5ZSAPQ62U5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOJXHE3TINRWG4YQ . You are receiving this because you are subscribed to this thread.Message ID: @.***>

freddyloks13 avatar Mar 06 '24 03:03 freddyloks13