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

feat: add AIP 4120 - Universe Domain support #1282

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Dec 7, 2023

No description provided.

@bshaffer bshaffer requested a review from a team as a code owner December 7, 2023 19:21
@bshaffer bshaffer changed the title feat: add AIP 4120 feat: add AIP 4120 - Universe Domain support Dec 12, 2023
Copy link

@TimurSadykov TimurSadykov left a comment

Choose a reason for hiding this comment

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

LGTM

@TimurSadykov
Copy link

@westarle FYI


Additional recommendations:

- GCE credentials **should** retrieve the universe domain in a 'lazy' way, when the end-user requests the `universe_domain` via the property/get method. They **should not** do it during the credentials initialization.

Choose a reason for hiding this comment

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

Who tests that the fetched credential universe matches the user-specified universe domain in the client lib and prevents mismatches?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This validation happens in the client libraries

@bshaffer
Copy link
Contributor Author

@westarle in anticipation of discussions happening later this week, can we get another review on this one from you?

aip/auth/4120.md Outdated Show resolved Hide resolved
aip/auth/4120.md Show resolved Hide resolved
aip/auth/4120.md Outdated Show resolved Hide resolved
aip/auth/4120.md Outdated Show resolved Hide resolved
aip/auth/4120.md Outdated
#### Authentication libraries **must** use the self-signed JWT flow when authenticating outside the GDU using Service Account credentials

Currently the self-signed JWT (SSJ) sub-flow is the default option for Service Account auth flow, but user parameters provided, such as `scope`, `audience`, or `useJwtWithScopes` can change that. E.g. setting `useJwtWithScopes` to `false` when specifying `scope` would result in auth library fallback to token exchange flow for service accounts.
The token exchange for Service Accounts is not a supported flow outside the GDU, and services must support SSJ with scopes. Therefore authentication libraries **must** use the self-signed JWT flow when authenticating outside the GDU using Service Account credentials. If the `useJwtWithScopes` parameter exists (whatever form it takes in specific languages), it **must** be ignored outside the GDU, and its documentation **must** be updated to reflect that.

Choose a reason for hiding this comment

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

Since this is a specification, could we just say "If the universe domain is not GDU, only self-signed JWT tokens may be used for authentication via bearer token when using service account key credentials."

And discard all the stuff that might change over the next few months/years like how the library works in GDU?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The useJwtWithScope option is already documented in https://google.aip.dev/auth/4111, so I think it's appropriate to have it as part of this spec as well.

| -------------------------------------------------- | ------ |
| 404 | The authentication libraries **must** set the `universe_domain` to the default value `"googleapis.com"`. |
| Timeout | The authentication libraries **must not** fall back to the `"googleapis.com"` default value. A retry mechanism is recommended. The authentication libraries **should** do the same action as when retrieving the access token times out. |
| Any other error | The authentication libraries **must not** fall back to the `"googleapis.com"` default value. The authentication libraries **should** do the same action as when the same error happens when retrieving the access token. |

Choose a reason for hiding this comment

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

Heads up: this has turned out to be a bit of a problem -- because "MDS" doesn't much have a specification we can refer to, customers have implemented "MDS" clones that don't match our expectations.

Beyond the scope of this AIP, but is there a way we can mitigate that problem without requiring changes to MDS clones or our libraries? Maybe a trusted parameter or environment variable available to auth libraries to fix the universe domain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

By accepting 404s to be googleapis.com, we should be handling all edgecases where other MDSs don't behave as expected (see line 66)

There's a potential for accepting a timeout as googleapis.com in the event that the library has already confirmed that it's running on MDS. But this is a temporary fix, and has not been accepted in the spec yet, so I think it's best to leave it out.


> This requirement applies **only** to the flows which are supported outside the GDU.

The code for the authentication flows **must not** contain hardcoded endpoints, endpoint-alikes or other authentication information unless this information is constant across all flows for all universes. Any information that is possible to read from the credentials file should be read from the credentials file. For example, `scopes` contain the "googleapis.com" domain but are still valid outside the GDU because their values are consistent across all universes, whereas the STS endpoint "sts.googleapis.com" is not allowed because it is different for every universe.

Choose a reason for hiding this comment

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

Is there a reference for what changes outside and what doesn't that we can refer to?

Copy link
Contributor Author

@bshaffer bshaffer Feb 13, 2024

Choose a reason for hiding this comment

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

There is no canonical list that I am aware of. I believe this is because "scopes" is currently the only value of googleapis.com that is allowed outside GDU - everything else should change based on the universe.

aip/auth/4120.md Outdated Show resolved Hide resolved
aip/auth/4120.md Outdated Show resolved Hide resolved
Copy link

@westarle westarle left a comment

Choose a reason for hiding this comment

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

Thanks for addressing review comments offline and in the PR! One last thing to update and it LGTM.


Currently the self-signed JWT (SSJ) sub-flow is the default option for Service Account auth flow, but user parameters provided, such as `scope`, `audience`, or `useJwtWithScopes` can change that. E.g. setting `useJwtWithScopes` to `false` when specifying `scope` would result in auth library fallback to token exchange flow for service accounts.
The token exchange for Service Accounts is not a supported flow outside the GDU, and services must support SSJ with scopes. Therefore authentication libraries **must** use the self-signed JWT flow when authenticating outside the GDU using Service Account credentials. If the `useJwtWithScopes` parameter exists (whatever form it takes in specific languages), it **must** be ignored outside the GDU, and its documentation **must** be updated to reflect that.
The token exchange for Service Accounts is not a supported flow outside the GDU, and services must support SSJ with scopes. Therefore authentication libraries **must** use the self-signed JWT flow when authenticating outside the GDU using Service Accountj key credentials. If the `UseJWTAccessWithScope` parameter exists (whatever form it takes in specific languages), it **must** be ignored outside the GDU, and its documentation **must** be updated to reflect that.
Copy link

@westarle westarle Feb 15, 2024

Choose a reason for hiding this comment

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

"The token exchange for Service Accounts" --> "Service Account Key authentication using OAuth" (from the SA Key AIP)

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