-
Notifications
You must be signed in to change notification settings - Fork 740
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
feat(web): Expose experimental account storage API #1494
Conversation
We prefer that non-test backend code be written in Java or Kotlin, rather than Groovy. The following files have been added and written in Groovy:
See our server-side commit conventions here. |
gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/AccountController.groovy
Outdated
Show resolved
Hide resolved
gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/AccountController.groovy
Outdated
Show resolved
Hide resolved
|
||
@PostMapping | ||
@ApiOperation('Creates a new account definition.') | ||
@PreAuthorize('isAuthenticated()') |
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.
Note: I've changed permissions here to allow any authenticated user to create an account definition. It's on them to specify relevant permissions. Requires WRITE permissions to update and otherwise use the account (same semantics as READ and WRITE permissions on accounts everywhere else).
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 forget how this was previously, but I suspect we'd like to lock this down more than this. At least WRITE permissions makes sense to me. I suppose other permissions likely need to be in place for spinnaker to be able to use an account, so I can see how opening it up could be ok...but if nothing else it feels like opening up to mistakes, or possibly even a denial of service as extra accounts use resources.
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.
Previously, I had an admin check on these endpoints. Given that users have to supply their own credentials references (and grant the running Spinnaker access to those secrets), there doesn't seem to be much to secure here. I was thinking perhaps a config setting or table for storing groups or roles that are allowed to create/update/delete resources here, though users will only be able to affect accounts they have permission to write, and they have the ability to grant permissions during create to whoever.
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.
Guess I need to learn more what's in a credentials reference...but isn't there still a possibility of some malicious user creating a ton of accounts -- even ones that spinnaker doesn't have access to -- and using a bunch of resources. I'm thinking of attempting to cache those accounts and then spamming the logs.
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.
That's a fair concern, though there's already a security setting for which group(s) are required in order to login. I haven't been able to think of a better way to manage the permissions to create accounts, though perhaps that could be a config setting rather than anything dynamic?
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.
For my own usage, I want users to be able to configure their own account credentials without involving the developers to update our config and redeploy anything. I also want to stop using shared account credentials, but that's difficult to do without providing a way for users to configure their own credentials (i.e., I don't have access to the same systems as my users do, so the user is responsible for linking their system to ours either way whether it be via importing a secret or by granting access to my shared credentials to their system). This is also why I'm thinking it might make sense to make configurable as some use cases could be like mine while other people's use cases could be more locked down.
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.
@dbyron-sf - I work with @jvz, and as jvz stated, we want people to be able to fully manage there accounts. That can be through creation or setting limitations.
One thing jvz and I have talked about is potentially allowing you to add a pluggable authorizer to allow users to specify the limitations that they wants. How does that sound?
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.
That could be ok...but sounds fancier/more complicated than maybe we need? Would a config flag be enough? I'm assuming fiat, so then it's configuring required permissions there...
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.
Yea, config flag would suffice as well. Generally permissions can get complicated, so having a pluggable authorizer may be nice. However, we can start with a config flag and if things get complicated, we can always change it :)
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.
After some offline conversations, sounds like we're going to stick with isAuthorized
for this PR and move to something that uses fiat as a second step. Is there some way to mark these endpoints as Alpha until they stabilize?
|
||
@PutMapping | ||
@ApiOperation('Updates an existing account definition.') | ||
@PreAuthorize("hasPermission(#definition.name, 'ACCOUNT', 'WRITE')") |
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 was also updated to allow users with WRITE permissions on the account to update it.
gate-web/src/main/groovy/com/netflix/spinnaker/gate/controllers/CredentialsController.groovy
Outdated
Show resolved
Hide resolved
This adds some of the REST APIs introduced in the experimental account storage API in Clouddriver to Gate. Initially, these APIs are only available for admins.
* fix(oauth): remove circular dependency on ExternalAuthTokenFilter bean in OAuth2SsoConfig when oauth2 is enabled (spinnaker#1492) Previously in spring 2.2.5, if oauth2 is enabled there is no circular dependency on ExternalAuthTokenFilter bean in OAuth2SsoConfig. In spring 2.2.13, if oauth2 is enabled there is a circular dependency error on ExternalAuthTokenFilter bean. This circular dependency results in an error when the gate application tries to start up. The application fails with error: BeanCurrentlyInCreationException: Error creating bean with name 'OAuth2SsoConfig': Bean with name 'OAuth2SsoConfig' has been injected into other beans [externalAuthTokenFilter] in its raw version as part of a circular reference, but has eventually been wrapped. To fix this error, add the Component annotation to ExternalAuthTokenFilter and remove the ExternalAuthTokenFilter bean from OAuth2SsoConfig. Co-authored-by: David Byron <dbyron@salesforce.com> * fix(web): disable keel by default as it is an optional service (spinnaker#1453) Co-authored-by: Justin Field <justin.field@armory.io> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * fix(api): fix movie quotes to match movie script (spinnaker#1423) Co-authored-by: Justin Field <justin.field@armory.io> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * chore(build): gradle 6.8.1 (spinnaker#1413) Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * chore(dependencies): Autobump korkVersion (spinnaker#1493) Co-authored-by: root <root@265713e22300> * chore(dependencies): Autobump korkVersion (spinnaker#1495) Co-authored-by: root <root@ca7e6659c3b3> * chore(dependencies): Autobump korkVersion (spinnaker#1496) Co-authored-by: root <root@7e927306ac3d> * fix(vulnerability): avoid expose gate endpoints (spinnaker#1497) * chore(dependencies): Autobump korkVersion (spinnaker#1501) Co-authored-by: root <root@01b270ba5f94> * chore(dependencies): Autobump fiatVersion (spinnaker#1504) Co-authored-by: root <root@73790eeb50df> * chore(dependencies): Autobump korkVersion (spinnaker#1502) Co-authored-by: root <root@a229fbda21b3> Co-authored-by: Matt <6519811+mattgogerly@users.noreply.github.com> * feat(web): Expose experimental account storage API (spinnaker#1494) * feat(web): Expose experimental account storage API This adds some of the REST APIs introduced in the experimental account storage API in Clouddriver to Gate. Initially, these APIs are only available for admins. * Combine account and credentials endpoints * Add docs on AccountDefinition * Add alpha annotations * chore(build): update mergify config (spinnaker#1506) Co-authored-by: Cameron Motevasselani <cameron@armory.io> * fix(gate): Typos in Account Management API (spinnaker#1510) * fix(gate/web): Fix typo in PreAuthorize annotation It appears that some variables were renamed while I worked on the PR and didn't update the annotations to match. * fix(gate/core): Fix retrofit signature error * fix(gate/web): Add explicit name property to AccountDefinition (spinnaker#1514) This fixes an authorization check error where Jackson knows how to handle the `name` property of an account definition, but SpEL does not see the property. Now the PostFilter annotation should work equivalently to the same filter check in Clouddriver. * chore(dependencies): Autobump korkVersion (spinnaker#1515) Co-authored-by: root <root@d38215f0a8da> * fix(authn/oauth2): prevent oauth2 redirect loops (spinnaker#1517) During setup of spinnaker authentication with oauth2 a common hurdle is a redirect loop. For example: spinnaker/spinnaker#5794 spinnaker/spinnaker#1630 Also, many threads in Slack discuss these problems. In fact this appears to be a common pitfall for the spring-security-oauth2-autoconfigure library in general. A light refresher on the ouath2 flow in play here seems worthwhile. The user is redirected from `/login` in gate to the external auth provider (google, github, etc.) and after successfully authenticating they are redirected back to the gate `/login` endpoint but this time with a code parameter that is to be used to request an access token. This request can fail for a variety of reasons, and if it does, the underlying spring library triggers a redirect to the `/error` endpoint. What causes the redirect loop for gate in particular (and for other users of the library in a similar fashion) is that the WebSecurityConfigurerAdapter in play is treating `/error` as an authenticated path and so instead of just returning with a 401, it re-redirects to `/login` and the redirect loop continues. My thought is that instead of a redirect loop, simply allowing the 401 to be returned will be a stronger more helpful signal as to what is going on. Hopefully it will save future first-time installers headaches. Spinnaker docs have included several troubleshooting hints and tips for how where you terminate SSL affects configuration etc. Even after following all of these and lots of spelunking through spinnaker github issues and combing over threads in slack, I found myself still experiencing a redirect loop even though I had applied all the combined wisdom that was applicable to my setup. As it turns out, I had a bad copy/paste of my client secret in my configuration. So the request to turn the code from google into an access token from google was failing with a 401. After much debugging and deep diving into the spring security code I found that had I turned on DEBUG in gate for these classes in gate-local.yml: ``` logging: level: org.springframework.security.web.authentication.SimpleUrlAuthenticationFailureHandler: DEBUG org.springframework.security.oauth2.client.filter.OAuth2ClientAuthenticationProcessingFilter: DEBUG ``` Then I would have seen in the logs that a 401 response was returned from google and perhaps it would have caused me to look closer at my botched client secret configuration. I think perhaps we don't want to require that all operators of spinnaker become spring-security-oauth2 experts. So I'm proposing adding `/error` to the list of paths in gate that aren't treated as authenticated. Thus short-circuiting the redirect loop and bringing to light helpful troubleshooting info that was previously more or less swallowed. * chore(ci): update setup-java GHA to v2 (spinnaker#1518) closes: spinnaker/spinnaker#6611 * fix(dependency): Issue with jackson-bom and kotlin-bom version conflict resolution while upgrading the spring-boot 2.3.x (spinnaker#1505) * fix(dependency): Introducing spring dependency management gradle plugin Spring boot has moved to gradle based dependency management from v2.3.x. This change has brought issue of conflict resolution failure of the Jackson-bom version and kotlin-bom version with gate service when it consumes the maven-bom generated by kork. The issue details are available in given link. https://docs.google.com/document/d/1Ck4KeoB1ER0aQUTnf3e-x-M3i2Ur0It7YaaxEMiMXls/edit To resolve this issue while upgrading gate service with spring v2.3.x, we must require the spring dependency management gradle plugin. * Revert "fix(dependency): Introducing spring dependency management gradle plugin" This reverts commit b3b2c9e. * fix(dependency): Issue with jackson-bom and kotlin-bom version conflict resolution while upgrading the spring-boot 2.3.x. The root cause of this issue is uncontrolled conflict resolution of jackson-bom and kotlin-bom dependency version imported from external maven BOM provided by kork-bom, as per the gradle documentation https://docs.gradle.org/6.9.1/userguide/platforms.html#sub:bom_import, we can use gradle enforcedPlatform closure as part of the implementation to strictly adhere the versions of direct and transitive dependencies imported BOM. implementation(enforcedPlatform("io.spinnaker.kork:kork-bom:$korkVersion")) * chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1519) Co-authored-by: root <root@92ccf252161e> * chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1520) Co-authored-by: root <root@8e952337838c> * chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1521) Co-authored-by: root <root@e15a7ff336aa> * chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1522) Co-authored-by: root <root@00e70a80e3dd> * chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1523) Co-authored-by: root <root@4cdab225ba98> * chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1524) Co-authored-by: root <root@2bf6cb9b4370> * chore(dependencies): Autobump fiatVersion (spinnaker#1528) Co-authored-by: root <root@69a0efe35201> * chore(ci): GHA - container image and apt package build & push (spinnaker#1529) to Google Artifact Registry see: spinnaker/rosco#841 Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * chore(ci): GHA - simplify build versioning (spinnaker#1530) - collapse version info gathering steps into single `build_variables` step - collapse version info parts into single string and use everywhere. - use short git sha: `(git rev-parse --short HEAD)` Note `build.yml` versioning is not compatible with Debian package building as gradle plugin enforces `^[0-9]+`. We don't publish master branch or release-* branches to GAR apt repository though. Prefixing the version with `<tag>-dev-` or something and publishing Debian packages is possible but may pollute `apt-cache policy spinnaker-rosco` output and overall be unnecessary with regular releases. * fix(plugins-test): try harder for the version of versionNotSupportedPlugin to actually not be supported (spinnaker#1532) Before this, a gate version >= 2.0.0 would cause versionNotSupportedPlugin to get used, causing tests to fail, and making it impossible to e.g. release gate. * chore(cit): GHA - plugin builds require SemVer (spinnaker#1531) I wanted to avoid confusion between a git tag `X` building version `X` and a master or release branch building version `X-dev-*` but it seems unavoidable. SemVer is required by plugins. See constraint: https://github.com/spinnaker/kork/blob/5dc6bb98615667f1b4f3e18445c1651d773c9f6b/kork-plugins/src/main/kotlin/com/netflix/spinnaker/kork/plugins/SpinnakerServiceVersionManager.kt#L47 changes: - fetch full git repository so that we can access previous tag in branch. Convert `release.yml` to this method instead of `run: git.. --unshallow`. - use previous git tag as start of version string. Cut the 'v' prefix from the tag, 'v1.2.3' -> '1.2.3' as required for Plugins (and Debians fwiw): `Caused by: Unexpected character 'LETTER(v)' at position '0', expecting '[DIGIT]'` - append `-dev-<branch_name|'pr'>` to designate that it is not an official version. The short git SHA and date time are NOT present on release versions (eg: 1.2.3) so that also differs. - do this version setting in `pr.yml` as well so we might pick up version issues in PR's and not just at merge. * chore(dependencies): Autobump korkVersion (spinnaker#1536) Co-authored-by: root <root@3f84b148af75> * chore(dependencies): Autobump spinnakerGradleVersion (spinnaker#1537) Co-authored-by: root <root@87e31b5c1a0c> * feat(credentials): Update account type discriminator (spinnaker#1533) This normalizes the type discriminator in account definitions to match that of account credentials instances (i.e., the type of CredentialsDefinition instance and Credentials instance are both specified through the "type" property). This also removes redundant authorization annotations that are better enforced by AccountDefinitionService in Clouddriver. Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> * resolved AuthConfig file conflict. Co-authored-by: Calvin Tse <sam.calvin.tse@gmail.com> Co-authored-by: David Byron <dbyron@salesforce.com> Co-authored-by: Emmanouil Katefidis <e.katefidis@outlook.com> Co-authored-by: Justin Field <justin.field@armory.io> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Thomas Swanson <tomswanson4444@gmail.com> Co-authored-by: Cameron Fieber <cfieber@netflix.com> Co-authored-by: spinnakerbot <spinbot@spinnaker.io> Co-authored-by: root <root@265713e22300> Co-authored-by: root <root@ca7e6659c3b3> Co-authored-by: root <root@7e927306ac3d> Co-authored-by: Cristhian Castaneda <ccastanedarivera@gmail.com> Co-authored-by: root <root@01b270ba5f94> Co-authored-by: root <root@73790eeb50df> Co-authored-by: root <root@a229fbda21b3> Co-authored-by: Matt <6519811+mattgogerly@users.noreply.github.com> Co-authored-by: Matt Sicker <boards@gmail.com> Co-authored-by: Cameron Motevasselani <cmotevasselani@gmail.com> Co-authored-by: Cameron Motevasselani <cameron@armory.io> Co-authored-by: Matt Sicker <msicker@apple.com> Co-authored-by: root <root@d38215f0a8da> Co-authored-by: Chris Phillips <4722632+chris-h-phillips@users.noreply.github.com> Co-authored-by: kskewes-sf <96093759+kskewes-sf@users.noreply.github.com> Co-authored-by: Sandesh <sandeshjainhyd@gmail.com> Co-authored-by: root <root@92ccf252161e> Co-authored-by: root <root@8e952337838c> Co-authored-by: root <root@e15a7ff336aa> Co-authored-by: root <root@00e70a80e3dd> Co-authored-by: root <root@4cdab225ba98> Co-authored-by: root <root@2bf6cb9b4370> Co-authored-by: root <root@69a0efe35201> Co-authored-by: David Byron <82477955+dbyron-sf@users.noreply.github.com> Co-authored-by: root <root@3f84b148af75> Co-authored-by: root <root@87e31b5c1a0c>
This adds some of the REST APIs introduced in the experimental account
storage API in Clouddriver to Gate. Initially, these APIs are only
available for admins.
spinnaker/spinnaker#6525
Requires spinnaker/clouddriver#5594