Add note signers #87
Add note signers #87
Conversation
|
I've not tested this with tink or other KMSs yet. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
=======================================
Coverage ? 14.53%
=======================================
Files ? 21
Lines ? 1390
Branches ? 0
=======================================
Hits ? 202
Misses ? 1172
Partials ? 16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
pkg/note/note.go
Outdated
| const ( | ||
| algEd25519 = 1 | ||
| algUndef = 255 | ||
| rsaID = "RSA-PKCS#1v1.5" |
There was a problem hiding this comment.
Can follow up later, but we should document this custom ID in a readme on log entry verification.
There was a problem hiding this comment.
@haydentherapper what's the reasoning behind your suggestion to use PKCS#1 as the identifier, vs PKCS#8 or PKIX? It has implications on whether I use https://pkg.go.dev/crypto/x509#MarshalPKCS1PublicKey or https://pkg.go.dev/crypto/x509#MarshalPKIXPublicKey to convert the key object to bytes, I just noticed I'm using the wrong one for this identifier.
There was a problem hiding this comment.
To differentiate between the two modern RSA schemes, PSS and PKCS#1v1.5. You're right that there are two ways to encode public keys, so can we include both the scheme and public key encoding in the ID name?
There was a problem hiding this comment.
Including encoding also matches up well with how we’ve named the supported keys - https://github.com/sigstore/protobuf-specs/blob/e3f5847bde2c42348d1b99add0737ab3193f45d0/protos/sigstore_common.proto#L69
pkg/signer/signer.go
Outdated
| ) | ||
|
|
||
| // New returns a Signer for the given KMS provider, Tink, or a private key file on disk. | ||
| func New(ctx context.Context, signer, pass, tinkKEKURI, tinkKeysetPath string) (signature.Signer, error) { |
There was a problem hiding this comment.
I know this was copied in, but I (who wrote this) never loved passing a bunch of strings, prone to mixing them up. Do you want to change this to a config struct (or file a follow up issue)?
pkg/signer/signer.go
Outdated
| func(s string) bool { | ||
| return strings.HasPrefix(signer, s) | ||
| }): | ||
| return kms.Get(ctx, signer, crypto.SHA256) |
There was a problem hiding this comment.
(Also can be a follow up) Related to the crypto agility work we're doing now, we found in Fulcio that hardcoding this hash caused ECDSA P384 and P521 keys to not work correctly since those are typically associated with SHA-384 and SHA-512, especially for KMS providers.
Can we add an option to pass in the hash algorithm?
pkg/signer/file.go
Outdated
| return nil, fmt.Errorf("file: provide a valid signer, %s is not valid: %w", keyPath, err) | ||
| } | ||
|
|
||
| signer, err := signature.LoadSignerVerifier(opaqueKey, crypto.SHA256) |
There was a problem hiding this comment.
(Also can do in a follow up) Can we change from a hardcoded hash to the typical signature-hash pairing in https://github.com/sigstore/sigstore/pull/2014/files#diff-90f2b413b0fbeae56caac0481e57d7dff1bde02810bce967bd3f8c7d98670010R242?
Any RSA key size uses SHA-256, ECDSA P256/P384/P521 uses SHA-256/384/512 respectively, and Ed25519 uses SHA-512 but that's harcoded into the implementation.
There was a problem hiding this comment.
I can change this to signature.LoadDefaultSignerVerifier which was added ... today.
There was a problem hiding this comment.
SG, I'll cut a sigstore/sigstore release now
There was a problem hiding this comment.
pkg/signer/tink.go
Outdated
| "path/filepath" | ||
| "strings" | ||
|
|
||
| tinkUtils "github.com/sigstore/rekor/pkg/signer/tink" |
There was a problem hiding this comment.
Can we pull in the Tink utilities to remove the dependency on rekor?
There was a problem hiding this comment.
it's a bit strange since it looks like we already do -- isn't that pkg/signer/tink/ in this PR?
There was a problem hiding this comment.
Yes, just forgot to update the import.
pkg/signer/tink/tink.go now exists in fulcio, rekor and rekor-tiles, maybe we should just put it in sigstore/sigstore?
There was a problem hiding this comment.
Yep, that sounds good.
pkg/signer/tink/tink.go
Outdated
| // See the License for the specific language governing permissions and | ||
| // limitations under the License. | ||
|
|
||
| // Copied from https://github.com/sigstore/rekor/blob/c820fcaf3afdc91f0acf6824d55c1ac7df249df1/pkg/signer/tink_test.go |
There was a problem hiding this comment.
| // Copied from https://github.com/sigstore/rekor/blob/c820fcaf3afdc91f0acf6824d55c1ac7df249df1/pkg/signer/tink_test.go | |
| // Copied from https://github.com/sigstore/rekor/blob/c820fcaf3afdc91f0acf6824d55c1ac7df249df1/pkg/signer/tink/tink.go |
although it's not a direct copy, there are at least some lint changes
This change copies most of the sigstore/rekor/pkg/signer package to enable signing checkpoints with an on-disk key file or a KMS provider, and adds a note helper package to convert the Rekor signers to the sumdb/note signing format. Signed-off-by: Colleen Murphy <colleenmurphy@google.com>
This change copies most of the sigstore/rekor/pkg/signer package to enable signing checkpoints with an on-disk key file or a KMS provider, and adds a note helper package to convert the Rekor signers to the sumdb/note signing format.
Fixes #9
Relates to sigstore/rekor#2062
Summary
Release Note
Documentation