openpubkey
PK Token Properties
I want to have a discussion about what our goals are with the PK Token because it impacts data structures which are always nice to get as right as possible, as soon as possible.
I propose that the PK Token should have the following three properties:
Property 1: The PK Token does not require any outside information in order to verify itself
- This isn't to say that external code shouldn't verify certain claims or parameters. For example, it would still be best practices to check that the issuer claim or a key algorithm match expected values, but a PK Token would only use its own values for verification.
Property 2: A valid PK Token should have, at a minimum:
- A single oidc provider signature whether that be in the form of rsa or gq (or other!)
- A single user signature over the same payload including client instance claims headers
Property 3: The PK Token supports any number of cosigners
- A PK Token can have zero or more cosigners without violating Property 1
Apologies for the late comment!
Property 1: I think this is fine but this stuff is tricky. I was worried that not requiring alg be checked against an externally provided value could be dangerous, but I think we're OK because we require use of JWK, so the alg in the JWK must match the alg in the JWS. Does that make sense?
Property 2: You mean we require one OP signature but we allow more than one and we require one user signature but we allow more than one, right? It would be strange to have multiple OP signatures I think, is there a use case for that? Multiple user signatures makes sense if you want to bind multiple keys.
Property 3: Makes sense to me!
Property 1: I think this is fine but this stuff is tricky. I was worried that not requiring alg be checked against an externally provided value could be dangerous, but I think we're OK because we require use of JWK, so the alg in the JWK must match the alg in the JWS. Does that make sense?
The emphasis I want to make with this property is the division of responsibilities. To your point, the internal pktoken validation might check that the JWK's algorithm is the same as the alg claim, that makes it "internally valid". But there is this second set of responsibilities like doing that external check on whether the value of the alg claim is acceptable. For example, if alg was "None" that's technically an accepted value so the pktoken would still internally verify, but that would be bad if that were the case so it may not be externally valid.
What I really want to accomplish with this property is to force this idea that you can validate the pk token anywhere, for the most part, like other json web things. Sure, you might not check whether it was issued by "google" instead of "evil.co" or whether the signing algorithm is "None", but if a pk token lands in your lap, you can validate it.
So, from a technical stance what I'm saying is that I want to put the bulk of the formatting and signature verification code into the pk token object itself, have something like (p *PKToken) Verify() error, and other entities like the client, verifier, provider, cosigner, etc. can do additional, unique checks e.g. that fields match expected values or fall within certain parameters.
Property 2: You mean we require one OP signature but we allow more than one and we require one user signature but we allow more than one, right? It would be strange to have multiple OP signatures I think, is there a use case for that? Multiple user signatures makes sense if you want to bind multiple keys.
I wanted to boil the pk token down to it's most minimally viable parts. I avoided making claims about "one or more" and keeping it "exactly one of" because, to your point, that might make sense for cic but not really for op but who knows what use cases people might have down the line. We can always add to the properties, if you want to say something about "one or more".
The way I have translated this (surreptitiously) into code already is in how I've defined the "New" function:
func New(idToken []byte, cicToken []byte, isGQ bool) (*PKToken, error)
If we lock down direct access to the object even further (as I'd like to) then this relationship becomes clear: the only way to create a new pktoken is minimally with both an op and a cic signature.
Only one OP per PK Token I see an iron requirement due to the fact that a PK Token can only contain a sub issued by a single OP.
I do worry that Property 2 could be read as excluding refresh tokens. Since there would now be two OP signatures per PK Token. I don't believe this is the intent of Property 2.
I'm mostly aligned on Property 2 & 3, I want to discuss Property 1 in this comment and think through why we might want to verify PK Tokens outside of the PK Token struct.
Option 1: Supply allowlist config struct at verification time, the allowlist would not be saved inside the PK Token and would need to be resupplied each time verify is called
err = pkt.Verify(cxt, &Allowlist{
AllowedProviders: []oidc.OpenIdProvider{opGoogle, ...},
AllowedCosigners: []cos.CosignerProvider{mfaCosigner, ...},
})
Pros:
- The parameters in Verify to enforce that an allowlist is specified. It is clear what verification rules are being applied. This is nice developer UX.
- Easy to try more than one Allowlist for the usecase, I want googleOp for this cosigner, but if that fails try githubOp without a cosigner.
- Stateless
Cons:
- Allowlist needs to be available to whatever function calls verify each time verify is called.
- Statelessness prevents caching results from verification
Option 2: Supply Allowlist to PK Token and store it as part of the pkt object.
pkt.AddAllowlist(&Allowlist{
AllowedProviders: []oidc.OpenIdProvider{opGoogle, ...},
AllowedCosigners: []cos.CosignerProvider{mfaCosigner, ...},
})
err = pkt.Verify(ctx)
Pros:
- You can cache verification results in the PK Token itself and track the validity of a PK Token in the PK Token struct.
Cons:
- Does not use the parameters in Verify to enforce that an allowlist is specified, e.g., someone could call pkt.Verify(ctx) without first adding an allowlist.
- PK Token now has verification metadata which is not derived from deserialization a json PK Token.
Option 3: A PK Token verifier struct:
allowlist := &Allowlist{
AllowedProviders: []oidc.OpenIdProvider{opGoogle, ...},
AllowedCosigners: []cos.CosignerProvider{mfaCosigner, ...},
}
verPkt := pktokenverifier.New(ctx, allowlist)
err = verPkt.Verify(pkt)
Pros:
- Very flexible: Lets developers bring their own validators without needing access to the internals of our package.
- Verification changes can happen independently from PK Token interface, can add new public key logs without changing provider interface.
- You can cache verification results and PK Token validity in the PKtokenVerifier itself.
- Enforce the creation of allowlists in the parameters to the PKTokenVerifier constructor
- No verification meta data stored in PK Token, PK Token is now just deserialization of the PK Token and helper methods
Cons:
- Verification lives outside of the PK Token
Edit: Cyclical dependencies
The issue I run into whenever moving verification out of the client is that the providers and cosignerProviders live in the client and the client also needs to call verify. This creates a cyclical dependency, client depends on verifier package, verifier package depends on client package.
I could solve this problem by creating a provider and cosigner interface in the verifier package. I'm not a fan of this approach because if we created interfaces for the verifier we would have two interfaces for same struct. A change to that interface would then require changing all the structs that depend on it and now another interface. It works, but it addresses the symptom not the underlying issue.
A better approach which addresses the underlying issue would be to break providers out into their own package providers which is imported by both client and verifier. The verifier could live inside the pktoken package, but I like how currently the pktoken package doesn't have dependencies on the providers. It provides a nice separation.
I'm not sure this is a v1 requirement or not as it is framed as a discussion rather than a task. How do we know when the discussion is complete?
From the perspective of a requirement, merging 72 should resolve it: https://github.com/openpubkey/openpubkey/pull/72
To me the completion of the discussion is what's required for v1 as these are properties that dictate the structure of our pk token, its interface, and its role in the architecture.
FYI I ended up choosing a similar approach moving the verification code into PR #68
I choose to use Option 3, a PKTokenVerifier Struct, but I kept it in the client package avoid making the refactor too large.