Skip to content

Fix 403 apk v3 signatures#645

Merged
g-k merged 8 commits into
mainfrom
fix-403-apk-v3-signatures
Apr 6, 2021
Merged

Fix 403 apk v3 signatures#645
g-k merged 8 commits into
mainfrom
fix-403-apk-v3-signatures

Conversation

@g-k

@g-k g-k commented Mar 22, 2021

Copy link
Copy Markdown
Contributor

fix: #403

refs: https://jira.mozilla.com/browse/RELENG-9

Functional Tests:

  • force verify test in apk2_test.go to fail artificially and confirm verify output includes v3 format verification
=== CONT  TestSignFile/VerifyWithAPKSigner                                                                                                   
    apk2_test.go:126: error verifying apk signature: %!s(<nil>)                                                                              
        Verifies                                                                                                                             
        Verified using v1 scheme (JAR signing): true                                                                                         
        Verified using v2 scheme (APK Signature Scheme v2): true                                                                             
        Verified using v3 scheme (APK Signature Scheme v3): true                                                                             
        Number of signers: 1   

r? @ajvb

NB: I'm going to hold off on landing this until I get the go ahead from RelEng and Mobile

@g-k
g-k force-pushed the fix-403-apk-v3-signatures branch from 8b262cb to 4ca11f2 Compare March 23, 2021 19:56
@coveralls

coveralls commented Mar 23, 2021

Copy link
Copy Markdown

Pull Request Test Coverage Report for Build 5960

  • 24 of 25 (96.0%) changed or added relevant lines in 1 file are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.08%) to 69.829%

Changes Missing Coverage Covered Lines Changed/Added Lines %
signer/apk2/apk2.go 24 25 96.0%
Files with Coverage Reduction New Missed Lines %
database/connect.go 2 65.57%
Totals Coverage Status
Change from base Build 5955: 0.08%
Covered Lines: 3136
Relevant Lines: 4491

💛 - Coveralls

@g-k
g-k marked this pull request as ready for review March 23, 2021 20:59
Comment thread signer/apk2/apk2.go
defer os.Remove(tmpAPKFile.Name())
ioutil.WriteFile(tmpAPKFile.Name(), file, 0755)

apkSigCmd := exec.Command("java", "-jar", "/usr/bin/apksigner", "sign",

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.

/usr/bin/apksigner is a shell script. No idea why we were calling it as a jar file before.

@g-k
g-k requested a review from ajvb March 23, 2021 21:07
ajvb
ajvb previously approved these changes Mar 24, 2021

@ajvb ajvb 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.

lgtm

@g-k
g-k force-pushed the fix-403-apk-v3-signatures branch 8 times, most recently from 677b78b to 9a04b42 Compare April 5, 2021 14:41
Comment thread signer/apk2/apk2_test.go
)

func assertNewSignerWithConfOK(t *testing.T, conf signer.Configuration) *APK2Signer {
s, err := New(apk2signerconf)

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.

This was a bug with the old test code.

Comment thread signer/apk2/apk2.go
args = append(args, "--v3-signing-enabled", "true")
} else {
// apksigner signs with v3 if the minsdk version supports it
args = append(args, "--v3-signing-enabled", "false")

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.

Have to explicitly disable v3 signing after upgrading apksigner

@g-k

g-k commented Apr 5, 2021

Copy link
Copy Markdown
Contributor Author

Got feedback from Aki that the mobile team would like to enable v3 sigs gradually, so autograph needs to support signing with and without v3 sigs.

I added a v3enabled mode to the apk2 signer and integration tests.

I didn't add unit tests, since the signer doesn't have them and it'd require a larger change to have it take a mocked or dummy filesystem or ioutil implementation.

r? @ajvb

@g-k
g-k requested a review from ajvb April 5, 2021 14:47
@g-k
g-k force-pushed the fix-403-apk-v3-signatures branch from 9a04b42 to 010ca53 Compare April 5, 2021 21:24
@g-k
g-k force-pushed the fix-403-apk-v3-signatures branch from 010ca53 to 60ba5d4 Compare April 6, 2021 13:14

@ajvb ajvb 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 great

@g-k
g-k merged commit 97d86ed into main Apr 6, 2021
@g-k
g-k deleted the fix-403-apk-v3-signatures branch April 6, 2021 16:17
@g-k

g-k commented Apr 6, 2021

Copy link
Copy Markdown
Contributor Author

Thanks AJ!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

apk v3 support

3 participants