actions / create-github-app-token

GitHub Action for creating a GitHub App Installation Access Token
https://github.com/marketplace/actions/create-github-app-token
MIT License
351 stars 49 forks source link

Research learnings and ideas from similar GitHub Actions #42

Open gr2m opened 1 year ago

gr2m commented 1 year ago

Follow up to https://github.com/actions/create-github-app-token/discussions/39#discussioncomment-6880255.

In preparation for #3 and #4 it might make sense to learn from similar actions.

gr2m commented 1 year ago

I looked through them again and two things that stood out for me

There is also a lot to learn in terms of documentation, in particular how to register a GitHub App.

Moser-ss commented 10 months ago

I was trying to migrate from the action peter-murray/workflow-application-token-action to this one, and I noticed this action does not support the private-key input as a Base64 encoded string.

Does it make sense to create an issue for that? I don't have any problem opening a PR to support that use case.

Also, an issue we have with the action peter-murray/workflow-application-token-action that didn't have any internal retry mechanism; maybe we can improve that in this action.

I will be happy to start working in some PRs to improve this action

gr2m commented 10 months ago

private-key input as a Base64 encoded string

what is the use case? Maybe the decoding a base64 string could be done in a separate step before using this action?

didn't have any internal retry mechanism

For retries, @octokit has the retry plugin, it's rather heavy though, due to its dependency on bottleneck. We try to keep the code of this action, including its dependencies, to a minimum. It might make sense to implement a custom retry in this case particular case

Moser-ss commented 10 months ago

So joining these two use cases, I have experienced failures getting the token from a GitHub app, so we start to use an extra action to retry Wandalen/wretry.action, then we have the issue with the input that is broken with multiline strings like the format of the PEM files, so we had to do encode the private key with base64.

Also, it is annoying to keep copying and pasting the ugly syntax of an action that retries another action. So, if this action itself has the internal retry, I don't even need the base64 encoding.

So, does it make sense to implement a retry logic inside the action?

gr2m commented 10 months ago

the input that is broken with multiline strings like the format of the PEM files, so we had to do encode the private key with base64

would replacing line breaks with "\n" work? That's what we do in environments where multi-line environment variables are not supported

if this action itself has the internal retry, I don't even need the base64 encoding

Perfect 👍🏼

I agree we should implement a retry, could you please file a separate issue for it?

newbloke82 commented 10 months ago

I would like to try and help implement the proxy support. Getting this working from self-hosted runners behind a corporate proxy is important for my project. I'm interested in changing the behaviour of the app to use GITHUB_API_URL as a default but allow for an override to deal with the use case of calling GHES from GHEC. This would also support GHES to GHES requests. This is useful for services like renovatebot/dependabot that need to lookup dependencies in private repos: https://docs.github.com/en/code-security/dependabot/working-with-dependabot/configuring-access-to-private-registries-for-dependabot https://docs.renovatebot.com/getting-started/private-packages/

gr2m commented 10 months ago

@newbloke82 makes sense, thank you for laying it out! Can you please first file an issue? I think tibdex/github-app-token is implementing proxy support the way you suggest, right? See https://github.com/tibdex/github-app-token/blob/3eb77c7243b85c65e84acfa93fdbac02fb6bd532/action.yml#L8-L10

vleon1a commented 2 months ago

private-key input as a Base64 encoded string

what is the use case? Maybe the decoding a base64 string could be done in a separate step before using this action?

In my case we migrate from an action expecting the private key to be Base64 encoded. One solution is to create a new secret with the plain private key, but i just tried to test how we could decode it on the fly. It does work, but it's quite the overhead in my opinion, especially to ensure the masking is done and multiline storage is working fine:

- name: Decode private key
  id: decoding
  run: |
    private_key=$(echo "${{ secrets.SEMANTIC_RELEASE_PRIVATE_KEY }}" | base64 -d) &> /dev/null
    while read -r line;
    do
      echo "::add-mask::${line}"
    done <<< "$private_key"
    echo 'private-key<<EOF' >> "$GITHUB_OUTPUT"
    echo "$private_key" >> "$GITHUB_OUTPUT"
    echo 'EOF' >> "$GITHUB_OUTPUT"
