feat(auth): Add support for gdch_service_account credentials#5551
feat(auth): Add support for gdch_service_account credentials#5551howardjohn wants to merge 1 commit intogoogleapis:mainfrom
gdch_service_account credentials#5551Conversation
This adds support for this credential type, following the Golang and Python implementations which both have the same support. Testing: unit tests included; manual testing done in a live GDCH environment.
| Ok(BASE64_URL_SAFE_NO_PAD.encode(json.as_bytes())) | ||
| } | ||
|
|
||
| fn ecdsa_der_to_jose(der: &[u8], field_len: usize) -> Result<Vec<u8>> { |
There was a problem hiding this comment.
we can drop this logic if we depend on p256 (currently a dev dep). https://github.com/googleapis/google-cloud-rust/compare/main...howardjohn:google-cloud-rust:idtoken/gdch_service_account-eclib?expand=1
lmk if its a good tradeoff.
There was a problem hiding this comment.
I think is preferable to use p256, as we don't want to own and maintain some code for ECSDA handling. I just need to double check the situation on security and advisories. We had a lot of issues with the idtoken feature and jsonwebtoken crate (which requires p256 in tests).
There was a problem hiding this comment.
Provided some initial review, but I think ideally we would need to break this down into smaller PRs to make it easier to review and ship it. I still need to evaluate gdch_service_account method and how it would fit into the SDK, so I'll have to get back you on that after reading more about it. As I pointed out on this early review, there are points of code duplication and I think we can avoid that.
Are you in contact with Customer Support asking for this feature ?
This is a core auth feature on a private cloud environment, which we can't exercise integration tests for it here on CI, so we need to double check and tests things before merging it.
| Ok(BASE64_URL_SAFE_NO_PAD.encode(json.as_bytes())) | ||
| } | ||
|
|
||
| fn ecdsa_der_to_jose(der: &[u8], field_len: usize) -> Result<Vec<u8>> { |
There was a problem hiding this comment.
I think is preferable to use p256, as we don't want to own and maintain some code for ECSDA handling. I just need to double check the situation on security and advisories. We had a lot of issues with the idtoken feature and jsonwebtoken crate (which requires p256 in tests).
| } | ||
|
|
||
| #[derive(serde::Deserialize)] | ||
| struct TokenResponse { |
There was a problem hiding this comment.
we already have STS exchange logic and exactly this struct at src/auth/src/credentials/internal/sts_exchange.rs. I think we should reuse that
| } | ||
|
|
||
| #[async_trait::async_trait] | ||
| impl TokenProvider for GdchServiceAccountTokenProvider { |
There was a problem hiding this comment.
This looks a lot like what we do for External Account and we have the SubjectTokenProvider trait that can be used to reuse the logic. I need to get more familiar with GDCH authentication to see how we can maybe extend things so we can make it closer to an External Account.
| |b: external_account::Builder, s: Vec<String>| b.with_scopes(s) | ||
| ), | ||
| "gdch_service_account" => Err(BuilderError::not_supported( | ||
| "gdch_service_account is supported by credentials::idtoken::Builder::new(audience)", |
There was a problem hiding this comment.
this looks wrong. This macro builds AccessTokenCredentials and it seems like it can be supported by this credential type. And the error mentions idtoken.
|
|
||
| type TestResult = std::result::Result<(), Box<dyn Error>>; | ||
|
|
||
| const ES256_PRIVATE_KEY: &str = "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIEUByN/Cd73iTqf85VeQ74wWaZr6sMnkMY25RvOIUJ94oAoGCCqGSM49\nAwEHoUQDQgAEHf1LlK7P4qdsjslUqKVx5AlEBXN9VLzYYhC700o2DOthBjBFU7Yu\nmohy0DCDBPJ9pfiCPe/lZSFlvdl8Xyz9Lg==\n-----END EC PRIVATE KEY-----\n"; |
There was a problem hiding this comment.
we have test private keys declared on multiple places. I think we should reuse this. We have previous work on the RSA_PRIVATE_KEY which is reuse for tests.
|
|
||
| type TestResult = std::result::Result<(), Box<dyn Error>>; | ||
|
|
||
| const ES256_PRIVATE_KEY: &str = "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIEUByN/Cd73iTqf85VeQ74wWaZr6sMnkMY25RvOIUJ94oAoGCCqGSM49\nAwEHoUQDQgAEHf1LlK7P4qdsjslUqKVx5AlEBXN9VLzYYhC700o2DOthBjBFU7Yu\nmohy0DCDBPJ9pfiCPe/lZSFlvdl8Xyz9Lg==\n-----END EC PRIVATE KEY-----\n"; |
| "format_version": "1", | ||
| "project": "test-project", | ||
| "private_key_id": "test-private-key-id", | ||
| "private_key": "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIEUByN/Cd73iTqf85VeQ74wWaZr6sMnkMY25RvOIUJ94oAoGCCqGSM49\nAwEHoUQDQgAEHf1LlK7P4qdsjslUqKVx5AlEBXN9VLzYYhC700o2DOthBjBFU7Yu\nmohy0DCDBPJ9pfiCPe/lZSFlvdl8Xyz9Lg==\n-----END EC PRIVATE KEY-----\n", |
There was a problem hiding this comment.
Reuse declared EC_PRIVATE_KEY for tests. We have previous work on the RSA_PRIVATE_KEY which is reuse for tests.
| "format_version": "1", | ||
| "project": "test-project", | ||
| "private_key_id": "test-private-key-id", | ||
| "private_key": "-----BEGIN EC PRIVATE KEY-----\nMHcCAQEEIEUByN/Cd73iTqf85VeQ74wWaZr6sMnkMY25RvOIUJ94oAoGCCqGSM49\nAwEHoUQDQgAEHf1LlK7P4qdsjslUqKVx5AlEBXN9VLzYYhC700o2DOthBjBFU7Yu\nmohy0DCDBPJ9pfiCPe/lZSFlvdl8Xyz9Lg==\n-----END EC PRIVATE KEY-----\n", |
There was a problem hiding this comment.
Reuse declared EC_PRIVATE_KEY for tests. We have previous work on the RSA_PRIVATE_KEY which is reuse for tests.
| pub mod external_account; | ||
| pub(crate) mod external_account_sources; | ||
| #[cfg(feature = "idtoken")] | ||
| pub(crate) mod gdch_service_account; |
There was a problem hiding this comment.
I don't think the gdch_service_account should be behind the idtoken feature. Probably a typo or copy/paste error.
| "external_account signer is not supported", | ||
| )), | ||
| "gdch_service_account" => Err(BuilderError::not_supported( | ||
| "gdch_service_account signer is not supported", |
There was a problem hiding this comment.
is this flow not supported or not implemented yet ?
There was a problem hiding this comment.
So looking through the Go impl a bit closer, I think this is not quite right. But also really annoying 🙂
Here is my understanding:
- GDCH has an "identity token" concept but its not really a typical idtoken. It is not a JWT idtoken.
- This is fetched via STS using the private_key from the credential json, with a required audience to be set
- In the go library, this is not supported on the idtoken flow; only on the standard credential flow (NewCredentialsFromFile). This requires a custom
audienceoption to be used or it errors.
I was getting these mixed up a bit and made it so you can only use the idtoken path for gdch_service_account; it should probably match the Go impl where there is no idtoken support and instead you use the credentials apth but with a audience pass in somewhere
This adds support for this credential type, following the Golang and Python implementations which both have the same support.
Testing: unit tests included; manual testing done in a live GDCH environment.
go impl for reference https://github.com/googleapis/google-cloud-go/blob/main/auth/credentials/internal/gdch/gdch.go