Handle node client certs #26

enj · 2019-05-10T19:25:22Z

Signed-off-by: Monis Khan mkhan@redhat.com

node_client.go

mrogers950 · 2019-05-10T19:55:19Z

main.go

-			return err
-		}
-		approvalMsg += " (no SAN validation)"
+		return fmt.Errorf("failed to list machines: %v", err)


Do we want to do this now? testing this PR would then be contingent on both the bootstrap self-approve script and PR to disable the auto-approve. just wondering

If it passes CI we know that we don't need the failover, right?

Also, you could get a transient error and hit the failover path because the code is too blunt. I think we need to remove it before GA so might as well do it now.

If it passes CI we know that we don't need the failover, right?

Yeah, otherwise removing it might stop us from bootstrapping with no machine API available. So let's see what CI says.. I am +1 on getting rid of the failover here though.

csr_check.go

enj · 2019-05-10T21:41:40Z

/retest

enj · 2019-05-10T23:56:16Z

/retest

enj · 2019-05-11T03:32:36Z

/retest

enj · 2019-05-11T12:17:22Z

/retest

enj · 2019-05-11T13:32:01Z

Once we got past the AWS issues this ran fine. Now I just need to add more unit tests.

enj · 2019-05-11T13:52:17Z

Well at least the serving certs bit is working fine. The client cert is failing on RBAC:

I0511 12:39:05.629859       1 main.go:122] CSR csr-5696g not authorized: failed to check if node ip-10-0-146-108.ec2.internal already exists: nodes "ip-10-0-146-108.ec2.internal" is forbidden: User "system:serviceaccount:openshift-cluster-machine-approver:machine-approver-sa" cannot get resource "nodes" in API group "" at the cluster scope

enxebre · 2019-05-13T08:35:09Z

csr_check.go

+}
+
+func findMatchingMachineFromInternalDNS(nodeName string, machines []v1beta1.Machine) (v1beta1.Machine, bool) {
+	for _, machine := range machines {


I'm not sure this is something we can guarantee being available for every cloud. Not sure NodeInternalDNS is exposed on the API in Azure/GCP. It might be only exposed via metadata which makes it unreachable and a no go for the machine API controllers in those clouds.

Any other way to very specifically find the machine?

would the privateIP be sufficient?
medium term we should include the node/machine providerID here but this is not supported today

The CSR only includes the (future) node name so we have to base the check on that...

couldn't we get the privateIP here (in future providerID as well) https://github.com/openshift/cluster-machine-approver/pull/26/files#diff-a482a1a664767249971b0b71ca9baaf0R194 and pass it down to findMatchingMachineBy*()? I see the node does not exist yet at that point

enxebre · 2019-05-13T09:00:52Z

csr_check.go

+		return fmt.Errorf("machine for node %s already has node ref", nodeName)
+	}
+
+	if !isWorkerMachine(nodeMachine) {


I understand we don't want masters to go through this however is this effectively dropping machines which does not contain https://github.com/openshift/cluster-machine-approver/pull/26/files#diff-a482a1a664767249971b0b71ca9baaf0R28? How about any additional user machines/machineSets? I wonder if we might want to have an specific label for this e.g machine.openshift.io/CSR: true or possible better the other way around and have masters to opt out with machine.openshift.io/CSR-need-approval: false WDYT?

I wanted the most strict whitelist based approach. This approval flow is only meant for workers so this check seemed appropriate. I am not for or against adding a new label, we just need to discuss that type of change with the other stakeholders.

mrogers950 · 2019-05-13T13:54:50Z

csr_check.go

@@ -88,39 +106,39 @@ func validateCSRContents(req *certificatesv1beta1.CertificateSigningRequest, csr
 // authorizeCSR authorizes the CertificateSigningRequest req for a node's server certificate.


This comment needs to be updated

mrogers950 · 2019-05-13T15:46:55Z

csr_check_test.go

+							certificatesv1beta1.UsageKeyEncipherment,
+							certificatesv1beta1.UsageDigitalSignature,
+							certificatesv1beta1.UsageClientAuth,
+							certificatesv1beta1.UsageServerAuth,


We should have variants that have less usages and wrong ones (that replace client with server) for good measure.

mrogers950 · 2019-05-13T16:14:23Z

csr_check.go

+		return fmt.Errorf("machine for node %s is not a worker", nodeName)
+	}
+
+	start := nodeMachine.CreationTimestamp.Add(-maxMachineDelta) // TODO maybe this should be really small to account only for clock drift


Reduce as discussed on call: 5-10 sec

mrogers950 · 2019-05-13T16:15:34Z

csr_check.go

+}
+
+func isWorkerMachine(machine v1beta1.Machine) bool {
+	return machine.Labels[machineRoleKey] == machineRoleTypeValue && machine.Labels[machineTypeKey] == machineRoleTypeValue


Change to the to-be-agreed-upon label

mrogers950 · 2019-05-13T19:50:23Z

@enj looks good, squash the commits and I'll tag.

mrogers950 · 2019-05-13T20:08:12Z

/lgtm

… client CSRs Because of the existense of this ClusterRoleBinding, kube-controller-manager approves all node client CSRs requests and therefore allowing wrongful actors to get node level client priviledges. cluster-machine-approver [1] takes on the role of approving only certain node client CSR requests. [1]: openshift/cluster-machine-approver#26

enj · 2019-05-13T21:11:32Z

/test all

enj · 2019-05-13T21:26:53Z

/refresh

derekwaynecarr · 2019-05-14T15:03:36Z

csr_check.go

+// 2. Node does not exist
+// 3. Use machine API internal DNS to locate matching machine based on node name
+// 4. Machine must not have a node ref
+// 5. Machine must opt-in to CSR auto approve via label


can we weigh pros/cons of label versus annotation?

derekwaynecarr · 2019-05-14T15:10:03Z

csr_check.go

+}
+
+func machineWantsCSRAutoApprove(machine v1beta1.Machine) bool {
+	return machine.Labels[machineCSRAutoApproveKey] == machineCSRAutoApproveValue


just want to know pros/cons of label versus annotation.

i tend to think of labels as grouping, and annotations as behavioral.

enj · 2019-05-14T15:47:38Z

/hold

enj · 2019-05-15T19:00:34Z

apiVersion: v1
kind: ConfigMap
metadata:
  name: machine-approver-config
  namespace: openshift-cluster-machine-approver
data:
  config.yaml: |-
    nodeClientCert:
      disabled: true

I0515 18:57:00.248954       1 config.go:23] machine approver config: {NodeClientCert:{Disabled:true}}
I0515 18:57:00.249006       1 main.go:183] Starting Machine Approver
I0515 18:57:25.336449       1 main.go:107] CSR csr-cwcjt added
I0515 18:57:25.344832       1 main.go:132] CSR csr-cwcjt not authorized: CSR csr-cwcjt for node client cert rejected as the flow is disabled
I0515 18:57:25.344856       1 main.go:164] Error syncing csr csr-cwcjt: CSR csr-cwcjt for node client cert rejected as the flow is disabled

Signed-off-by: Monis Khan <mkhan@redhat.com>

mrogers950 · 2019-05-15T19:28:41Z

/hold cancel
/lgtm

openshift-ci-robot · 2019-05-15T19:28:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enj, mrogers950

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [mrogers950]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

enj · 2019-05-15T19:30:28Z

/cherrypick release-4.1

openshift-cherrypick-robot · 2019-05-15T19:30:29Z

@enj: once the present PR merges, I will cherry-pick it on top of release-4.1 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

mrogers950 · 2019-05-15T19:40:32Z

/retest

mrogers950 · 2019-05-15T19:48:42Z

/retest

enj · 2019-05-15T22:26:59Z

/retest

enj · 2019-05-16T00:45:39Z

/retest

openshift-bot · 2019-05-16T01:52:30Z

/retest

Please review the full test history for this PR and help us cut down flakes.

