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.

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

Signing: extract and verify intermediate key #2715

Merged
merged 3 commits into from
Jun 30, 2023

Conversation

NachoSoto
Copy link
Contributor

Last step of the new Signature format. Follow up to #2679 and #2698.

This reverts the public key change in #2679, since that was the intermediate key.
This now extracts the new intermediate public key from the signature, and verifies it using the public key.

@NachoSoto NachoSoto requested review from bisho and a team June 26, 2023 22:28
data: intermediatePublicKey))

do {
return try Self.createPublicKey(with: intermediatePublicKey)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We considered caching this, but I just ran a benchmark on my device and the whole process (creating this key and verifying the original signature) takes 0.17 seconds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I meant 0.17ms!

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Love this! Going to try to get this done in Android ASAP

}

guard let expirationDate = Self.extractAndVerifyIntermediateKeyExpiration(intermediateKeyExpiration) else {
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

So if the intermediate key is expired, we assume it's an attacker correct? As long as the backend makes sure this doesn't happen, this should be fine 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup /cc @bisho

Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Love this! Going to try to get this done in Android ASAP

@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from e9e2d8e to 4d7e631 Compare June 27, 2023 14:31
@NachoSoto NachoSoto force-pushed the nacho/verify-intermediate-key branch from 0d1bd54 to 7aa47ba Compare June 27, 2023 14:32
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch 3 times, most recently from b4e4adc to dd41618 Compare June 29, 2023 16:12
@NachoSoto NachoSoto force-pushed the nacho/verify-intermediate-key branch from 7aa47ba to 4dc2d8a Compare June 29, 2023 16:28
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch 7 times, most recently from fb1b4c9 to 7181dba Compare June 29, 2023 19:02
@NachoSoto NachoSoto force-pushed the nacho/verify-intermediate-key branch 2 times, most recently from 54b4e49 to 69a2415 Compare June 29, 2023 21:14
@NachoSoto NachoSoto requested a review from tonidero June 29, 2023 21:14
Copy link
Contributor

@tonidero tonidero left a comment

Choose a reason for hiding this comment

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

Looks good!

@tonidero
Copy link
Contributor

Some tests seem to be failing though

@NachoSoto
Copy link
Contributor Author

Some tests seem to be failing though

Yeah that's testVerifyKnownSignatureWithNoNonceAndNoEtag because I need a signature signed with the real private root key, so I'm waiting for offerings to have static signatures.

@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from ba53303 to 1876819 Compare June 30, 2023 19:10
Base automatically changed from nacho/changed-etag-signatures-2 to main June 30, 2023 19:24
@NachoSoto NachoSoto force-pushed the nacho/verify-intermediate-key branch from 69a2415 to fcbc752 Compare June 30, 2023 19:29
@NachoSoto NachoSoto force-pushed the nacho/verify-intermediate-key branch from fcbc752 to bef3bef Compare June 30, 2023 19:30
@NachoSoto NachoSoto enabled auto-merge (squash) June 30, 2023 19:31
@NachoSoto NachoSoto merged commit e0ffea5 into main Jun 30, 2023
@NachoSoto NachoSoto deleted the nacho/verify-intermediate-key branch June 30, 2023 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants