feat(auth): add external account url sourced credentials #2217

alvarowolfx · 2025-05-19T18:00:59Z

Fixes #2079

codecov · 2025-05-19T18:15:24Z

Codecov Report

Attention: Patch coverage is 83.56164% with 12 lines in your changes missing coverage. Please review.

Project coverage is 89.58%. Comparing base (9bb3c6e) to head (e4601c2).
Report is 11 commits behind head on main.

Files with missing lines	Patch %	Lines
...ls/external_account_sources/url_sourced_account.rs	70.83%	7 Missing ⚠️
src/auth/src/credentials/external_account.rs	90.90%	4 Missing ⚠️
src/auth/src/credentials.rs	80.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2217      +/-   ##
==========================================
+ Coverage   89.08%   89.58%   +0.49%     
==========================================
  Files          62       64       +2     
  Lines        2392     2449      +57     
==========================================
+ Hits         2131     2194      +63     
+ Misses        261      255       -6

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

src/auth/src/credentials/external_account.rs

src/auth/src/credentials/external_account_sources/url_sourced_account.rs

src/auth/src/credentials/external_account.rs

sai-sunder-s · 2025-05-19T19:40:38Z

src/auth/src/credentials/external_account.rs

+        let token = Token {
+            token: token_res.access_token,
+            token_type: token_res.token_type,
+            expires_at: Some(Instant::now() + Duration::from_secs(token_res.expires_in)),
+            metadata: None,


Can We instead make the exchange_token method return the Token itself?

src/auth/src/credentials/external_account.rs

src/auth/src/credentials/external_account_sources.rs

coryan

The copyright year nits are blocking. I have not done an in-depth review either, not that you should need one, just letting you know.

coryan

Please restore the copyright year for credentials.rs the rest of my comments are just suggestions.

src/auth/src/credentials.rs

src/auth/src/credentials/external_account.rs

coryan

I don't have substantive suggestions, lots of nits, so approved.

src/auth/src/credentials/external_account.rs

src/auth/src/credentials/external_account_sources/url_sourced_account.rs

src/auth/src/credentials.rs

sai-sunder-s · 2025-05-23T17:09:51Z

src/auth/src/credentials.rs

@@ -17,6 +17,9 @@ pub mod mds;
 pub mod service_account;
 pub mod user_account;

+pub mod external_account;


Lets keep this private until we figure out twosigma's requirements regarding openssl and custom request client.

The problem is that by doing that, I can't use the module on the auth integration tests.

sai-sunder-s · 2025-05-23T17:11:35Z

src/auth/src/credentials/external_account.rs

+}
+
+#[derive(Serialize, Deserialize, Debug, Clone, PartialEq)]
+pub struct CredentialSourceFormat {


Does CredentialSourceFormat, CredentialSourceHeaders, ExecutableConfig, CredentialSource need to be public?

sai-sunder-s · 2025-05-23T17:31:07Z

src/auth/src/credentials/external_account_sources/url_sourced_account.rs

+            Some(format) => {
+                let json_response: Value = serde_json::from_str(&response_text).unwrap();
+                let subject_token = json_response
+                    .get(&format.subject_token_field_name)


Please add the format of the response in comments or link to a source. If not this is hard to understand.

the response from the subject token provider is dynamic and have a field named defined by the subject_token_field_name. So it's something like { "{subject_token_field_name}": "a token" }

sai-sunder-s · 2025-05-23T17:35:05Z

src/auth/src/credentials/external_account.rs

+    /// Creates a new builder using [external_account_config] JSON value.    
+    ///
+    /// [external_account_config]: https://cloud.google.com/iam/docs/workload-download-cred-and-grant-access#download-configuration
+    pub fn new(external_account_config: Value) -> Self {


With this design, if for whatever reason the user wants to build a workload credential through code instead of loading from config, they have no way of doing it.

Can we design in such a way that every workload cred type has its own builder.

I can create a URLSourcedExternalAccountCredential struct and that would provide a builder interface. Under the hood it would use an ExternalAccountCredential, so it's mostly a wrapper and more direct way of creating the type

The builder for url sourced cred should also allow user to override scopes atleast. When user is building a cred, they could override scopes. If you do not have scopes in byoid builders, user would call with_scopes and it would do nothing for some cred types.

google-cloud-rust/src/auth/src/credentials.rs

Line 424 in dee6b3f

pub fn with_scopes<I, S>(mut self, scopes: I) -> Self

This will also solve your macro problem

sai-sunder-s · 2025-05-23T17:51:38Z

src/auth/src/credentials/external_account.rs

+                )
+            }
+            _ => {
+                unreachable!("expected Url Sourced credential")


this is cool!

sai-sunder-s · 2025-05-23T17:52:18Z

src/auth/src/credentials/external_account_sources/url_sourced_account.rs

+}
+
+#[async_trait::async_trait]
+impl SubjectTokenProvider for UrlSourcedCredentials {


unit test for UrlSourcedCredentials?

src/auth/integration-tests/src/lib.rs

feat(auth): add external account url sourced credentials

57b4264

alvarowolfx requested a review from sai-sunder-s May 19, 2025 18:00

coryan reviewed May 19, 2025

View reviewed changes

src/auth/src/credentials/external_account.rs Outdated Show resolved Hide resolved

coryan reviewed May 19, 2025

View reviewed changes

src/auth/src/credentials/external_account_sources/url_sourced_account.rs Outdated Show resolved Hide resolved

fix: license header year

f11ee98

sai-sunder-s requested changes May 19, 2025

View reviewed changes

coryan requested changes May 19, 2025

View reviewed changes

alvarowolfx added 2 commits May 20, 2025 14:40

feat: add external account builder

b2ba552

Merge branch 'main' into feat-auth-url-sourced-creds

055990d

coryan requested changes May 20, 2025

View reviewed changes

fix: update CredentialsProvider impl to match cacheable resource trait

84b81e0

coryan marked this pull request as ready for review May 20, 2025 20:41

coryan requested a review from a team as a code owner May 20, 2025 20:41

coryan previously approved these changes May 20, 2025

View reviewed changes

feat: change CredentialSource to an enum

42e989f

alvarowolfx dismissed coryan’s stale review via 42e989f May 20, 2025 21:10

alvarowolfx added 9 commits May 21, 2025 10:58

fix: handle err for url sourced creds

fca8675

fix: lint issues

ad56efd

test: move tests closer to the external account mod

041003b

fix: get rid of Box<dyn SubjectTokenProvider>

39c0e18

docs: improve builder docs

06534a4

fix: builder example should be compilable by rustc

3b84492

fix: lint issues on builder docs

c859658

fix: handle scopes properly for external account

e8d2371

test: add integration tests for workload identity url sourced

99c140b

alvarowolfx requested review from sai-sunder-s and coryan May 23, 2025 15:48

alvarowolfx added 2 commits May 23, 2025 11:53

fix: typo and toml formatting

6917c63

fix: lint auth readme

e059453

sai-sunder-s reviewed May 23, 2025

View reviewed changes

alvarowolfx added 4 commits May 23, 2025 15:43

chore: rename UrlSorucedCredentials to SubjectTokenProvider

9b434fe

fix: lint issues

80afb38

feat: add option to override scopes

1d8ea19

fix: change visibility on internal structs

e4601c2

feat(auth): add external account url sourced credentials #2217

Are you sure you want to change the base?

feat(auth): add external account url sourced credentials #2217

Uh oh!

Conversation

alvarowolfx commented May 19, 2025

Uh oh!

codecov bot commented May 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

coryan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented May 19, 2025 •

edited

Loading