…lete PR for cluster-machine-approver [1] is taking over the approval of CSRs for client certificates for Machines that end up as Nodes in Openshift clusters. But during bootstrapping, cluster-machine-approver is not available and therefore, this service is required to approve CSRs until we have successfully bootstrapped the control plane, after which cluster-machine-approver or users take over the role of approving any new CSRs. Currently, all CSRs are automatically approved without any condition, this PR scopes it to only during bootstrapping phase, securing the endpoint for later use. [1]: openshift/cluster-machine-approver#26

enj · 2019-05-16T07:38:13Z

/retest

openshift-cherrypick-robot · 2019-05-16T08:51:51Z

@enj: new pull request created: #27

In response to this:

/cherrypick release-4.1

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…lete PR for cluster-machine-approver [1] is taking over the approval of CSRs for client certificates for Machines that end up as Nodes in Openshift clusters. But during bootstrapping, cluster-machine-approver is not available and therefore, this service is required to approve CSRs until we have successfully bootstrapped the control plane, after which cluster-machine-approver or users take over the role of approving any new CSRs. Currently, all CSRs are automatically approved without any condition, this PR scopes it to only during bootstrapping phase, securing the endpoint for later use. [1]: openshift/cluster-machine-approver#26

… client CSRs Because of the existense of this ClusterRoleBinding, kube-controller-manager approves all node client CSRs requests and therefore allowing wrongful actors to get node level client priviledges. cluster-machine-approver [1] takes on the role of approving only certain node client CSR requests. [1]: openshift/cluster-machine-approver#26

…lete PR for cluster-machine-approver [1] is taking over the approval of CSRs for client certificates for Machines that end up as Nodes in Openshift clusters. But during bootstrapping, cluster-machine-approver is not available and therefore, this service is required to approve CSRs until we have successfully bootstrapped the control plane, after which cluster-machine-approver or users take over the role of approving any new CSRs. Currently, all CSRs are automatically approved without any condition, this PR scopes it to only during bootstrapping phase, securing the endpoint for later use. [1]: openshift/cluster-machine-approver#26

zeenix · 2019-07-02T10:44:56Z

csr_check.go

+	nodeBootstrapperUsername = "system:serviceaccount:openshift-machine-config-operator:node-bootstrapper"
+
+	maxMachineClockSkew = 10 * time.Second
+	maxMachineDelta     = 10 * time.Minute


May I ask where this choice of number comes from? In our CI system (nested virt env), we seem to be quite a bit late:

E0701 18:15:26.348374 1 main.go:174] CSR csr-xl8fs creation time 2019-07-01 18:15:26 +0000 UTC not in range (2019-07-01 17:40:40 +0000 UTC, 2019-07-01 17:50:50 +0000 UTC)

We are not performing any cryptographic verification. Instead we are making a best effort check based on the state of the machine API. Thus we need to keep the window very short to prevent the chance of an attacker guessing the correct name of a new node. Increasing the window gives the attacker more time and lowers the security of this approver.

@enj Thanks for explaining. However the time of choice, seems very arbitrary to me. Please correct me if I'm wrong since I'm no security export but isn't 10mis a very large window already for an attacker? Does this check really add any real security to the system?

We also rate limit based on the total amount of outstanding CSRs. I would not call it great security, but we make it hard enough to exploit for the time being.

Note that the solution is not to increase the window. The correct solution is to cryptographically tie the CSR with the new node that is being provisioned. Then an attacker cannot create a CSR to steal credentials. It would also allow nodes to create more than one CSR (it allows them to not store state about previously requested CSRs).

Thanks again for your quick reply and detailed explanation.

I'm actually more suggesting if the window is needed at all. Is the 10 mins time short enough for attackers to not be able to exploit? If it's not, I would rather suggest this check be removed as it's causing us problems and not helping in that case. The real solution you mention, sounds good and it won't affect us negatively.

You have to know when that 10 min windows occurs - it is actually pretty hard to know without access to the machine API or the underlying cloud API. That basically leaves brute force methods, which the rate limiting does a good job of preventing. Without the window you could just lay down a handful of guess CSRs and see if you get lucky. The windows forces a much more involved attack. Basically it forces an active attacker instead of a passive one. The implementations of the machine API need to properly coordinate the creation of the machine object and the related CSR object.

