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

sign binaries and images with sigstore cosign #207

Merged
merged 1 commit into from
Feb 14, 2024

Conversation

cpanato
Copy link
Contributor

@cpanato cpanato commented Sep 30, 2022

  • sign binaries and images with sigstore cosign
    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:

$ COSIGN_EXPERIMENTAL=1 cosign verify ctadeu/opentelemetry-collector-contrib:99.99.02

Verification for index.docker.io/ctadeu/opentelemetry-collector-contrib:99.99.02 --
The following checks were performed on each of these signatures:
  - The cosign claims were validated
  - Existence of the claims in the transparency log was verified offline
  - Any certificates were verified against the Fulcio roots.

[{"critical":{"identity":{"docker-reference":"index.docker.io/ctadeu/opentelemetry-collector-contrib"},"image":{"docker-manifest-digest":"sha256:767e608e8b6c81dbb156bcbfef574799146ec2c8d61939df22dc7843f8c40f95"},"type":"cosign container image signature"},"optional":{"1.3.6.1.4.1.57264.1.2":"push","1.3.6.1.4.1.57264.1.3":"5e0832f29d20202e82289a1899153923570ed741","1.3.6.1.4.1.57264.1.4":"Release","1.3.6.1.4.1.57264.1.5":"cpanato/opentelemetry-collector-releases","1.3.6.1.4.1.57264.1.6":"refs/tags/v99.99.02","Bundle":{"SignedEntryTimestamp":"MEYCIQCav9gQ9BApFC2i1eUzGDnmQ3luS5QxIQni5Xi5GIf4LQIhAPXS01OlF4XWrWgoD3f9VkTe8pbgijb7PCK/jD/riHzj","Payload":{"body":"eyJhcGlWZXJzaW9uIjoiMC4wLjEiLCJraW5kIjoiaGFzaGVkcmVrb3JkIiwic3BlYyI6eyJkYXRhIjp7Imhhc2giOnsiYWxnb3JpdGhtIjoic2hhMjU2IiwidmFsdWUiOiJhYjY3NTUxYjE4NWJmZGJkZWIyNGE0YTY2MTFjYjFkNDUwZGEwMjcwMDcwYjBiOWFiNzhhNzk1N2EyYjBmNzc4In19LCJzaWduYXR1cmUiOnsiY29udGVudCI6Ik1FVUNJQW1PVm1LbW84eTlWclNEVk9xQmU4ckllZjhURzc4M0lJSzhxeFFyYjlDSkFpRUFvcFJqNnFHblBwN1Fxa0NTMVd2RkMrbkgxaUJyZ1Z0L1RaSUVPSWJvYmdzPSIsInB1YmxpY0tleSI6eyJjb250ZW50IjoiTFMwdExTMUNSVWRKVGlCRFJWSlVTVVpKUTBGVVJTMHRMUzB0Q2sxSlNVUjVSRU5EUVRBeVowRjNTVUpCWjBsVldtbDRXazByY1ZGQlkzVlJUMHhtU0hGRGNtbDFNWFpJUmt3NGQwTm5XVWxMYjFwSmVtb3dSVUYzVFhjS1RucEZWazFDVFVkQk1WVkZRMmhOVFdNeWJHNWpNMUoyWTIxVmRWcEhWakpOVWpSM1NFRlpSRlpSVVVSRmVGWjZZVmRrZW1SSE9YbGFVekZ3WW01U2JBcGpiVEZzV2tkc2FHUkhWWGRJYUdOT1RXcEpkMDlVVFhkTlZGRjZUVlJCTlZkb1kwNU5ha2wzVDFSTmQwMVVVVEJOVkVFMVYycEJRVTFHYTNkRmQxbElDa3R2V2tsNmFqQkRRVkZaU1V0dldrbDZhakJFUVZGalJGRm5RVVU1ZVdzeVQzQkNOVTkzU25Oc2FXSk5hbXRzTVhGWVFsWldkV2hxVjJkcGIwdEtkaThLUm1OaVlYcGhkakZRWTJ0VFpXdENlVTFSVTNwc1JtaG5lVFpaTjJsUWRXeEtVa3RTVlhOTFMzWlJWR1JKYTFoYVJVdFBRMEZ0ZDNkblowcHZUVUUwUndwQk1WVmtSSGRGUWk5M1VVVkJkMGxJWjBSQlZFSm5UbFpJVTFWRlJFUkJTMEpuWjNKQ1owVkdRbEZqUkVGNlFXUkNaMDVXU0ZFMFJVWm5VVlZuWTBsRENtWmlTVnBsYUZkQ00wODVMMVUyVFU1a1VEWnRWRFZGZDBoM1dVUldVakJxUWtKbmQwWnZRVlV6T1ZCd2VqRlphMFZhWWpWeFRtcHdTMFpYYVhocE5Ga0tXa1E0ZDJaQldVUldVakJTUVZGSUwwSklTWGRqU1ZwMVlVaFNNR05JVFRaTWVUbHVZVmhTYjJSWFNYVlpNamwwVERKT2QxbFhOV2hrUnpoMllqTkNiQXBpYmxKc1lrZFdkRnBZVW5sbFV6RnFZako0YzFwWFRqQmlNMGwwWTIxV2MxcFhSbnBhV0UxMlRHMWtjR1JIYURGWmFUa3pZak5LY2xwdGVIWmtNMDEyQ21OdFZuTmFWMFo2V2xNMU5WbFhNWE5SU0Vwc1dtNU5kbVJIUm01amVUa3lUMVJyZFU5VWEzVk5SRWwzVDFGWlMwdDNXVUpDUVVkRWRucEJRa0ZSVVhJS1lVaFNNR05JVFRaTWVUa3dZakowYkdKcE5XaFpNMUp3WWpJMWVreHRaSEJrUjJneFdXNVdlbHBZU21waU1qVXdXbGMxTUV4dFRuWmlWRUZUUW1kdmNncENaMFZGUVZsUEwwMUJSVU5DUVZKM1pGaE9iMDFFV1VkRGFYTkhRVkZSUW1jM09IZEJVVTFGUzBSV2JFMUVaM3BOYlZsNVQxZFJlVTFFU1hkTmJWVTBDazFxU1RSUFYwVjRUMFJyTlUxVVZYcFBWRWw2VGxSamQxcFhVVE5PUkVWM1JsRlpTMHQzV1VKQ1FVZEVkbnBCUWtKQlVVaFZiVlp6V2xkR2VscFVRVElLUW1kdmNrSm5SVVZCV1U4dlRVRkZSa0pEYUdwalIwWjFXVmhTZGt3eU9YZGFWelV3V2xkNGJHSlhWakJqYm10MFdUSTVjMkpIVm1wa1J6bDVURmhLYkFwaVIxWm9ZekpXZWsxRFJVZERhWE5IUVZGUlFtYzNPSGRCVVZsRlJUTktiRnB1VFhaa1IwWnVZM2s1TWs5VWEzVlBWR3QxVFVSSmQyZFphMGREYVhOSENrRlJVVUl4Ym10RFFrRkpSV1YzVWpWQlNHTkJaRkZCU1ZsS1RIZExSa3d2WVVWWVVqQlhjMjVvU25oR1duaHBjMFpxTTBSUFRrcDBOWEozYVVKcVduWUtZMmRCUVVGWlQwOTZkVGhoUVVGQlJVRjNRa2ROUlZGRFNVTkJjVkJLUm1OQmNqZHFZMVJsTHpCTFRXVjJSVTlRU21ad04xbzNSRGt6WkdaS1N6VjZUd293YUROc1FXbEJjWFUxZDBOVWQxUldibE5pTm1aWk0wMUdNMDlMYkVVNFV6aFZla3hNVW1GdmFuQkRORkJDYlhoUVJFRkxRbWRuY1docmFrOVFVVkZFQ2tGM1RuQkJSRUp0UVdwRlFUZGhkMkZDWnk5cWJtMXVRbXRvWkhWU1drUkJLMHRXTjJKTFJqWTVhMnB3T0RSUloxbG9ja3d5YTNoT2JEZzVZWFpYUzI0S1EwSjBaelowTUhacWJYY3hRV3BGUVRoUE1VMTRaekJWUVdRd2NESkxVa1pUWkdOeVQzRklhVWRHTURCTlIyTmthRWd3ZVVwWFp5c3liRk5NUzNNeVNncHdPRk50VjJKb05ucERZWEJyVW0xMENpMHRMUzB0UlU1RUlFTkZVbFJKUmtsRFFWUkZMUzB0TFMwSyJ9fX19","integratedTime":1664548271,"logIndex":4290562,"logID":"c0d23d6ad406973f9559f3ba2d1ca01f84147d8ffc5b8445c224f98b9591801d"}},"Issuer":"https://token.actions.githubusercontent.com","Subject":"https://github.com/cpanato/opentelemetry-collector-releases/.github/workflows/release.yaml@refs/tags/v99.99.02"}}]

