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

robknight · 2024-06-20T00:03:34Z

Closes https://linear.app/0xparc-pcd/issue/0XP-974/zuauth-doesnt-check-event-id-when-revealed

Fixes authenticate so that, if the event ID is revealed and event IDs are configured as filters, the revealed event ID must match one of the filtered values.

Also updates the tests to be more careful in identifying the kind of exception thrown.

artwyman

This is good, but thinking through the security model, I don't think it's complete yet. Basically every field in pcd.claims should be validated against the expected configuration (unless it's purely an output field like nullifierHash). pcd.verify() proves that the claims are proven, but doesn't rule out someone sending you a PCD proving the wrong claims.

I've made some comments on individual lines where I think there's more to check.

artwyman · 2024-06-21T21:13:46Z

packages/lib/zuauth/src/server.ts

@@ -31,14 +33,15 @@ export async function authenticate(
  const pcd = await ZKEdDSAEventTicketPCDPackage.deserialize(serializedPCD.pcd);

  if (!(await ZKEdDSAEventTicketPCDPackage.verify(pcd))) {
-    throw new Error("ZK ticket PCD is not valid");
+    throw new ZuAuthAuthenticationError("ZK ticket PCD is not valid");
  }

  if (pcd.claim.watermark.toString() !== watermark) {


I think the externalNullifier should be passed in by the caller to be verified here too (if it's set at all).
In general, it should be verified by anyone who makes use of the nullifierHash.

The external nullifier seems redundant if the watermark is used. What extra security or utility is it providing?

They're for different purposes. Watermark avoids replay attacks of the exact same proof by tying it to a specific transaction. Nullifier is tied to the user's identity, to ensure that if the same user generates a new proof they generate the same nullifier, to detect things like double-voting. In Zupoll (as I understand it) the watermark is tied to the contents of this ballot to ensure you vote on what you intended, while the external nullifier is the same for all votes on a particular poll, so that if you come back to vote a different ballot again later the nullifier detects it.

The question is whether the user of this library is going to make use of the nullifier. Since this layer of code doesn't know for sure, I think it should check everything. Likely if the user didn't want to use a nullifier they wouldn't set the external nullifier anyway, and the check will be trivial.

This like several of my other suggestions are based on the principal that the prover should not modify any of the arguments to the proof before generating (other than the args they're supposed to fill in, like the ticket). If they do so, they're cheating, and should be caught by these checks.

artwyman · 2024-06-21T21:17:57Z

packages/lib/zuauth/test/zuauth.spec.ts

    }
+    expect(thrown).to.be.true;
  });

  it("should not authenticate PCDs with the wrong event ID", async function () {


Why is this test case not new or modified? It seems like this test case is supposed to be testing the eventID check, but was already passing when that check was missing. That suggests to me that there might be something wrong with the test, which was causing it to throw for some other reason. Is that why you added the new ZuAuthAuthenticationError type? I think it may be worth doing a check on the error message to make sure you're getting an exception specifically from the case you expect, vs. accidentally triggering a public key check instead, or something similar.

The original test was broken and this one isn't. The only real change is the mechanism by which we test for exceptions, which is one that works rather than one that doesn't.

artwyman · 2024-06-21T21:24:01Z

packages/lib/zuauth/src/server.ts

+  if (
+    eventIds.size > 0 &&
+    pcd.claim.partialTicket.eventId &&
+    !eventIds.has(pcd.claim.partialTicket.eventId)


If eventId is not revealed, you need to check whether validEventIds matches the configured set of eventIds. Looking at the code elsewhere, it seems like this only happens if the number of eventIds is <= 20, which is the limit of the circuit. That does seem to mean that if you have >20 and eventID isn't revealed, there's no secure guarantee.

If eventId is not revealed, you need to check whether validEventIds matches the configured set of eventIds.

Why?

In general, all the claims should be checked against expectations, so that a prover can't fool you by sending a proof of something other than the conditions you asked for. That's because there's no cryptographic guarantee the prover can't change the args before generating the proof. validEventIds is simply a part of that principal.

The "If eventId is not revealed" part of my statement is simply acknowledging that checking a single revealed event ID is stricter than validEventIds, so if eventId is revealed and checked here it's okay to skip checking validEventIds. I'd probably go ahead and check everything, though, just to be safe and clear.

See my longer comment below for more thoughts on the interactions between the different ways of checking event ID.

artwyman · 2024-06-21T21:30:07Z

packages/lib/zuauth/src/server.ts

      "Signing key does not match any of the configured public keys"
    );
  }

+  if (
+    eventIds.size > 0 &&
+    pcd.claim.partialTicket.eventId &&


I think you should also check that either eventID is revealed, or validEventIDs is used and checked (see below), and throw if neither is true. Unless you think there's a case where the user intentionally doesn't want the eventID checked (in which case why would it be in the config?).

My thought process which leads to this:
One could imagine in general checking that all the fields in partialTicket which should be revealed are actually revealed. You want to avoid cases where the remote site decided to give you a proof which doesn't reveal enough data.

Probably in the general ZuAuth case that's up to the 3rd-party code using the library, which will presumably notice if any field they depend on is missing. However for fields checked by ZuAuth (eventID, productID) the app code might reasonably assume that it doesn't need to bother looking at those fields, since ZuAuth already checked them.

A case I'm a bit worried about is this scenario:

Proof config has >20 event IDs so no use of validEventIds

eventID is configured to be revealed

Malicious remote app generates a proof with eventID not revealed

This code doesn't check anything

App code doesn't make use of event ID (assuming that ZuAuth has verified it) and never notices.

artwyman · 2024-06-21T21:30:37Z

packages/lib/zuauth/src/server.ts

+      "Event ID does not match any of the configured event IDs"
+    );
+  }
+
  if (
    productIds.size > 0 &&
    pcd.claim.partialTicket.productId &&


As with eventID above, I think it might be necessary to throw here if the productID isn't revealed when it should be.

robknight · 2024-07-03T21:12:41Z

I've made several changes here. The server-side code now takes a full set of arguments, identical to those provided to the client (and so including fieldsToReveal). It will check that all of the fields configured to be revealed are in fact defined.

In addition, if event or product IDs are provided to authenticate against, then the relevant fields must be revealed. I have removed the use of the validEventIds feature in preference for a simpler and easier-to-understand comparison of the revealed value against the configured values.

The tests have been updated to check specific error messages, and to test the cases where there is a mismatch in the revealed fields used when making the proof and those provided to the authenticate function.

artwyman

Thanks, I like the thoroughness of the checks vs. config, particular if it's truly always the same config used for the proof request (I didn't trace all the codepaths to confirm that).

I made a few tactical suggestions in comments. My main remaining discussion point is about what the impact of these changes might be on the assumptions of existing users/apps. Specifically:

Is there any case left where it's allowable to not reveal eventID and productID? I saw when they're now enforced, but I also saw them being set to true in some new places. I'm not familiar enough with the full scope of the code to know what unchanged cases might remain where they're not set to true. It seems like if there are existing apps which were choosing not to reveal those fields, it's a bit surprising if our library suddenly starts revealing them.
The removal of the validEventIDs feature is a bit worrying in the same way. If there are apps which were depending on it, their privacy model is suddenly changing. Also that was specifically a feature we talked about a lot at Devconnect, and used in ZKTelegram for login and ZuRat, so removing it silently seems poor from a library compatibility standpoint. Maybe it should be left in the same state as before, where it functions only within specific size limits? I don't to overstress it given we're going to have a much more safe and flexible alternative with PODTickets and GPCs, but I also don't like the idea of changing the assumption of existing working apps, particularly on a security feature.

Both points are things I'm wiling to defer to you on after discussion, but thought were worth raising explicitly.

artwyman · 2024-07-04T07:30:48Z

packages/lib/zuauth/src/server.ts

+  keyof EdDSATicketFieldsToReveal,
+  keyof ITicketData
+> = {
+  revealAttendeeEmail: "attendeeEmail",


I think this can be done programmatically rather than with a lookup table.

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

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.

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

artwyman · 2024-07-04T07:36:49Z

packages/lib/zuauth/src/server.ts

+
+  // For each of the fields configured to be revealed, check that the claim
+  // contains values.
+  for (const [revealedField, fieldName] of Object.entries(revealedFields)) {


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 ); }

artwyman · 2024-07-04T07:39:50Z

packages/lib/zuauth/src/server.ts

+  for (const [revealedField, fieldName] of Object.entries(revealedFields)) {
+    if (fieldsToReveal[revealedField as keyof EdDSATicketFieldsToReveal]) {
+      checkIsDefined(pcd.claim.partialTicket[fieldName], fieldName);
+    }
  }

  const publicKeys = config.map((em) => em.publicKey);


I'm a bit nervous that each of these 3 fields are being checked individually, rather than as a triple of (key, eventID, productID) which would confirm that they aren't mixed together in the wrong way.
If all issuers are well-behaved about using unique IDs, that should be a non-issue, though.

I don't understand the problem here. Can you give me an example?

My config says that valid tickets to look like: (pubkeyA, eventA, productA1), (pubkeyA, eventA, productA2), (pubkeyB, eventB, productB1).

I'm presented with a ticket containing (pubkeyB, eventA, productA1). This is not actually a valid combination because the signer for B shouldn't be signing tickets from A. However the code here accepts it.

Alternately since there's usually only one signer, I could see a ticket (pubkey, eventB, productA1). This is invalid because it's mixing products from A with event B. The code here accepts it anyway.

It's an unlikely issue given there's currently only one issuer (Zupass/PODBox) and it uses unique IDs. But in theory if the signers are independent we shouldn't trust them not to step on each other. The code here could avoid the risk by forming a set of triples of (pubkey, eventID, productID) and checking that the presented IDs match exactly one triple. It's a bit more complicated because of the fact that some of the fields might not be revealed, so it's not a simple set membership check.

With GPC proofs, it'll be possible to prove that the triple is in a list in a ZK way directly.

artwyman · 2024-07-04T07:44:11Z

packages/lib/zuauth/src/zuauth.ts

@@ -135,8 +147,7 @@ export function constructZkTicketProofUrl(zuAuthArgs: ZuAuthArgs): string {
    },
    validEventIds: {
      argumentType: ArgumentTypeName.StringArray,
-      value:
-        eventIds.length !== 0 && eventIds.length <= 20 ? eventIds : undefined,
+      value: undefined,


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.

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.

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.

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.

…ccount

…ecked

…ions

…duct ID

robknight · 2024-07-12T18:00:28Z

I've made further modifications:

From a ZuAuth perspective, it doesn't make sense to include validEventIds, because in cases where the event ID is hidden it is not possible to be certain that the (hidden) matching event ID really did belong to a ticket with the expected signing key for that event ID (there could be a duplicate event ID from another signer whose tickets are also in the set of valid tickets, but not for the event ID used).

I've tightened up all of the checks that ensure that the claim matches the configuration. This is important because we want to ensure that the same configuration is used to derive the prove screen URL on the client side, and for authentication on the server-side. Any mismatch may suggest a configuration error, or use of a credential prepared with a different configuration (though the watermark should be sufficient to prevent credential replay attacks). Therefore we now check:

that all fields configured to be revealed are revealed
that all fields configured not to be revealed are not revealed
that the watermark and external nullifier match those in the configuration
that validEventIds is not set

I have also added some tests, and cleaned up the implementation of existing tests. This includes checking the precise exceptions thrown for each failure case.

artwyman

Looking good. A few questions remaining.
I didn't review the utests in detail.

artwyman · 2024-07-12T20:45:27Z

packages/lib/zuauth/src/server.ts

+  return (
+    isEqualEdDSAPublicKey(claim.signer, config.publicKey) &&
+    claim.partialTicket.eventId === config.eventId &&
+    !!config.productId &&


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

Ping on this comment? Am I misreading this?

artwyman · 2024-07-12T20:54:06Z

packages/lib/zuauth/src/server.ts

 ): Promise<ZKEdDSAEventTicketPCD> {
+  /**


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 //.

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.

artwyman · 2024-07-12T20:57:06Z

packages/lib/zuauth/src/server.ts

+      checkIsUndefined(pcd.claim.partialTicket[fieldName], fieldName);
+    }
+  }
+


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.

I've added a check for this too.

artwyman · 2024-07-16T18:40:11Z

packages/lib/zuauth/src/server.ts

@@ -47,8 +47,8 @@ function claimMatchesConfiguration(
  return (
    isEqualEdDSAPublicKey(claim.signer, config.publicKey) &&
    claim.partialTicket.eventId === config.eventId &&
-    !!config.productId &&
-    claim.partialTicket.productId === config.productId
+    (config.productId === undefined ||


What's the semantics in PODBox if productID isn't defined? The underlying PCD always has a productID, so is it meant to be ignored? Is it set to be equal to the eventID?
This code implements the former (productID will be ignored) which seems fine. We could be extra-strict and require that the productID not be revealed when the config doesn't specify it, but that seems optional

robknight requested a review from artwyman June 20, 2024 00:03

artwyman requested changes Jun 21, 2024

View reviewed changes

robknight force-pushed the robknight/0xp-974-zuauth-doesnt-check-event-id-when-revealed branch from 4578d48 to 95c00dc Compare July 3, 2024 13:03

robknight requested a review from artwyman July 3, 2024 21:23

artwyman requested changes Jul 4, 2024

View reviewed changes

robknight force-pushed the robknight/0xp-974-zuauth-doesnt-check-event-id-when-revealed branch 2 times, most recently from 8215588 to bb47cbf Compare July 6, 2024 11:29

robknight added 6 commits July 10, 2024 15:01

Throw exceptions when event ID doesn't match configuration

250e623

More server-side checks of validity of client-provided PCD

cc2b5e5

Upgrade example apps to new API

628c7c0

Check external nullifier on server side

1dbf722

More careful checking of watermark and nullifier, taking types into a…

c6b914f

…ccount

Allow event ID to be unrevealed if 20 or fewer event IDs are being ch…

fe70abe

…ecked

robknight force-pushed the robknight/0xp-974-zuauth-doesnt-check-event-id-when-revealed branch from bb47cbf to fe70abe Compare July 12, 2024 09:27

robknight added 8 commits July 12, 2024 12:28

Test all of the possible server-side failures

ac7f09a

Check for unexpectedly revealed fields

f033c9f

Make use of validEventIds explicit and test for incoherent configurat…

1c544d3

…ions

Ensure precise match to configured pattern of public key-event ID-pro…

e07f3bf

…duct ID

Improve comments

312aa8a

Remove validEventIds functionality

6831f58

Use generic Error class

cb6f324

Simplify variable names

ae9ecf1

robknight requested a review from artwyman July 12, 2024 18:00

artwyman requested changes Jul 12, 2024

View reviewed changes

robknight added 2 commits July 15, 2024 10:13

Check that validEventIds is not set

d483bbf

Fix error message matches in tests

a98d8d7

robknight requested a review from artwyman July 15, 2024 16:54

Handle configs without product IDs properly, and add test for this

f8c1d92

artwyman approved these changes Jul 16, 2024

View reviewed changes

robknight added this pull request to the merge queue Jul 16, 2024

Merged via the queue into main with commit f43d4ed Jul 16, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

robknight commented Jun 20, 2024

artwyman left a comment

artwyman Jun 21, 2024

robknight Jun 25, 2024

artwyman Jun 25, 2024 •

edited

Loading

artwyman Jun 21, 2024

robknight Jun 25, 2024 •

edited

Loading

artwyman Jun 21, 2024

robknight Jun 25, 2024

artwyman Jun 25, 2024

artwyman Jun 21, 2024

artwyman Jun 21, 2024

robknight commented Jul 3, 2024

artwyman left a comment

artwyman Jul 4, 2024

robknight Jul 12, 2024

artwyman Jul 12, 2024

artwyman Jul 4, 2024

artwyman Jul 4, 2024

robknight Jul 8, 2024

artwyman Jul 9, 2024

artwyman Jul 4, 2024

robknight Jul 12, 2024

robknight Jul 12, 2024

artwyman Jul 12, 2024

robknight commented Jul 12, 2024

artwyman left a comment

artwyman Jul 12, 2024

artwyman Jul 15, 2024

artwyman Jul 12, 2024

robknight Jul 15, 2024

artwyman Jul 12, 2024

robknight Jul 15, 2024

artwyman Jul 16, 2024

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

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

Conversation

robknight commented Jun 20, 2024

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robknight Jun 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robknight commented Jul 3, 2024

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

robknight commented Jul 12, 2024

artwyman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

artwyman Jun 25, 2024 •

edited

Loading

robknight Jun 25, 2024 •

edited

Loading