[BACK-2165] [BACK-2164] [BACK-2163] Add app attestation and assertion api documentation. #58

lostlevels · 2023-01-26T21:14:00Z

This adds attestation and assertion documentation. Attestation is the verifying an app is a valid instance of an iOS app and assertion is requesting a secret of some kind after attestation is verified.

I was debating whether to split this into its own reference/attestation.v1.yaml file or not as it's not technically auth I don't think but it is part of the auth service to avoid creating a new service, but decided for simplicity to define the routes in reference/auth.v1.yaml.

tjotala · 2023-02-21T20:53:30Z

reference/common/models/base64.v1.yaml

@@ -0,0 +1,4 @@
+title: Base64
+type: string


We could use the format value to indicate base64 encoding of the string type: https://swagger.io/docs/specification/data-models/data-types/ - but I like the explicit pattern

We could use the format value to indicate base64 encoding of the string type: https://swagger.io/docs/specification/data-models/data-types/ - but I like the explicit pattern

I'm not against the explicit pattern at all, but I would like to see a $ref to his used in more places, so that I don't have to squint at the regexp in so many different places and try to double-check that there wasn't a cut and paste error made. :)

tjotala

LGTM!

This adds attestation and assertion documentation. Attestation is the verifying an app is a valid instance of an iOS app and assertion is requesting a secret of some kind after attestation is verified, in this case X.509 certificates that can be used for client authentication. Add response to successful assertion.

reference/auth/models/assertionverify.v1.yaml

reference/auth/models/attestationverify.v1.yaml

ewollesen

Mostly looks good, just a typo or two.

Most of my suggestions can be taken or left.

ewollesen · 2024-03-19T19:48:30Z

reference/auth.v1.yaml

@@ -998,3 +1094,21 @@ components:
                required:
                  - code
                  - reason
+    AppChallenge:
+      description: 'Challenge generated by server and which client should use in later operations.'


Consider a re-wording?

Suggested change

description: 'Challenge generated by server and which client should use in later operations.'

description: A server-generated challenge proving a client application's identity to Tidepool.

ewollesen · 2024-03-19T19:51:34Z

reference/auth/models/assertionverify.v1.yaml

+      partner:
+        description: Code name of partner to retrieve certificate from.
+        type: string
+        minLength: 1


If this is an enum, then minLength is not necessary, and can only confuse things, right? I suggest removing this.

Suggested change

minLength: 1

ewollesen · 2024-03-19T19:56:49Z

reference/auth/models/assertionverify.v1.yaml

+    type: object
+    properties:
+      challenge:
+        description: The previously returned assertion challenge.


This is what a client receives after posting to /v1/(assertions|attestations)/challenges right?

The body of that call is a JSON object, but this property is defined as a string, so probably that means it's base64 encoded? If so, that should be called out here.

Or does the endpoint expect the original JSON object supplied, in which case this isn't a strong really at all, but rather an object as defined in ./auth/models/appchallenge.v1.yaml.

Or if it's a string, but not base64 encoded, well, that'd be bad, it should be base64 encoded, else you run into a lot of quote-escaping issues and things get really ugly.

ewollesen · 2024-03-19T19:57:56Z

reference/auth/models/assertionverify.v1.yaml

+    $ref: './keyid.v1.yaml'
+    description: Base64 encoded key Id received from Apple App Attest API.
+required:
+  - attestation


Suggested change

- attestation

- assertion

Based on the filename and line 5 of this file, I assume this was a typo?

ewollesen · 2024-03-19T19:59:45Z

reference/auth/models/attestationverify.v1.yaml

+    pattern: '^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$'
+    description: Base64 encoded data received from Apple App Attest API. User must base64 encode the binary data received from Apple.
+  challenge:
+    type: string


See my comment on the challenge field of assertionverify.v1.yaml.

ewollesen · 2024-03-19T20:13:52Z

reference/auth/models/newappchallenge.v1.yaml

@@ -0,0 +1,10 @@
+title: New App Challenge


nit: I wouldn't bother putting "new" in the filename or title... It isn't important, and can only confuse people (was there an older one?)

ewollesen · 2024-03-19T20:15:24Z

reference/auth/models/newappchallenge.v1.yaml