@cpanato cpanato requested review from a team and mx-psi September 30, 2022 14:57
@cpanato
Copy link
Contributor Author

cpanato commented Sep 30, 2022

cc @mattmoor @jpkrohling

@cpanato
Copy link
Contributor Author

cpanato commented Sep 30, 2022

i think we need to increase the timeout, but this is not related to this change

@mattmoor
Copy link

mattmoor commented Oct 1, 2022

Awesome, thanks @cpanato 🤩

@jpkrohling
Copy link
Member

I see the following in the build log:

  ⨯ release failed after 54m44s              error=sign: cosign failed: exit status 1: Using payload from: dist/otelcol_0.61.0-SNAPSHOT-1f6151b_darwin_arm64.tar.gz
Generating ephemeral keys...
Retrieving signed certificate...

        Note that there may be personally identifiable information associated with this signed artifact.
        This may include the email address associated with the account with which you authenticate.
        This information will be used for signing this artifact and will be stored in public transparency logs and cannot be removed later.
Non-interactive mode detected, using device flow.
Enter the verification code WWNP-JDBM in your browser at: https://oauth2.sigstore.dev/auth/device?user_code=WWNP-JDBM
Code will be valid for 300 seconds
Error: signing dist/otelcol_0.61.0-SNAPSHOT-1f6151b_darwin_arm64.tar.gz: getting key from Fulcio: retrieving cert: error obtaining token: expired_token
main.go:62: error during command execution: signing dist/otelcol_0.61.0-SNAPSHOT-1f6151b_darwin_arm64.tar.gz: getting key from Fulcio: retrieving cert: error obtaining token: expired_token