- name: Generate GitHub App Token
  id: generate-token
  uses: actions/create-github-app-token@31c86eb3b33c9b601a1f60f98dcbfd1d70f379b4 # v1.10.3
  with:
    app-id: ${{ secrets.SEMANTIC_RELEASE_APP_ID }}
    private-key: ${{ steps.decoding.outputs.private-key }}

The output is a bit ugly, but at least it's concealed:

Run actions/create-github-app-token@31c86eb3b33c9b601a1f60f98dcbfd1d70f379b4
  with:
    app-id: ***
    private-key: ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
  ***
    github-api-url: https://api.github.com/
owner and repositories not set, creating token for the current repository ("my-repo")

would replacing line breaks with "\n" work? That's what we do in environments where multi-line environment variables are not supported

This works also, and is more clean than the previous one:

- name: Decode private key
  id: decoding
  run: |
    private_key=$(echo "${{ secrets.SEMANTIC_RELEASE_PRIVATE_KEY }}" | base64 -d | awk 'BEGIN {ORS="\\n"} {print}' | head -c -2) &> /dev/null
    echo "::add-mask::$private_key"
    echo "private-key=$private_key" >> "$GITHUB_OUTPUT"
- name: Generate GitHub App Token
  id: generate-token
  uses: actions/create-github-app-token@31c86eb3b33c9b601a1f60f98dcbfd1d70f379b4 # v1.10.3
  with:
    app-id: ${{ secrets.SEMANTIC_RELEASE_APP_ID }}
    private-key: ${{ steps.decoding.outputs.private-key }}

If the action is not planned to handle the private key being potentially encoded in Base64, I think we should add this in the readme, to avoid people leaking their private keys (I can open a PR)

gr2m commented 2 months ago

If the action is not planned to handle the private key being potentially encoded in Base64, I think we should add this in the readme, to avoid people leaking their private keys (I can open a PR)

that's a good idea and thank you for offering. Let's add a note here: https://github.com/actions/create-github-app-token?tab=readme-ov-file#private-key. You could also link to your comment where as an example on how to decode a base64-encoded key

If this comes up more often we can reconsider to add built-in base64 decoding, but so far I've only heard it from you.

lewismiddleton commented 1 month ago

private-key input as a Base64 encoded string

what is the use case? Maybe the decoding a base64 string could be done in a separate step before using this action?

To advocate for supporting Base64 encoded private keys, our use case is using AWS SecretsManager to store GitHub app credentials.

SecretsManager supports plaintext secrets or JSON objects. To store the private key (with whitespace) as a JSON key value pair we need to encode it.

We expect a secret format of:

{
  "app-id": 123456,
  "private-key": "base64-encoded-private-key"
}

The aws-actions/aws-secretsmanager-get-secrets action supports parse-json-secrets: true which can format environment variables based on the properties.

We then consume this in a GitHub Actions job like this:

    - name: Retrieve AWS Secret
      uses: aws-actions/aws-secretsmanager-get-secrets
      with:
        parse-json-secrets: true
        secret-ids: |
          GH_APP, ${{ inputs.app-credentials-secret-name }}

    - name: Generate a token
      id: generate-token
      uses: tibdex/github-app-token
      with:
        app_id: ${{ env.GH_APP_APP_ID }}
        private_key: ${{ env.GH_APP_PRIVATE_KEY }}

Having native support for Base64 encoded keys is an advantage as it avoids needing to insert decoding steps ourselves.

Implementing something like the above always feels risky as it's quite easy to break the masking if you're doing development against a GitHub Actions workflow. If the private key gets leaked, we need to manually rotate it in the GitHub UI and then repopulate the AWS secret.

For compatibility with actions/create-github-app-token and aversion to implementing decoding ourselves, we've instead ended up storing the app's id and private key in separate secrets. See implementation below.

This feels wrong because

  - name: Retrieve AWS Secret
    uses: aws-actions/aws-secretsmanager-get-secrets@v2
    with:
      secret-ids: |
        APP_ID, ${{ inputs.app-id-secret }}
        PEM_FILE, ${{ inputs.app-private-key-secret }}

  - name: Generate token
    id: create-github-app-token
    uses: actions/create-github-app-token@v1
    with:
      app-id: ${{ env.APP_ID }}
      private-key: ${{ env.PEM_FILE }}

The tibdex implementation is much neater for our use case as I'm sure you'd agree. On the other hand, it is preferable to use the official implementation from https://github.com/actions. I'd really appreciate if this was added as a feature to get the best of both worlds :)