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

replace context with Options in Signer, Verifier, and PublicKeyProvider interfaces #70

Closed
wants to merge 1 commit into from

Conversation

dekkagaijin
Copy link
Member

This allows signature.Signers to provide crypto.Signers to common libraries.

Signed-off-by: Jake Sanders jsand@google.com

@dekkagaijin
Copy link
Member Author

/cc @bobcallaway @asraa @dlorenc PTAL, here's my 2nd counter-proposal to #69

Sign(ctx context.Context, rawMessage io.Reader) (signature []byte, err error)

// Returns a standard Golang `crypto.Signer` which uses the given context (if applicable) in addition to default SignerOpts and a MessagePreprocessor.
CryptoSigner(ctx context.Context) (crypto.Signer, crypto.SignerOpts, MessagePreprocessor, error)
Copy link
Member Author

Choose a reason for hiding this comment

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

@bobcallaway wdyt of this kind of interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

/cc @asraa

Copy link
Member

Choose a reason for hiding this comment

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

I think this will work; if all the caller has is the digest (instead of the entire message), its a bit awkward to have to get the CryptoSigner() to be able to sign; also we wouldn't be able to verify in that case but im not sure how frequent of a situation that is.

I guess I'm still slightly in favor of the functional options approach (listed below since Jake and I iterated on it over Slack) - but I would be happy either way.

type Signer interface {
        SignMessage(message []byte, opts ...SignerOption) ([]byte, error)
        crypto.Signer
        // Public() crypto.PublicKey
        // Sign(rand io.Reader, digest []byte, opts crypto.SignerOpts) ([]byte, error)
        PublicWithContext(context.Context) (crypto.PublicKey, error)
}

SignerOptions: WithContext(), WithDigest(), WithRand()
// WithContext(context.Context)    -> specifies the context for communication with external service
// WithRand(io.Reader)             -> specifies a source of entropy used during the signing process (defaults to crypto/rand.Reader if option is not specified)
// WithDigest([]byte, crypto.Hash) -> specifies the digest value of the message, along with the hash function used to compute it

type Verifier interface {
        VerifySignature(signature []byte, message []byte, opts ...VerifierOption) error
}

VerifierOptions: WithContext(), WithDigest()
// WithContext(context.Context)    -> specifies the context for communication with external service
// WithDigest([]byte, crypto.Hash) -> specifies the digest value of the message, along with the hash function used to compute it

// context could be set at the time of instantiation, or passed in via crypto.SignerOpts like this:
type GCPContextSignerOpts struct {
        Hash    crypto.Hash
        Context context.Context
}

Copy link
Member Author

Choose a reason for hiding this comment

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

if all the caller has is the digest (instead of the entire message), its a bit awkward to have to get the CryptoSigner() to be able to sign; also we wouldn't be able to verify in that case but im not sure how frequent of a situation that is.

IMHO we want to abstract those implementation details away for the high-level interfaces. There'll definitely be use-cases where those details are relevant complexity, but I don't think we should make the complexity mandatory in order to support them

Copy link
Member

Choose a reason for hiding this comment

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

I tend to agree with Bob's approach here, as this allows a simple interface with no mandatory args to pass (with no need to nil out values), so it could be no args to just sign a message, or if required a specific context (as one example), is required, we have the flexibility to pass in exactly what we need.

Copy link
Member

Choose a reason for hiding this comment

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

Functional arguments work for me, I'm not picky though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm I think the outline Bob has above will make it very easy to use CryptoSigner. + 1

I might be missing something, why even have SignMessage?

Copy link
Member

Choose a reason for hiding this comment

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

SignMessage is there as a counterpart to VerifyMessage, so that given a payload and a SignerVerifier sv, you should be able to perform the combination of:

sv.VerifyMessage(sv.SignMessage(message),message)

(this exact syntax isn't possible due to error handling, but hopefully the intent is clear).

Copy link
Member

Choose a reason for hiding this comment

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

#69 has been updated to implement the functional options approach discussed above... comments welcome!

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm happy enough with everything in #69 other than making our Signers also declare crypto.Signer. That's not to say that the specific implementations can't or shouldn't implement crypto.Signer directly, but IMHO there should be a sane way to pass request context to an implementation that requires it. PTAL at my proposal for dealing with that: https://github.com/sigstore/sigstore/pull/70/files#diff-d408de98cc9d5ef6f06ab23cdeab573b79f589766bcdf05ebf207628f8ac63f6

// Returns a standard Golang `crypto.Signer` which uses the given RPC options (if applicable) in addition to default SignerOpts and a MessagePreprocessor.
CryptoSigner(rpcOpts ...RPCOption) (crypto.Signer, error)

@dekkagaijin dekkagaijin force-pushed the signer branch 6 times, most recently from 66ab908 to eb9d961 Compare June 29, 2021 17:31
@dekkagaijin dekkagaijin changed the title [WIP] add CryptoSigner() method to signature.Signers replace context with Options in Signer, Verifier, and PublicKeyProvider interfaces Jun 29, 2021
…vider methods

Signed-off-by: Jake Sanders <jsand@google.com>
@dekkagaijin
Copy link
Member Author

reconciled in #69

@dekkagaijin dekkagaijin closed this Jul 1, 2021
@dekkagaijin dekkagaijin deleted the signer branch July 20, 2021 19:00
@dekkagaijin dekkagaijin restored the signer branch July 20, 2021 19:00
@dekkagaijin dekkagaijin deleted the signer branch July 26, 2021 18:26
@dekkagaijin dekkagaijin restored the signer branch July 26, 2021 18:27
mtrmac pushed a commit to mtrmac/sigstore that referenced this pull request Mar 10, 2023
Signed-off-by: Dan Lorenc <dlorenc@google.com>
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.

None yet

6 participants