i think we need to increase the timeout, but this is not related to this change

Are you talking about a CI timeout? If so, we should do it before this change gets merged.

@cpanato
Copy link
Contributor Author

cpanato commented Oct 3, 2022

@jpkrohling i will split this change to get the workflows change before this one then we can properly test that

@cpanato
Copy link
Contributor Author

cpanato commented Oct 3, 2022

splitted: this should go first #211

@cpanato cpanato force-pushed the GH-203 branch 2 times, most recently from f04b08b to 4354c38 Compare October 5, 2022 15:22
@cpanato cpanato closed this Oct 5, 2022
@cpanato cpanato reopened this Oct 5, 2022
Copy link
Member

@jpkrohling jpkrohling left a 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

@cpanato
Copy link
Contributor Author

cpanato commented Oct 5, 2022

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?

I did a rehearsal in my fork, you can check in the PR description
what I usually do is cut a rc.1 tag to test it and mark that as a pre release

@jpkrohling
Copy link
Member

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.

@cpanato
Copy link
Contributor Author

cpanato commented Oct 5, 2022

I would recommend the to cut a rc tag

@tigrannajaryan
Copy link
Member

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?

@Aneurysm9
Copy link
Member

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 COSIGN_EXPERIMENTAL bit), but I think we'd need a better understanding of how key material is handed and what guarantees are provided by the underlying service.

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?

Copy link
Member

@tigrannajaryan tigrannajaryan left a 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.

@tigrannajaryan
Copy link
Member

Relevant previously open issue: open-telemetry/opentelemetry-specification#2560
Ideally we want a consistent, Otel-wide solution. It is very important that do our best to minimize the risk of a malicious actor signing binaries using Otel's signature, which means it is likely the TC needs to be tasked with guarding the signing keys.

@cpanato
Copy link
Contributor Author

cpanato commented Oct 7, 2022

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 COSIGN_EXPERIMENTAL bit), but I think we'd need a better understanding of how key material is handed and what guarantees are provided by the underlying service.

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?

Sigstore will be GA this month and the COSIGN_EXPERIMENTAL will be removed, but we are using this for more than a year in Kubernetes releases and other projects without issues.

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 id-token permission in the GitHub actions that will generate an OAuth token that will be used with fulcio to generate the certificates.
this approach is that we are using in kubernetes and in other projects as well, including all projects in sigstore.
off course, if the maintainers want to have a dedicated key we can generate that but will need to maintain that

@tigrannajaryan
Copy link
Member

@open-telemetry/technical-committee is anyone familiar with sigstore and can sign off on this change?

@jpkrohling
Copy link
Member

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.

@cpanato
Copy link
Contributor Author

cpanato commented Oct 13, 2022

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

@tigrannajaryan
Copy link
Member

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:

  • Who is capable of creating executables that are "endorsed" by OpenTelemetry signature in any way?

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.

@tigrannajaryan
Copy link
Member

@tigrannajaryan, my understanding is that this can now be implemented, as @cpanato ran sessions explaining what's behind code signing and how it works. The SIG Security is also comfortable supporting OTel maintainers in case of questions. Do you still have a blocking concern about this?

cc @open-telemetry/sig-security-maintainers

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.

@jpkrohling
Copy link
Member

Is the Collector signing process and the process to verify the signature by end users documented somewhere?

@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?

get the TC's sign off before going ahead

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.

@tigrannajaryan
Copy link
Member

get the TC's sign off before going ahead

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.

@cpanato
Copy link
Contributor Author

cpanato commented Nov 24, 2023

PR to add documentation in how to verify the signatures: open-telemetry/opentelemetry-collector#8990

@tigrannajaryan
Copy link
Member

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.

@jpkrohling
Copy link
Member

@tigrannajaryan, do you have good news for us?

@tigrannajaryan
Copy link
Member

@tigrannajaryan, do you have good news for us?

We have a queue of topics, will see if we can fit this one next week.

@tigrannajaryan
Copy link
Member

The TC has discussed this topic. I am posting on behalf of the TC:

  • The choice of code signing tooling and processes should be delegated to Security SIG.
  • We ask Security SIG to review the usage of sigstore for Collector and decide whether it should be used as the tool of choice for signing.
  • We ask Security SIG to consider that the tooling that is chosen for the Collector will be likely also used for signing other artifacts in the future (e.g. SDKs or other executables such as OpAMP Supervisor) so the tooling needs to be useful more broadly, not just for the Collector.
  • We ask Security SIG to report back their findings after they make a decision.
  • We ask Security SIG to review code signing already used by Otel Java and work with Java maintainers to decide whether there is a need to converge on sigstore in the future.

@open-telemetry/sig-security-maintainers

cc @open-telemetry/technical-committee

@jpkrohling
Copy link
Member

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:

  • @cpanato will update this PR so that it focuses only on the signing parts, removing the sbom
  • @jpkrohling will get in touch with the Java SIG and see if they see any concerns about moving to sigstore

@cartersocha
Copy link

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?

@jpkrohling
Copy link
Member

Tracking issue for using sigstore for our Java artifacts: open-telemetry/opentelemetry-java#6149

@cartersocha
Copy link

@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?

@tigrannajaryan
Copy link
Member

We ask Security SIG to report back their findings after they make a decision.

@cartersocha has this been done? If yes please post a link.

We ask Security SIG to review code signing already used by Otel Java and work with Java maintainers to decide whether there is a need to converge on sigstore in the future.

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?

@jpkrohling
Copy link
Member

jpkrohling commented Jan 18, 2024

@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.

@tigrannajaryan
Copy link
Member

@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.

@cpanato
Copy link
Contributor Author

cpanato commented Feb 13, 2024

is this good to merge? @jpkrohling

@jpkrohling
Copy link
Member

From my side, it's good to merge, but @tigrannajaryan still has a block on this PR.

@tigrannajaryan tigrannajaryan dismissed their stale review February 14, 2024 16:58

Nothing blocking anymore.

@jpkrohling jpkrohling merged commit 5671ef8 into open-telemetry:main Feb 14, 2024
14 checks passed
@cpanato cpanato deleted the GH-203 branch February 15, 2024 16:12
@cpanato
Copy link
Contributor Author

cpanato commented Feb 15, 2024

cool, thanks
i will check if need to update anything

@ThomsonTan
Copy link

With signore cosign enabled, is the signing blob, like .sigstore file, also published to the artifacts, so the build can be verfiied locally via cosign verify?

@jpkrohling
Copy link
Member

The signing instructions are being worked on by @cpanato, but if you have further suggestions, feel free to open an issue/PR!

mx-psi added a commit to open-telemetry/opentelemetry-collector that referenced this pull request May 29, 2024
**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]>
steves-canva pushed a commit to Canva/opentelemetry-collector that referenced this pull request Jun 14, 2024
…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]>
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.

Start to sign image releases with cosign
9 participants