refactor attestation logic into cosign.Attestor
#1124
Conversation
Signed-off-by: Jake Sanders <jsand@google.com>
Signed-off-by: Jake Sanders <jsand@google.com>
Signed-off-by: Jake Sanders <jsand@google.com>
| }{ | ||
| { |
There was a problem hiding this comment.
Use {{ and }, { to reduce the bloat on these.
| } | ||
|
|
||
| // NewInTotoAttestor returns a `cosign.Attestor` which uploads the InToto attestation to Rekor | ||
| func NewInTotoAttestor(inner cosign.Attestor, rClient *client.Rekor) cosign.Attestor { |
There was a problem hiding this comment.
Why InToto here and not the interface name? Isn't the in-toto part implicit in the payload we are passed? I'd have thought DSSE if anything, but 🤷
|
|
||
| var _ cosign.Attestor = (*attestorWrapper)(nil) | ||
|
|
||
| func (ra *attestorWrapper) Attest(ctx context.Context, payload io.Reader) (oci.Signature, crypto.PublicKey, error) { |
There was a problem hiding this comment.
You put the fulcio signer/attestor in the same file, but split things here. Can we be consistent?
There was a problem hiding this comment.
? did you mean the payload signer/attestor?
yeah, I can
| Signatures: []dsse.Signature{ | ||
| { | ||
| Sig: base64.StdEncoding.EncodeToString(sig), | ||
| }, | ||
| }, |
There was a problem hiding this comment.
| Signatures: []dsse.Signature{ | |
| { | |
| Sig: base64.StdEncoding.EncodeToString(sig), | |
| }, | |
| }, | |
| Signatures: []dsse.Signature{{ | |
| Sig: base64.StdEncoding.EncodeToString(sig), | |
| }}, |
internal/pkg/cosign/attest.go
Outdated
| // Attestor creates attestations in the form of `oci.Signature`s | ||
| type Attestor interface { | ||
| // Attest creates an attestation, in the form of an `oci.Signature`, from the given payload. | ||
| // The signature and payload are stored as an envelope in `osi.Signature.Payload()` |
| var s icos.Signer | ||
| s = ipayload.NewSigner(sv, nil, nil) | ||
| s = ipayload.NewSigner(sv, nil, nil, nil, nil) |
There was a problem hiding this comment.
This has a code smell to it. We're going something wrong here 😅
There was a problem hiding this comment.
Potential solutions: 1) populate the struct directly 2) options 3) hoist the cert stuff into a higher-level wrapper
| // NewSigner returns a `cosign.Signer` which uses the given `signature.Signer` to sign requested payloads. | ||
| // The cert and chain, if provided, will be included in returned `oci.Signature`s. | ||
| func NewSigner(s signature.Signer, | ||
| sOpts []signature.SignOption, | ||
| pkOpts []signature.PublicKeyOption, | ||
| certPEM, chainPEM []byte) cosign.Signer { |
There was a problem hiding this comment.
This is an enormous signature. We are growing payload.NewSigner into the monstrosity that was previously in cmd. The whole point of the fulcio package is to have that encapsulate the variants that deal with chain-based signing, so I think it's time for fulcio.NewSigner to stop wrapping payload.NewSigner and take on some of this logic itself.
There was a problem hiding this comment.
I can split the cert and chain into their own layer. Alternatively, we could expose the struct directly.
I think it's time for fulcio.NewSigner to stop wrapping payload.NewSigner and take on some of this logic itself
I don't agree, since I don't think we want to limit the Fulcio path to keyless
Signed-off-by: Jake Sanders <jsand@google.com>
Signed-off-by: Jake Sanders <jsand@google.com>
dekkagaijin
force-pushed
the
attest-only
branch
from
b441219
to
6458dd7
Compare
Signed-off-by: Jake Sanders <jsand@google.com>
Signed-off-by: Jake Sanders <jsand@google.com>
Signed-off-by: Jake Sanders <jsand@google.com>
Signed-off-by: Jake Sanders <jsand@google.com>
|
This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR was closed because it has been stalled for 10 days with no activity. |
This is the
Attestationequivalent of the previousSignerrefactoring, plus nips and tucks here and thereTODO:
/pkglibrariesTickets: #844 #931