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

Research learnings and ideas from similar GitHub Actions #42

Open
gr2m opened this issue Sep 4, 2023 · 10 comments
Open

Research learnings and ideas from similar GitHub Actions #42

gr2m opened this issue Sep 4, 2023 · 10 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@gr2m
Copy link
Contributor

gr2m commented Sep 4, 2023

Follow up to #39 (comment).

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

@gr2m gr2m added good first issue Good for newcomers help wanted Extra attention is needed labels Sep 4, 2023
@gr2m
Copy link
Contributor Author

gr2m commented Sep 4, 2023

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.

@parkerbxyz parkerbxyz changed the title Research learnings and ideas from similar ideas Research learnings and ideas from similar GitHub Actions Sep 11, 2023
@Moser-ss
Copy link
Contributor

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
Copy link
Contributor Author

gr2m commented Oct 26, 2023

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
Copy link
Contributor

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
Copy link
Contributor Author

gr2m commented Oct 29, 2023

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
Copy link

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
Copy link
Contributor Author

gr2m commented Nov 2, 2023

@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
Copy link
Contributor

vleon1a commented Jul 8, 2024

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
Copy link
Contributor Author

gr2m commented Jul 8, 2024

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.

parkerbxyz added a commit that referenced this issue Jul 11, 2024
Addressing this comment
#42 (comment)

---------

Co-authored-by: Parker Brown <17183625+parkerbxyz@users.noreply.github.com>
@lewismiddleton
Copy link

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

  • the app ID isn't really a secret, we just want it parameterised away so that we don't have hard coded magic numbers in various workflows
  • it feels wasteful (AWS charges per secret per month) to store these in two separate values when they refer to one conceptual thing
  • the app id and private key have a similar lifecycle (e.g. they both get populated at creation), storing in the same secret makes sense
  - 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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants