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

feat(tls): generate client certificates for agents #938

Merged
merged 2 commits into from
Sep 10, 2024

Conversation

ebaron
Copy link
Member

@ebaron ebaron commented Sep 6, 2024

Welcome to Cryostat! 👋

Before contributing, make sure you have:

  • Read the contributing guidelines
  • Linked a relevant issue which this PR resolves
  • Linked any other relevant issues, PR's, or documentation, if any
  • Resolved all conflicts, if any
  • Rebased your branch PR on top of the latest upstream main branch
  • Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
  • Signed all commits: git commit -S -m "YOUR_COMMIT_MESSAGE"

Fixes: #936

Description of the change:

  • Creates a uniquely-named certificate for each target namespace for use by Cryostat agents within it. The certificate has a wildcard hostname within the namespace subdomain, and supports client and server usages.
    1. Create a Certificate in the install namespace with a cluster unique name (hashed install_namespace/cr_name/target_namespace)
    2. Once cert-manager creates the secret in the install namespace, copy it to the target namespace
    3. If the target namespace is removed, the Certificate and its secret will be deleted, along with the copy in the target namespace
    4. If the CR is deleted, the finalizer allows the operator to delete the secret copies from all target namespaces
  • Adds a test for finalizing CA cert secret copies in target namespaces
  • Makes use of the TARGET_NAMESPACES variable in the Makefile, previously used for ClusterCryostat

Motivation for the change:

How to manually test:

  1. Deploy PR
  2. Create multi-namespace Cryostat
  3. Add/remove namespaces
  4. Verify certificates/secrets are created/deleted in expected namespaces

@ebaron ebaron added the feat New feature or request label Sep 6, 2024
@ebaron ebaron requested a review from a team September 6, 2024 20:57
@mergify mergify bot added the safe-to-test label Sep 6, 2024
@ebaron
Copy link
Member Author

ebaron commented Sep 6, 2024

/build_test

Copy link

github-actions bot commented Sep 6, 2024

/build_test : At least one test failed ❌.
View Actions Run.

@andrewazores
Copy link
Member

I tried creating a Cryostat with targetNamespaces: ['cryostat', 'apps1', 'apps2'], where cryostat is also the installation namespace. Then I used oc get secrets -n $ns to check that the cert secret was created in each, which it was. Then I tried oc delete project apps1 - oc get secrets -n apps1 shows nothing (obviously), oc get secrets -n cryostat shows all three secrets still present. Then I did oc new-project apps1 and oc get secrets -n apps1, oc get secrets -n cryostat - the secret still exists in the installation namespace, but did not get (re-)copied into apps1.

I guess for this to work the Operator would also need to be watching for any namespaces to be created/deleted in the cluster, so that it can reconcile by deleting the original secrets if namespaces are deleted, or recreating/re-copying them if a matching namespace is recreated too?

Otherwise, the change itself looks good, though it seems that scorecard tests are failing.

@ebaron
Copy link
Member Author

ebaron commented Sep 9, 2024

I guess for this to work the Operator would also need to be watching for any namespaces to be created/deleted in the cluster, so that it can reconcile by deleting the original secrets if namespaces are deleted, or recreating/re-copying them if a matching namespace is recreated too?

I suppose other objects that are placed in target namespaces would suffer from the same issue (e.g. role bindings, CA secrets). This might be a bit tricky to handle. Since we can't have cross-namespace owner references, maybe we could do something like this:

  1. Apply a label to all cross-namespace objects pointing to the namespace/name of the CR
  2. Add a custom controller watch on cross-namepsace object types (e.g. secret, rolebinding) that checks for the label(s)
  3. If the label exists, enqueue a reconcile request for that CR's namespace/name
  4. Controller recreates any missing objects

As for deleting the certificate object from the install namespace for deleted namespaces, we could issue a get request for the namespace and, if it doesn't exist, delete the certificate and secret. I think the namespace deletion event should be captured with the above custom controller watch.

@ebaron
Copy link
Member Author

ebaron commented Sep 9, 2024

/build_test

Copy link

github-actions bot commented Sep 9, 2024

/build_test completed successfully ✅.
View Actions Run.

@ebaron
Copy link
Member Author

ebaron commented Sep 10, 2024

I filed #941 for the target namespace issue. I'll work on that next in a follow-up PR.

@ebaron ebaron merged commit 14e9331 into cryostatio:main Sep 10, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Story] Generate self-signed certificates for agents
2 participants