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: support universe domain in service account and metadata credentials #482

Merged
merged 24 commits into from
Dec 14, 2023

Conversation

bshaffer
Copy link
Contributor

@bshaffer bshaffer commented Sep 20, 2023

Requirements

  • 1. Authentication libraries must read the universe_domain property from the credentials files (done feat: add support for universe domain (phase 1) #477)
  • 2. Authentication libraries must allow the end-user to set the universe_domain in code
  • 3. Authentication libraries must use the universe_domain variable/property/method name
  • 4. Authentication libraries must not use hardcoded non-constant information in the authentication flows
  • 5. Authentication libraries must implement retrieving the universe_domain from the VM Metadata Server
  • 6. Authentication libraries must expose the universe_domain in all credentials classes (done in feat: add support for universe domain (phase 1) #477)
  • 7. Authentication libraries must add the universe_domain property to the token authentication credentials constructor (if supported) and document the default behavior Not applicable unless we add an Access Token credential type
  • 8. Authentication libraries must only attempt self-signed JWT flow for Service Accounts when authenticating outside googleapis.com
  • 9. Authentication libraries must throw an Exception if domain-delegation is configured for Service Accounts when authenticating outside googleapis.com
  • 10. Authentication libraries must implement IdToken flow for Service Accounts when authenticating outside googleapis.com or throw an exception

Dependencies

Base automatically changed from add-universe-domain-phase1 to main November 28, 2023 15:47
@bshaffer bshaffer marked this pull request as ready for review November 28, 2023 15:52
@bshaffer bshaffer requested a review from a team as a code owner November 28, 2023 15:52
src/Credentials/GCECredentials.php Outdated Show resolved Hide resolved
src/Credentials/GCECredentials.php Show resolved Hide resolved
tests/Credentials/GCECredentialsTest.php Show resolved Hide resolved
@bshaffer bshaffer changed the title feat: add universe domain (phase2) feat: support universe domain in service account and metadata credentials Dec 1, 2023
@@ -164,7 +164,7 @@ public function __construct(
]);

$this->projectId = $jsonKey['project_id'] ?? null;
$this->universeDomain = $jsonKey['universe_domain'] ?? null;
$this->universeDomain = $jsonKey['universe_domain'] ?? self::DEFAULT_UNIVERSE_DOMAIN;
Copy link
Contributor

@vishwarajanand vishwarajanand Dec 13, 2023

Choose a reason for hiding this comment

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

Should we also throw in case there's domain-delegation?
I am not very sure but it appears that we need to throw (near line#146) in case sub as well as universe_domain is set in the key, as per AL9 of the universe domain spec

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for catching this, I completely missed it. PHP definitely supports domain-delegation

* @param string $sub an email address account to impersonate, in situations when
* the service account has been delegated domain wide access.

I'll look more into it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I added the exception and test in 5f8b409, and added a fix for domain-wide delegation that we had missed in #505

Copy link
Contributor

Choose a reason for hiding this comment

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

Approved it

Copy link
Contributor

@vishwarajanand vishwarajanand left a comment

Choose a reason for hiding this comment

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

Lgtm, approving since there are no major comments.

Tests:

  1. I tested the changes on apis-tpclp.goog universe domain and validated that ServiceAccountCredentials generates a valid access_token while using the provided universe domain value.

  2. I was also able to query the compute instances in the project on the URL: https://compute.apis-tpclp.goog/compute/staging_v1/<more params>, which captures the universe domain in the endpoint.

  3. When I use the access token generated in step#1, and use the regular compute api:
    https://compute.googleapis.com/compute/staging_v1/<more params> I see an expected failure with error code 401 and status UNAUTHENTICATED:

Request had invalid authentication credentials. Expected OAuth 2 access token, login cookie or other valid authentication credential. See https://developers.google.com/identity/sign-in/web/devconsole-project.

@bshaffer bshaffer merged commit e4aa874 into main Dec 14, 2023
12 checks passed
@bshaffer bshaffer deleted the add-universe-domain-phase2 branch December 14, 2023 15:30
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