Feel free to bring this up in the ABC arch call for further discussion if desired.

Thanks again for another very thorough and detailed answer. Much appreciated!

You'll have to understand that this breaks our (Openshift Installer on libvirt) CI right now. It's a nested environment so understandably things are slower than normal, despite the fact that the VM has 8 core, 32G RAM and and SSD drive assigned to it. Apart from arguing for removal or extension of the time window check, if you have any other suggestions on how to fix or workaround this issue, please do let me know cause this is very high priority for us (multiple teams).

I'm afraid I don't know what ABC is. I'm still relatively new here but I'd love to join if we could discuss it there. :)

openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 10, 2019

openshift-ci-robot requested review from mrogers950 and sjenning May 10, 2019 19:28

mrogers950 reviewed May 10, 2019

View reviewed changes

node_client.go Show resolved Hide resolved

mrogers950 reviewed May 10, 2019

View reviewed changes

csr_check.go Show resolved Hide resolved

enxebre reviewed May 13, 2019

View reviewed changes

mrogers950 reviewed May 13, 2019

View reviewed changes

enj force-pushed the enj/i/node_client branch from df72578 to f63854b Compare May 13, 2019 19:59

abhinavdahiya mentioned this pull request May 13, 2019

operator: remove the cluster role binding that auto approves all node client CSRs openshift/machine-config-operator#741

Merged

abhinavdahiya mentioned this pull request May 13, 2019

data: add approve-csr service to approve CSRs until bootstrap is complete openshift/installer#1747

Merged

derekwaynecarr reviewed May 14, 2019

View reviewed changes

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 14, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 15, 2019

Handle node client certs

d7e76c4

Signed-off-by: Monis Khan <mkhan@redhat.com>

enj force-pushed the enj/i/node_client branch from a89d98d to d7e76c4 Compare May 15, 2019 19:27

openshift-ci-robot assigned mrogers950 May 15, 2019

openshift-ci-robot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels May 15, 2019

openshift-merge-robot merged commit 74b193c into openshift:master May 16, 2019

openshift-cherrypick-robot mentioned this pull request May 16, 2019

[release-4.1] Handle node client certs #27

Merged

runcom mentioned this pull request May 16, 2019

Bug 1707162: Remove the cluster role binding that auto approves all node client CSRs openshift/machine-config-operator#768

Merged

zeenix reviewed Jul 2, 2019

View reviewed changes

enxebre mentioned this pull request Jul 11, 2019

Max CSR limit #31

Closed

		@@ -88,39 +106,39 @@ func validateCSRContents(req *certificatesv1beta1.CertificateSigningRequest, csr
		// authorizeCSR authorizes the CertificateSigningRequest req for a node's server certificate.

Handle node client certs #26

Handle node client certs #26

Conversation

enj commented May 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented May 10, 2019

enj commented May 10, 2019

enj commented May 11, 2019

enj commented May 11, 2019

enj commented May 11, 2019

enj commented May 11, 2019

enxebre May 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre May 13, 2019 • edited Loading

Choose a reason for hiding this comment

enxebre May 13, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrogers950 commented May 13, 2019

mrogers950 commented May 13, 2019

enj commented May 13, 2019

enj commented May 13, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enj commented May 14, 2019

enj commented May 15, 2019

mrogers950 commented May 15, 2019

openshift-ci-robot commented May 15, 2019

enj commented May 15, 2019

openshift-cherrypick-robot commented May 15, 2019

mrogers950 commented May 15, 2019

mrogers950 commented May 15, 2019

enj commented May 15, 2019

enj commented May 16, 2019

openshift-bot commented May 16, 2019

enj commented May 16, 2019

openshift-cherrypick-robot commented May 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre May 13, 2019 •

edited

Loading

enxebre May 13, 2019 •

edited

Loading

enxebre May 13, 2019 •

edited

Loading