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

[Bug Fix] gke: enable basic auth #129

Merged
merged 4 commits into from
Jan 4, 2020
Merged

Conversation

hasheddan
Copy link
Member

@hasheddan hasheddan commented Jan 3, 2020

Signed-off-by: hasheddan georgedanielmangum@gmail.com

Description of your changes

Currently the v1beta1 GKECluster is not usable from Crossplane due to the fact that the client certificate that is issued by GKE does not contain cluster-admin privileges. Basic auth with username / password is automatically enabled in v1alpha3 GKECluster, which makes it impossible to turn off. This update enables basic auth only when a user provides a username. It is strictly additive to the API and ensures that connection is possible while still providing the ability to lock down a cluster by not issuing a client certificate or enabling basic auth.

If merged, this should be backported to release-0.4 and patch release v0.4.1 should be created.

Fixes #128

References:
https://crossplane.slack.com/archives/CEG3T90A1/p1578059370045600
kubernetes/kubernetes#65400
https://issuetracker.google.com/u/0/issues/111101728

To test this fix I have done the following:

Dynamically provisioned a GKE cluster with issueClientCertificate: true and verified that basic auth is enabled and username / password are included in the connection secret:

apiVersion: compute.crossplane.io/v1alpha1
kind: KubernetesCluster
metadata:
  name: app-kubernetes
spec:
  classSelector:
    matchLabels:
      example: "true"
  writeConnectionSecretToRef:
    name: k8scluster
---
apiVersion: container.gcp.crossplane.io/v1beta1
kind: GKEClusterClass
metadata:
  name: gkecluster-standard
  labels:
    example: "true"
specTemplate:
  forProvider:
    location: us-central1 # indicates regional cluster
    masterAuth:
      clientCertificateConfig:
        issueClientCertificate: true
  writeConnectionSecretsToNamespace: crossplane-system
  providerRef:
    name: gcp-provider
  reclaimPolicy: Delete

Dynamically provisioned a GKE cluster with masterAuth omitted and verified that basic auth is not enabled and username / password are not included in the connection secret:

apiVersion: compute.crossplane.io/v1alpha1
kind: KubernetesCluster
metadata:
  name: app-kubernetes-omit
spec:
  classSelector:
    matchLabels:
      omit: "true"
  writeConnectionSecretToRef:
    name: omitk8scluster
---
apiVersion: container.gcp.crossplane.io/v1beta1
kind: GKEClusterClass
metadata:
  name: gkecluster-omit
  labels:
    omit: "true"
specTemplate:
  forProvider:
    location: us-central1 # indicates regional cluster
  writeConnectionSecretsToNamespace: crossplane-system
  providerRef:
    name: gcp-provider
  reclaimPolicy: Delete

Dynamically provisioned a GKE cluster with issueClientCertificate: false and verified that basic auth is not enabled and username / password are not included in the connection secret:

apiVersion: compute.crossplane.io/v1alpha1
kind: KubernetesCluster
metadata:
  name: app-kubernetes-noauth
spec:
  classSelector:
    matchLabels:
      noauth: "true"
  writeConnectionSecretToRef:
    name: noauthk8scluster
---
apiVersion: container.gcp.crossplane.io/v1beta1
kind: GKEClusterClass
metadata:
  name: gkecluster-noauth
  labels:
    noauth: "true"
specTemplate:
  forProvider:
    location: us-central1 # indicates regional cluster
    masterAuth:
      clientCertificateConfig:
        issueClientCertificate: false
  writeConnectionSecretsToNamespace: crossplane-system
  providerRef:
    name: gcp-provider
  reclaimPolicy: Delete

I have also scheduled KubernetesApplications to both the first and second examples above (after also provisioning node pools) and have confirmed that scheduling is successful for the first and fails for the second.

Checklist

I have:

  • Run make reviewable to ensure this PR is ready for review.
  • Ensured this PR contains a neat, self documenting set of commits.
  • Updated any relevant documentation, examples, or release notes.
  • Updated the dependencies in app.yaml to include any new role permissions.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
@hasheddan hasheddan added bug Something isn't working security labels Jan 3, 2020
@hasheddan hasheddan requested review from negz and muvaf January 3, 2020 16:19
Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
@upbound-bot
Copy link
Collaborator

60% (+0.02%) vs master 60%

@negz
Copy link
Member

negz commented Jan 3, 2020

@hasheddan Would it be possible to instead expose the master username (but not password) in our spec, and automatically generate a master password only if and when a master username is specified? This might be a little less surprising than automatically enabling basic auth when cert auth is enabled, but would still allow users to provision a cluster they can authenticate to by explicitly enabling basic auth and/or cert auth.

@hasheddan
Copy link
Member Author

@negz yes that would certainly be possible. The only downside to that is that it does change the API. However, since it is purely additive I don't believe it would require a version bump?

@negz
Copy link
Member

negz commented Jan 3, 2020

However, since it is purely additive I don't believe it would require a version bump?

That's my understanding. It would be additive and not change existing behaviour, and thus be backward compatible.

Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
Signed-off-by: hasheddan <georgedanielmangum@gmail.com>
@hasheddan
Copy link
Member Author

@negz updates have been made. I also ran the same set of tests but with this additional configuration:

---
apiVersion: compute.crossplane.io/v1alpha1
kind: KubernetesCluster
metadata:
  name: app-kubernetes-basicauth
  labels:
    basicauth: "true"
spec:
  classSelector:
    matchLabels:
      basicauth: "true"
  writeConnectionSecretToRef:
    name: basicauthk8scluster
---
apiVersion: container.gcp.crossplane.io/v1beta1
kind: GKEClusterClass
metadata:
  name: gkecluster-basicauth
  labels:
    basicauth: "true"
specTemplate:
  forProvider:
    location: us-central1 # indicates regional cluster
    masterAuth:
      username: myCoolUser
  writeConnectionSecretsToNamespace: crossplane-system
  providerRef:
    name: gcp-provider
  reclaimPolicy: Delete

I confirmed that myCoolUser was set as username and basic auth was enabled. I also confirmed that a KubernetesApplication could be successfully scheduled to the cluster. Lastly, I confirmed that only asking for a client certificate would not enable basic auth and provisioning would fail.

@upbound-bot
Copy link
Collaborator

61% (+0.04%) vs master 60%

@negz
Copy link
Member

negz commented Jan 3, 2020

@hasheddan I was thinking we'd generate and set a random password if the admin user was specified. It seems that instead we submit only the username. What does this mean for basic auth - is it possible to authenticate by specifying the admin username and an empty password?

@hasheddan
Copy link
Member Author

@negz yep! If you provide a username and do not provide a password, GCP automatically generates it and then we propagate it back in the connection Secret.

@vadasambar
Copy link

This works! Thank you @hasheddan !

@hasheddan
Copy link
Member Author

@vadasambar glad to hear that! #133 will make this available in the next patch release :)

@hasheddan hasheddan changed the title [Bug Fix] gke: enable basic auth when client cert is enabled [Bug Fix] gke: enable basic auth Jan 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working security
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Kubeconfig in connection secret is not working in the latest (0.4) version
4 participants