Skip to content
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

fix(k8s-rbac): update permission mapping between k8s and cryostat #1042

Merged
merged 13 commits into from
Aug 26, 2022

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Aug 16, 2022

Fixes #979
Related to cryostatio/cryostat-operator#440

Use alternatives for mapping between k8s resource to cryostat since flightRecorder/Recordings are deprecated.

@tthvo tthvo marked this pull request as draft August 16, 2022 22:37
@tthvo
Copy link
Member Author

tthvo commented Aug 16, 2022

Marked as draft to update tests and check with running cluster.

@tthvo
Copy link
Member Author

tthvo commented Aug 19, 2022

Currently, this is my idea for refining the mappings:

# Access pods (i.e application) and establish connections (i.e. using service instead of endpoints)
TARGET=pods,services 
# Access Cryostat to operate (i.e. execute) recordings
RECORDING=pods,pods/exec,cryostats.operator.cryostat.io
# Mount certificates from secrets into Cryostat
CERTIFICATE=pods,secrets,cryostats.operator.cryostat.io 
# Access Cryostat to add credentials (i.e. via webUI I think)
CREDENTIALS=pods,cryostats.operator.cryostat.io 

Any thoughts @andrewazores @ebaron?

@ebaron
Copy link
Member

ebaron commented Aug 22, 2022

Currently, this is my idea for refining the mappings:

# Access pods (i.e application) and establish connections (i.e. using service instead of endpoints)
TARGET=pods,services 
# Access Cryostat to operate (i.e. execute) recordings
RECORDING=pods,pods/exec,cryostats.operator.cryostat.io
# Mount certificates from secrets into Cryostat
CERTIFICATE=pods,secrets,cryostats.operator.cryostat.io 
# Access Cryostat to add credentials (i.e. via webUI I think)
CREDENTIALS=pods,cryostats.operator.cryostat.io 

Any thoughts @andrewazores @ebaron?

I think this is good. While the certificates themselves may not be sensitive, the act of telling Cryostat to trust them should be privileged. I would also add Secrets for credentials as well, since that is a common use case for Secrets in Kubernetes.

@tthvo
Copy link
Member Author

tthvo commented Aug 23, 2022

@andrewazores I believe #625 is resolved since #1037? Just come across a FIXME to disable reading kubeconfig in OpenShiftAuthManagerTest.

@andrewazores
Copy link
Member

@tthvo thanks: #1057

@tthvo tthvo marked this pull request as ready for review August 23, 2022 18:58
@tthvo
Copy link
Member Author

tthvo commented Aug 23, 2022

@ebaron @andrewazores Ready for review :D

For tests, I just removed references in deprecated apis. There was no issue with subresource being used in tests (I thought it would be part of the getResource but using getSubResource is the right way).

@tthvo tthvo force-pushed the update-cryostat-permissions branch from 611f4fd to 614abcf Compare August 23, 2022 19:04
andrewazores
andrewazores previously approved these changes Aug 23, 2022
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

ebaron
ebaron previously approved these changes Aug 24, 2022
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me. This should probably be committed close to cryostatio/cryostat-operator#440, otherwise the OAuth tokens will not have the correct set of permissions.

@tthvo
Copy link
Member Author

tthvo commented Aug 24, 2022

Right, I will update cryostatio/cryostat-operator#440 oauth role with these default permissions and tests it.

@tthvo
Copy link
Member Author

tthvo commented Aug 25, 2022

Since including Secrets in mapping is denied due escalation security issue, I am thinking of its "closely" resembling resource ConfigMap though I am not sure if it is good alternative.

See original cryostatio/cryostat-operator#440 (comment)

@ebaron
Copy link
Member

ebaron commented Aug 25, 2022

Since including Secrets in mapping is denied due escalation security issue, I am thinking of its "closely" resembling resource ConfigMap though I am not sure if it is good alternative.

See original cryostatio/cryostat-operator#440 (comment)

Config Maps are similar, but not used for sensitive data, while Secrets are. I can't think of any other reasonable substitute though, so I'd say either use Config Maps or remove it entirely.

@tthvo
Copy link
Member Author

tthvo commented Aug 25, 2022

Right, I would just remove secrets instead of adding ConfigMap as it could means unprotected data which is not the case for certificates and credentials.

@tthvo tthvo dismissed stale reviews from ebaron and andrewazores via 8f38ceb August 25, 2022 17:14
@tthvo
Copy link
Member Author

tthvo commented Aug 25, 2022

I found some ConfigMap that actually contains certificate data. I think in the case of certificates, we could use ConfigMap?

$ oc get cm
NAME                       DATA   AGE
auth-properties            1      24h
kube-root-ca.crt           1      65d
openshift-service-ca.crt   1      65d

Checking kube-root-ca.crt

$ oc get cm/kube-root-ca.crt -o json
{
    "apiVersion": "v1",
    "data": {
        "ca.crt": [...]
    },
    "kind": "ConfigMap",
    "metadata": {
        "annotations": {
            "kubernetes.io/description": "Contains a CA bundle that can be used to verify the kube-apiserver when using internal endpoints such as the internal service IP or kubernetes.default.svc. No other usage is guaranteed across distributions of Kubernetes clusters."
        },
        "creationTimestamp": "2022-08-25T15:24:24Z",
        "name": "kube-root-ca.crt",
        "namespace": "default",
        "resourceVersion": "17608",
        "uid": "6698a4f7-a1a3-431b-a8fb-89f2ac097313"
    }
}

@tthvo tthvo marked this pull request as draft August 25, 2022 18:51
@ebaron
Copy link
Member

ebaron commented Aug 25, 2022

I found some ConfigMap that actually contains certificate data. I think in the case of certificates, we could use ConfigMap?

I don't think we gain much from doing that. Certificates can be stored in Config Maps because they aren't sensitive, but for Cryostat, the certificate permission allows the user to tell Cryostat to trust a certificate. This should be somewhat of a privileged action, and I don't think creating/updating config maps in the namespace captures it. Permission to create/update the Cryostat CR is better, because that captures more of an admin role.

@tthvo
Copy link
Member Author

tthvo commented Aug 25, 2022

Oh right! Make sense! Thanks a lot for explaining! I will finish up features to add custom role on top of default and clean up tests before marking this ready,

@ebaron
Copy link
Member

ebaron commented Aug 25, 2022

Thanks, once this is ready, I'll test this one along with the operator PR and we should (hopefully) be done.

@tthvo tthvo marked this pull request as ready for review August 26, 2022 00:14
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Works well for me!

@ebaron ebaron merged commit 6db0486 into cryostatio:main Aug 26, 2022
@tthvo tthvo deleted the update-cryostat-permissions branch August 27, 2022 04:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Task] Select default permissions that do not use FlightRecorder/Recording
3 participants