Skip to content

Trusted Entitlements: new Signature format#2679

Merged
NachoSoto merged 2 commits into
mainfrom
nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping
Jun 30, 2023
Merged

Trusted Entitlements: new Signature format#2679
NachoSoto merged 2 commits into
mainfrom
nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping

Conversation

@NachoSoto

@NachoSoto NachoSoto commented Jun 21, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

New format:

  • 32 bytes: intermediate public key
  • 4 bytes: Expiration (in days since epoch)
  • 64 bytes: intermediate public key signature, signed with the root private key
  • 16 bytes: salt
  • 64 bytes: payload signature:
    • salt
    • nonce (if present)
    • request time (as int string)
    • etag (if present)
    • payload

This also adds support for optional nonces for "static" signatures, which is required for #2667.

@NachoSoto NachoSoto requested a review from a team June 21, 2023 21:02
NachoSoto
NachoSoto commented Jun 22, 2023
View reviewed changes
Comment thread Sources/Security/Signing.swift Outdated
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch 10 times, most recently from af640da to 76c0c54 Compare June 22, 2023 22:12
@NachoSoto NachoSoto changed the title [WIP] Trusted Entitlements: new Signature format Trusted Entitlements: new Signature format Jun 22, 2023
@NachoSoto NachoSoto marked this pull request as ready for review June 22, 2023 22:12
NachoSoto
NachoSoto commented Jun 22, 2023
View reviewed changes
Comment thread Tests/UnitTests/Networking/SignatureVerificationHTTPClientTests.swift Outdated

@NachoSoto NachoSoto Jun 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The fix for this is quite long so it's coming in a separate PR.

This was referenced Jun 22, 2023
Merged
Merged
tonidero
tonidero reviewed Jun 23, 2023
View reviewed changes

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Should we hold off merging this to main until everything is ready so we don't break things in main? This looks good though!

Comment thread Sources/Security/Signing.swift Outdated

@tonidero tonidero Jun 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

So this will be the key used to verify the intermediate key correct? Should we rename it to make it clearer?

@NachoSoto NachoSoto Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I still need to figure out that part.

@NachoSoto NachoSoto Jun 26, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I left this here so tests pass, but in the last PR this will be reverted, since the actual public key hasn't changed.

Comment thread Sources/Security/Signing+ResponseVerification.swift Outdated
Comment thread Sources/Security/Signing.swift Outdated
Comment thread Sources/Security/Signing.swift Outdated

@tonidero tonidero Jun 23, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is nice! ❤️

@NachoSoto

NachoSoto commented Jun 23, 2023

Copy link
Copy Markdown
Contributor Author

Should we hold off merging this to main until everything is ready so we don't break things in main?

Yeah for sure 👍🏻
I'm trying to get integration tests pointing to the canary at least so we can start verifying those.

@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch 5 times, most recently from 250270c to b0782ab Compare June 26, 2023 21:07
@NachoSoto NachoSoto requested review from aboedo and bisho June 26, 2023 22:25
@NachoSoto NachoSoto mentioned this pull request Jun 26, 2023
Merged
tonidero
tonidero reviewed Jun 27, 2023
View reviewed changes
Comment thread Sources/Security/Signing+ResponseVerification.swift
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch from b0782ab to a11ae88 Compare June 27, 2023 14:29
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch 3 times, most recently from 62010bf to e654254 Compare June 29, 2023 19:02
tonidero
tonidero approved these changes Jun 30, 2023
View reviewed changes

@tonidero tonidero left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good!

Comment thread Sources/Security/Signing.swift
return false
}

// Fixme: verify public key

@tonidero tonidero Jun 30, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this still needed?

@NachoSoto NachoSoto Jun 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's done in #2715 :)

@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch from 9d3ee66 to 92b1d1c Compare June 30, 2023 16:06
@NachoSoto NachoSoto enabled auto-merge (squash) June 30, 2023 16:09
@NachoSoto NachoSoto disabled auto-merge June 30, 2023 16:09
@NachoSoto NachoSoto enabled auto-merge (squash) June 30, 2023 16:12
@NachoSoto NachoSoto disabled auto-merge June 30, 2023 16:12
@NachoSoto NachoSoto enabled auto-merge (squash) June 30, 2023 16:13
NachoSoto
NachoSoto commented Jun 30, 2023
View reviewed changes
Comment thread Tests/BackendIntegrationTests/SignatureVerificationIntegrationTests.swift Outdated

@NachoSoto NachoSoto Jun 30, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

⚠️ Disabling for now.

NachoSoto
NachoSoto commented Jun 30, 2023
View reviewed changes
Comment thread Tests/BackendIntegrationTests/BaseBackendIntegrationTests.swift Outdated

@NachoSoto NachoSoto Jun 30, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

⚠️ Disabling for now.

@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch 3 times, most recently from be175cc to 6d66630 Compare June 30, 2023 17:29
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch from 6d66630 to 4d8fd34 Compare June 30, 2023 18:10
@NachoSoto NachoSoto merged commit d457cb0 into main Jun 30, 2023
@NachoSoto NachoSoto deleted the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch June 30, 2023 18:50
NachoSoto added a commit that referenced this pull request Jun 30, 2023
@NachoSoto
Signing: extract and verify intermediate key (#2715)
e0ffea5 
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.
@RCGitBot RCGitBot mentioned this pull request Jul 7, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@tonidero tonidero tonidero approved these changes
@bisho bisho Awaiting requested review from bisho
@aboedo aboedo Awaiting requested review from aboedo

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@NachoSoto @tonidero