-
Notifications
You must be signed in to change notification settings - Fork 13
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
ROX-11640: RHSSO dynamic clients rotation API #1236
Conversation
Skipping CI for Draft Pull Request. |
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.
Just suggested some naming/method changes.
Please also update the API documentation(./openapi/fleet-manager-private-admin.yaml
).
@@ -415,6 +417,22 @@ func (h adminCentralHandler) GetCentralDefaultVersion(w http.ResponseWriter, r * | |||
handlers.Handle(w, r, cfg, http.StatusOK) | |||
} | |||
|
|||
func (h adminCentralHandler) RotateSecrets(w http.ResponseWriter, r *http.Request) { |
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.
The name of this handler could be ambiguous. We also store other secrets within a central request of FM DB which are not rotated by this operation. I'd suggest to make it more specific like you did in the service, something like `RotateCentralRHSSOClient'.
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.
My original idea was that we might want to extend this endpoint with more secret rotation in the future, however, there's no indication it will ever be the case. I'll rename the handler 👍
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 need to rename, see comment below.
@@ -256,6 +256,9 @@ func (s *options) buildAPIBaseRouter(mainRouter *mux.Router, basePath string, op | |||
adminCentralsRouter.HandleFunc("/{id}", adminCentralHandler.Update). | |||
Name(logger.NewLogEvent("admin-update-central", "[admin] update central by id").ToString()). | |||
Methods(http.MethodPatch) | |||
adminCentralsRouter.HandleFunc("/{id}/rotate-secrets", adminCentralHandler.RotateSecrets). |
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.
See comment about the handler name, the route should also reflect that.
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.
Hmm, I think we have two ways to go here:
- Either keep this one "generic" and leave it open for extending it.
- Create "RPC-style" API which has options like
rotate/dynamic-client
,rotate/dns
or whatever else we might want to rotate for an existing instance in the future.
Having the extensibility in mind, I wouldn't mind keeping it "unspecific" and have the option to extend it further down the road.
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.
Good point. Lets keep it generic. I already have another use case in mind for this endpoint. Rotating the backed up namespace secrets if we need to do CA rotation.
@@ -256,6 +256,9 @@ func (s *options) buildAPIBaseRouter(mainRouter *mux.Router, basePath string, op | |||
adminCentralsRouter.HandleFunc("/{id}", adminCentralHandler.Update). | |||
Name(logger.NewLogEvent("admin-update-central", "[admin] update central by id").ToString()). | |||
Methods(http.MethodPatch) | |||
adminCentralsRouter.HandleFunc("/{id}/rotate-secrets", adminCentralHandler.RotateSecrets). | |||
Name(logger.NewLogEvent("admin-rotate-central-secrets", "[admin] rotate central secrets by id").ToString()). | |||
Methods(http.MethodPatch) |
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'd prefer http.MethodPost
at this point. For me PATCH
indicates that I partially update the resource with the resource body I send. Which doesn't match to what the endpoint is doing.
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 think POST
does not apply here because POST
indicates the creation of a new resource, while PATCH
indicates partial modification of an existing resource.
I agree with you that having PATCH
without a body is weird as well(although If I read correctly does not go against RFC. IMHO out of all 3 potential methods(POST
, PUT
, PATCH
) I think PATCH
comes the closest to the standard.
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.
What we're doing here isn't really REST since it's not necessarily related to CRUD
(you don't really create a resource, or update it per-se, but rather trigger an action).
Looking at the overall industry, POST
seems to be the common standard to use when triggering actions like revoking things, let's use it instead of PATCH
.
Both are also open for extending it, like we said in case we want to rotate more secrets than just the sso.r.c dynamic client.
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.
POST
it is
@@ -256,6 +256,9 @@ func (s *options) buildAPIBaseRouter(mainRouter *mux.Router, basePath string, op | |||
adminCentralsRouter.HandleFunc("/{id}", adminCentralHandler.Update). | |||
Name(logger.NewLogEvent("admin-update-central", "[admin] update central by id").ToString()). | |||
Methods(http.MethodPatch) | |||
adminCentralsRouter.HandleFunc("/{id}/rotate-secrets", adminCentralHandler.RotateSecrets). | |||
Name(logger.NewLogEvent("admin-rotate-central-secrets", "[admin] rotate central secrets by id").ToString()). | |||
Methods(http.MethodPatch) |
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.
What we're doing here isn't really REST since it's not necessarily related to CRUD
(you don't really create a resource, or update it per-se, but rather trigger an action).
Looking at the overall industry, POST
seems to be the common standard to use when triggering actions like revoking things, let's use it instead of PATCH
.
Both are also open for extending it, like we said in case we want to rotate more secrets than just the sso.r.c dynamic client.
@@ -256,6 +256,9 @@ func (s *options) buildAPIBaseRouter(mainRouter *mux.Router, basePath string, op | |||
adminCentralsRouter.HandleFunc("/{id}", adminCentralHandler.Update). | |||
Name(logger.NewLogEvent("admin-update-central", "[admin] update central by id").ToString()). | |||
Methods(http.MethodPatch) | |||
adminCentralsRouter.HandleFunc("/{id}/rotate-secrets", adminCentralHandler.RotateSecrets). |
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.
Hmm, I think we have two ways to go here:
- Either keep this one "generic" and leave it open for extending it.
- Create "RPC-style" API which has options like
rotate/dynamic-client
,rotate/dns
or whatever else we might want to rotate for an existing instance in the future.
Having the extensibility in mind, I wouldn't mind keeping it "unspecific" and have the option to extend it further down the road.
pkg/errors/errors.go
Outdated
ErrorClientRotationNotConfigured ServiceErrorCode = 116 | ||
ErrorClientRotationNotConfiguredReason string = "RHSSO client rotation is not configured" |
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.
Is it really ClientRotationNotConfigured
or rather DynamicClientNotConfigured
?
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.
It's possible for client rotation not to be configured but dynamic clients to be configured, because in code we give preference to static configuration if it's configured
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.
Should we call it DynamicClientNotUsed
then? My point is we don't have a configuration for client rotation, and if anyone other than the two of us reads the error message they might be confused as to what is going on / why it isn't working; and which configuration option to set 😅
@@ -2,12 +2,10 @@ package dinosaurmgrs | |||
|
|||
import ( | |||
"context" | |||
"fmt" | |||
"github.com/stackrox/acs-fleet-manager/internal/dinosaur/pkg/rhsso" |
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.
Let's format this properly and put it below to the non-standard packages.
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.
LGTM
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaus67, ivan-degtiarenko, johannes94 The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Co-authored-by: dhaus67 <[email protected]>
New changes are detected. LGTM label has been removed. |
Description
This PR creates an admin API that:
In case of failure to either use new client or delete the previous one, client ID is logged that allows for ACSCS engineer to manually delete stale client.
In case dynamic clients are not configured,
404
status code is returned.The dynamic clients API client is initialized once, however, it might not be initialized properly if dynamic clients are not configured. Whether dynamic clients are configured is checked on every request.
Checklist (Definition of Done)
Test manual
ROX-12345: ...
Test manual
TODO: Add manual testing efforts
(You need to set up
SSO_CLIENT_ID_DEFAULT
andSSO_CLIENT_SECRET_DEFAULT
, as well as specify--central-idp-client-id
as empty string)make deploy/bootstrap
make deploy/dev
cloud-service-sensible-declarative-configs
secret and see client ID and secret values/rotate-secrets
endpointcloud-service-sensible-declarative-configs