Creates OpkClient constructor and moves singer/alg/gqflag into the OpkClient struct

[EthanHeilman]

The main change is to take three of the five parameters to client.Auth(...) and move them into the client struct.

This the current definition of Auth:

func (o *OpkClient) Auth(ctx context.Context, signer crypto.Signer, alg jwa.KeyAlgorithm, extraClaims map[string]any, signGQ bool) (*pktoken.PKToken, error) 

This is the definition of Auth after this PR:

func (o *OpkClient) Auth(ctx context.Context, opts ...AuthOpts) (*pktoken.PKToken, error)

OpkClient constructor

We then create a OpkClient constructor that supports optional parameters via a variadic function. This lets us initialize these values with defaults but provides flexibility if developers want to override these defaults.

Creating an opkClient, all default values

opkClient, err := client.New(op)

Creating an opkClient overriding all default values

opkClient, err := client.New(
        op,
	client.WithCosignerProvider(&cosignerProvider),
	client.WithSigner(signer, alg),
	client.WithSignGQ(true))

TODOs

• Transform signer/alg/signGQ parameters in the client.Auth parameters to struct variables.
• Moves the client unittest to use client.Auth rather than client.OidcAuth
• Updates examples to use client.Auth
• ExtraClaims as options list in client.Auth
• OpkClient.New constructor with getters and setters
• Tests for optional parameter passing to the constructor and auth
• Package docs updates

Why move them into the struct?

Signer/Alg

The signer will be used by for the Cosigner Refresh and POP Auth client functions so it makes sense to bundle them as instance variables of the client. Additionally allows passing of the client into functions without also having to pass the signer and alg variables separately. For example look at how putting the signer and the alg into the struct cleans up the SSH example:

Before

client := &client.OpkClient{
     Op: &op,
}
gqFalse := false
certBytes, seckeySshPem, err := createSSHCert(context.Background(), client, signer, alg, gqFalse, principals)
...
func createSSHCert(cxt context.Context, client *client.OpkClient, signer crypto.Signer, alg jwa.KeyAlgorithm, gqFlag bool, principals []string) ([]byte, []byte, error) {
	pkt, err := client.Auth(cxt, signer, alg, map[string]any{}, gqFlag)

After

opkClient, err := client.New(
	&op,
	client.WithSigner(signer, alg),
	client.WithSignGQ(false),
)
certBytes, seckeySshPem, err := createSSHCert(context.Background(), opkClient, principals)
...
func createSSHCert(cxt context.Context, client *client.OpkClient, principals []string) ([]byte, []byte, error) {
	pkt, err := client.Auth(cxt)

This reduces the number of moving pieces and makes code written using the client simpler, more concise and removes the possibility bugs where another signer is substituted as the client signer.

This pattern allows users to bring their own signer using WithSigner(), but if they don't specify a signer we create a signer for them. This means that someone writing a simple app using OpenPubkey has to write much less boilerplate.

Before

opk := &client.OpkClient{
	Op:   &op
}
clientKey, err := util.GenKeyPair(jwa.ES256)
pkt, err := opk.Auth(context.TODO(), clientKey, jwa.ES256, map[string]any{}, false)

After

opkClient, err := client.New(&op)
pkt, err := opk.Auth(context.TODO())

This also enables login/logout functionality when a client can be torn down and the signer deleted.

SignGQ

This follows the same justification as Signer/Alg. A client shouldn't want to mix GQ and non-GQ tokens. Configuring this up front when creating the client provides a single place to check how the client is configured. Someone wanted to audit that SignGQ=true only needs to check client creation, not each code path where Auth is called.

Backwards compatibility?

No one is using the client.Auth method currently as it was just introduced in the last PR a few days ago. We can make this change with or without client.New without breaking anything downstream.

[lgmugnier]

Overall, this is a fantastic improvement that I'm super excited to see

[lgmugnier]

I think this should be full words: ClientOptions

[EthanHeilman]

In trying to decide what to name this I looked up how options are named in golang official crypto library to try to keep things consistent.

As far as I can tell they use Options when the full variable name is just Options such as in src/crypto/ed25519/ed25519.go
func VerifyWithOptions(publicKey PublicKey, message, sig []byte, opts *Options)

but when the are specifying a prefix they use Opts rather than Option as in src/crypto/crypto.go:
Sign(rand io.Reader, digest []byte, opts SignerOpts) (signature []byte, err error

I decided to stick with this pattern from go crypto.

[lgmugnier]

While I do like this pattern better than what we have now, I think it might be more valuable to create our own interface and put this logic in there. I think this would be beneficial for many reasons:

• Allows people to use our library to generate keys that they then store without having to go through the client
• Allows people to use it even when not using the OPK Client
• Let's people add more stuff into their JWK or otherwise format to their use case
• Keeps it clear that it is a strict requirement of the process
• Let's us simplify our own internal functional logic by only having to deal with a single object instead of multiple
• Let's us provide more helpful formatting options for saving values as strings without having to cast unsafely
• Easily expand support for different generation algorithms without having to mess around with the client

[lgmugnier]

All this being said, I think it's fine to put in what you have and then I can put up a PR quickly after to do the above if it's something we want to do.

[EthanHeilman]

You are talking about the eternal dream of a SignerAndAlg interface?

[lgmugnier]

What's the value of returning this variable?

[EthanHeilman]

Good question:

1. It is an important property of the client so it seems likely that a developer may wish to check it is set the way they expect it.

2. client_test.go needs it to test that the option is set correctly https://github.com/openpubkey/openpubkey/pull/93/files#diff-43e8423bcc9bb4d9fcd3ce8da7e0197e64d49376e957292eadba0316d57b000dR73

[lgmugnier]

I think it makes the most sense to private these variables as well so that we can control creation in the New function

[EthanHeilman]

+1

My plan to do this is as follows:

1. Make cosP private since nothing external depends on it. Keep Op public for now.
2. Create Getters for Op and CosP. This will set us up for making OpkClient an interface.
3. Get PRs into downstream projects to move them to client.New() and OpkClient.Auth().
4. Once they are moved over, make op private.

I can do 1 and 2 in this PR and then 4 in a separate PR that makes OpkClient.OidcAuth() private as well

[EthanHeilman]

Just checked in code that gets us 1 and 2. Once this PR is merged, I can create the PRs to switch everyone over to new.Client and Auth(). None of the unittests or examples depend on cosP or Op being public values setting us up for 3.

[lgmugnier]

I believe this can be removed now

[EthanHeilman]

Fixed

[lgmugnier]

Suggested change

	// commitment claim is typically the nonce or aud claim in the ID Token).
	// commitment claim is typically the nonce or aud claim in the ID Token.

[EthanHeilman]

Fixed

[jonnystoten]

This is great stuff! ❤️

[jonnystoten]

and a tiny typo

[jonnystoten]

LGTM!