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.

Sign up for GitHub

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

Throw exceptions when event ID doesn't match configuration #1802

Merged
merged 17 commits into from
Jul 16, 2024
Merged

Throw exceptions when event ID doesn't match configuration #1802

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
250e623
Throw exceptions when event ID doesn't match configuration
robknight Jun 20, 2024
cc2b5e5
More server-side checks of validity of client-provided PCD
robknight Jul 3, 2024
628c7c0
Upgrade example apps to new API
robknight Jul 3, 2024
1dbf722
Check external nullifier on server side
robknight Jul 3, 2024
c6b914f
More careful checking of watermark and nullifier, taking types into a…
robknight Jul 6, 2024
fe70abe
Allow event ID to be unrevealed if 20 or fewer event IDs are being ch…
robknight Jul 12, 2024
ac7f09a
Test all of the possible server-side failures
robknight Jul 12, 2024
f033c9f
Check for unexpectedly revealed fields
robknight Jul 12, 2024
1c544d3
Make use of validEventIds explicit and test for incoherent configurat…
robknight Jul 12, 2024
e07f3bf
Ensure precise match to configured pattern of public key-event ID-pro…
robknight Jul 12, 2024
312aa8a
Improve comments
robknight Jul 12, 2024
6831f58
Remove validEventIds functionality
robknight Jul 12, 2024
cb6f324
Use generic Error class
robknight Jul 12, 2024
ae9ecf1
Simplify variable names
robknight Jul 12, 2024
d483bbf
Check that validEventIds is not set
robknight Jul 15, 2024
a98d8d7
Fix error message matches in tests
robknight Jul 15, 2024
f8c1d92
Handle configs without product IDs properly, and add test for this
robknight Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion apps/consumer-client/src/pages/examples/zuauth/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,9 @@ export default function ZuAuth(): JSX.Element {
});

if (result.type === "pcd") {
setAuthenticated(await serverLogin(result.pcdStr, config));
setAuthenticated(
await serverLogin(result.pcdStr, config, fieldsToReveal)
);
}
})();
}, [fieldsToReveal, configString]);
Expand Down
6 changes: 4 additions & 2 deletions apps/consumer-client/src/pages/examples/zuauth/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import { ITicketData } from "@pcd/eddsa-ticket-pcd";
import { PipelineEdDSATicketZuAuthConfig } from "@pcd/passport-interface";
import { EdDSATicketFieldsToReveal } from "@pcd/zk-eddsa-event-ticket-pcd";
import urlJoin from "url-join";
import { CONSUMER_SERVER_URL } from "../../../constants";

Expand Down Expand Up @@ -33,7 +34,8 @@ export async function logout(): Promise<void> {
*/
export async function serverLogin(
serialized: string,
config: PipelineEdDSATicketZuAuthConfig[]
config: PipelineEdDSATicketZuAuthConfig[],
fieldsToReveal: EdDSATicketFieldsToReveal
): Promise<Partial<ITicketData>> {
const response = await fetch(urlJoin(CONSUMER_SERVER_URL, `auth/login`), {
method: "POST",
Expand All @@ -43,7 +45,7 @@ export async function serverLogin(
"Access-Control-Allow-Origin": "*",
"Content-Type": "application/json"
},
body: JSON.stringify({ pcd: serialized, config })
body: JSON.stringify({ pcd: serialized, config, fieldsToReveal })
});

return await response.json();
Expand Down
10 changes: 5 additions & 5 deletions apps/consumer-server/src/routing/routes/zuauth/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,11 @@ export function login(
return;
}

const pcd = await authenticate(
req.body.pcd,
session.watermark,
req.body.config
);
const pcd = await authenticate(req.body.pcd, {
watermark: session.watermark,
config: req.body.config,
fieldsToReveal: req.body.fieldsToReveal
});

session.ticket = pcd.claim.partialTicket;

Expand Down
11 changes: 10 additions & 1 deletion examples/zuauth/src/app/api/login/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,16 @@ export async function POST(req: NextRequest) {

try {
const session = await getIronSession<SessionData>(cookieStore, ironOptions);
const pcd = await authenticate(body.pcd, session.watermark ?? "", config);
const pcd = await authenticate(body.pcd, {
watermark: session.watermark ?? "",
config,
fieldsToReveal: {
revealAttendeeEmail: true,
revealAttendeeName: true,
revealEventId: true,
revealProductId: true
}
});

session.user = pcd.claim.partialTicket;
await session.save();
Expand Down
4 changes: 3 additions & 1 deletion examples/zuauth/src/app/home.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,9 @@ export default function Home() {
zupassUrl: process.env.NEXT_PUBLIC_ZUPASS_SERVER_URL as string,
fieldsToReveal: {
revealAttendeeEmail: true,
revealAttendeeName: true
revealAttendeeName: true,
revealEventId: true,
revealProductId: true
},
watermark,
config: config
Expand Down
2 changes: 2 additions & 0 deletions packages/lib/zuauth/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,10 @@
"@pcd/tsconfig": "0.11.1",
"@semaphore-protocol/identity": "^3.15.2",
"@types/chai": "^4.3.5",
"@types/chai-as-promised": "^7.1.5",
"@types/mocha": "^10.0.1",
"@types/react": "^18.2.0",
"chai-as-promised": "^7.1.1",
"eslint": "^8.57.0",
"mocha": "^10.2.0",
"ts-mocha": "^10.0.0",
Expand Down
150 changes: 122 additions & 28 deletions packages/lib/zuauth/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,73 @@
import { isEqualEdDSAPublicKey } from "@pcd/eddsa-pcd";
import { PipelineEdDSATicketZuAuthConfig } from "@pcd/passport-interface";
import { ITicketData } from "@pcd/eddsa-ticket-pcd";
import { PipelineZuAuthConfig } from "@pcd/passport-interface";
import {
EdDSATicketFieldsToReveal,
ZKEdDSAEventTicketPCD,
ZKEdDSAEventTicketPCDClaim,
ZKEdDSAEventTicketPCDPackage,
ZKEdDSAEventTicketPCDTypeName
} from "@pcd/zk-eddsa-event-ticket-pcd";
import { ZuAuthArgs } from ".";

/**
* Check if a given field is defined.
*/
function checkIsDefined<T>(
field: T | undefined,
fieldName: string
): field is T {
if (field === undefined || field === null) {
throw new Error(
`Field "${fieldName}" is undefined and should have a revealed value`
);
}
return true;
}

/**
* Check if a given field is undefined.
*/
function checkIsUndefined(field: unknown, fieldName: string): boolean {
if (field !== undefined) {
throw new Error(
`Field "${fieldName}" is defined and should not have a revealed value`
);
}
return true;
}

/**
* Check if an individual configuration matches the claim from the PCD.
*/
function claimMatchesConfiguration(
claim: ZKEdDSAEventTicketPCDClaim,
config: PipelineZuAuthConfig
): boolean {
return (
isEqualEdDSAPublicKey(claim.signer, config.publicKey) &&
claim.partialTicket.eventId === config.eventId &&
!!config.productId &&
Copy link
Member

Choose a reason for hiding this comment

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

This means a config with no productID never matches? Seems like you might want an OR here for the case where productID is undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Ping on this comment? Am I misreading this?

claim.partialTicket.productId === config.productId
);
}

const revealedFields: Record<
keyof EdDSATicketFieldsToReveal,
keyof ITicketData
> = {
revealAttendeeEmail: "attendeeEmail",
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 can be done programmatically rather than with a lookup table.

  output = input.slice(6, 7).toLowerCase() + input.slice(7);

Copy link
Member Author

Choose a reason for hiding this comment

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

This makes logical sense, but TypeScript really wants keys into objects to have the correct types. The key type for ITicketData is keyof ITicketData and not string, so we can't construct a valid type by doing string manipulation.

Copy link
Member

Choose a reason for hiding this comment

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

You could do it with an as somewhere, I think, but fair enough to stick with type safety.

revealAttendeeName: "attendeeName",
revealAttendeeSemaphoreId: "attendeeSemaphoreId",
revealEventId: "eventId",
revealIsConsumed: "isConsumed",
revealIsRevoked: "isRevoked",
revealProductId: "productId",
revealTicketCategory: "ticketCategory",
revealTicketId: "ticketId",
revealTimestampConsumed: "timestampConsumed",
revealTimestampSigned: "timestampSigned"
} as const;

/**
* Authenticates a ticket PCD.
Expand All @@ -20,9 +83,11 @@ import {
*/
export async function authenticate(
pcdStr: string,
watermark: string,
config: PipelineEdDSATicketZuAuthConfig[]
{ watermark, config, fieldsToReveal, externalNullifier }: ZuAuthArgs
): Promise<ZKEdDSAEventTicketPCD> {
/**
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think the double * in /** here marks this as being for typedoc documentation, which doesn't seem relevant inside the body of the function. Could be just /* or else //.

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 I don't use the double-asterisk, then VSCode does not insert a new asterisk at the beginning of the subsequent lines. From that I inferred a distinction between using /* ... */ as a means of "commenting out", and /** ... */ as a means of writing multi-line comments, but thiat is just my interpretation of VSCode's logic.

* Check to see if our inputs are valid, beginning with the PCD.
*/
const serializedPCD = JSON.parse(pcdStr);
if (serializedPCD.type !== ZKEdDSAEventTicketPCDTypeName) {
throw new Error("PCD is malformed or of the incorrect type");
Expand All @@ -34,37 +99,66 @@ export async function authenticate(
throw new Error("ZK ticket PCD is not valid");
}

if (pcd.claim.watermark.toString() !== watermark) {
throw new Error("PCD watermark doesn't match");
/**
* The configuration array must not be empty.
*/
if (config.length === 0) {
throw new Error("Configuration is empty");
}

const publicKeys = config.map((em) => em.publicKey);
const productIds = new Set(
// Product ID is optional, so it's important to filter out undefined values
config
.map((em) => em.productId)
.filter((productId) => productId !== undefined)
);

if (
publicKeys.length > 0 &&
!publicKeys.find((pubKey) =>
isEqualEdDSAPublicKey(pubKey, pcd.claim.signer)
)
) {
/**
* Check if the external nullifier matches the configuration.
*/
if (externalNullifier !== undefined) {
if (pcd.claim.externalNullifier === undefined) {
throw new Error(
"PCD is missing external nullifier when one was provided"
);
}
if (
pcd.claim.externalNullifier.toString() !== externalNullifier.toString()
) {
throw new Error("External nullifier does not match the provided value");
}
} else if (pcd.claim.externalNullifier !== undefined) {
throw new Error(
"Signing key does not match any of the configured public keys"
"PCD contains an external nullifier when none was provided"
);
}

if (
productIds.size > 0 &&
pcd.claim.partialTicket.productId &&
!productIds.has(pcd.claim.partialTicket.productId)
) {
throw new Error(
"Product ID does not match any of the configured product IDs"
);
if (pcd.claim.watermark !== watermark.toString()) {
throw new Error("PCD watermark does not match");
}

/**
* Check that the revealed fields in the PCD match the expectations set out
* in {@link revealedFields}. This is to ensure the consistency between the
* configuration passed to this function, and the configuration used on the
* client-side when generating the PCD.
*/
for (const [revealedField, fieldName] of Object.entries(revealedFields)) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion without the lookup table:

  for (const [revealedFieldName, shouldReveal] of Object.entries(fieldsToReveal)) {
    checkIsDefined(
      pcd.claim.partialticket[revealedFieldName.slice(6, 7).toLowerCase() + revealedFieldName.slice(7)],
      revealedFieldName
    );
  }

if (fieldsToReveal[revealedField as keyof EdDSATicketFieldsToReveal]) {
checkIsDefined(pcd.claim.partialTicket[fieldName], fieldName);
} else {
checkIsUndefined(pcd.claim.partialTicket[fieldName], fieldName);
}
}

Copy link
Member

Choose a reason for hiding this comment

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

If you're now desupporting validEventIds, should you check here that it's required to be undefined? I don't think it changes the security (given you're checking that all the necessary fields are being revealed), but it would catch a configuration 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.

I've added a check for this too.

/**
* Our inputs are formally valid. Now we check to see if any of the
* configuration patterns match the claim in the PCD.
*/
let match = false;

for (const em of config) {
if (claimMatchesConfiguration(pcd.claim, em)) {
match = true;
break;
}
}

if (!match) {
throw new Error("PCD does not match any of the configured patterns");
}

return pcd;
Expand Down
15 changes: 12 additions & 3 deletions packages/lib/zuauth/src/zuauth.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function zuAuthRedirect(args: ZuAuthRedirectArgs): void {
*/
export function constructZkTicketProofUrl(zuAuthArgs: ZuAuthArgs): string {
const {
zupassUrl = "https://zupass.org",
zupassUrl = "https://zupass.org/",
returnUrl,
fieldsToReveal,
watermark,
Expand Down Expand Up @@ -114,6 +114,16 @@ export function constructZkTicketProofUrl(zuAuthArgs: ZuAuthArgs): string {
publicKeys.push(em.publicKey);
}

if (!fieldsToReveal.revealEventId) {
throw new Error("The event ID must be revealed for authentication");
}

if (productIds.length > 0 && !fieldsToReveal.revealProductId) {
throw new Error(
"When product IDs are specified for authentication, the product ID field must be revealed"
);
}

const args: ZKEdDSAEventTicketPCDArgs = {
ticket: {
argumentType: ArgumentTypeName.PCD,
Expand All @@ -135,8 +145,7 @@ export function constructZkTicketProofUrl(zuAuthArgs: ZuAuthArgs): string {
},
validEventIds: {
argumentType: ArgumentTypeName.StringArray,
value:
eventIds.length !== 0 && eventIds.length <= 20 ? eventIds : undefined,
value: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

This is losing a feature which we advertized at Devconnect. How sure are you that nobody is depending on it?
Does ZKTelegram use ZuAuth? We definitely went our of our way to make the point that logging in to DMS or posting via ZuRat didn't reveal which Devconnect subevent you attended. It would be odd to silently undo that.

It would be possible to keep using this field when you know it's sufficient (number of eventIDs is <=20, and no productIDs are used). That being said, I don't like that it was an implicit feature before. It would be better if it were an explicit configuration choice by the developer to use this feature, with the library throwing an exception if the dev tries to use this feature with an overlarge list.

Note that when we get to PODTickets we can have longer lists, and also triples of (key, eventID, productID), so maybe looking ahead to that is the right way to not worry too much about getting this perfect.

Copy link
Member Author

Choose a reason for hiding this comment

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

ZuAuth was not used at Devconnect, because it didn't exist then. The underlying code in ZkEdDSAEventTicketPCD and passport-interface was used, and still permits the use of validEventIds.

My thinking was that validEventIds is not a great feature to expose - it's confusing because it only works for event IDs and not product IDs. The more features we expose, the more we need to test and to document, and the purpose of ZuAuth is specifically to reduce the API surface area when compared to using the underlying libraries directly.

That said, I can make it an optionally-configurable feature that is off by default, which seems like the best of both worlds.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm. After implementing this, I remember why I didn't want to do it: the validEventIds model is just fundamentally not helpful for authentication in a world with multiple signers. The signer is always revealed, but unless you can determine the connection between the signer and the event ID, you can't rule out the possibility of some event ID having been used by the "wrong" signer. Passing a validEventIds check is just never good enough for authentication purposes, which is why we can't rely on it.

Copy link
Member

Choose a reason for hiding this comment

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

Understood. The limitation of that feature to validEventIds does assume that the signing key and product ID are either always a single value (e.g. signing key for last year's events), or don't matter for authentication (e.g. the product IDs for devconnect). It would be possible to check for those scenarios, or leave them up to the user to configure properly. However given comments elsewhere I'm okay with removing this so long as it's not affecting the use cases which were built to depend on it (like ZuRat for Devconnect). With PODTickets we can bring back a much more flexible version of this.

userProvided: false
},
fieldsToReveal: {
Expand Down
Loading
Loading