+  keyId:
+    type: string
+    pattern: '^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$'
+    description: Base64 encoded key Id received from Apple App Attest API.


Suggested change

keyId:

type: string

pattern: '^(?:[A-Za-z0-9+/]{4})*(?:[A-Za-z0-9+/]{2}==|[A-Za-z0-9+/]{3}=)?$'

description: Base64 encoded key Id received from Apple App Attest API.

keyId:

$ref: './keyid.v1.yaml'

ewollesen · 2024-03-19T20:18:43Z

reference/auth/models/palmtreerequest.v1.yaml

+type: object
+properties:
+  csr:
+    description: Base64 encoded certificate signing request.


If the details matter... The Coastal request data indicates that it is "base64 encoded DER certificates".

Here, it says only "Base64 encoded"... Is it like Coastal, in that it's base64 DER, or rather, is it PEM (which is also base64 encoded, but with a slightly alternative structure).

Not a big deal to call it out, but you could save someone a bit of head scratching.

ewollesen · 2024-03-19T20:20:11Z

reference/auth/models/palmtreeresponse.v1.yaml

+    properties:
+      body:
+        type: string
+        description: PEM encoded certificate.


In another comment, I mentioned a difference between "base64 encoded", "base64 encoded DER" and now it gets even weirder as we add "PEM" to mix.

So I urge you be consistent and exact in these terms to save people headaches later.

ewollesen · 2024-03-19T20:21:59Z

reference/common/models/base64.v1.yaml

@@ -0,0 +1,4 @@
+title: Base64
+type: string


We could use the format value to indicate base64 encoding of the string type: https://swagger.io/docs/specification/data-models/data-types/ - but I like the explicit pattern

I'm not against the explicit pattern at all, but I would like to see a $ref to his used in more places, so that I don't have to squint at the regexp in so many different places and try to double-check that there wasn't a cut and paste error made. :)

tjotala reviewed Feb 21, 2023

View reviewed changes

tjotala previously approved these changes Feb 21, 2023

View reviewed changes

lostlevels dismissed tjotala’s stale review via 6407a5e July 24, 2023 20:02

lostlevels force-pushed the BACK-2165-app-attestation branch from 6407a5e to fae9ab7 Compare July 24, 2023 20:05

lostlevels force-pushed the BACK-2165-app-attestation branch from fae9ab7 to f500172 Compare July 24, 2023 20:05

lostlevels added 4 commits July 24, 2023 13:10

Fix typo.

32c274b

Update descriptions.

952bc61

Update partner body fields.

548cbce

format

eb24cf9

tjotala reviewed Aug 9, 2023

View reviewed changes

reference/auth/models/assertionverify.v1.yaml Show resolved Hide resolved

tjotala reviewed Aug 9, 2023

View reviewed changes

reference/auth/models/assertionverify.v1.yaml Show resolved Hide resolved

tjotala reviewed Aug 9, 2023

View reviewed changes

reference/auth/models/attestationverify.v1.yaml Show resolved Hide resolved

lostlevels added 4 commits August 31, 2023 11:20

Update api for palmtree.

fb66302

Fix typos.

00c3f30

Remove duplicate property.

f5ca75c

Fix palmtree payload.

b1fa3a1

lostlevels mentioned this pull request Sep 6, 2023

[BACK-2637] palmtree api #90

Open

ewollesen requested changes Mar 19, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BACK-2165] [BACK-2164] [BACK-2163] Add app attestation and assertion api documentation. #58

[BACK-2165] [BACK-2164] [BACK-2163] Add app attestation and assertion api documentation. #58

lostlevels commented Jan 26, 2023

tjotala Feb 21, 2023

ewollesen Mar 19, 2024

tjotala left a comment

ewollesen left a comment

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

ewollesen Mar 19, 2024

	description: 'Challenge generated by server and which client should use in later operations.'
	description: A server-generated challenge proving a client application's identity to Tidepool.

[BACK-2165] [BACK-2164] [BACK-2163] Add app attestation and assertion api documentation. #58

Are you sure you want to change the base?

[BACK-2165] [BACK-2164] [BACK-2163] Add app attestation and assertion api documentation. #58

Conversation

lostlevels commented Jan 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjotala left a comment

Choose a reason for hiding this comment

ewollesen 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