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

Cosign reorg #323

Merged
merged 3 commits into from
Sep 12, 2024
Merged

Cosign reorg #323

merged 3 commits into from
Sep 12, 2024

Conversation

hayleycd
Copy link
Contributor

Closes #322.

Summary

Reorganizing documentation ahead of including language client information

Release Note

NONE

Copy link

netlify bot commented Aug 28, 2024

Deploy Preview for docssigstore ready!

Name Link
🔨 Latest commit fd31d8f
🔍 Latest deploy log https://app.netlify.com/sites/docssigstore/deploys/66e3105a5cbd770008c092f0
😎 Deploy Preview https://deploy-preview-323--docssigstore.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@hayleycd hayleycd marked this pull request as draft August 28, 2024 15:08
…tion. Quickstart surfaced to top level. Signing, verifying, key management, and system configuration nested under cosign category.

Signed-off-by: hayleycd <[email protected]>
@hayleycd hayleycd marked this pull request as ready for review August 29, 2024 00:14
Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

The top level reorg looks good to me. Left some small comments inline.

@@ -10,7 +10,7 @@ This support is enabled through the [PIV protocol](https://csrc.nist.gov/project
and the [go-piv](https://github.com/go-piv/piv-go) library, which is not included in the standard release. Use `make cosign-pivkey-pkcs11key`, or `go build -tags=pivkey,pkcs11key ./cmd/cosign`, to build `cosign` with support for hardware tokens.

---
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the markdownlint bot is complaining about these borders, they're probably not needed if this is being changed from a note to a regular section.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @cmurphy! I am happy to fix this, but to give you some more context:

  1. There are ten errors that showed up in the markdown linter. This is one of them.
  2. However, I had previously fixed ten other errors. You see them in places like here.
  3. When I fixed those ten there were another ten found.

Basically I am not sure how many errors will eventually show up in chunks of ten, and I asked @haydentherapper whether it was worth chasing all of these down when I am not actually bringing in new errors and the check didn't technically fail. I opened issue #325 to investigate this.

Let me know what you think.

Copy link
Contributor

Choose a reason for hiding this comment

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

I only pointed this one out because it looks like it was introduced by this change - the note header went from a bolded NOTE to an actual level-2 header, and there is now an error about the heading being surrounded by a blank line, which would not have existed previously. I certainly agree that it's not worth chasing all the preexisting errors down, but it would be better not to introduce new ones.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmurphy Gotcha! That makes sense. It just got lost in the noise. I will fix.


Language specific clients (like [sigstore-python](https://github.com/sigstore/sigstore-python)) are other options for signing and verifying, but Cosign is a great, language agnostic place to start.

This quickstart will walk you through how to sign and verify a blob and a container. Although keyless signing is recommended, this quickstart will also show you how to sign using a generated key.
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this is out of scope for this reorg PR, but since we're recommending keyless signing maybe that's what the focus of this quickstart should be?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cmurphy So it does go through keyless signing first, and then signing with a generated key at the end. Would you like for me to remove the generated key section?

I think I could reword things so that it is clearer that the first part of the quickstart is for keyless signing and is preferred. However, a quickstart does not need to highlight every feature so I am comfortable removing the generated key section all together.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I was initially thinking to just remove the non-keyless signing part since I imagine the quickstart as a very brief, easy introduction, but I'd also be fine with just rewording this to de-emphasize signing with a generated key. If we go with the latter, either removing the second sentence entirely or changing it to something like

Suggested change
This quickstart will walk you through how to sign and verify a blob and a container. Although keyless signing is recommended, this quickstart will also show you how to sign using a generated key.
This quickstart will walk you through how to sign and verify a blob and a container. It will introduce keyless signing, which is the recommended method, as well as signing with a generated key, which is necessary for some use cases.

I think it's mainly starting the sentence with "Although" that's throwing me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that there is a lot going on in this quickstart, so I opted to remove the section. Let me know what you think.

Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Contributor

@haydentherapper haydentherapper left a comment

Choose a reason for hiding this comment

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

This looks great!

Copy link
Contributor

Choose a reason for hiding this comment

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

gitsign is a unique case, it's more like Cosign in that it's a tool built on top of an SDK to sign a specific format (commits, rather than containers/blobs like Cosign).

What do you think about an additional section for Sigstore tooling that is not Cosign? Something like:

If we were to ever refactor Cosign to be smaller in scope to only support container signing, it would then just move under Sigstore tooling, though I don't expect that to happen. But it would give us a category to expand over time as more use cases for Sigstore signing arise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@haydentherapper I like this idea. Would you like to see it incorporated in this pull request, or in a future one? GitSign is easy enough. It would take a little more to move model signing in just because it is currently not in the docs, but it isn't a big deal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to do this as a follow up. We can also create a placeholder and ask one of the maintainers from model signing to help fill in some details. I'll create an issue.


Cosign is a command line utility that is used to sign software artifacts and verify signatures using Sigstore.

Language specific clients (like [sigstore-python](https://github.com/sigstore/sigstore-python)) are other options for signing and verifying, but Cosign is a great, language agnostic place to start.
Copy link
Contributor

Choose a reason for hiding this comment

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

I might slightly rephrase to add that the language specific clients are SDKs to build tooling on top of, and though some may offer basic CLIs, Cosign is the recommended tool for most signing and verifying.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@haydentherapper Thanks! That makes sense. I have updated the wording a bit. This part will change again once the language clients are added to the docs. We could also cut this section for now and reintroduce something once there is something more to link to within the documentation (I want to link to the upcoming 'language client' section of the docs).

@haydentherapper haydentherapper merged commit 8467db8 into sigstore:main Sep 12, 2024
6 checks passed
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.

Reorganize documentation ahead of including more language client information
3 participants