-
Notifications
You must be signed in to change notification settings - Fork 8
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
Adds support for Private Verifiable Tokens #15
Conversation
} | ||
|
||
async issue(tokReq: TokenRequest2): Promise<TokenResponse2> { | ||
const blindedElt = VOPRF.group.desElt(tokReq.blindedMsg); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export class EvaluationRequest {
constructor(public readonly blinded: Array<Elt>) {}
serialize(): Uint8Array {
return joinAll(toU16LenPrefixClass(this.blinded))
}
I guess the privacy pass protocol is "set" here, but it's odd not using Evaluation.deserialize and EvaluationRequest.deserialize?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, so Evaluation in voprf-ts is of the Batched variety, from:
https://datatracker.ietf.org/doc/draft-ietf-privacypass-batched-tokens/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't really see any "canonical" encodings of OPRF types here:
https://github.com/cloudflare/circl/blob/main/oprf/client.go
?
From OPRF spec:
Applications serialize protocol messages between client and server
for transmission. Elements and scalars are serialized to byte
arrays, and values of type Proof are serialized as the concatenation
of two serialized scalars. Deserializing these values can fail, in
which case the application MUST abort the protocol raising a
DeserializeError failure.
I guess serialize is for matching the vectors?
expect(toHexListClass(finData.blinds)).toEqual(vi.Blind)
expect(toHexListClass(evalReq.blinded)).toEqual(vi.BlindedElement)
const ev = await server.blindEvaluate(evalReq, info)
expect(toHexListClass(ev.evaluated)).toEqual(vi.EvaluationElement)
expect(ev.proof && toHexListClass([ev.proof])).toEqual(
vi.Proof && vi.Proof.proof
)
const output = await client.finalize(finData, ev, info)
expect(toHexListUint8Array(output)).toEqual(vi.Output)
No, it seems everything there serialized is either Scalar/Element or Proof(Scalar*2).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
first pass of review on the API changes this PR proposes
src/issuance.ts
Outdated
|
||
if (issuerResponse.status !== 200) { | ||
const body = await issuerResponse.text(); | ||
throw new Error(`tokenRequest failed with code:${issuerResponse.status} response:${body}`); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note for future: having a dedicated file with errors would allow some sort of matching down the line, and a more consistent information passed to the users.
src/issuance.ts
Outdated
return { tokResBytes }; | ||
} | ||
|
||
export async function issuanceProtocolPub( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export async function issuanceProtocolPub( | |
export async function fetchPublicToken( |
I could not understand the function name easily. The method could either be wrap them in one object issuanceProtocol = { fetchPublic, fetchPrivate }
, define them as method on TokenRequest
for each token type (might be worth having an interface), or have another name that's easier to understand.
In addition, there is a lot of similarities with issuanceProtocolPriv
. This method shares the same issue with naming, and both might be abstracted somehow to avoid duplication, as it's only going to grow with new token types (RL, batched, grease)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I create an interface called PrivacyPassClient
not the best name, but allows us to move forward.
gg: VOPRF_GROUP, | ||
hashID: VOPRF_HASH, | ||
hash: Oprf.Crypto.hash, | ||
dst: '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
define dst
as a separate constant to better identify it down the line. It might be worth having a recommended destination in cloudflare/voprf-ts, but I'm not sure there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not even actually needed to decode a DLEQProof. A circular dependency (that I'm foggy on the ramifications of) triggered a refactoring to put the hash etc in the DLEParams. Really all you need is the group id for decoding the proof, as it is just two Scalar objects: {c, s}
But then again, you shouldn't even need to decode the proof at all here, except for the that the voprf-ts lib api accepts "wet" parameters (decoded VOPRF objects, not bytes). If the the EvalReq and Eval were already encode as objects, with all leaves as Uint8Array already, then at the application level (i.e. here in privacypass-ts) you could pick and choose from the fields and place them in app level packets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ala:
export type DLEQProof = { c: Scalar; s: Scalar; serialize(): Uint8Array }
// OPRF protocol only specifies encodings for these elements
export type OPRFElement = Elt | Scalar | DLEQProof
// So we only encode elements/bytes, and leave packet encoding to application layer
export interface EvaluationRequest {
readonly blinded: Array<Serialized<Elt>>
}
export interface BaseEvaluation {
readonly mode: ModeID
readonly evaluated: Array<Serialized<Elt>>
}
export interface VerifiableEvaluation extends BaseEvaluation {
readonly proof: Serialized<DLEQProof>
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All this serialization code in voprf-ts is actually unused (beyond the tests), because the application layer defines its own packets, encoding in the fields piecemeal:
serialize(): Uint8Array {
let proofBytes = new Uint8Array()
if (this.proof && (this.mode == Oprf.Mode.VOPRF || this.mode == Oprf.Mode.POPRF)) {
proofBytes = this.proof.serialize()
}
return joinAll([
...toU16LenPrefixClass(this.evaluated),
Uint8Array.from([this.mode]),
proofBytes
])
}
static deserialize(suite: SuiteID, bytes: Uint8Array, ...arg: CryptoProviderArg): Evaluation {
const group = getSuiteGroup(suite, arg)
const { head: evalList, tail } = fromU16LenPrefixDes(group.eltDes, bytes)
let proof: DLEQProof | undefined
const proofSize = DLEQProof.size(group)
const proofBytes = tail.subarray(1, 1 + proofSize)
const mode = tail[0]
switch (mode) {
case Oprf.Mode.OPRF: // no proof exists.
break
case Oprf.Mode.VOPRF:
case Oprf.Mode.POPRF:
proof = DLEQProof.deserialize(group.id, proofBytes, ...arg)
break
default:
assertNever('Oprf.Mode', mode)
}
return new Evaluation(mode, evalList, proof)
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding DST and all other params, those will be abstracted in the voprf-ts package. For now let's go with that, a refactor will solve that issue later.
615425f
to
29e1dce
Compare
29e1dce
to
22405cb
Compare
I found the name Client a bit surprising given it only supported one
request at a time. ClientContext or similar? Or make it stateless? Matching
[finData, evalReq] of voprf-ts ?
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks OK.
I agree with @sublimator the naming for Client
could be improved. I tend to like stateless APIs, so maybe that's a follow up: make it stateless, then Client is fine.
Adds support for Private Verifiable Tokens based on VOPRF (P-384, SHA-384)
Closes #2