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/Verify with Non-PGP Key Material #1120

Merged
merged 4 commits into from
Nov 29, 2022
Merged

Conversation

tri-adam
Copy link
Member

@tri-adam tri-adam commented Nov 4, 2022

Description of the Pull Request (PR):

Add --key option to singularity sign and singularity verify, which expect PEM-encoded key material that will be used to sign/verify the image. Add end-to-end tests to cover --key usage.

Extend internal/app/singularity package to support signing with a signature.Signer and verifying with a signature.Verifier.

Centralize test keys and images that are used by multiple packages to test/.

This fixes or addresses the following GitHub issues:

@tri-adam tri-adam self-assigned this Nov 4, 2022
@tri-adam tri-adam force-pushed the sign-verify-keys branch 5 times, most recently from 65cd03b to 158763d Compare November 25, 2022 16:22
Extend singularity package to support signing with a signature.Signer
and verifying with a signature.Verifier.
Add basic sign/verify using PEM-encoded SSL keys. Add E2E tests for the
same.
@tri-adam tri-adam force-pushed the sign-verify-keys branch 2 times, most recently from a598e77 to 8c861ce Compare November 28, 2022 21:08
@tri-adam tri-adam marked this pull request as ready for review November 28, 2022 21:49
Copy link
Member

@dtrudg dtrudg left a comment

Choose a reason for hiding this comment

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

This is looking good to me. I have a couple of questions...

  1. Should the '--key' flag have an associated env var, so that it's straightforward to set a default key for all verification operations etc.?

  2. If the top level test directory is only going to contain test data, and not code for tests, would it be reasonable to use testdata instead? As I understand it, the go toolchain would then ignore any .go files in there.... which would make it a good place to 'hide' deprecated plugin examples that we may still need to test compile for a time, but don't want to keepin in examples, have appearing in go doc etc.

Directory and file names that begin with "." or "_" are ignored by the go tool, as are directories named "testdata".

👍 on centralising the test data. I think there might be some more that is essentially duplicated between unit and e2e tests, that should be found and dug out at some point when we have some time.

DefaultValue: "",
Name: "key",
Usage: "path to the private key file",
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to associate this with an env var also?

Potentially a user could set an env var to have the key used always, without needing to specify the flag, in an environment that requires that?

DefaultValue: "",
Name: "key",
Usage: "path to the public key file",
}
Copy link
Member

Choose a reason for hiding this comment

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

Ditto the signing - do we want to expose with an env var also?

@tri-adam
Copy link
Member Author

  1. Should the '--key' flag have an associated env var, so that it's straightforward to set a default key for all verification operations etc.?

Good idea, although this may introduce some subtlety for signing, since the type of signature being generated would then depend on the environment. Perhaps it would be worth adding some visual feedback?

Environment variable not set:

$ singularity sign image.sif 
Signing image with PGP key material
Signature created and applied to image.sif

Environment variable set:

$ export SIGN_KEY=private.pem
$ singularity sign image.sif 
Signing image with key material from 'private.pem'
Signature created and applied to image.sif
  1. If the top level test directory is only going to contain test data, and not code for tests, would it be reasonable to use testdata instead? As I understand it, the go toolchain would then ignore any .go files in there.... which would make it a good place to 'hide' deprecated plugin examples that we may still need to test compile for a time, but don't want to keepin in examples, have appearing in go doc etc.

Directory and file names that begin with "." or "_" are ignored by the go tool, as are directories named "testdata".

I wonder if it might be worth following the guidelines here, as I think the current structure was based on those recommendations to some extent? That wouldn't preclude us from having a test/testdata directory for the reasons you've articulated, while also supporting test code (e2e->test/e2e, for example).

@tri-adam
Copy link
Member Author

Also going to raise a situation that just occurred to me... in the event that an image is signed both with PGP and non-PGP key material:

$ singularity sign image.sif
$ singularity sign --key private.pem image.sif

This can't be successfully verified, since using --key disables PGP key material as a source:

$ singularity verify image.sif 
Verifying image: image.sif
FATAL:   Failed to verify container: integrity: key material not provided for DSSE envelope signature
$ singularity verify --key public.pem image.sif 
Verifying image: image.sif
FATAL:   Failed to verify container: integrity: key material not provided for PGP clear-sign signature

Perhaps verify should instead consider --key to be additive to PGP key material sources?

@dtrudg
Copy link
Member

dtrudg commented Nov 29, 2022

I wonder if it might be worth following the guidelines here, as I think the current structure was based on those recommendations to some extent? That wouldn't preclude us from having a test/testdata directory for the reasons you've articulated, while also supporting test code (e2e->test/e2e, for example).

Okay, agreed. I don't really think that pattern is one we should necessarily be wedded to, for the reasons articulated in golang-standards/project-layout#117 - but we are sorta following it in places :-)

Perhaps verify should instead consider --key to be additive to PGP key material sources?

That would make sense to me... but, tbh, I'd prefer to throw it out to at least some minimal discussion if we can. The reason being that there is also a bit of a question on and / or behavior on PGP signatures alone... before we possibly combine them with DSSE.

This on the apptainer/sif fork also comes to mind once we think about signature combinations - apptainer/apptainer#462

R.E. the env var stuff... yup, the visual feedback makes perfect sense.

These latter 2 points seem like they can be captured as separate issues, and addressed. So putting a ✅ on here now.

@tri-adam
Copy link
Member Author

These latter 2 points seem like they can be captured as separate issues, and addressed. So putting a ✅ on here now.

Thanks, I'll merge this. I've opened #1148 and #1150 to open up discussion on the relevant points.

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.

2 participants