-
Notifications
You must be signed in to change notification settings - Fork 20
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(auth): add config option to deploy oauth2_proxy instead of openshift-oauth-proxy #803
feat(auth): add config option to deploy oauth2_proxy instead of openshift-oauth-proxy #803
Conversation
4820a25
to
48fe34d
Compare
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 we may just want to handle this by using the IsOpenShift
method instead of adding a CRD property. Downstream we want this code path to be inaccessible since we don't plan to build/release the OAuth2 proxy.
Hmm, okay. I can do that, but it does mean that once installed on OpenShift the only way to control who has access to the application will be around who has the right roles to pass the SAR (https://github.com/cryostatio/cryostat-operator/pull/799/files#diff-587612cdc407d5ab4abf28789670950c567419bc4ada045dda2e0c1070e74470R632). But I guess that's already how it works in 2.4 if you allow the OpenShift auth integration, so that's still at parity, right? |
Right |
Isn't there also the Basic Auth option now too? |
Yes, but enabling that in OpenShift gives you the option of logging in with OpenShift SSO or with Basic auth - it doesn't disable the OpenShift SSO part. So it's just another way to define users, ones who don't necessarily have OpenShift logins, but it doesn't provide a way to block access to OpenShift logins. I don't think there is a way to configure the OpenShift OAuth Proxy to turn off the OpenShift SSO integration, but maybe there's a way to configure it to be just slightly broken on purpose... it would still present the login button to the user, but maybe it can be made to always fail at that stage. Not sure if that would be worthwhile. |
I suppose for full 2.4 parity we'd need the ability to customize the permissions used for the SAR. |
Would it make sense to add that directly as structures within the CR's new |
Ah, I also just realized I forgot to fix this up: cryostat-operator/internal/controllers/common/resource_definitions/resource_definitions.go Line 638 in d455719
I'll do that now. |
I think the CR API looks a little clumsy, but all these cases are covered:
What is not possible is deploying on OpenShift, disabling SSO, and enabling Basic only. It's also a little clumsy that on OpenShift this defaults to enabling authz, but on non-OpenShift it defaults to deploying wide open. As suggested in #206 we could generate a secret and enable Basic auth by default and attach that to the CR status like the Grafana credentials in 2.4. This way the non-OpenShift user does need to have some kind of cluster access to be able to read the CR status or to read the secret contents in order to log in. |
internal/controllers/common/resource_definitions/resource_definitions.go
Show resolved
Hide resolved
internal/controllers/common/resource_definitions/resource_definitions.go
Outdated
Show resolved
Hide resolved
api/v1beta2/cryostat_types.go
Outdated
// The TokenReview that programmatic clients (CLI utilities and others presenting Bearer auth tokens) must pass in | ||
// order to access the application. If not specified, the default role required is "create pods/exec" in the Cryostat | ||
// application's installation namespace. | ||
// +optional | ||
// +operator-sdk:csv:customresourcedefinitions:type=spec | ||
TokenReview *authzv1.ResourceAttributes `json:"tokenReview,omitempty"` |
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 TokenReview right here? That API is used to determine who a token belongs to isn't it? Also, do we need to have separate options for interactive vs non-interactive?
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 guess it is actually TokenAccessReview
, not just TokenReview
. I'll rename it.
Also, do we need to have separate options for interactive vs non-interactive?
I think conceptually it makes sense to require the same access reviews either way, but unfortunately the way the proxy works is that you configure the interactive access as a list of resources to check for the SAR, whereas the Bearer auth access is only a single object with route prefixes mapped to TARs.
cryostatio/cryostat-helm#132 (comment)
So I think the most understandable model from the end user's point of view is probably to have a single AccessReview
field, which is used for both TokenAccessReview on a catch-all /
route prefix, and as a singleton list for SubjectAccessReview. This way there is just one particular role that any client needs (human user or programmatic client) to access any route, other than the health check endpoints.
The most flexible model is to have a list of SARs and a separate value for TAR but again with the catch-all /
route prefix. This lets the CR represent all of the configuration possible on the proxy, but it does mean that you can do weird things like require more stringent permissions for browser access than for Bearer token access to the same endpoint.
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.
FWIW over in the Helm chart, I have it as a single Bearer TokenAccessReview on the catch-all /
prefix, and a list of SubjectAccessReviews. The SAR list defaults to a singleton of the same required permission as the TAR.
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.
Ah so the delegate URLs option doesn't support a set of permissions per path? I agree with the simplest option then. One SubjectAccessReview field with one ResourceAttributes struct, and this will get used for both openshift-sar
and openshift-delegate-urls
.
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 unfortunately correct, the delegate URLs option which enables Bearer token handling only takes a single permission per path prefix. But it also doesn't allow for inspecting the request at all beyond checking that the request path matches the path prefix, so we can't even differentiate between ex. GET
and POST
to apply different role mapping.
internal/controllers/common/resource_definitions/resource_definitions.go
Outdated
Show resolved
Hide resolved
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.
Looks good! Just a couple more doc suggestions you can take or leave
Co-authored-by: Elliott Baron <[email protected]> Signed-off-by: Andrew Azores <[email protected]>
Welcome to Cryostat! 👋
Before contributing, make sure you have:
main
branch[chore, ci, docs, feat, fix, test]
git commit -S -m "YOUR_COMMIT_MESSAGE"
Related to #710
Fixes #206
Description of the change:
This enables the Operator to deploy
oauth2_proxy
in place ofopenshift-oauth-proxy
.openshift-oauth-proxy
is deployed by default when the Operator detects that it is installed on OpenShift. There is a new CRD option to disable this integration and force the use ofoauth2_proxy
instead. If installed on a non-OpenShift Kubernetes thenoauth2_proxy
will always be deployed.If
oauth2_proxy
is deployed it will also respect the same htpasswd Basic auth configuration that I added in #802. If no htpasswd Basic auth is configured thenoauth2_proxy
will allow any requests to pass without authz.Motivation for the change:
How to manually test:
oc create secret generic basicauth --from-file=htpasswd.conf
) patch the CR to add:disableOpenShiftSso: true
basicAuth
objectdisableOpenShiftSso
flag should now be ineffective and OAuth2 Proxy should be installed regardless. ThebasicAuth
config option should still function as outlined in the previous steps.