10000 Fix: Remove UTF-8 encoding to prevent mangling of binary secrets by matthiasfehr · Pull Request #599 · docker/actions-toolkit · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Fix: Remove UTF-8 encoding to prevent mangling of binary secrets #599

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

Merged
merged 1 commit into from
Feb 19, 2025

Conversation

matthiasfehr
Copy link
Contributor

This PR removes the explicit encoding: 'utf-8' from the secret resolution code to ensure binary files are not corrupted. Previously, reading and writing secrets as UTF-8 caused issues when the secret was a binary file. By switching to raw buffers, the code now correctly supports both binary and text-based secrets without any unintended transformations.

  • Removed encoding: 'utf-8' from fs.readFileSync.
  • Ensured raw buffers are used when reading and writing secrets, preventing data corruption.
  • Maintains existing behavior for text-based secrets, while adding proper support for binary data.

@crazy-max
Copy link
Member
crazy-max commented Feb 19, 2025

This PR removes the explicit encoding: 'utf-8' from the secret resolution code to ensure binary files are not corrupted.

I was looking at the initial implementation and it was added in https://github.com/docker/build-push-action/pull/296/files#diff-f73f5f656aeb1aac9f0e62af243004b96603175d3d897231961ab4245d7a5a50R41

I think your change is fine but I just wonder what's your use case to have a bin formatted secret to use in your Dockerfile. Do you mind to share?

Also I think instead of doing both fs.readFileSync and fs.writeFileSync we could just do a fs.copyFileSync for file-based secrets like:

  public static resolveSecret(kvp: string, file: boolean): [string, string] {
    const [key, value] = Build.parseSecretKvp(kvp);
    const secretFile = Context.tmpName({tmpdir: Context.tmpDir()});
    if (file) {
      if (!fs.existsSync(value)) {
        throw new Error(`secret file ${value} not found`);
      }
      fs.copyFileSync(value, secretFile);
    } else {
      fs.writeFileSync(secretFile, value);
    }
    return [key, secretFile];
  }

@matthiasfehr
Copy link
Contributor Author

I have a docker build stage in my Dockerfile where I sign the built executable, and for this I need to pass in a p12 certificate file. This works when directly passed as secret argument to docker but not with this action.

I have updated the PR to use directly the copyFileSync method.

Copy link
Member
@crazy-max crazy-max left a comment

Choose a reason for hiding this comment

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

Can you squash your commits? Otherwise LGTM thanks!

Signed-off-by: Matthias Fehr <matthias@monostream.com>
@crazy-max crazy-max merged commit ea9281e into docker:main Feb 19, 2025
86 checks passed
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.

2 participants
0