Trusted entitlements: Use new signature verification format by tonidero · Pull Request #1111 · RevenueCat/purchases-android

tonidero · 2023-06-29T10:32:53Z

Description

Third PR for SDK-3200

Adds support to the new signature format (salt + nonce + TS + etag + content)
Adds support for intermediate signatures verification
Makes nonce optional in preparation of static endpoint signing.

Based on #1109 and #1110.

codecov · 2023-06-29T10:43:06Z

Codecov Report

Merging #1111 (c5249fd) into main (e6e498c) will decrease coverage by 0.05%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##             main    #1111      +/-   ##
==========================================
- Coverage   85.16%   85.12%   -0.05%     
==========================================
  Files         186      186              
  Lines        6554     6568      +14     
  Branches      929      932       +3     
==========================================
+ Hits         5582     5591       +9     
- Misses        601      603       +2     
- Partials      371      374       +3

Impacted Files	Coverage Δ
...com/revenuecat/purchases/strings/NetworkStrings.kt	`0.00% <ø> (ø)`
...s/common/verification/SignatureVerificationMode.kt	`80.95% <66.66%> (-13.50%)`	⬇️
...at/purchases/common/verification/SigningManager.kt	`93.47% <89.28%> (-6.53%)`	⬇️
...java/com/revenuecat/purchases/common/HTTPClient.kt	`90.00% <100.00%> (+0.56%)`	⬆️

tonidero · 2023-06-29T10:33:28Z

Currently nonce will never be null if shouldSignResponse, but that will change with the new static endpoints we want to sign. Right now I can't add tests to this change due to that limitation.

A separate PR for that is fine 👍🏻

tonidero · 2023-06-29T10:44:02Z

I wasn't sure whether to add these, but I thought it might be useful if we need to debug things. Also, they are test keys, not real keys so I think it should be fine.

tonidero · 2023-06-29T10:46:09Z

Note that now we aren't using the real root key but a new test key. It was difficult to test using the real key since the current responses may become outdated once the intermediate key expires. Still plan to add some integration tests that test this against the backend.

Hmm how so? The expiration is also encoded in the signature.

Right but we don't have the real root private key in order to create a signed intermediate key with an expiration date far in the future. If not, we would need to be changing the signature every time the intermediate key expires I think? I guess maybe we could get a signature that expires far into the future but not sure if that would have any security concerns... Probably not but I will think about that a bit more

Oooh I see. I got around that by creating fake private and public root keys in some of these tests.

Right, that's what I'm doing here. I basically used test private/public keys to generate signatures with different values that I need. But we are not testing using the real production key as we were before... But I think it's fine if that's covered by integration tests that actually hit the backend.

tonidero · 2023-06-29T15:03:55Z

This was unused now so I removed it

tonidero · 2023-06-29T15:04:56Z

Marking it as ready for review but we need to hold this until the backend is deployed.

NachoSoto

Just a question about the double-layer architecture.

NachoSoto · 2023-06-29T15:13:45Z

A separate PR for that is fine 👍🏻

NachoSoto · 2023-06-29T15:14:35Z

I think it's slightly weird that this has both of these types. Why would IntermediateSignatureHelper not be inside SignatureVerifier?

Oh I see SigningManager is the type on top. But it seems like it only uses IntermediateSignatureHelper though?

Yeah, the root key is only used by the IntermediateSignatureHelper so we can probably just remove the signatureVerifier from here.

I removed the signatureVerifier field from the enum here: 9efce80. Lmk what you think!

NachoSoto · 2023-06-29T15:16:36Z

Hmm how so? The expiration is also encoded in the signature.

NachoSoto · 2023-06-29T15:17:41Z

tonidero · 2023-06-29T15:13:41Z

I'm not sure if this check is needed, but it makes sense to me.

Yeah I didn't do this but it makes sense.

tonidero · 2023-06-29T15:28:38Z

Right but we don't have the real root private key in order to create a signed intermediate key with an expiration date far in the future. If not, we would need to be changing the signature every time the intermediate key expires I think? I guess maybe we could get a signature that expires far into the future but not sure if that would have any security concerns... Probably not but I will think about that a bit more

NachoSoto · 2023-06-29T21:07:22Z

Yeah I didn't do this but it makes sense.

NachoSoto · 2023-06-29T21:07:33Z

tonidero · 2023-07-03T10:54:53Z

Merging into integration branch for now

### Description Third PR for SDK-3200 - Adds support to the new signature format (salt + nonce + TS + etag + content) - Adds support for intermediate signatures verification - Makes nonce optional in preparation of static endpoint signing. Based on #1109 and #1110.

### Description Integration branch for the changes in trusted entitlements. Includes changes from: - #1111 - #1114 - #1118 - #1119 - #1124

tonidero added the refactor label Jun 29, 2023

tonidero commented Jun 29, 2023

View reviewed changes

tonidero added the HOLD label Jun 29, 2023

tonidero commented Jun 29, 2023

View reviewed changes

tonidero marked this pull request as ready for review June 29, 2023 15:04

tonidero requested a review from a team June 29, 2023 15:05

NachoSoto reviewed Jun 29, 2023

View reviewed changes

tonidero commented Jun 29, 2023

View reviewed changes

NachoSoto approved these changes Jun 29, 2023

View reviewed changes

tonidero force-pushed the toniricodiez/sdk-3200-verify-intermediate-signature-android-2 branch from e2cd630 to 01e5594 Compare June 30, 2023 07:12

Base automatically changed from toniricodiez/sdk-3200-verify-intermediate-signature-android-2 to main June 30, 2023 07:21

tonidero added 6 commits June 30, 2023 09:24

Trusted entitlements: Use new signature verification format

bcd7739

Add comment specifying test keys used to generate signature

a6cbd9a

Add more tests

f530431

Add tests for signing with both etag and payload

3e83d5b

Whoops

4a12774

Remove uneeded field from verification mode enum

0533130

tonidero force-pushed the toniricodiez/sdk-3200-verify-intermediate-signature-android-3 branch from 9efce80 to 0533130 Compare June 30, 2023 07:24

tonidero added 2 commits June 30, 2023 09:26

Extract createIntermediateSignatureHelper

291fc74

Fix detekt

c5249fd

tonidero changed the base branch from main to new-trusted-entitlements-signature-format July 3, 2023 10:54

tonidero removed the HOLD label Jul 3, 2023

tonidero merged commit 15e79d8 into new-trusted-entitlements-signature-format Jul 3, 2023

tonidero deleted the toniricodiez/sdk-3200-verify-intermediate-signature-android-3 branch July 3, 2023 10:55

tonidero mentioned this pull request Jul 3, 2023

Trusted entitlements: New trusted entitlements signature format #1117

Merged

tonidero added a commit that referenced this pull request Jul 7, 2023

Trusted entitlements: New trusted entitlements signature format (#1117)

e8a90a0

### Description Integration branch for the changes in trusted entitlements. Includes changes from: - #1111 - #1114 - #1118 - #1119 - #1124

Uh oh!

Conversation

tonidero commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codecov Bot commented Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tonidero Jun 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented Jun 29, 2023

Uh oh!

NachoSoto left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tonidero commented Jul 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

tonidero commented Jun 29, 2023 •

edited

Loading

codecov Bot commented Jun 29, 2023 •

edited

Loading

tonidero Jun 29, 2023 •

edited

Loading