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

Trusted Entitlements: add support for signing request headers #3424

Merged
merged 7 commits into from
Dec 21, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Nov 16, 2023

This adds support for including arbitrary headers in the signature verification, therefore preventing tampering of them.

@NachoSoto NachoSoto added the refactor A code change that neither fixes a bug nor adds a feature label Nov 16, 2023
@NachoSoto NachoSoto requested a review from a team November 16, 2023 19:25
@NachoSoto NachoSoto changed the base branch from main to http-client-test-headers November 16, 2023 21:42
@NachoSoto NachoSoto force-pushed the trusted-entitlements-headers branch 2 times, most recently from 658c490 to b777a9f Compare November 16, 2023 21:47
@NachoSoto NachoSoto force-pushed the http-client-test-headers branch 2 times, most recently from 700fc54 to 50ad627 Compare November 16, 2023 23:07
@@ -148,6 +148,14 @@ extension HTTPClient {
}
}

static func headerParametersForSignatureHeader(with headers: RequestHeaders) -> RequestHeaders {
if let header = HTTPRequest.headerParametersForSignatureHeader(headers: headers) {
return [RequestHeader.headerParametersForSignature.rawValue: header]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is tested by snapshot tests.

Base automatically changed from http-client-test-headers to main November 17, 2023 20:19
NachoSoto added a commit that referenced this pull request Nov 17, 2023
This allows us to verify exactly what headers are included in all
requests.

It also simplifies the implementation for #3424.
@NachoSoto NachoSoto force-pushed the trusted-entitlements-headers branch 3 times, most recently from 9ec2f4b to 132dca7 Compare December 8, 2023 19:23
NachoSoto added a commit that referenced this pull request Dec 8, 2023
…er hash

Follow up to #3424. This adds coverage to ensure that the backend continues to sign correctly for old SDK versions that don't support this.
@NachoSoto NachoSoto changed the title [WIP] Trusted Entitlements: add support for signing request headers Trusted Entitlements: add support for signing request headers Dec 8, 2023
@NachoSoto NachoSoto marked this pull request as ready for review December 8, 2023 20:39
}

@available(iOS 13.0, macOS 10.15, tvOS 13.0, watchOS 6.2, *)
static func signingParameterHash(_ values: [String]) -> String {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now a shared method for both POST body and header parameters.

@@ -276,6 +283,7 @@ extension Signing.SignatureParameters {
nonce +
path +
postParameterHash +
headerParametersHash +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to unit tests I was able to get this right. Initially I had this before the post hash.

let response = """
{"request_date":"2023-12-08T19:17:04Z","request_date_ms":1702063024731,"subscriber":{"entitlements":{},"first_seen":"2023-12-08T19:13:02Z","last_seen":"2023-12-08T19:13:02Z","management_url":null,"non_subscriptions":{},"original_app_user_id":"$RCAnonymousID:6ca4535c42714f88abc99c563703f113","original_application_version":null,"original_purchase_date":null,"other_purchases":{},"subscriptions":{}}}\n
"""
let expectedSignature = "x2qnlHOl5WuzGi4TbSUVHxzlKELRCfrRYG9XAiso7ucZTQAAYEZqbguA3X0YfCJqCKh2hnTLSdEr4R+t23xBlTxceWZu2TJjK3461UJKpUnrwXDv+tYo2K54IoS3/tsEr3VmB5ppKAq0P2CR7SwbsDPpxUlHBcl5/4XJvb/DHOnTKjIVd4WJ+57LLWvIV9sDHnj9XxiBez+p5cEjez1RtUis0XdCfAFXU8XfAq6ggiEJKX4F"
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've generated these from the backend, so we can confirm that these real signatures are valid.

@NachoSoto NachoSoto force-pushed the trusted-entitlements-headers branch 2 times, most recently from 03c0f9f to ad9af4c Compare December 8, 2023 23:37
NachoSoto added a commit that referenced this pull request Dec 9, 2023
…er hash

Follow up to #3424. This adds coverage to ensure that the backend continues to sign correctly for old SDK versions that don't support this.
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 great!


extension HTTPRequest {

static func headerParametersForSignatureHeader(headers: Headers) -> String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm this name sounds weird to me... Maybe headerParametersHeaderForSigning? (To mimick postParametersHeaderForSigning

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 was trying to avoid the double "header" word in headerParametersHeaderForSigning, which I think reads kinda confusingly :/

Copy link
Contributor

@tonidero tonidero Dec 20, 2023

Choose a reason for hiding this comment

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

Hmm yeah... Not sure... maybe it's ok as it is. FFTI

self.path.supportsSignatureVerification,
let body = self.requestBody {
self.path.supportsSignatureVerification {
result += HTTPClient.headerParametersForSignatureHeader(with: defaultHeaders)
Copy link
Contributor

Choose a reason for hiding this comment

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

FFTI I was wondering whether we should pass the result headers instead of only the defaultHeaders to future-proof it in case we need to sign any of the other headers... This should work for now though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah if we need to that will get caught by tests, so we can change this to support that.

@NachoSoto NachoSoto force-pushed the trusted-entitlements-headers branch 2 times, most recently from 33a7047 to 26eb5bf Compare December 20, 2023 20:58
NachoSoto added a commit that referenced this pull request Dec 20, 2023
…er hash

Follow up to #3424. This adds coverage to ensure that the backend continues to sign correctly for old SDK versions that don't support this.
NachoSoto added a commit that referenced this pull request Dec 20, 2023
…er hash

Follow up to #3424. This adds coverage to ensure that the backend continues to sign correctly for old SDK versions that don't support this.
@NachoSoto
Copy link
Contributor Author

This should be ready, but the signature is still wrong on the load shedder

Copy link

codecov bot commented Dec 21, 2023

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (275e21b) 85.97% compared to head (730c8b5) 86.02%.
Report is 5 commits behind head on main.

Files Patch % Lines
Sources/Security/HTTPRequest+Signing.swift 95.55% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3424      +/-   ##
==========================================
+ Coverage   85.97%   86.02%   +0.05%     
==========================================
  Files         240      241       +1     
  Lines       17494    17552      +58     
==========================================
+ Hits        15040    15099      +59     
+ Misses       2454     2453       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@NachoSoto
Copy link
Contributor Author

Merging this! 🎉

@NachoSoto NachoSoto merged commit fb76e66 into main Dec 21, 2023
22 checks passed
@NachoSoto NachoSoto deleted the trusted-entitlements-headers branch December 21, 2023 19:53
@NachoSoto NachoSoto removed the blocked label Dec 21, 2023
NachoSoto added a commit that referenced this pull request Dec 21, 2023
…er hash

Follow up to #3424. This adds coverage to ensure that the backend continues to sign correctly for old SDK versions that don't support this.
NachoSoto added a commit that referenced this pull request Dec 21, 2023
…er hash (#3505)

Follow up to #3424. This adds coverage to ensure that the backend
continues to sign correctly for old SDK versions that don't support
this.
Copy link
Member

@aboedo aboedo left a comment

Choose a reason for hiding this comment

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

I'm late to the game but great work on this!

NachoSoto pushed a commit that referenced this pull request Dec 22, 2023
**This is an automatic release.**

### RevenueCatUI
* `Paywalls`: add header image to `watchOS` paywalls (#3542) via
NachoSoto (@NachoSoto)
* `Paywalls`: improve template 5 landscape layout (#3534) via NachoSoto
(@NachoSoto)
* `Paywalls`: fix template 5 footer loading view alignment (#3537) via
NachoSoto (@NachoSoto)
* `Paywalls`: improve template 1 landscape layout (#3532) via NachoSoto
(@NachoSoto)
* `Paywalls`: fix `ColorInformation.multiScheme` on `watchOS` (#3530)
via NachoSoto (@NachoSoto)
### Other Changes
* `Trusted Entitlements`: tests for signature verification without
header hash (#3505) via NachoSoto (@NachoSoto)
* `.debugRevenueCatOverlay`: added `Locale` (#3539) via NachoSoto
(@NachoSoto)
* `Trusted Entitlements`: add support for signing request headers
(#3424) via NachoSoto (@NachoSoto)
* `CI`: Add architecture to cache keys (#3538) via Mark Villacampa
(@MarkVillacampa)
* `Paywalls Tester`: remove double close button (#3531) via NachoSoto
(@NachoSoto)
* Fix `RevenueCatUI` snapshot tests (#3526) via NachoSoto (@NachoSoto)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
refactor A code change that neither fixes a bug nor adds a feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants