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: update handling of 304 responses #2698

Merged
merged 4 commits into from
Jun 30, 2023
Merged

Trusted Entitlements: update handling of 304 responses #2698

merged 4 commits into from
Jun 30, 2023

Conversation

NachoSoto
Copy link
Contributor

@NachoSoto NachoSoto commented Jun 23, 2023 •
edited
Loading

Changes:

  • ETagManager no longer stores the verification mode
  • Created new VerifiedHTTPResponse, which has the VerificationResult, extracted from HTTPResponse
  • Removed VerificationResult.from(cache:response:)
  • Changed SignatureVerificationHTTPClientTests to test using the real ETagManager, so they become more "end to end" unit tests. A lot of them were relying on implementation details of the mock instead of the real ETagManager.
  • Removed several HTTPClient tests that were checking a behavior that was impossible (for example, no verification result despite it being enabled), or that checked behavior based on the cached VerificationResult

Depends on #2679 and #2697.

@NachoSoto NachoSoto requested a review from a team June 23, 2023 00:07
@NachoSoto NachoSoto mentioned this pull request Jun 23, 2023
Merged
@NachoSoto NachoSoto marked this pull request as ready for review June 23, 2023 00:16
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from f3654ae to 90a95d8 Compare June 23, 2023 00:17
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch 2 times, most recently from 38c9d99 to fd49f15 Compare June 23, 2023 15:47
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from 90a95d8 to 12283d5 Compare June 23, 2023 15:48
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch from fd49f15 to 5b3a59d Compare June 23, 2023 21:33
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from 12283d5 to 5741e29 Compare June 23, 2023 21:33
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch from 5b3a59d to 83757b0 Compare June 26, 2023 20:36
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch 2 times, most recently from 4875780 to 6d5da7b Compare June 26, 2023 21:06
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch from 250270c to b0782ab Compare June 26, 2023 21:07
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from 6d5da7b to e9e2d8e 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/changed-etag-signatures-2 branch from e9e2d8e to 4d7e631 Compare June 27, 2023 14:31
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch 2 times, most recently from 88a3120 to 62010bf Compare June 28, 2023 22:52
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch 4 times, most recently from b4e4adc to dd41618 Compare June 29, 2023 16:12
NachoSoto
NachoSoto commented Jun 29, 2023
View reviewed changes
Sources/Networking/HTTPClient/HTTPResponse.swift
var responseHeaders: HTTPClient.ResponseHeaders
var body: Body
var requestDate: Date?
var verificationResult: VerificationResult
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 removed from HTTPResponse now, so it no longer defaults to .notRequested.

NachoSoto
NachoSoto commented Jun 29, 2023
View reviewed changes
Sources/Networking/HTTPClient/HTTPResponse.swift

/// Equivalent to `HTTPURLResponse.value(forHTTPHeaderField:)`
/// In keeping with the HTTP RFC, HTTP header field names are case-insensitive.
func value(forHeaderField field: 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.

A nice refactor here: this is now defined on the protocol HTTPResponseType so it's available for both response types.
Also no longer accepting any String, enforcing that we use this only with HTTPClient.ResponseHeaders and not HTTPClient.RequestHeaders

@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch 3 times, most recently from 1265476 to c101849 Compare June 29, 2023 18:11
@NachoSoto NachoSoto requested review from tonidero and a team June 29, 2023 18:57
NachoSoto
NachoSoto commented Jun 29, 2023
View reviewed changes
Sources/Security/VerificationResult.swift

/// - Returns: the most restrictive ``VerificationResult`` based on the cached verification and
/// the response verification.
static func from(cache cachedResult: Self, response responseResult: Self) -> Self {
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 gone 🎉

@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch from 62010bf to e654254 Compare June 29, 2023 19:02
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from fb1b4c9 to 7181dba Compare June 29, 2023 19:02
@NachoSoto
Copy link
Contributor Author

This is ready again 🎉

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.

Everything makes sense!

Sources/Networking/HTTPClient/HTTPResponse.swift

// MARK: - VerifiedHTTPResponse

struct VerifiedHTTPResponse<Body: HTTPResponseBody>: HTTPResponseType {
Copy link
Contributor

Choose a reason for hiding this comment

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

In Android, I'm still keeping only the HTTPResponse object (in android that's HTTPResult. In Android, that's created within the ETagManager, (which I admit, it's a weird responsibility to have there), but we don't need to default to NOT_REQUESTED.

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 kept this refactor cause it made things cleaner on iOS.

@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 force-pushed the nacho/changed-etag-signatures-2 branch from 7181dba to ba53303 Compare June 30, 2023 16:07
@NachoSoto NachoSoto force-pushed the nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping branch 4 times, most recently from 6d66630 to 4d8fd34 Compare June 30, 2023 18:10
Base automatically changed from nacho/sdk-3164-ios-dont-parse-nonce-for-offerings-and-entitlement-mapping to main June 30, 2023 18:50
@NachoSoto
Trusted Entitlements: update handling of 304 responses
8e78737 
- `ETagManager` no longer knows anything about signatures
- `HTTPClient` loads cached response from `ETagManager` first before verifying signature with the cached body
- Created new `VerifiedHTTPResponse`, which has the `VerificationResult`, extracted from `HTTPResponse`
- Removed `VerificationResult.from(cache:response:)`
- Updated `MockETagManager` to reflect behavior changed in #2666
- Removed `ETagManager` tests about verification since they're no longer relevant
- Removed several `HTTPClient` tests that were checking a behavior that was impossible (for example, no verification result despite it being enabled), or that checked behavior based on the cached `VerificationResult`
@NachoSoto
ETagManager: added support for storing result again
02f9fa3
@NachoSoto
Using real ETagManager
0bf3c2b
@NachoSoto
Added comment
1876819
@NachoSoto NachoSoto force-pushed the nacho/changed-etag-signatures-2 branch from ba53303 to 1876819 Compare June 30, 2023 19:10
@NachoSoto NachoSoto enabled auto-merge (squash) June 30, 2023 19:10
@NachoSoto NachoSoto disabled auto-merge June 30, 2023 19:10
@NachoSoto NachoSoto enabled auto-merge (squash) June 30, 2023 19:10
@NachoSoto NachoSoto merged commit 3f538fc into main Jun 30, 2023
@NachoSoto NachoSoto deleted the nacho/changed-etag-signatures-2 branch June 30, 2023 19:24
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