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

Add documentation for signing artifacts #2382

Merged
merged 6 commits into from
Aug 2, 2022

Conversation

gaiksaya
Copy link
Member

Signed-off-by: Sayali Gaikawad gaiksaya@amazon.com

Description

Adds documentation for signing artifacts

Issues Resolved

resolves #1502

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
@gaiksaya gaiksaya requested a review from a team as a code owner July 28, 2022 22:21
@gaiksaya gaiksaya requested a review from dblock July 28, 2022 22:21
@gaiksaya gaiksaya self-assigned this Jul 28, 2022
@gaiksaya gaiksaya added the documentation Improvements or additions to documentation label Jul 28, 2022
Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
@codecov-commenter
Copy link

codecov-commenter commented Jul 28, 2022

Codecov Report

Merging #2382 (ed31631) into main (447757d) will increase coverage by 0.32%.
The diff coverage is n/a.

@@             Coverage Diff              @@
##               main    #2382      +/-   ##
============================================
+ Coverage     94.62%   94.95%   +0.32%     
============================================
  Files           209      157      -52     
  Lines          4316     4140     -176     
  Branches         29       19      -10     
============================================
- Hits           4084     3931     -153     
+ Misses          226      209      -17     
+ Partials          6        0       -6     
Impacted Files Coverage Δ
...tifactsQualifier_OpenSearch_Dashboards_Jenkinsfile
tests/jenkins/jobs/CreateReleaseTag_Jenkinsfile
.../jenkins/jobs/PromoteArtifacts_actions_Jenkinsfile
tests/jenkins/jobs/BuildDockerImage_Jenkinsfile
tests/jenkins/jobs/Hello_Jenkinsfile
tests/jenkins/jobs/PublishNotification_Jenkinsfile
...sts/jenkins/jobs/DetectTestDockerAgent_Jenkinsfile
tests/jenkins/jobs/UploadToS3_Jenkinsfile
tests/jenkins/jobs/PromoteYumRepos_Jenkinsfile
tests/jenkins/jobs/BuildShManifest_Jenkinsfile
... and 42 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

README.md Outdated
Windows code signing uses EV (Extended Validated) code signing certificates.


**MacOS:**
Copy link
Member

Choose a reason for hiding this comment

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

Our signing workflow doesn't support signing for MacOS at this time but the signer client does.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think @prudhvigodithi is working on adding it. In that case I'll remove everything related to macos for now. We can add back when we add macos signing functionality

Copy link
Member Author

Choose a reason for hiding this comment

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

Done! cc: @bbarani

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
README.md Outdated
| RPM | SHA512 | RSA | 4096 |


**Signing RPM artifacts:**
Copy link
Member

Choose a reason for hiding this comment

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

Also for RPM artifacts signing, I think we only signing them via jenkins signArtifacts library, which is not related to our internal signer client. Should we also include it here?
Or we could separate this from PGP or windows signing (which uses signer client) cause in line 128 you say "We use the client for all types of signing"
WDYT? cc. @peterzhuamazon

Copy link
Member Author

Choose a reason for hiding this comment

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

Signing RPM artifacts is a different category I have added that it is yet to be integrated with signer client in the description.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Some nits.

README.md Outdated
@@ -125,12 +125,33 @@ See [src/test_workflow](./src/test_workflow) for more information.

#### Signing Artifacts

The signing step takes the manifest file created from the build step and signs all its component artifacts using a tool called `opensearch-signer-client` (in progress of being open-sourced). The input requires a path to the build manifest and is expected to be inside the artifacts directory with the same directories mentioned in the build step.
For all types of Signing within OpenSearch project we use `opensearch-signer-client` (in progress of being open-sourced) which is a wrapper around internal signing system and is only available for authenticated users. The input requires a path to the build manifest or directory containing all the artifacts or a single artifact. The tool currently supports following platforms for signing.
Copy link
Member

Choose a reason for hiding this comment

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

lowercase signing


Usage:
Copy link
Member

Choose a reason for hiding this comment

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

It says above ".. supports the following platforms for signing" but then has usage. Split the paragraph above into usage and platforms.

README.md Outdated
```bash
./sign.sh builds/opensearch/manifest.yml
```

**PGP**:
Copy link
Member

Choose a reason for hiding this comment

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

Make these ##### sections, so they appear in TOC

README.md Outdated
```bash
./sign.sh builds/opensearch/manifest.yml
```

**PGP**:

Anything can be signed using PGP signing eg: tarball, any type of file, etc. A .sig file will be returned containing the signature. OpenSearch and OpenSearch dashboards distributions, components such as data prepper, etc as well as maven artifacts are signed using PGP signing. You can see how to verify the signatures by visiting the opensearch website (https://opensearch.org/verify-signatures.html).
Copy link
Member

Choose a reason for hiding this comment

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

Link opensearch website - I would say See [this page](https://opensearch.org/verify-signatures.html) for how to verify signatures.

README.md Outdated

**Windows:**

Windows signing can be used to sign windows executables such as .msi, .msp, .msm, .cab, .dll, .exe, .appx, .appxbundle, .msix, .msixbundle, .sys, .vxd, .ps1, .psm1, and any PE file that is supported by Signtool.exe (https://docs.microsoft.com/en-us/dotnet/framework/tools/signtool-exe). Various windows artifacts such as SQL OBDC, opensearch-cli, etc are signed using this method.
Copy link
Member

Choose a reason for hiding this comment

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

Use markdown link for the docs.

README.md Outdated


**Signing RPM artifacts:**
We have an issue (https://github.com/opensearch-project/opensearch-build/issues/1547) open to add RPM signing functionality to the above signing system. However, temporarily we are signing the RPM artifacts via shell script which uses a macros template (https://github.com/opensearch-project/opensearch-build/blob/main/scripts/pkg/sign_templates/rpmmacros). More details in this commit (https://github.com/opensearch-project/opensearch-build/commit/950d55c1ed3f82e98120541fa40ff506338c1059). Currently we are only signing OpenSearch and OpenSearch dashboards RPM distribution using this method.
Copy link
Member

Choose a reason for hiding this comment

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

Generally avoid using "we".

You can say "RPMs are signed ..." and "See issue X ... for

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
@gaiksaya
Copy link
Member Author

gaiksaya commented Aug 1, 2022

@dblock Addressed all the comment. Please re-review. Thanks!

@gaiksaya gaiksaya requested a review from dblock August 2, 2022 00:41
README.md Outdated
| RPM | SHA512 | RSA | 4096 |


### Signing RPM artifacts:
Copy link
Member

Choose a reason for hiding this comment

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

Super nit: these titles don’t need : and Should be Capitalized

Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
Signed-off-by: Sayali Gaikawad <gaiksaya@amazon.com>
@gaiksaya gaiksaya merged commit 142f0c9 into opensearch-project:main Aug 2, 2022
@gaiksaya gaiksaya deleted the signing branch August 2, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Document signing (do we sign with SHA256?)
4 participants