[SS-188] Check the token_uri in Google Cloud Service Account keys#36997
Conversation
| // want to send requests to the actual Google OAuth2 token API. | ||
| ConnectionDetails::Gcp(gcp) => vec
There was a problem hiding this comment.
So this is tuple of item ID (which I assume points to json credentials) and a validation function for that json.
Then more broadly this is structured so we can add validation functions from varied ConnectionDetails.
Commentary: Returning a mapping of catalog item and function from each connection detail to later call feels similar to me to having an abstract method of a parent class implemented by a handful of subclasses.
There was a problem hiding this comment.
re: Commentary
The short answer is yes, it's definitely similar. And enum variants are kinda like subclasses.
Slightly longer answer: #36997 (comment)
| secret_id: CatalogItemId, | ||
| contents: &str, | ||
| ) -> Result<(), AdapterError> { | ||
| for user_id in self.catalog().get_entry(&secret_id).used_by() { |
There was a problem hiding this comment.
user_id is a bit of a confusing name for the dependent catalog item ids.
Also, just to confirm:
- used_by is only direct dependencies
- connection always directly references the the secret -- as opposed to some possibility of transitive relationships/constraints
- reads from catalog are fast in-memory reads so sequential reads or relatively high counts of used_by entries aren't a concern for latency of processing the query
There was a problem hiding this comment.
- Yes
- For GCP Connections, yes. (I actually don't know if it holds for all the other connection types 😮)
- Good question. Time to ask Marty. EDIT: Yes, we have a cheap in-memory view of the catalog, including
used_by.
| .caching_secrets_reader | ||
| .read_string(gcp.credentials_json) | ||
| .await | ||
| .and_then(|json| GcpServiceAccountKeyTokenUri::validate_json(&json)) |
There was a problem hiding this comment.
Just to confirm, at a high level we're doing two things in this PR overall:
- Porting this logic to more cases -- i.e. validating connection details on alter secret ad not just creating the connection
- Updating the code to make it more abstract so we can set up constraints on the contents of a secret depended on by a connection
There was a problem hiding this comment.
I had a poke at making it less abstract (I mean, there's only one branch, "Gcp". Come on.) but I couldn't do it.
So I added a comment to secret_content_guards to explain why it's like that.
/// We rely on the caller to actually execute these checks because we don't know:
/// - which secrets the caller cares about
/// - which secrets require an async operation to fetch
///
/// For example, the ALTER SECRET caller should only perform checks on its own secret,
/// while the ALTER CONNECTION caller fetches and checks every secret from its connection.
pub fn secret_content_guards(
&self,
) -> Vec<(CatalogItemId, fn(&str) -> Result<(), anyhow::Error>)>| > ALTER SECRET gcp_good AS '{"type":"service_account","project_id":"y","client_email":"a@b.com","private_key":"${gcp-ssrf-key}","token_uri":"https://oauth2.googleapis.com/token"}' | ||
|
|
||
| > CREATE SECRET gcp_good_2 AS '{"type":"service_account","project_id":"x","client_email":"a@b.com","private_key":"${gcp-ssrf-key}","token_uri":"https://oauth2.googleapis.com/token"}' | ||
| > ALTER CONNECTION gcp_alter_conn SET (SERVICE ACCOUNT KEY = SECRET gcp_good_2) WITH (VALIDATE = false); |
There was a problem hiding this comment.
This testing looks good to me.
| let caching_secrets_reader = self.caching_secrets_reader.clone(); | ||
| let secrets_controller = Arc::clone(&self.secrets_controller); | ||
| let payload = self.extract_secret(session, &mut secret_as)?; | ||
| let contents = std::str::from_utf8(&payload).expect("validated as UTF-8 by extract_secret"); |
There was a problem hiding this comment.
validated that "validated as UTF-8 by extract_secret" is true
2 participants
This PR adds a simple framework for checking the Secrets used by a Connection when either the Secret or the Connection changes.
So far, it's just a hook to check GCP secrets for a bad
token_urifield.This is a followup to #36694.
Just for fun: Opus vs Fable code comparison