Closed JohnathonBowers closed 6 months ago
Hi @JohnathonBowers
I saw your code. Excellent work! Just one thing I think is not efficient. The variable GH_APP_DOMAIN
and secret CHANGELOG_PR_BRIDGE_SECRET_KEY
should not be located in the base OpenSearch repo that contains the opensearch_changelog_workflow.yml
Imagine we have to send the secret CHANGELOG_PR_BRIDGE_SECRET_KEY to 20 teams. It is a risk. Also, what if the secret is updated? Better to manage it in this reusable workflow. Same with the variable GH_APP_DOMAIN
Why we don't try the following:
GH_APP_DOMAIN:
description: "GitHub App domain for API calls"
required: true
CHANGELOG_PR_BRIDGE_SECRET_KEY:
description: "API key for communicating with the changelog PR bridge"
required: true
run
section
runs:
using: "node16"
main: "src/main.js"
env: # new
CHANGELOG_PR_BRIDGE_SECRET_KEY: ${{ secrets.CHANGELOG_PR_BRIDGE_SECRET_KEY }} # new
const CHANGELOG_PR_BRIDGE_SECRET_KEY = process.env.CHANGELOG_PR_BRIDGE_SECRET_KEY;
Thinking this would be a proper way to go. @ashwin-pc, @BSFishy what do you think?
Hi @JohnathonBowers
I saw your code. Excellent work! Just one thing I think is not efficient. The variable
GH_APP_DOMAIN
and secretCHANGELOG_PR_BRIDGE_SECRET_KEY
should not be located in the base OpenSearch repo that contains theopensearch_changelog_workflow.yml
Imagine we have to send the secret CHANGELOG_PR_BRIDGE_SECRET_KEY to 20 teams. It is a risk. Also, what if the secret is updated? Better to manage it in this reusable workflow. Same with the variable GH_APP_DOMAIN
Why we don't try the following:
- Remove these lines in the action.yml file
GH_APP_DOMAIN: description: "GitHub App domain for API calls" required: true CHANGELOG_PR_BRIDGE_SECRET_KEY: description: "API key for communicating with the changelog PR bridge" required: true
- Add these last lines in that same file, under the
run
sectionruns: using: "node16" main: "src/main.js" env: # new CHANGELOG_PR_BRIDGE_SECRET_KEY: ${{ secrets.CHANGELOG_PR_BRIDGE_SECRET_KEY }} # new
- In the const.js file create the const variable for the key as follows:
const CHANGELOG_PR_BRIDGE_SECRET_KEY = process.env.CHANGELOG_PR_BRIDGE_SECRET_KEY;
Thinking this would be a proper way to go. @ashwin-pc, @BSFishy what do you think?
The problem with using an environment variable in a workflow run is that the variables can be exposed in the build output. The GitHub docs recommend using secrets for sensitive information like our API key. Here is a screenshot of the relevant section:
Someone who has administrative access to OpenSearch repositories can set the API key as an organization secret and configure which repositories have access to use the secret in workflow runs. Here is the relevant section from the GitHub docs on organization secrets:
Setting the API Key as an organization secret would mean that multiple repositories could use that secret in their workflow runs without knowing the actual value of the secret. This would maintain both privacy and extensibility for the API key.
The only additional caveat is that whoever sets the API key as an organization secret would also need to have access to the deployed instance of the Express App housed in the OpenSearch_Changeset_Bot
repository so that they can configure the API key as an environment variable there.
I follow your idea. But let's separate two things. The GITHUB_APP_DOMAIN
variable must not be set as a secret. It can, but you can set it as a variable. There is no need to protect this information. Regarding the CHANGELOG_PR_BRIDGE_SECRET_KEY,
this certainly has to be stored as a secret.
What I do not understand is why you say that the approach I am recommending will expose the API KEY in the built output of the OpenSearch Changelog Workflow
service. This service is a reusable workflow, and secrets and variables can be defined in its related GitHub repository. If we set the API KEY secret in this repo and my suggestion indeed works, all repos in OSD, when calling this reusable workflow, will be able to call the express endpoint indirectly without the need to know the API KEY. The only ones who need to know that this API KEY are the OpenSearch Changelog Workflow
service and the OpenSearch Changelog PR BRIDGE
service.
Thanks for your feedback, @BigSamu . First of all, for the sake of clarity and simplicity, I'll use the following terms to refer to the two different repositories we are talking about:
Action Repository: This is the Open_Search_Parse_Changelog
repository, the repository where our reusable GitHub Action's code resides.
Consuming Repository: This is any repository that invokes our reusable action in a workflow run (defined in a .github/workflows/
directory). OpenSearch Dashboards would be a consuming repository in our scenario.
The important point to keep in mind is that environment variables that are configured in the Action repository are scoped to that repository alone and are not accessible by consuming repositories that invoke that Action. Additionally, environment variables are stored as plain text and can potentially be exposed in build logs. They are not suited for sensitive data like API keys.
If we want to make sensitive data available to an Action in a workflow run, we have to configure it in the consuming repository as a secret and pass it explicitly to that Action in the consuming repository's workflow file. Secrets are encrypted and masked and will not be exposed in build logs.
In summary, because of scoping limitations in GitHub repositories, configuring an API key as an environment variable in the Action repository will not make that key available to consuming repositories' workflow runs. Additionally, it is not a secure method for storing sensitive data. That's why we need to configure the key as a secret in the consuming repository.
To the point about the GITHUB_APP_DOMAIN
, I agree that base URLs are less-sensitive information and may be configured as environment variables. And this decision is not as important for the initial rollout of the workflow and release notes process, so I'm fine with postponing it. However, eventually, I would recommend configuring the GITHUB_APP_DOMAIN
as another secret in consuming repositories. This is primarily so that developers who are setting up their local environment will be able to configure their ngrok
tunnel domain without risking its exposure.
The approach seems fine to me, just a few code comments :)
Have secrets at an organisational level, you mean? I was trying to avoid that in my discussion with @JohnathonBowers. I want to check if we can save the secrets in our reusable workflow. However, if you think it is OK, we should proceed with his suggestion.
Description
These changes add an API Key in the HTTP request headers, making it possible for workflows using the
OpenSearch_Parse_Changelog_Action
to communicate securely with theOpenSearch_Changeset_Bot
. Additionally, these changes make it possible for the value of theGH_APP_DOMAIN
constant to be retrieved from an input secret rather than needing to be hard-coded. This will be especially useful for developers setting up a local environment, as they will not be required to expose their tunnel domain publicly.Features
Before an HTTP request is made, the logic makes sure that both the
GH_APP_DOMAIN
andCHANGELOG_PR_BRIDGE_SECRET_KEY
values have been retrieved from the workflow's input secrets.HTTP requests are sent with the API key stored in the
"X-API-Key"
headerIn order to distinguish between 401 status codes that result from a GitHub App installation error and an API key validation error, the catch block for each HTTP request checks to see if the error message coming from the server includes "GitHub App". If it does, it throws a new
GitHubAppSuspendedOrNotInstalledError()
Testing
These changes have been tested manually in my local environment to make sure that communication with the
OpenSearch_Changeset_Bot
works as expected.IMPORTANT: Both the
GH_APP_DOMAIN
andCHANGELOG_PR_BRIDGE_SECRET_KEY
input secrets need to be named as such and configured as repository secrets in the base repository where the workflow using this Action runs. Theparse_changelog_workflow.yml
file in the base repository should look like this:Issues
This PR accompanies PR#4 opened in the
OpenSearch_Changeset_Bot
repository. It also resolves Issue #50 in this repository.