feat!: workload identity federation support #1131
Conversation
…#1022) This defines internal utilities to handle: - OAuth client authentication - Standard OAuth response parsing into native Javascript errors.
google-cla
bot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #1131 +/- ##
==========================================
+ Coverage 91.65% 93.74% +2.09%
==========================================
Files 21 28 +7
Lines 4157 6253 +2096
Branches 430 654 +224
==========================================
+ Hits 3810 5862 +2052
- Misses 347 391 +44
Continue to review full report at Codecov.
|
| @@ -363,6 +366,213 @@ async function main() { | |||
| main().catch(console.error); | |||
| ``` | |||
|
|
|||
| ## Workload Identity Federation | |||
There was a problem hiding this comment.
This is a massive addition to the README, and I worry a little about the size. I am ok keeping it here for now (non-blocking feedback), but we need a better documentation strategy for auth. @tbpg
There was a problem hiding this comment.
@bcoe also pointed out the same issue which I agree with. There are larger plans to update the official authentication docs: https://cloud.google.com/docs/authentication. Though this could be delayed until Q3. Currently, they are only updating the workload identity federation docs:
- https://cloud.google.com/iam/docs/access-resources-aws
- https://cloud.google.com/iam/docs/access-resources-azure
- https://cloud.google.com/iam/docs/access-resources-oidc
It would be a good idea to align with the overall documentation effort in the cloud docs and the GitHub repos.
| * @param {number} length The length of the string to generate. | ||
| * @return {string} A random string of the provided length. | ||
| */ | ||
| const generateRandomString = length => { |
There was a problem hiding this comment.
As a general rule, I try to avoid mutating a variable passed into a function. Since this is a primitive I think? it's passed by value and shouldn't matter, but a for loop is generally easier to rationalize. Also, why wouldn't this be a regular function?
There was a problem hiding this comment.
Done:
- Used regular
function. - Used
forloop. - Stopped mutating input variable.
| scopes: 'https://www.googleapis.com/auth/cloud-platform', | ||
| }); | ||
|
|
||
| // TODO: switch to using IAM client SDK once v1 API has all the v1beta |
There was a problem hiding this comment.
Please make sure we're filing issues on GitHub for any TODOs so we don't lose them.
| @@ -84,6 +84,7 @@ | |||
| "fix": "gts fix", | |||
| "pretest": "npm run compile", | |||
| "docs": "compodoc src/", | |||
| "samples-setup": "cd samples/ && npm link ../ && npm run setup && cd ../", | |||
There was a problem hiding this comment.
this seems like a reasonable addition to the script, presumably so that you could setup the sample environment outside of running tests?
There was a problem hiding this comment.
Yes that is correct. This is very useful if you want to set up the sample environment in a different project or locally.
src/auth/authclient.ts
Outdated
|
|
||
| /** | ||
| * The main authentication interface. It takes an optional url which when | ||
| * present is the endpoint> being accessed, and returns a Promise which |
src/auth/awsclient.ts
Outdated
| export interface AwsClientOptions extends BaseExternalAccountClientOptions { | ||
| credential_source: { | ||
| environment_id: string; | ||
| // Region can also be determine from the AWS_REGION environment variable. |
There was a problem hiding this comment.
typo: be determined
src/auth/baseexternalclient.ts
Outdated
| private getProjectNumber(audience: string): string | null { | ||
| // STS audience pattern: | ||
| // //iam.googleapis.com/projects/$PROJECT_NUMBER/locations/... | ||
| const components = audience.split('/'); |
There was a problem hiding this comment.
[style] I would prefer a simple regex here:
const match = audience.match(/^\/\/iam.googleapis.com\/projects\/([^\/]+)/);
if (!match) {
return null;
}
return match[1];
src/auth/googleauth.ts
Outdated
| * Gets the project ID from external account client if available. | ||
| */ | ||
| private async getExternalAccountClientProjectId(): Promise<string | null> { | ||
| if (this.jsonContent && this.jsonContent.type === EXTERNAL_ACCOUNT_TYPE) { |
There was a problem hiding this comment.
[style, optional] invert the condition to reduce nesting of the code
src/auth/identitypoolclient.ts
Outdated
| this.formatType, | ||
| this.formatSubjectTokenFieldName | ||
| ); | ||
| } else { |
src/auth/identitypoolclient.ts
Outdated
| try { | ||
| // Resolve path to actual file in case of symlink. Expect a thrown error | ||
| // if not resolvable. | ||
| filePath = fs.realpathSync(filePath); |
There was a problem hiding this comment.
using sync fs API in an async function does not seem right. On a network filesystem, these calls can block for considerable amount of time. Can we replace with async API e.g. fs.realpath, fs.lstat?
| * @param buffer The Buffer input to covert. | ||
| * @return The ArrayBuffer representation of the input. | ||
| */ | ||
| function toArrayBuffer(buffer: Buffer): ArrayBuffer { |
There was a problem hiding this comment.
I like this answer more - just slice the proper part of buffer.buffer without any iteration.
| * @param arrayBuffer The ArrayBuffer input to covert. | ||
| * @return The Buffer representation of the input. | ||
| */ | ||
| function toBuffer(arrayBuffer: ArrayBuffer): Buffer { |
There was a problem hiding this comment.
Buffer.from(arrayBuffer) will do it
| "SecretAccessKey" : "Y8AfSaucF37G4PpvfguKZ3/l7Id4uocLXxX0+VTx", | ||
| "Token" : "IQoJb3JpZ2luX2VjEIz//////////wEaCXVzLWVhc3QtMiJGMEQCIH7MHX/Oy/OB8OlLQa9GrqU1B914+iMikqWQW7vPCKlgAiA/Lsv8Jcafn14owfxXn95FURZNKaaphj0ykpmS+Ki+CSq0AwhlEAAaDDA3NzA3MTM5MTk5NiIMx9sAeP1ovlMTMKLjKpEDwuJQg41/QUKx0laTZYjPlQvjwSqS3OB9P1KAXPWSLkliVMMqaHqelvMF/WO/glv3KwuTfQsavRNs3v5pcSEm4SPO3l7mCs7KrQUHwGP0neZhIKxEXy+Ls//1C/Bqt53NL+LSbaGv6RPHaX82laz2qElphg95aVLdYgIFY6JWV5fzyjgnhz0DQmy62/Vi8pNcM2/VnxeCQ8CC8dRDSt52ry2v+nc77vstuI9xV5k8mPtnaPoJDRANh0bjwY5Sdwkbp+mGRUJBAQRlNgHUJusefXQgVKBCiyJY4w3Csd8Bgj9IyDV+Azuy1jQqfFZWgP68LSz5bURyIjlWDQunO82stZ0BgplKKAa/KJHBPCp8Qi6i99uy7qh76FQAqgVTsnDuU6fGpHDcsDSGoCls2HgZjZFPeOj8mmRhFk1Xqvkbjuz8V1cJk54d3gIJvQt8gD2D6yJQZecnuGWd5K2e2HohvCc8Fc9kBl1300nUJPV+k4tr/A5R/0QfEKOZL1/k5lf1g9CREnrM8LVkGxCgdYMxLQow1uTL+QU67AHRRSp5PhhGX4Rek+01vdYSnJCMaPhSEgcLqDlQkhk6MPsyT91QMXcWmyO+cAZwUPwnRamFepuP4K8k2KVXs/LIJHLELwAZ0ekyaS7CptgOqS7uaSTFG3U+vzFZLEnGvWQ7y9IPNQZ+Dffgh4p3vF4J68y9049sI6Sr5d5wbKkcbm8hdCDHZcv4lnqohquPirLiFQ3q7B17V9krMPu3mz1cg4Ekgcrn/E09NTsxAqD8NcZ7C7ECom9r+X3zkDOxaajW6hu3Az8hGlyylDaMiFfRbBJpTIlxp7jfa7CxikNgNtEKLH9iCzvuSg2vhA==", | ||
| "Expiration" : "2020-08-11T07:35:49Z" | ||
| } |
| "credential_source": { | ||
| "file": "./test/fixtures/external-subject-token.txt" | ||
| } | ||
| } |
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "access_token": "HEADER.SIMULATED_JWT_PAYLOAD.SIGNATURE" | |||
| } | |||
There was a problem hiding this comment.
Thanks for the through review @alexander-fenster! I made all the requested changes.
Using workload identity federation, applications can access Google Cloud resources from Amazon Web Services (AWS), Microsoft Azure or any identity provider that supports OpenID Connect (OIDC). Workload identity federation is recommended for non-Google Cloud environments as it avoids the need to download, manage and store service account private keys locally.