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

Registries Proxy: Support feeding a base64 encoded configuration #2404

Merged
merged 8 commits into from
Aug 20, 2024

Conversation

marcogario
Copy link
Contributor

@marcogario marcogario commented Aug 2, 2024

Extend the start-proxy action to support passing the configuration as a base64 encoded variable. The reason to do so, is that this information is being passed by Actions as a secret. Actions recommends against passing structured data as a secret because it becomes harder to scrub it. Therefore, we send the information as a simple string.

We introduce the new input registries_credentials in parallel with the previous input to make it easier to test this while we are still building the remaining parts of the system. We will deprecate and remove the old property (registry_secrets) in the future. Note that this entire action is meant to be for internal use only.

Finally, in this PR I also refactored a bit the logic to more clearly distinguish the key steps, perform some validation of the input, and thread the logger for increase observability.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

src/start-proxy-action.ts Fixed Show fixed Hide fixed
@marcogario marcogario marked this pull request as ready for review August 13, 2024 14:00
@marcogario marcogario requested a review from a team as a code owner August 13, 2024 14:00
src/start-proxy-action.ts Show resolved Hide resolved
src/start-proxy-action.ts Show resolved Hide resolved
function getCredentials(): Credential[] {
const encodedCredentials = actionsUtil.getOptionalInput("registries_credentials");
if (encodedCredentials !== undefined) {
const credentialsStr = Buffer.from(encodedCredentials, "base64").toString();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like some extra hoops to go through for the workflow file to send base64 encoded credentials. Does it make more sense to accept non-encoded credentials and only base64 encode them in the action?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, there are some extra steps, but I think they are justified.

The proxy takes a JSON object as previously defined in registry_secrets. The problem is that Actions recommends not using structured data within secrets:

To help ensure that GitHub redacts your secret in logs, avoid using structured data as the values of secrets.

Therefore, we encode it as a single string to make scrubbing possible.

The underlying reason we need a struct is that (due to the integration with default setup) we need to provide a potentially arbitrary number of credentials, and we cannot easily know which ones/how many beforehand.

start-proxy/action.yml Show resolved Hide resolved
src/start-proxy-action.ts Fixed Show fixed Hide fixed
@marcogario marcogario force-pushed the marcogario/proxy_64 branch 3 times, most recently from 7156568 to a9c1d1c Compare August 15, 2024 16:34
@marcogario
Copy link
Contributor Author

@aeisenberg this is ready for another review. I am also assigning @aibaars so he can shepherd this to completion in case there are issues to address.

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome. Thanks. Looks great.

@aibaars aibaars merged commit 512e306 into main Aug 20, 2024
310 checks passed
@aibaars aibaars deleted the marcogario/proxy_64 branch August 20, 2024 10:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants