ebourg / jsign

Java implementation of Microsoft Authenticode for signing Windows executables, installers & scripts
https://ebourg.github.io/jsign
Apache License 2.0
250 stars 107 forks source link

Access Denied for jsign when using from Github Action Role #228

Closed hongkongkiwi closed 2 weeks ago

hongkongkiwi commented 3 weeks ago

There's something a bit strange about the way that jsign uses AWS credentials.

I'm using it in a Github Actions role where I assume a role using this:

      - name: Configure AWS credentials
        id: configure-aws
        uses: aws-actions/configure-aws-credentials@v4
        with:
          aws-region: us-east-1
          role-to-assume: ${{ vars.myRole }}
          role-session-name: GitHubActions
          unset-current-credentials: true

After this, I can use any of the aws cli commands just fine e.g.

      - name: get caller identity
        run: |
          aws sts get-caller-identity
          aws kms get-public-key --key-id "<MY_KEY_ID>"

However, when I pass my creds to jsign it gives AccessDenied, now this same command works fine locally (in this case I'm not assuming a role, just using an access key and secret and then using aws sts get-session-token

The github actions should already have a session token and temporary credentials from the previous step and shouldn't actually need me to do anything else.

Any aws command, or any tool which implements the AWS sdk authentication methods natively just works fine. So that's what makes me think there's just something a little bit unusual about the way that jsign is doing this.

Any ideas?

hongkongkiwi commented 3 weeks ago

Here's more info on the actual error, although it doesn't add much.

It seems like it's a misconfiguration, but the fact is that the key can be used just fine using the aws cli commands and same credentials one step earlier is strange.

All that's happening here is consolidating those credentials into the JSON_AWS_CREDS var and passing that (you can see a printout of the var there, but obviously the creds are starred).

failed to load signer "signer #1"
java.lang.RuntimeException: java.security.KeyStoreException: java.io.IOException: AccessDeniedException: null
    at net.jsign.jca.JsignJcaProvider$JsignJcaKeyStore.engineAliases(JsignJcaProvider.java:150)

> Task :app:signApk FAILED
JSIGN_AWS_CREDS: ***|***|***
    at net.jsign.jca.AbstractKeyStoreSpi.engineContainsAlias(AbstractKeyStoreSpi.java:68)
    at net.jsign.jca.AbstractKeyStoreSpi.engineIsKeyEntry(AbstractKeyStoreSpi.java:84)
    at java.base/java.security.KeyStore.isKeyEntry(KeyStore.java:1346)
    at com.android.apksigner.SignerParams.loadPrivateKeyAndCertsFromKeyStore(SignerParams.java:304)
    at com.android.apksigner.SignerParams.loadPrivateKeyAndCerts(SignerParams.java:202)
    at com.android.apksigner.ApkSignerTool.getSignerConfig(ApkSignerTool.java:438)
    at com.android.apksigner.ApkSignerTool.sign(ApkSignerTool.java:353)
36 actionable tasks: 36 executed
    at com.android.apksigner.ApkSignerTool.main(ApkSignerTool.java:92)
Caused by: java.security.KeyStoreException: java.io.IOException: AccessDeniedException: null
    at net.jsign.jca.AmazonSigningService.aliases(AmazonSigningService.java:144)
    at net.jsign.jca.SigningServiceKeyStore.engineAliases(SigningServiceKeyStore.java:54)
    at java.base/java.security.KeyStore.aliases(KeyStore.java:1287)
    at net.jsign.jca.JsignJcaProvider$JsignJcaKeyStore.engineAliases(JsignJcaProvider.java:148)
    ... 8 more
Caused by: java.io.IOException: AccessDeniedException: null
    at net.jsign.jca.RESTClient.query(RESTClient.java:159)
    at net.jsign.jca.RESTClient.post(RESTClient.java:77)
    at net.jsign.jca.AmazonSigningService.query(AmazonSigningService.java:228)
    at net.jsign.jca.AmazonSigningService.aliases(AmazonSigningService.java:138)
    ... 11 more
hongkongkiwi commented 3 weeks ago

Ah, I found the issue. jsign requires kms:ListKeys permissions on the AWS account, which is a little bit unusual since I'm explicitly passing in the key-id as the keystore alias, so there should be no need to iterate through all keys on my account.

Why does it do this?

ebourg commented 3 weeks ago

For Authenticode signing the kms:ListKeys permission is only required if the alias is not specified. But if you use apksigner it's likely that the aliases are enumerated at some point, even if that's not required for signing. jarsigner has a similar behaviour.

ebourg commented 3 weeks ago

Looking at your stacktrace:

at net.jsign.jca.AbstractKeyStoreSpi.engineContainsAlias(AbstractKeyStoreSpi.java:68)
at net.jsign.jca.AbstractKeyStoreSpi.engineIsKeyEntry(AbstractKeyStoreSpi.java:84)
at java.base/java.security.KeyStore.isKeyEntry(KeyStore.java:1346)
at com.android.apksigner.SignerParams.loadPrivateKeyAndCertsFromKeyStore(SignerParams.java:304)
at com.android.apksigner.SignerParams.loadPrivateKeyAndCerts(SignerParams.java:202)
at com.android.apksigner.ApkSignerTool.getSignerConfig(ApkSignerTool.java:438)
at com.android.apksigner.ApkSignerTool.sign(ApkSignerTool.java:353)

SignerParams.loadPrivateKeyAndCertsFromKeyStore() calls KeyStore.isKeyEntry(), I assume to check if the key exists, which necessarily translates into an extra AWS API call, either kms:ListKeys as currently implemented, or kms:DescribeKey.

hongkongkiwi commented 3 weeks ago

Is it necessary to even check if the key exists with an API call? Since isKeyEntry is a standard method, could we simply have it return yes with the key-id that was passed in already without making an API call.

When we attempt to use it for an operation (say Describe, Sign etc), it will return an error then if it doesn't exist.

ebourg commented 3 weeks ago

Is it necessary to even check if the key exists?

No, but that's what apksigner does. I'm not sure it would be wise to make the keystore implemented by Jsign to lie when asked if a key exists.

hongkongkiwi commented 2 weeks ago

I'm not super happy with this behaviour, I think it's a security issue allowing something to iterate through all keys in the whole AWS account for no specific reason (except to find a key with name that's passed).

I understand though that you have no plans to change this behaviour as it emulates how normal keystores work together with apksigner, so i'll close the issue.