Add static account support to GCP secrets engine

[mweigel]

Hi all, I've started working on implementing static account functionality in the GCP secrets engine. I've also added the ability to rotate the root service account credentials. It seems to be working well so far and it'd be great to know if I'm on the right track and get some feedback before I go any further, thanks :)

TODO:

• Access token and service account key generation functions for static accounts
• More tests

[codecov]

Codecov Report

Merging #956 (f657368) into main (2786bb8) will increase coverage by 0.79%.
The diff coverage is 94.11%.

@@            Coverage Diff             @@
##             main     #956      +/-   ##
==========================================
+ Coverage   81.78%   82.58%   +0.79%     
==========================================
  Files          65       65              
  Lines        2992     3026      +34     
==========================================
+ Hits         2447     2499      +52     
+ Misses        545      527      -18     

Impacted Files	Coverage Δ
hvac/api/secrets_engines/gcp.py	89.36% <94.11%> (+41.02%)	⬆️

... and 1 file with indirect coverage changes

[briantist]

Hi @mweigel ! Welcome and thanks for putting this up.

I looked it over and I definitely think you're on the right track, it looks pretty good to me.
But I'm also not familiar with GCP at all so the terminology and such is pretty foreign to me, and I could be missing something.

I would at least recommend getting your local environment set up (see CONTRIBUTING.md), and ensuring you run the linter/formatter before pushing to avoid the CI lint failures:

poetry run black .
poetry run flake8 .

I would definitely like to get the diff coverage up as well, so looking forward to more tests :)

I'll see if any of the other maintainers have experience with GCP, but the code looks pretty straightforward, thanks again!

[mweigel]

@briantist Thanks for taking a look :) I'll keep working on this. I have my environment set up so I can run unit and integration tests. I'm testing out the new functionality in my GCP account currently.

[mweigel]

Ok, I've had some more time to work on this.

• I've added a unit test for each of the methods in the GCP secrets engine (covering existing methods as well) and coverage seems decent. I might have even gone overboard so see what you think. Happy to remove/refactor if it's too much.
• I've been testing the new functionality with my Vault and GCP setup and it seems to work well.

A couple of things to note that don't necessarily have to be implemented in this PR:

• The "/gcp/token/:roleset" and "/gcp/key/:roleset" Vault endpoints used by the existing "generate_oauth2_access_token" and "generate_service_account_key" roleset methods are deprecated.
• Vault has recently added impersonated-account token generation endpoints

So there are a couple of options for token and service account key generation. Both options leave the existing "generate_oauth2_access_token" and "generate_service_account_key" methods as they are (in this PR) so existing clients are not impacted. These methods could be marked as deprecated and removed in a future release.

1. Add a new method for each credential generation endpoint

Something like:

• generate_roleset_oauth2_access_token() -> "/gcp/roleset/:roleset/token"
• generate_roleset_service_account_key() -> "/gcp/roleset/:roleset/key"
• generate_impersonated_account_oauth2_access_token() -> "/gcp/impersonated-account/:impersonated-account/token"

This is essentially the road I've already started down in this PR by adding the "generate_static_account_oauth2_access_token" and "generate_static_account_service_account_key" methods (those names still feel a little clunky to me).

2. Make two new more generic credential generation methods and add an "account type" option

Something like:

• generate_secret_oauth2_access_token(account_type, ...)
• generate_secret_service_account_key(account_type, ...)

These methods would then call the, roleset, static account or impersonated account endpoints depending on what's chosen. This would mean replacing the methods I've just added. Not a big change.

I don't have a strong preference either way and I'm happy to implement either :)

[briantist]

@mweigel I've only skimmed the recent changes but I really appreciate the tests, I don't personally see it as overboard, a 41+% increase in coverage is lovely.

I'm going to see if any other maintainers are a little more familiar with GCP and can take a look.
In addition to me not being familiar with the platform, I've recently had a flood of external things taking up my attention so I've had precious little time, so hopefully I can get some additional review but so far this looks great and I'm going to try not to be a bottleneck on it.

Thanks!

[mweigel]

Great, thanks! Again, I appreciate you taking a look :)

[mweigel]

Hi @briantist, no worries. I used the "sync fork" button on my copy of the project and it has updated my branch with the last changes from main. Unfortunately it looks like a merge commit has been created though. I've also allowed maintainers to make changes to the branch.

If this is no good I can always close this PR and open a new one based on main right from the start. Let me know what you think, cheers.

[briantist]

Hi @briantist, no worries. I used the "sync fork" button on my copy of the project and it has updated my branch with the last changes from main. Unfortunately it looks like a merge commit has been created though. I've also allowed maintainers to make changes to the branch.

If this is no good I can always close this PR and open a new one based on main right from the start. Let me know what you think, cheers.

Thank you! I A merge commit is no problem in our PRs because we squash before merging them into main, resulting a single commit. Your branch contained some commits we didn't want in there though, so I just completed a rebase and force push now that the box was checked.

[briantist]

@mweigel thanks for this contribution! I appreciate you sticking with it and I love to see tests added so this is great. We're actively trying to smooth out the process of contributing so I hope we'll see more from you if you're able, thanks again!

[mweigel]

@mweigel thanks for this contribution! I appreciate you sticking with it and I love to see tests added so this is great. We're actively trying to smooth out the process of contributing so I hope we'll see more from you if you're able, thanks again!

Hi Brian, thanks for all your work on getting this in. I'll be around, and will actively be using this functionality once released, so if there are any issues that come up feel free to get in touch. I'll look at adding the GCP impersonated accounts as well which should be a smaller change. :)