Fix signing the artifacts for .asc signatures #2850

gaiksaya · 2022-11-04T01:27:41Z

Description

Currently when we say we want .asc signature, we only change the extension of the signature file which it is still a binary. This PR fixes the issue where an actual enamored signature is generated based on the binary signature.
This PR also changes the default signature_type from .asc to .sig.

Follow up, need to make changes in jenkinsFiles that assumes default signature as .asc

Issues Resolved

resolves #368

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 <[email protected]>

prudhvigodithi · 2022-11-07T19:58:56Z

Hey @gaiksaya I'm ok with this PR, as the default output a .sig file, its good we convert to proper .asc file instead of just renaming the file to .asc, however should jenkinsfile changes (especially the maven publish) should go along with this PR?
Thank you
@peterzhuamazon @zelinh

prudhvigodithi · 2022-11-07T20:04:39Z

Also just add @gaiksaya once the function is executed __convert_to_asc that outputs a .asc file, can you confirm by running validation

gpg --verify <>.tar.gz.asc <>.tar.gz`

Aslo we still need to import global .asc key right using gpg --import GLOBAL.asc?

gaiksaya · 2022-11-07T21:24:06Z

Hey @gaiksaya I'm ok with this PR, as the default output a .sig file, its good we convert to proper .asc file instead of just renaming the file to .asc, however should jenkinsfile changes (especially the maven publish) should go along with this PR? Thank you @peterzhuamazon @zelinh

Let me add those files as well.

gaiksaya · 2022-11-07T21:24:53Z

Also just add @gaiksaya once the function is executed __convert_to_asc that outputs a .asc file, can you confirm by running validation
gpg --verify <>.tar.gz.asc <>.tar.gz`
Aslo we still need to import global .asc key right using gpg --import GLOBAL.asc?

Hey! the verification part is taken care by parent method here https://github.com/opensearch-project/opensearch-build/pull/2850/files#diff-b97cb5a1ac1ecef8c9c134f702092d1ba8818099a722cc5da21d73f8ded3e018R25

No need to import as global public key is the only thing required which is taken care by respective workflows.

Signed-off-by: Sayali Gaikawad <[email protected]>

gaiksaya · 2022-11-07T22:42:04Z

Let me add those files as well.

Done @prudhvigodithi

zelinh

Overall this looks good to me. Just one thing do we need to change the groovy lib signArtifacts to accommodate this change?

zelinh · 2022-11-07T23:58:09Z

src/sign_workflow/signer_pgp.py

@@ -33,7 +34,7 @@ def is_valid_file_type(self, file_name: str) -> bool:

    def sign(self, artifact: str, basepath: Path, signature_type: str) -> None:
        filename = os.path.join(basepath, artifact)
-        signature_file = filename + signature_type
+        signature_file = filename + ".sig"


Maybe we extract this .sig to a class variable called default_sig_type instead of hardcoded here?
Also let's rename this to sig_signature_file as this would always be .sig type in correspond to the asc_.. below.

we cannot make that as class variable as the filename is not static and it is calculated from method params.
Also, I have instead named the other file as asc_signature_file. It is just a variable name.

prudhvigodithi · 2022-11-08T00:50:05Z

Overall this looks good to me. Just one thing do we need to change the groovy lib signArtifacts to accommodate this change?

Hey @zelinh you mean this signArtifacts? adding @peterzhuamazon

gaiksaya · 2022-11-08T01:54:45Z

We do not need to change anything on jenkins library side as it takes all the args from jenkins and gives it to python code as it is
Example: https://github.com/opensearch-project/opensearch-build-libraries/blob/main/vars/signArtifacts.groovy#L132

peterzhuamazon · 2022-11-08T19:49:06Z

src/sign_workflow/sign_args.py

@@ -26,7 +26,7 @@ def __init__(self) -> None:
        parser.add_argument("target", type=Path, help="Path to local manifest file or artifact directory")
        parser.add_argument("-c", "--component", type=str, nargs='*', dest="components", help="Component or components to sign")
        parser.add_argument("--type", help="Artifact type")
-        parser.add_argument("--sigtype", choices=self.ACCEPTED_SIGNATURE_FILE_TYPES, help="Type of signature file.", default=".asc")
+        parser.add_argument("--sigtype", choices=self.ACCEPTED_SIGNATURE_FILE_TYPES, help="Type of signature file.", default=".sig")


So sigtype is still kinda useless here because you hard code it to .sig and convert to .asc later.

The acceptable types are sig and asc only https://github.com/opensearch-project/opensearch-build/pull/2850/files#diff-57b5e80836a33ceb7cbcdfb92fba22572d31fc761d817039e05119fa531c54f5R15
It will only convert to .asc if user asks for it. /the default is .sig but .asc is an option too
If we need more types in future for pgp we need to refactor the code. However, the upstream (opensearch-signer-client) only supports binary signatures so this hardcoding seems okay to me for now

See the if condition here: https://github.com/opensearch-project/opensearch-build/pull/2850/files#diff-b97cb5a1ac1ecef8c9c134f702092d1ba8818099a722cc5da21d73f8ded3e018R49

# By Peter Zhu (8) and others # Via GitHub * upstream/main: (24 commits) Updated manifests. (opensearch-project#2894) Update ref for 2.4.0 with tags (opensearch-project#2906) Fix status syntax for gradle check (opensearch-project#2907) Add the consolidated release notes for 2.4.0 release (opensearch-project#2887) Revert set output command (opensearch-project#2903) Fix status code for gradle check with retry (opensearch-project#2902) Upgrade actions and remove deprecated set ouput commands (opensearch-project#2901) Update opensearch-2.4.0.yml (opensearch-project#2896) Resolve Window IntegTest copy and startup issues (opensearch-project#2892) Update IM ref to include fix (opensearch-project#2890) Fix manifest workflow failure (opensearch-project#2889) Install twine on clients image (opensearch-project#2884) Freeze the ref with commit ID (opensearch-project#2882) Add latest url update toggle on distribution workflows (opensearch-project#2881) Update promoteArtifacts to support win/zip and add timer for gradle check windows (opensearch-project#2869) Fix signing the artifacts for .asc signatures (opensearch-project#2850) Update the ref with 2.4 branch for OSD. (opensearch-project#2855) Update the OSD manifest with commit ID (opensearch-project#2862) Add gradle check support for windows agent (opensearch-project#2860) Add default copy for opensearch-dashboards yml and docker entrypoint (opensearch-project#2861) ... # Conflicts: # scripts/components/OpenSearch-Dashboards/install.sh

gaiksaya added 3 commits November 2, 2022 18:36

Add asc support

411cf85

Signed-off-by: Sayali Gaikawad <[email protected]>

Fix tests

0d15482

Signed-off-by: Sayali Gaikawad <[email protected]>

Fix tests

860ee53

Signed-off-by: Sayali Gaikawad <[email protected]>

gaiksaya requested a review from a team as a code owner November 4, 2022 01:27

Add signature type to maven workflows

3089994

Signed-off-by: Sayali Gaikawad <[email protected]>

zelinh reviewed Nov 8, 2022

View reviewed changes

prudhvigodithi approved these changes Nov 8, 2022

View reviewed changes

peterzhuamazon reviewed Nov 8, 2022

View reviewed changes

gaiksaya merged commit 3176aff into opensearch-project:main Nov 9, 2022

gaiksaya deleted the add-asc-support branch November 9, 2022 00:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix signing the artifacts for .asc signatures #2850

Fix signing the artifacts for .asc signatures #2850

gaiksaya commented Nov 4, 2022

prudhvigodithi commented Nov 7, 2022

prudhvigodithi commented Nov 7, 2022

gaiksaya commented Nov 7, 2022

gaiksaya commented Nov 7, 2022 •

edited

Loading

gaiksaya commented Nov 7, 2022

zelinh left a comment

zelinh Nov 7, 2022

gaiksaya Nov 8, 2022

prudhvigodithi commented Nov 8, 2022

gaiksaya commented Nov 8, 2022

peterzhuamazon Nov 8, 2022

gaiksaya Nov 8, 2022 •

edited

Loading

gaiksaya Nov 8, 2022

Fix signing the artifacts for .asc signatures #2850

Fix signing the artifacts for .asc signatures #2850

Conversation

gaiksaya commented Nov 4, 2022

Description

Issues Resolved

prudhvigodithi commented Nov 7, 2022

prudhvigodithi commented Nov 7, 2022

gaiksaya commented Nov 7, 2022

gaiksaya commented Nov 7, 2022 • edited Loading

gaiksaya commented Nov 7, 2022

zelinh left a comment

Choose a reason for hiding this comment

zelinh Nov 7, 2022

Choose a reason for hiding this comment

gaiksaya Nov 8, 2022

Choose a reason for hiding this comment

prudhvigodithi commented Nov 8, 2022

gaiksaya commented Nov 8, 2022

peterzhuamazon Nov 8, 2022

Choose a reason for hiding this comment

gaiksaya Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

gaiksaya Nov 8, 2022

Choose a reason for hiding this comment

gaiksaya commented Nov 7, 2022 •

edited

Loading

gaiksaya Nov 8, 2022 •

edited

Loading