-
Notifications
You must be signed in to change notification settings - Fork 89
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
Implemented DeviceAttestation and DeviceAssertion validation. #837
Conversation
private const val CLIENT_ID_PIXEL7A = "ax8S6Z5dKhX7ywLxLx3ZzrVo" | ||
|
||
@OptIn(ExperimentalEncodingApi::class) | ||
val ATTESTATION_PIXEL7A = Base64.decode(""" |
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.
Can we add a comment indicating how to create these values, in case someone wants to add tests for other devices, or we need to refresh one of these for some reason?
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.
Done
val authData = authenticatorDataItem.value | ||
|
||
val signature = EcSignature.fromDerEncoded(256, signatureItem.value) |
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.
Other calls to fromDerEncoded make it clearer what this first parameter means (eg. EcCurve.P256.bitSize). Can we use a similar constant here, rather than a magic number?
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.
Yes, done.
val attestationDict = Cbor.decode(blob.toByteArray()) | ||
val attestationData = attestationDict["authData"].asBstr | ||
val credentialIdLength = attestationData.readInt16(53) |
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.
Add constants for these magic number offsets?
Same below...
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.
Added constants for flags/sizes/offsets.
|
||
// This certificate is from https://developer.android.com/training/articles/security-key-attestation | ||
// We really only care about the private key in this certificate. | ||
// Note that this certificate expires on May 24 2026 GMT. |
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.
That's close enough that it'll probably cause a problem in a year or so. Is there anything we can do to ensure this is always up to date? Or would that introduce other risks?
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.
Added TODO to create API to read the raw key from the cert. I think it should not be difficult, we do not need to use the key, only extract and compare it.
) { | ||
if (requireGmsAttestation) { | ||
// Google root certificate uses RSA private key (and not EC key that we currently support | ||
// in Kotlin Multiplatform code). Instead of comparing the keys, just replace the root |
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.
Are we creating more work for ourselves in the long run, with this? Is there a better solution?
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.
We should go back to comparing keys (once we have KMP API for that). If not there are a couple of somewhat hackish ways to fix it: e.g. we could create our own root certificate with EC key (that only we would trust) and then issue a certificate for the actual Google root key with expiration far in the future. The main downside is that this will confuse people.
Signed-off-by: Peter Sorotokin <[email protected]>
39a22ad
to
8ca184c
Compare
Implemented DeviceAttestation and DeviceAssertion for both iOS and Android including validation.
This includes:
Also tested manually on iOS, Pixel 6a, 7a, and Android emulator.