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.

Sign up for GitHub

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

Jump to bottom

Trusted Entitlements: new Signature format #2679

Merged
merged 2 commits into from
Jun 30, 2023
Merged

Trusted Entitlements: new Signature format #2679

merged 2 commits into from
Jun 30, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Jun 21, 2023 •
edited
Loading

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 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
Tests/UnitTests/Networking/SignatureVerificationHTTPClientTests.swift
@@ -216,6 +216,8 @@ final class InformationalSignatureVerificationHTTPClientTests: BaseSignatureVeri
}

func testValidSignatureWithETagResponse() throws {
XCTExpectFailure("Not yet implemented")
Copy link
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
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.

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

Sources/Security/Signing.swift Outdated
@@ -103,9 +120,8 @@ enum Signing: SigningType {

// MARK: -

private static let publicKey = "UC1upXWg5QVmyOSwozp755xLqquBKjjU+di6U8QhMlM="
private static let publicKey = "nVoKJjLhhTNo19Mkjr5DEmgMf361HWxxMyctC10Ob7c="
Copy link
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?

Copy link
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.

Copy link
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.

Sources/Security/Signing+ResponseVerification.swift Show resolved Hide resolved
Sources/Security/Signing.swift Outdated Show resolved Hide resolved
Sources/Security/Signing.swift
)
}

}

// MARK: - Private

extension Signing {

enum SignatureComponent: CaseIterable, Comparable {
Copy link
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
Copy link
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 bisho and aboedo June 26, 2023 22:25
@NachoSoto NachoSoto mentioned this pull request Jun 26, 2023
Merged
@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
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!

Sources/Security/Signing.swift
return false
}

// Fixme: verify public key
Copy link
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?

Copy link
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
Tests/BackendIntegrationTests/SignatureVerificationIntegrationTests.swift Outdated
@@ -25,6 +25,8 @@ class BaseSignatureVerificationIntegrationTests: BaseStoreKitIntegrationTests {
fileprivate var invalidSignature: Bool = false

override func setUp() async throws {
throw XCTSkip("Tests disabled until backend deploys new signature format")
Copy link
Contributor Author

@NachoSoto NachoSoto Jun 30, 2023 •
edited
Loading

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
Tests/BackendIntegrationTests/BaseBackendIntegrationTests.swift
return .enforced(Signing.loadPublicKey())
// Disabled until the backend deploys the new signature format
// return .enforced(Signing.loadPublicKey())
return .disabled
Copy link
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