-
Notifications
You must be signed in to change notification settings - Fork 62
Use Registry Credentials when passed to env #1497
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR enables injecting centrally managed registry credentials via the GITHUB_REGISTRIES_PROXY environment variable and merges them with credentials fetched from the API.
- Import the new
Credentialtype and defaultgetCredentials()to an empty list. - Add
credentialsFromEnv()to decode, parse, and mask base64-encoded credentials fromGITHUB_REGISTRIES_PROXY. - Update tests to set
GITHUB_REGISTRIES_PROXYfor the new behavior.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/main.ts | Imported Credential, updated credential merging logic, and added credentialsFromEnv |
| tests/main.test.ts | Set GITHUB_REGISTRIES_PROXY in the test setup |
Comments suppressed due to low confidence (3)
tests/main.test.ts:45
- Tests currently only set a valid env var; consider adding cases for invalid base64, invalid JSON, and verifying that secrets are masked via core.setSecret.
process.env.GITHUB_REGISTRIES_PROXY = Buffer.from(
src/main.ts:222
- Add a JSDoc comment describing the expected format of GITHUB_REGISTRIES_PROXY (base64-encoded JSON array of Credentials) and the function’s behavior on parse errors.
function credentialsFromEnv(): Credential[] {
src/main.ts:223
- [nitpick] The variable name
registriesProxyStris a bit generic; consider renaming toencodedRegistryCredentialsor similar for clarity.
const registriesProxyStr = process.env.GITHUB_REGISTRIES_PROXY
8a46224 to
2fb55ab
Compare
0dcdbfd to
5e9e1e1
Compare
| core.setSecret(e['auth-key']) | ||
| } | ||
|
|
||
| // TODO: Filter down to only credentials relevant to this job. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At the rate we're adding ecosystems, some of which overlap (e.g. dotnet_sdk and nuget) I think we should inject all secrets to all job types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or the filtering should be done in the UI somehow per-repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like it should be fairly straight forward to figure out which ones we need? This is what Secret Scanning does:
I'm ok punting for now and revisiting though
bbd4eab to
7cc5ee6
Compare
In order to allow us to pass centrally managed registry credentials, this PR implements a basic version that grabs the Base64 encoded credential blob from the environment and passes them to the Proxy.
Co-authored-by: Jake Coffman <[email protected]>
7cc5ee6 to
e4bcc78
Compare
# Conflicts: # dist/main/index.js.map # src/api-client.ts
In order to allow us to pass centrally managed registry credentials, this PR implements a basic version that grabs the Base64 encoded credential blob from the environment and passes them to the Proxy.
Right now by default these credentials will not be passed.