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

Resolves #2685 #2853

Merged
merged 4 commits into from
Mar 31, 2023
Merged

Resolves #2685 #2853

merged 4 commits into from
Mar 31, 2023

Conversation

derek-burdick
Copy link
Contributor

@derek-burdick derek-burdick commented Mar 30, 2023

Summary

Resolves #2685
Resolves #1489

PKCS11 sessions should be opened read only. Read/Write sessions are only required when importing or deleting objects. pkcs11.CKF_SERIAL_SESSION is always required for backwards compatibility.

Release Note

PKCS11 sessions are now opened read only.

Documentation

pkcs11 ctx.OpenSession should only be read only and serial.

Signed-off-by: Derek Burdick <derek-burdick@users.noreply.github.com>
pkcs11 tools use env.VariablePKCS11ModulePath as default if not provided through flag module-path

Signed-off-by: Derek Burdick <derek-burdick@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Mar 30, 2023

Codecov Report

Merging #2853 (c337724) into main (062dd84) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2853      +/-   ##
==========================================
- Coverage   29.47%   29.47%   -0.01%     
==========================================
  Files         151      151              
  Lines        9678     9679       +1     
==========================================
  Hits         2853     2853              
- Misses       6386     6387       +1     
  Partials      439      439              
Impacted Files Coverage Δ
cmd/cosign/cli/options/pkcs11_tool.go 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@znewman01
Copy link
Contributor

Thank you for this fix! This is probably a pain to check in our existing pkcs11_test.go, so I won't make you do that. Have you tested this (either with a write-protected token or a standard one)?

CLI commands now respect COSIGN_PKCS11_MODULE_PATH environment variable.

Signed-off-by: Derek Burdick <derek-burdick@users.noreply.github.com>
@derek-burdick
Copy link
Contributor Author

derek-burdick commented Mar 31, 2023

Can you send me command to trigger pkcs11_test.go? I have some golang experience, but not much around testing.

I have softhsmv2 installed via brew on macos.

export COSIGN_PKCS11_MODULE_PATH=/opt/homebrew/Cellar/softhsm/2.6.1/lib/softhsm/libsofthsm2.so

./cosign pkcs11-tool list-tokens

Listing tokens of PKCS11 module '/opt/homebrew/Cellar/softhsm/2.6.1/lib/softhsm/libsofthsm2.so'
Token in slot 293923252
	Label: cosign
	Manufacturer: SoftHSM project
	Model: SoftHSM v2
	S/N: 11ea1eac9184e9b4

Token in slot 1480030964
	Label: My token 1
	Manufacturer: SoftHSM project
	Model: SoftHSM v2
	S/N: 1a91f6f2d8377af4

Token in slot 2
	Label:
	Manufacturer: SoftHSM project
	Model: SoftHSM v2
	S/N:

./cosign pkcs11-tool list-keys-uris --slot-id 1480030964
Enter PIN for PKCS11 token 'My token 1':
Listing URIs of keys in slot '1480030964' of PKCS11 module '/opt/homebrew/Cellar/softhsm/2.6.1/lib/softhsm/libsofthsm2.so'

I have also merged a partial fix for #1489, which allows the list-* commands to respect COSIGN_PKCS11_MODULE_PATH

Let me know if there is additional testing I can do.

…is not set

Signed-off-by: Derek Burdick <derek-burdick@users.noreply.github.com>
@derek-burdick
Copy link
Contributor Author

Ok, I have imported a key and tested signing using softhsm2, with a read only session.

./cosign pkcs11-tool list-keys-uris --slot-id 293923252
Enter PIN for PKCS11 token 'cosign':
Listing URIs of keys in slot '293923252' of PKCS11 module '/opt/homebrew/Cellar/softhsm/2.6.1/lib/softhsm/libsofthsm2.so'
Object 0
	Label: cosign
	ID: 0a
	URI: pkcs11:token=cosign;slot-id=293923252;id=%0a;object=cosign?module-path=/opt/homebrew/Cellar/softhsm/2.6.1/lib/softhsm/libsofthsm2.so&pin-value=cosign

./cosign sign --key 'pkcs11:token=cosign;slot-id=293923252;id=%0a;object=cosign?module-path=/opt/homebrew/Cellar/softhsm/2.6.1/lib/softhsm/libsofthsm2.so&pin-value=cosign' zzzmyorgzzz.azurecr.io/core/alpine:3.17.2
WARNING: no x509 certificate retrieved from the PKCS11 token
WARNING: Image reference zzzmyorgzzz.azurecr.io/core/alpine:3.17.2 uses a tag, not a digest, to identify the image to sign.
    This can lead you to sign a different image than the intended one. Please use a
    digest (example.com/ubuntu@sha256:abc123...) rather than tag
    (example.com/ubuntu:latest) for the input to cosign. The ability to refer to
    images by tag will be removed in a future release.

WARNING: "zzzmyorgzzz.azurecr.io/core/alpine" appears to be a private repository, please confirm uploading to the transparency log at "https://rekor.sigstore.dev"
Are you sure you would like to continue? [y/N] n
not uploading to transparency log
Pushing signature to: zzzmyorgzzz.azurecr.io/core/alpine

@znewman01 znewman01 enabled auto-merge (squash) March 31, 2023 16:47
@znewman01
Copy link
Contributor

Thank you so much! That testing is sufficient for now.

@znewman01 znewman01 merged commit a6d039a into sigstore:main Mar 31, 2023
@github-actions github-actions bot added this to the v1.14.0 milestone Mar 31, 2023
@derek-burdick derek-burdick deleted the pkcs11-readonly-2685 branch April 10, 2023 19:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants