-
Notifications
You must be signed in to change notification settings - Fork 14
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
Adds CI for testing annotation-based authn on OpenShift with DAP #111
Conversation
6ab45cb
to
adf9017
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.
This looks great and the build is passing. Are we planning to address the TODOs before merging ? If not I think we should get rid of the comments and create some issues instead.
utils.sh
Outdated
if [[ $conjur_crb != "" ]]; then | ||
kubectl get clusterrole $conjur_crb -o yaml | ||
else | ||
echo "Couldn't find ClusterRole beginning with 'conjur-authenticator'" |
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.
We should really migrate ClusterRole
to Role
. I'd thought there was an issue for this in the epic but I see that ClusterRole
is alive and well. 😓
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 what I'd like to do here is to:
- Continue using a ClusterRole, consistent with what cyberark/kubernetes-conjur-deploy does for DAP
- BUT, switch to using a (namespace-scoped) RoleBinding, rather than rely on the ClusterRoleBinding
that the Helm chart now creates. This will require two changes:
- Include creation of a (namespace-scoped) RoleBinding for Conjur OSS helm deployments #114 (Include creation of RoleBinding in demo scripts)
- Eliminate default creation of ClusterRoleBinding cyberark/conjur-oss-helm-chart#95 (Eliminate creation of ClusterRoleBinding in Helm chart)
The reason that I'm not too worried about using ClusterRole
is that it's just a convenient template. By itself it doesn't add permissions until it's used/applied by a ClusterRoleBinding
or a (namespace-scoped) RoleBinding
.
The existing use of ClusterRoleBinding
is worrisome, though, since that gives authn-k8s RBAC permissions for EVERY namespace. It's much better to use namespace-scoped RoleBindings
for each particular namespace that requires authn-k8s authentication (i.e. principle of least privilege).
I filed the issues listed above. I tried making a fix for those and testing, but something isn't quite right. Will get that working tomorrow.
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.
Regarding the TODO's, I filed a PR for addressing the RBAC in cyberark/kubernetes-conjur-deploy (cyberark/kubernetes-conjur-deploy#158)...
But I can't test:
- With OpenShift using DAP + these demos until the above PR #158 is merged.
- With OpenShift using Conjur OSS Helm chart until all of the OpenShift support is working (Guy's PR)
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.
@doodlesbykumbi : I deleted all of the TODO
s, and I updated the Description above to say that
the work of testing OpenShift using DeploymentConfigs
as application identity
will be covered by:
The current CI test cases test authn-k8s on OpenShift platforms using host-ID-based authentication. This change adds test cases for testing authn-k8s on OpenShift using DAP and the newer-style, annotation-based authentication, where the Kubernetes resources being authenticated are configured as annotations on the host definition in the Conjur policy. Addresses Issue #109
adf9017
to
a216c61
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.
Thank you for addressing those comments, and creating all those issues. The path forward and the PR look good to me.
Adds CI for testing annotation-based authn on OpenShift with DAP
The current CI test cases test authn-k8s on OpenShift platforms using host-ID-based
authentication. This change adds test cases for testing authn-k8s on OpenShift using
DAP and the newer-style, annotation-based authentication, where the Kubernetes
resources being authenticated are configured as annotations on the host definition
in the Conjur policy.
NOTE: The annotation-based tests that are being added do not include the use of
the OpenShift
DeploymentConfigs
resources as application identity. This will beaddressed with the following:
Addresses Issue #109