-
Notifications
You must be signed in to change notification settings - Fork 115
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
support url-sourced external account #222
Conversation
8ca11e5
to
cecc05e
Compare
cecc05e
to
99cea66
Compare
/// JSON schema of URL-sourced credentials' format. | ||
#[derive(Serialize, Deserialize, Debug, Clone)] | ||
#[serde(tag = "type")] | ||
pub enum UrlCredentialSourceFormat { |
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.
This enum represents the format of document that the url returns. I avoid UrlFormat
since it sounds "the format of url". I don't think UrlCredentialSourceFormat
is good. Suggestion welcome.
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.
Using modules is another option
pub mod credential_source {
pub mod url {
pub enum Format { ... }
}
}
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.
no I think it's fine the way you've done it
@@ -47,10 +48,33 @@ pub enum CredentialSource { | |||
/// file | |||
file: String, | |||
}, | |||
// TODO: Microsoft Azure and URL-sourced credentials |
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.
nice!
/// JSON schema of URL-sourced credentials' format. | ||
#[derive(Serialize, Deserialize, Debug, Clone)] | ||
#[serde(tag = "type")] | ||
pub enum UrlCredentialSourceFormat { |
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.
no I think it's fine the way you've done it
@@ -72,6 +101,39 @@ impl ExternalAccountFlow { | |||
{ | |||
let subject_token = match &self.secret.credential_source { | |||
CredentialSource::File { file } => tokio::fs::read_to_string(file).await?, | |||
CredentialSource::Url { |
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.
this looks good to me, have you tested it in a real deployment? Unfortunately I don't have the means to do it myself, lacking an appropriate cloud environment.
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.
I confirmed this code works for my case (workload identity federation in github actions).
This PR adds url-sourced external account (describe in
https://google.aip.dev/auth/4117#determining-the-subject-token-in-microsoft-azure-and-url-sourced-credentials ).