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

use Android keystore from Jenkins credentials #10078

Merged
merged 1 commit into from
Feb 27, 2020
Merged

Conversation

jakubgs
Copy link
Member

@jakubgs jakubgs commented Feb 25, 2020

To improve security of Keystore I'm moving it to Jenkins credentials store.
This way we won't have to store the Keystore on individual CI hosts, only the master-01.

@status-github-bot
Copy link

Pull Request Checklist

  • Docs: Updated the documentation, if affected
  • Docs: Added or updated inline comments explaining intention of the code
  • Tests: Ensured that all new UI elements have been assigned accessibility IDs
  • Tests: Signaled need for E2E tests with label, if applicable
  • Tests: Briefly described what was tested and what platforms were used
  • UI: In case of UI changes, ensured that UI matches Figma
  • UI: In case of UI changes, requested review from a Core UI designer
  • UI: In case of UI changes, included screenshots of implementation

@status-im-auto
Copy link
Member

status-im-auto commented Feb 25, 2020

Jenkins Builds

Click to see older builds (15)
Commit #️⃣ Finished (UTC) Duration Platform Result
bee2129 #1 2020-02-25 14:57:57 ~12 min android-e2e 📄log
✔️ bee2129 #1 2020-02-25 14:59:44 ~14 min ios 📦ipa
bee2129 #1 2020-02-25 15:08:50 ~23 min android 📄log
✔️ 5ebc53d #2 2020-02-25 15:30:08 ~9 min ios 📦ipa 📲
5ebc53d #2 2020-02-25 15:32:25 ~11 min android 📄log
5ebc53d #2 2020-02-25 15:32:29 ~12 min android-e2e 📄log
✔️ cc434bb #3 2020-02-25 15:59:05 ~11 min ios 📦ipa 📲
cc434bb #3 2020-02-25 16:00:11 ~12 min android 📄log
cc434bb #3 2020-02-25 16:00:11 ~12 min android-e2e 📄log
✔️ 1618d14 #4 2020-02-25 16:27:13 ~9 min ios 📦ipa 📲
1618d14 #4 2020-02-25 16:30:51 ~13 min android-e2e 📄log
1618d14 #4 2020-02-25 16:30:51 ~13 min android 📄log
✔️ 5d003d2 #5 2020-02-25 17:03:15 ~11 min ios 📦ipa 📲
5d003d2 #5 2020-02-25 17:04:48 ~12 min android-e2e 📄log
5d003d2 #5 2020-02-25 17:04:54 ~12 min android 📄log
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 9ff11dd #6 2020-02-25 17:35:08 ~9 min ios 📦ipa 📲
✔️ 9ff11dd #6 2020-02-25 17:36:24 ~11 min android-e2e 📦apk 📲
✔️ 9ff11dd #7 2020-02-25 17:43:26 ~18 min android 📦apk 📲
✔️ 64a214b #8 2020-02-27 05:49:44 ~13 min android 📦apk 📲
✔️ 64a214b #7 2020-02-27 05:49:44 ~14 min android-e2e 📦apk 📲
✔️ 64a214b #7 2020-02-27 05:50:10 ~14 min ios 📦ipa 📲

@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

Oh lovely, looks like it doesn't work:

java.lang.RuntimeException: java.lang.RuntimeException:
  com.android.ide.common.signing.KeytoolException:
  Failed to read key **** from store "****":
    Keystore was tampered with, or password was incorrect

Might be related to how Jenkins changes the permissions of the file.

@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

I tried doing chmod 640 on the keystore but that didn't work:

+ chmod 640 ****
...
java.lang.RuntimeException: java.lang.RuntimeException: com.android.ide.common.signing.KeytoolException: Failed to read key **** from store "****": Keystore was tampered with, or password was incorrect

@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

Whelp. I ran md5sum on the Keystore file produced by Jenkins from credentials and got:

[Pipeline] sh (hide)
16:51:12  + chmod 640 ****
[Pipeline] sh
16:51:12  + ls -l ****
16:51:12  -rw-r----- 1 jenkins jenkins 2159 Feb 25 15:51 ****
[Pipeline] sh
16:51:12  + md5sum ****
16:51:12  4000bd8b1f8b86f16e8688773e8ee88b  ****

But that's not a correct checksum. That means Jenkins modifies the contents when creating the file.

@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

I crated a test-file.txt credential of same type:
https://ci.status.im/credentials/store/system/domain/_/credential/test-file.txt/
Which simply contains text TEST and hash an MD5 sum of:

2debfdcf79f03e4a65a667d21ef9de14

And ran this test CI job to verify this:

pipeline {
    agent { label "linux" }
    
    stages {
        stage('Test') {
            steps {
                withCredentials([
                    file(
                        credentialsId: 'test-file.txt',
                        variable: 'TEST_FILE'
                    )]
                ) {
                    // contains text "TEST"
                    sh "ls -l ${env.TEST_FILE}"
                    sh "md5sum ${env.TEST_FILE}"
                    // expected sum
                    sh "echo TEST | md5sum"
                }
            }
        }
    }
}

And the result was correct:

+ ls -l ****
-r-------- 1 jenkins jenkins 5 Feb 25 16:10 ****
[Pipeline] sh
+ md5sum ****
2debfdcf79f03e4a65a667d21ef9de14  ****
[Pipeline] sh
+ echo TEST
+ md5sum
2debfdcf79f03e4a65a667d21ef9de14  -

https://ci.status.im/job/tests/job/secret-file-test/1/

So there must have been something wrong with how I uploaded the Keystore file. I guess.

@jakubgs jakubgs requested a review from a team as a code owner February 25, 2020 16:17
@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

Okay, scping the file again and re-uploading to Jenkins fixed the MD5 sum, but I'm still getting:

Failed to read key **** from store "****": Keystore was tampered with, or password was incorrect

So it must be something more than that.

@jakubgs jakubgs force-pushed the use-ci-cred-keystore branch 4 times, most recently from c7051bd to 9ff11dd Compare February 25, 2020 17:19
@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

It works! Somehow I managed to upload the wrong keystore twice. 3rd time's the charm.

@corpetty
Copy link
Contributor

Does this put extra security scrutiny on master-01? Is this a point of failure in case something goes wrong. What could go wrong?

@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

Does this put extra security scrutiny on master-01?

Not sure what you mean by that. Before the file was directly on the filesystem of 5 Linux CI hosts. Now it's encrypted in the credentials.xml file on master-01 which means it requires both access to the filesystem and admin access to Jenkins UI to extract it. And that's in ADDITION to the two passwords you need to unlock the keystore itself and then the specific key in the keystore.

What more do you want?

@jakubgs
Copy link
Member Author

jakubgs commented Feb 25, 2020

Is this a point of failure in case something goes wrong.

You mean as in we lose the host? Or what?
I have an encrypted version of the Keystore. If you want we can add it to infra-pass.

What could go wrong?

That's a very broad question. Not sure what to tell you. I explained how the credentials store works. You can check how extracting a value from it works here:
https://stackoverflow.com/a/37683492

@jakubgs
Copy link
Member Author

jakubgs commented Feb 27, 2020

Merging this as a partial fix. We can improve upon it in the future.

This way we don't have to store it on individual CI hosts.

Signed-off-by: Jakub Sokołowski <jakub@status.im>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants