-
Notifications
You must be signed in to change notification settings - Fork 163
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
sign binaries and images with sigstore cosign #207
Conversation
i think we need to increase the timeout, but this is not related to this change |
Awesome, thanks @cpanato 🤩 |
I see the following in the build log:
Are you talking about a CI timeout? If so, we should do it before this change gets merged. |
@jpkrohling i will split this change to get the workflows change before this one then we can properly test that |
splitted: this should go first #211 |
f04b08b
to
4354c38
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I wonder if there's a way to try that out so that we can confirm it works before the next release. Should we try a patch release?
cc @open-telemetry/collector-approvers
I did a rehearsal in my fork, you can check in the PR description |
Yes, that was great, but I wonder if the same will just work here. Confidence is high, but perhaps we want certainty before the next release. |
I would recommend the to cut a rc tag |
I am not familiar with sigstore. Does it use private keys to sign or something else? If it does where is the private key stored and who is responsible for generating they key and guarding it? |
I have similar questions. The PR looks good (modulo the It seems to use https://github.com/sigstore/fulcio which claims that it isn't production ready and will have changes to its root keys before it is. What happens at that point? How do they generate/use key material and how do we maintain expected properties of a signature using key material we do not control? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking this temporarily until we understand how the secrets are generated, stored and guarded.
Relevant previously open issue: open-telemetry/opentelemetry-specification#2560 |
Sigstore will be GA this month and the In this proposed approach we are not managing keys and we will generate ephemeral keys to sign that will have a short live while we run the signing process, we will use the |
@open-telemetry/technical-committee is anyone familiar with sigstore and can sign off on this change? |
I'm not part of the TC, but I have seen this in practice and believe that what we have here is the current state of the art for open-source projects. |
this is a similar approach that several other projects are using to sign binaries/containers :), I am happy to clarify other questions or jump in a call if needed |
It would be great to discuss this in the Collector SIG meeting next week. My primary concern is this:
I believe the answer should be that this must be limited to the TC and to automated flows which are explicitly signed off by the TC. |
Is the Collector signing process and the process to verify the signature by end users documented somewhere? I would like to see this documentation and get the TC's sign off before going ahead. |
@cpanato, would you please open a new PR for the collector's README in draft mode, stating that we are using cosign/sigstore to sign our artifacts, with explicit instructions on verifying the collector binaries?
Alright, but can we be offered a date on when this would happen? The TC is notoriously overloaded and both the SIG Collector and SIG Security would like to move forward with this. Both SIGs have been briefed about this already. |
I will add this to TC's agenda as soon as we have the signing/verifying process documented. We can aim for 2 week turnaround after that. |
PR to add documentation in how to verify the signatures: open-telemetry/opentelemetry-collector#8990 |
Thanks, I will add this to TC's agenda next week, but we skipped a meeting due to holidays, so may not be able to review all items next week. |
@tigrannajaryan, do you have good news for us? |
We have a queue of topics, will see if we can fit this one next week. |
The TC has discussed this topic. I am posting on behalf of the TC:
@open-telemetry/sig-security-maintainers cc @open-telemetry/technical-committee |
We had @cpanato today in the SIG Security call and are comfortable moving forward with cosign for signing binaries and container images. We understand how it works, how it would be implemented, and how other CNCF projects are using it already. We also had questions about language-specific tooling, and we were pointed to the Maven support for sigstore: https://github.com/sigstore/sigstore-maven-plugin . Looking around, there's also a Gradle plugin that can be used: https://github.com/sigstore/sigstore-java/tree/main/sigstore-gradle . Other languages seem to be present, such as JS, Rust, and Python. The next steps are:
|
Just getting back from the holidays and checking in. @jpkrohling did you get a chance to talk with the Java sig? Are we good to move forward here? |
Tracking issue for using sigstore for our Java artifacts: open-telemetry/opentelemetry-java#6149 |
@tigrannajaryan anything else needed from the sig-security side? Does Java need to implement the new process or give a formal assent before moving forward? |
@cartersocha has this been done? If yes please post a link.
I see a link posted by @jpkrohling but not sure if this work was completed. Have you been able to review what Java uses and work with maintainers? |
@tigrannajaryan, I talked to @jack-berg via Slack (https://cloud-native.slack.com/archives/C014L2KCTE3/p1702488450468229), who seemed open to exploring this idea. I lost track of that conversation over the holidays but created this issue here: open-telemetry/opentelemetry-java#6149 Given that there's an openness in considering this by Java, I'd like to move forward with this here while work progresses on Java as well (which might take longer, as my Gradle skills are rusty these days). If signing the collector with sigstore doesn't work, or if we come across a blocker in the Java work that would force us to reconsider sigstore, we can revert this change. |
@jpkrohling sounds good to me. I see no reason to block the work on the Collector as long as the overall plan remains as requested by the TC. |
is this good to merge? @jpkrohling |
From my side, it's good to merge, but @tigrannajaryan still has a block on this PR. |
cool, thanks |
With signore cosign enabled, is the signing blob, like |
The signing instructions are being worked on by @cpanato, but if you have further suggestions, feel free to open an issue/PR! |
**Description:** <Describe what has changed. - add documentation regarding verify images signatures related to open-telemetry/opentelemetry-collector-releases#207 Fixes: #9610 /assign @jpkrohling --------- Signed-off-by: cpanato <[email protected]> Co-authored-by: Juraci Paixão Kröhling <[email protected]> Co-authored-by: Pablo Baeyens <[email protected]>
…emetry#8990) **Description:** <Describe what has changed. - add documentation regarding verify images signatures related to open-telemetry/opentelemetry-collector-releases#207 Fixes: open-telemetry#9610 /assign @jpkrohling --------- Signed-off-by: cpanato <[email protected]> Co-authored-by: Juraci Paixão Kröhling <[email protected]> Co-authored-by: Pablo Baeyens <[email protected]>
also generate sboms for archives and packages
Fixes: #203
Rehearsal: https://github.com/cpanato/opentelemetry-collector-releases/releases/tag/v99.99.02
checking signed image: