refactor: parameterize apple root certificate

[jstewmon]

👋 @MasterKale per discussion #124, this is a PR to parameterize the apple verifier root certificate.

There are a small number of root certificates that are currently baked into the library (not just Apple-related) that really should be default values that can then be overridden by environment variables.

I didn't go with environment variables because that wouldn't allow for arbitrary coupling of data and logic. Using an environment variable would limit us to one value per process. In my particular use case - I want to support both Apple WebAuthn with their WebAuthn Root CA AND Apple App Attest with their App Attest Root CA.

Conceptually, I'm going for a dependency injection strategy (vs option threading). I maintained the existing interfaces by using the default exports to export instances configured with the default settings.

I think I saw another PR in the past that used classes, and it wasn't appealing. I can swap the classes for function factories if you want. I thought I'd get your feedback on using classes first because they offer better performance if constructed frequently (admittedly, these shouldn't be), and I think they're a little easier to maintain.

The diff is a bit messy, so I'll try to tag the interesting bits that changed. Once you've weighed in on the strategy, I'll follow up with some new test, and I can apply the strategy to the other relevant places.

[MasterKale]

Hey @jstewmon, welcome back. Thank you for putting in the effort to architect a solution to the problem of injecting certificates into the verification process.

Here are my initial reactions:

1. Not a fan of mixed classes and functions for verifiers

I don't like the mix of attestation statement verifiers that aren't all functions. So much of the architecture of this project can be stateless because it's largely data in/decision out, and as a result the code can remain easy to trace through and reason about.

I can appreciate what you're trying to accomplish with that pattern, but what about this instead: a new CertificateService singleton, like MetadataService, that you can pass custom certificates into at startup for specific attestation types. This service could contain defaults for formats that require them ("apple", "android-safetynet", and "android-key" for now), while letting RP admins override those certificates. Then, the verification methods can remain functions, and perhaps verifyAttestation() can orchestrate passing in certs from CertificateService when it executes the individual verification methods for ease of testing these methods.

2. Iterating on defaultFormatVerifiers

I like this part of the PR, associating verification functions to execute with specific attestation formats. An AppAttest verification method wouldn't be appropriate for inclusion in this library, but for developers like yourself I think there's a chance to add a simple API for adding custom attestation verifications.

The direction that comes to mind for this is we leave the current verifications hard-coded as-is, but add in a final if else () {} block at the end that checks if any custom formats are specified, then checks for one that matches the current response's "fmt". If it finds one then it passes in all values (similar to how you consolidated all of the various attestation variables in this PR into one big blob of values that get passed to all methods to use as they see fit). Then the overall structure of the current verifyAttestation() would remain the same with the addition of a new argument and an extra if else branch.

---

Does this feedback make sense? I'm spitballing here with initial impressions. My high level goal with these impressions is to think of away to leave as much of this stuff as-is while offering:

1. The ability to specify custom root certs (mostly so that if I vanish 10 years from now RP devs can simply restart their server with new certs and minimal disruption.
2. A potential avenue for devs like yourself to augment their WebAuthn RPs with additional capabilities, while prioritizing WebAuthn spec compliance above all else.

Let me know what you think.

[jstewmon]

Thanks for the feedback, and sorry for the time lapse - I've been busy and I wanted letting your suggestions marinate because they didn't resonate with me right away, which I'll explain below.

1. Not a fan of mixed classes and functions for verifiers

👌

I don't like the mix of attestation statement verifiers that aren't all functions. So much of the architecture of this project can be stateless because it's largely data in/decision out, and as a result the code can remain easy to trace through and reason about.

I know what you mean - the code reads the same way that it executes. The dilemma with this is that when there are variants, the code will need control flow, which may cascade throughout the stack. A few variants are manageable, but can quickly become unwieldy - that's why I was trying to leverage the confluent verifier interfaces (even if they incidentally were so).

I can appreciate what you're trying to accomplish with that pattern, but what about this instead: a new CertificateService singleton, like MetadataService, that you can pass custom certificates into at startup for specific attestation types. This service could contain defaults for formats that require them ("apple", "android-safetynet", and "android-key" for now), while letting RP admins override those certificates. Then, the verification methods can remain functions, and perhaps verifyAttestation() can orchestrate passing in certs from CertificateService when it executes the individual verification methods for ease of testing these methods.

There are two downsides I thought of with this approach:

1. verifyAttestationResponse is already very (overly?) complex. Every time I look at it, I feel an urge to try to break it down into smaller pieces, but I struggle to see clean interfaces. It's complex, but effective. I just feel a misgiving about making it more complex.
2. The verifier interfaces would diverge. If it's truly incidental that they are currently so closely aligned, this may not be a valid concern, but it seemed like a move in the wrong direction. What do you think?

I suppose this concern is minimal if the relevant verifiers all gain one new param property like caRootCertificate.

If we go with CertificateService, would you see this having methods like get and set and/or properties that align with the ATTESTATION_FORMAT enum values?

2. Iterating on defaultFormatVerifiers

I like this part of the PR, associating verification functions to execute with specific attestation formats. An AppAttest verification method wouldn't be appropriate for inclusion in this library, but for developers like yourself I think there's a chance to add a simple API for adding custom attestation verifications.

The direction that comes to mind for this is we leave the current verifications hard-coded as-is, but add in a final if else () {} block at the end that checks if any custom formats are specified, then checks for one that matches the current response's "fmt". If it finds one then it passes in all values (similar to how you consolidated all of the various attestation variables in this PR into one big blob of values that get passed to all methods to use as they see fit). Then the overall structure of the current verifyAttestation() would remain the same with the addition of a new argument and an extra if else branch.

I'm inferring from this that it is largely incidental that the verifier interfaces aligned so closely. When I was working on my first draft of this PR, I supposed that the series of if statements had grown over time without noticing that the verifier interfaces were all compatible with respect to the preconditions provided by verifyAttestationResponse.

My intuition was that there is some long term maintainability advantage in codifying expectation that the verifier interfaces should be compatible with the argument verifyAttestaionResponse is able to provide.

My high level goal with these impressions is to think of away to leave as much of this stuff as-is while offering:

I provided some thoughts above on why I made some changes that weren't strictly necessary. I may have projected a vision for maintainability that isn't what you had in mind.

2. A potential avenue for devs like yourself to augment their WebAuthn RPs with additional capabilities, while prioritizing WebAuthn spec compliance above all else.

I'm reluctant to push for anything very specific to satisfy my particular use case. As I mentioned in #124, the existing helpers provide most of what one needs to easily work with Apple App Attest - the only duplication that's really needed is the verifyApple function to use an alternate certificate.

I think your suggestion would lend itself to my use case by adding a field the verifyApple (and the two android verifiers) parameters for caRootCertificate. I'm happy to do that with this PR if aren't worried about the concerns I've raise with the approach.

On a related note, what's up with verifyAndroidKey not checking a root ca certificate? I read some of the related issues (e.g. https://github.com/fido-alliance/conformance-test-tools-resources/issues/567), and I saw they were closed a while back.

Tangentially, (sorry to hijack the conversation) is there a simple explanation why (IIUC) FIDOU2F does not require a root ca verification, packed uses root ca certificates from the metadata service, and the others have a statically configured root ca certificate? Not fully understanding this made me feel uncertain about how this PR would relate to the rest of the library. Specifically, the concerns I mentioned above about the differences in verifier interfaces (and behavior) with some verifiers taking a certificate as a parameter while others use the metadata service.

Don't feel like you have to address my every concern - if my concerns lack merit or you just know what you want to do, we can just get on with it. 😅

[MasterKale]

1. verifyAttestationResponse is already very (overly?) complex. Every time I look at it, I feel an urge to try to break it down into smaller pieces, but I struggle to see clean interfaces. It's complex, but effective. I just feel a misgiving about making it more complex.

I dunno if it helps, but attestation response was written this way because the process of verifying these responses breaks down into two stages:

1. Fundamental response structure verification
2. If applicable, attestation statement verification

I decided to go for a single, tall method because the logic flow mirrors the spec; in terms of flow control there's not a lot more going on than, "if the response structure is bad, throw an error." I opted for more verbose code in case the spec changes later and I need to refactor a check here or add a check there. There's not a whole lot of code reuse by design, so that changes to verification in one part of the spec won't affect the other parts that haven't changed.

2. The verifier interfaces would diverge. If it's truly incidental that they are currently so closely aligned, this may not be a valid concern, but it seemed like a move in the wrong direction. What do you think?

...

I'm inferring from this that it is largely incidental that the verifier interfaces aligned so closely. When I was working on my first draft of this PR, I supposed that the series of if statements had grown over time without noticing that the verifier interfaces were all compatible with respect to the preconditions provided by verifyAttestationResponse.

My intuition was that there is some long term maintainability advantage in codifying expectation that the verifier interfaces should be compatible with the argument verifyAttestaionResponse is able to provide.

The verifier interfaces were defined to reduce as much duplicated parsing of responses as possible. If an attestation statement verifier needs a piece of data that was pulled out earlier during structure verification then I give it that value instead of having it re-parse the structure from an earlier form of it (for example authData is already decoded from the attestationObject so that gets passed in, instead of just throwing attestationObject at the verifier and having it decode authData back out again.)

To clarify, it's entirely coincidental that the verifier arguments are so similar. I purposefully avoided building a reusable API around these methods because the WebAuthn spec is scoped tightly, and I let that guide my architectural decisions. Future attestation formats would mean the addition of another else if branch to the bottom of the existing ones.

If we go with CertificateService, would you see this having methods like get and set and/or properties that align with the ATTESTATION_FORMAT enum values?

I'm glad you asked this, because that's how it'd work in my mind. CertificateService would be instantiated with the current default root certs (AppleWebAuthnRootCertificate, etc...) mapped to their corresponding ATTESTATION_FORMAT identifer. RP devs could override these root certs via a set method like this:

CertificateService.setCertificates("apple", [strRootCertPEM1, strRootCertPEM2]);

Then, inside verifyApple() (not as an additional argument) I'd change this:

const certPath = x5c.map(convertX509CertToPEM);
certPath.push(AppleWebAuthnRootCertificate);

To something like this to use the service's get method:

const certPath = [
  ...x5c.map(convertX509CertToPEM),
  ...CertificateService.getCertificates("apple"),
];

I think your suggestion would lend itself to my use case by adding a field the verifyApple (and the two android verifiers) parameters for caRootCertificate. I'm happy to do that with this PR if aren't worried about the concerns I've raise with the approach.

Does my explanation of a potential "CertificateService" and its API seem like it would enable you to implement AppAttest verification with the existing verifyApple()?

Don't feel like you have to address my every concern - if my concerns lack merit or you just know what you want to do, we can just get on with it. 😅

I am always open to (well-intentioned) feedback and suggestions like yours, and am not afraid to incorporate other's changes (as you know!) I am humbled and thank you for taking a continued interest in this project, and for spending the time to engage me in this conversation.

The only thing I can think to end this comment with is: my ongoing vision for this project (and therefore its scope) is to track the evolution of the WebAuthn API and make it easy for developers to implement it. I'm willing to fudge a bit here or there for the sake of easier implementation (like making it possible to pass in an array of expected origins or RP IDs instead of just single ones so one RP can service multiple websites), but I must push back on suggestions to augment and expand functionality outside of the confines of the API for the sake of this vision ✌️

[MasterKale]

On a related note, what's up with verifyAndroidKey not checking a root ca certificate? I read some of the related issues (e.g. fido-alliance/conformance-test-tools-resources#567), and I saw they were closed a while back.

I think part of the problem is I could never find the expected root certificate like I could for "apple" and "android-safetynet". Conformance passed without that check so I never felt huge pressure to go back and solve that, simple as that 😬

Tangentially, (sorry to hijack the conversation) is there a simple explanation why (IIUC) FIDOU2F does not require a root ca verification,

I've never understood "fido-u2f" as needing such verification. Conformance testing never checked for such verification either...

...packed uses root ca certificates from the metadata service

Conformance testing drove this too...

...and the others have a statically configured root ca certificate?

Unless MetadataService is in play (another bit of functionality added to pass FIDO conformance testing), these static root certs were observed first-hand as being needed for successful direct attestation verification of responses from actual hardware. The certs were long-lived enough that I thought it okay to hard-code them in. Your questions led me to rediscovering that GlobalSignRootCAR2 in verifyAndroidSafetyNet.ts expires this December so your PR prompting me to allow for certificate overrides is timely 😱

[jstewmon]

Thank you for explaining your philosophy on how the code is organized - I think I can better align this (and future) contributions without so much deliberation. 😄

Does my explanation of a potential "CertificateService" and its API seem like it would enable you to implement AppAttest verification with the existing verifyApple()? ... inside verifyApple() (not as an additional argument)

const certPath = [
  ...x5c.map(convertX509CertToPEM),
  ...CertificateService.getCertificates("apple"),
];

^ This version would not because it doesn't allow the logic inside verifyApple to be used with a format-specific root certificate. (The fmt of AppAttest is apple-appattest.)

The description from your original suggestion would:

the verification methods can remain functions, and perhaps verifyAttestation() can orchestrate passing in certs from CertificateService when it executes the individual verification methods for ease of testing these methods.

I thought from this description you had in mind adding a param property (e.g. caRootCertificates: string[]) to verifyAndroidKey, verifyAndroidSafetyNet and verifyApple such that verifyAttestationResponse could provide the value at the call site like caRootCertificates: CertificateService.getCertificates(fmt). This would allow those verify functions to be called any anyone with any root certificates.

[jstewmon]

On a related note, what's up with verifyAndroidKey not checking a root ca certificate? I read some of the related issues (e.g. fido-alliance/conformance-test-tools-resources#567), and I saw they were closed a while back.

I think part of the problem is I could never find the expected root certificate like I could for "apple" and "android-safetynet". Conformance passed without that check so I never felt huge pressure to go back and solve that, simple as that 😬

This is something I'm also interested in, and I think is related to this work. There are 3 "well known" Android root certificates - 2 of them mean the key is hardware secured. The other (hardcoded in Android source) means the statement could be spoofed, since the key was not a secret.

I see the first hardware certificate and the hardcoded-in-Android-source-code certificate in the metadata service blob, plus at least one other one, which might fall into the "not a play store device" category.

This is important to me because I want to only allow attestation responses that use one of the secure hardware root certificates.

I think the implication of this is that the CertificateService would need to return a list of certificate chains, any of which is considered valid. I wonder if other library users would want to accept a variety of the root certificates but also know which one was considered valid - to express that, the certificate service would need to support a label, which can be threaded back through the verifyAttestionResponse result.

Thoughts on this?

[MasterKale]

I thought from this description you had in mind adding a param property (e.g. caRootCertificates: string[]) to verifyAndroidKey, verifyAndroidSafetyNet and verifyApple such that verifyAttestationResponse could provide the value at the call site like caRootCertificates: CertificateService.getCertificates(fmt). This would allow those verify functions to be called any anyone with any root certificates.

I'm warming to this idea, and can't help but combine it with your idea to combine the interfaces of all of the verifier methods into a single blob of values from which the methods are free to pull what they need. This could open the door to an API for RP devs to register their own formats, like "apple-appattest", that would be considered when verifyAttestationResponse goes down the list of official formats.

But the blocker to that then becomes, how do I expose all of the server helper methods so that devs can create their own verifiers? And then my mind goes towards a new "@simplewebauthn/server-helpers" package and then I start to blanch because it's a level of complexity I don't really want to maintain right now. Not to mention these kinds of features seem more appropriate for a full-fat RP, not a "feature" library like this one.

Maybe there's an easy way to expose helpers as "@simplewebauthn/server/helpers" so there's not a fourth package to maintain? I've seen some projects use it but I've never tried for sublevel imports before. This is all I see when I try to import from helpers:

[jstewmon]

But the blocker to that then becomes, how do I expose all of the server helper methods so that devs can create their own verifiers? And then my mind goes towards a new "@simplewebauthn/server-helpers" package and then I start to blanch because it's a level of complexity I don't really want to maintain right now. Not to mention these kinds of features seem more appropriate for a full-fat RP, not a "feature" library like this one.

Maybe there's an easy way to expose helpers as "@simplewebauthn/server/helpers" so there's not a fourth package to maintain? I've seen some projects use it but I've never tried for sublevel imports before. This is all I see when I try to import from helpers:

It's not a functional problem for package consumers to import from @simplewebauthn/server/dist/... - you can see that's what I did in the snippet I posted in #124. But, it's undesirable b/c it implies building upon internals, which could be problematic b/c the package maintainer isn't considering those part of the public API when considering compatibility and versioning.

I'm aware of two options to express that helpers are part of the public API:

• Export them from the existing singular entry point (main). (.e.g in server/index.ts, import * as helpers from './helpers.ts'; export helpers;)
• Use "exports" in package.json (see Package entry points). This feature was added as experimental in Node 12 and became stable in Node 13, so it offers less compatibility than exporting from main. Support for exports is still forthcoming in some packages. For example, I had to use enhanced-resolve with jest when I upgraded to the current major version of the jose package (check out its package.json to see how this feature looks in practice). Also worth noting is that Typescript still has an open issue https://github.com/microsoft/TypeScript/issues/33079 for this feature, but if your exports match your file system layout, it just works (e.g. the jose package works fine with TS).

[MasterKale]

Hey @jstewmon, thanks for your patience.

• Export them from the existing singular entry point (main). (.e.g in server/index.ts, import * as helpers from './helpers.ts'; export helpers;)

Regarding exposing the helpers, I like this approach. Let's combine this with your proposal to unify the API of all the verifiers to take a single blob of data, and add a "caRootCertificates" argument instead of calling a "CertificateService" within each verifier.

Now I'm thinking about what CertificateService might look like. Maybe at server init you could do something like this to specify the certificates:

import { CertificateService } from '@simplewebauthn/server';

CertificateService.setCertificates("apple-appattest", [/* PEM certs here, loaded from the filesystem */]);

Specify an API for custom verifiers...

interface AttestationFmtVerifier = (opts: OptsBlob) => Promise<boolean>;

verifyAttestation(
  ...opts,
  customVerifiers: { [fmt: string]: AttestationFmtVerifier } = {}
);

Then within verifyAttestation():

} else if (fmt === ATTESTATION_FORMAT.NONE) {
  if (Object.keys(attStmt).length > 0) {
    throw new Error('None attestation had unexpected attestation statement');
  }
  // This is the weaker of the attestations, so there's nothing else to really check
  verified = true;
} else if (customVerifiers[fmt]) {
  const rootCerts: string[] = CertificateService.getCertificates(fmt);
  // In actuality `rootCerts` could be initialized as part of `optsBlob` earlier in the code
  verified = await customVerifiers[fmt]({ ...optsBlob, rootCerts });
} else {

What do you think?

[MasterKale]

(Also if you rebase you'll pull in the latest API docs changes, which should resolve the CI issues here)

[MasterKale]

@jstewmon It's been a while since this PR has seen activity. Since we last spoke I've merged in some work that addresses most of what we covered here:

• PR #138 introduced SettingsService to allow for setting root certificates per attestation statement format to use in cert path validation . It added path validation for attestation formats configured with root certs, and added and updated root certs for "android-key", "android-safetynet", "apple". And it also laid some groundwork for adding custom verifiers, though that functionality was not a part of this PR.
• PR #140 is a WIP to expose the various helper methods within @simplewebauthn/server for piecemeal use to enable devs to, for example, piece together an Apple App Attest verifier.

I'm going to close out this PR for now as most of what we covered in here has been captured in these PR's. If you want to pursue a "custom verifiers" feature I'd love to see what you can do with the latest version of the server codebase.