Skip to content

SDN-5342: Signer username validation #2560

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Apr 10, 2025
Merged

SDN-5342: Signer username validation #2560

merged 4 commits into from
Apr 10, 2025

Conversation

pperiyasamy
Copy link
Member

In CNO we have an approver that signs certs automatically, without checking any identity information, so this PR enhances the signer to check on CSR's user name that is populated by API server upon receiving CSR request from the nodes. So it's sufficient to verify the user name before signing the CSR.

It also adds required unit tests to test signer controller, it needed to change from using kubernetes.Clientset to cnoclient.Client for accessing CSR's UpdateApproval API. This needs to be tested on OCP cluster.

cc @kyrtapz @trozet @jcaamano

@openshift-ci-robot openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Nov 8, 2024
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Nov 8, 2024 •
edited by openshift-ci bot
Loading

@pperiyasamy: This pull request references SDN-5342 which is a valid jira issue.

In response to this:

In CNO we have an approver that signs certs automatically, without checking any identity information, so this PR enhances the signer to check on CSR's user name that is populated by API server upon receiving CSR request from the nodes. So it's sufficient to verify the user name before signing the CSR.

It also adds required unit tests to test signer controller, it needed to change from using kubernetes.Clientset to cnoclient.Client for accessing CSR's UpdateApproval API. This needs to be tested on OCP cluster.

cc @kyrtapz @trozet @jcaamano

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 openshift-eng/jira-lifecycle-plugin repository.

@pperiyasamy pperiyasamy force-pushed the signer-username-validation branch from 326b533 to 868580a Compare November 8, 2024 18:14
@pperiyasamy
Copy link
Member Author

/retest

@pperiyasamy
Copy link
Member Author

/assign @kyrtapz @trozet @jcaamano

@pperiyasamy pperiyasamy force-pushed the signer-username-validation branch from 868580a to 123242b Compare November 13, 2024 15:45
@pperiyasamy
Copy link
Member Author

/retest

@martinkennelly
Copy link
Contributor

/assign @martinkennelly

@pperiyasamy
Copy link
Member Author

/testwith openshift/cluster-network-operator/master/e2e-aws-ovn-ipsec-upgrade openshift/machine-config-operator#4854

@huiran0826
Copy link

huiran0826 commented Mar 5, 2025 •
edited
Loading

pre-merge tested it, sent CSR manually via kubectl for signing, CNO rejected that CSR which was expected.

% cat  cno-csr.yaml                               
apiVersion: certificates.k8s.io/v1
kind: CertificateSigningRequest
metadata:
  generateName: ipsec-csr-ip-test-
  labels:
    k8s.ovn.org/ipsec-csr: test-node
spec:
  request: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURSBSRVFVRVNULS0tLS0KTUlJQ2lEQ0NBWEFDQVFBd1F6RUxNQWtHQTFVRUJoTUNWVk14RmpBVUJnTlZCQW9NRFc5MmJtdDFZbVZ5Ym1WMApaWE14RFRBTEJnTlZCQXNNQkd0cGJtUXhEVEFMQmdOVkJBTU1CRlJGVTFRd2dnRWlNQTBHQ1NxR1NJYjNEUUVCCkFRVUFBNElCRHdBd2dnRUtBb0lCQVFERC96ZGhpYWppVlJJQmhMeTIwR1RENWE1QTBuZEVsVXg1ckEyWHVNZ0gKL2g1Yk11SWlvOUhOR1lVdnVwU1ZZNFJhV01CbC9yRFZYQzZPenZzVk53Vk92UVlnTnlhcVJzWnJrY1lSUTAycApsT0NFWTM3VStLQXN3RDF4amxiTGp3YU56Z2hsVVU5U2c0TFY4VkorQ3JLcFZId2laelBoVGpLUVVBZTBJbHRMCmlXRmNwTit2NC9GeGdKcXRKQXZtUjFZenN0ZUVmOEFwZExvNHpyUDJuMEJzUmx5TnlTVUxDWWFFekpkbXE2QWkKckVVdEVFekVNUEtqR1ZJZzljZDlvYVpuUTZDKzlXcUQwcUttWmJXZ2piNXV5OEpmdGxKNGxqbjNRNExPZWNnNgpqUFNXZ3VIV1NMYUFzYmg2bFNMdEkydVNRNnppYlBMdkIwWTR4WnA4ZjV4ckFnTUJBQUdnQURBTkJna3Foa2lHCjl3MEJBUXNGQUFPQ0FRRUFzQW00VDhHK3lUekJnWTBaeE1oeEQ0eFFWSk84Nm1FSmRsaVZMMUVsaTlIcG9Wa1MKNHRmWlpuNXdBUGhKRGtSMjZCUjJseE9hZDJYdWhCWkl0MjFqOS9CWFNtdmJpbmc2R3hva2pGWFN2VFh1eHNIUgpDdUhWZ09DcUpxN1puOCtnT0ZaMG51OGVIcUNSRDFyZjBEdUdmNWphbDN4UVhOY0xMSEo5VEFLVGdnU0lqL21GCk9QS21wMEJEL3l3VkkrWlpNQ3ZFYWNLMUhINTV6Zk1rK2VSRTlIdWVpMWk4SkswYmZqZUI5Z3BFek9ZSEhVZ2oKOTd0NHE2YkZUTnIrbTh3N3FUYzZaU0I2Z20yVUhOYTVIRUtKaXdMdWdvR0V2K0wzamZsUE5idlBReVVRbkFpVAo1TDV3RFYxV2ZBV2FHdk5saXZYWm9sVGloTFpEQWl1S25tRkRmUT09Ci0tLS0tRU5EIENFUlRJRklDQVRFIFJFUVVFU1QtLS0tLQo= 
  signerName: network.openshift.io/signer
  usages:
  - ipsec tunnel
  username: system:ovn-node:ip-10-0-10-68

% oc get csr -o yaml
apiVersion: v1
items:
- apiVersion: certificates.k8s.io/v1
  kind: CertificateSigningRequest
  metadata:
    creationTimestamp: "2025-03-05T08:24:45Z"
    generateName: ipsec-csr-ip-test-
    labels:
      k8s.ovn.org/ipsec-csr: test-node
    name: ipsec-csr-ip-test-ndkd8
    resourceVersion: "108541"
    uid: 85838c3f-7d56-44a6-95c3-1a617131eed6
  spec:
    extra:
      authentication.kubernetes.io/credential-id:
      - X509SHA256=daad4da1f04861c673084eb815d3eb5df80b4cc84d7570f4694c15d6f45477da
    groups:
    - system:masters
    - system:authenticated
    request: LS0tLS1CRUdJTiBDRVJUSUZJQ0FURSBSRVFVRVNULS0tLS0KTUlJQ2lEQ0NBWEFDQVFBd1F6RUxNQWtHQTFVRUJoTUNWVk14RmpBVUJnTlZCQW9NRFc5MmJtdDFZbVZ5Ym1WMApaWE14RFRBTEJnTlZCQXNNQkd0cGJtUXhEVEFMQmdOVkJBTU1CRlJGVTFRd2dnRWlNQTBHQ1NxR1NJYjNEUUVCCkFRVUFBNElCRHdBd2dnRUtBb0lCQVFERC96ZGhpYWppVlJJQmhMeTIwR1RENWE1QTBuZEVsVXg1ckEyWHVNZ0gKL2g1Yk11SWlvOUhOR1lVdnVwU1ZZNFJhV01CbC9yRFZYQzZPenZzVk53Vk92UVlnTnlhcVJzWnJrY1lSUTAycApsT0NFWTM3VStLQXN3RDF4amxiTGp3YU56Z2hsVVU5U2c0TFY4VkorQ3JLcFZId2laelBoVGpLUVVBZTBJbHRMCmlXRmNwTit2NC9GeGdKcXRKQXZtUjFZenN0ZUVmOEFwZExvNHpyUDJuMEJzUmx5TnlTVUxDWWFFekpkbXE2QWkKckVVdEVFekVNUEtqR1ZJZzljZDlvYVpuUTZDKzlXcUQwcUttWmJXZ2piNXV5OEpmdGxKNGxqbjNRNExPZWNnNgpqUFNXZ3VIV1NMYUFzYmg2bFNMdEkydVNRNnppYlBMdkIwWTR4WnA4ZjV4ckFnTUJBQUdnQURBTkJna3Foa2lHCjl3MEJBUXNGQUFPQ0FRRUFzQW00VDhHK3lUekJnWTBaeE1oeEQ0eFFWSk84Nm1FSmRsaVZMMUVsaTlIcG9Wa1MKNHRmWlpuNXdBUGhKRGtSMjZCUjJseE9hZDJYdWhCWkl0MjFqOS9CWFNtdmJpbmc2R3hva2pGWFN2VFh1eHNIUgpDdUhWZ09DcUpxN1puOCtnT0ZaMG51OGVIcUNSRDFyZjBEdUdmNWphbDN4UVhOY0xMSEo5VEFLVGdnU0lqL21GCk9QS21wMEJEL3l3VkkrWlpNQ3ZFYWNLMUhINTV6Zk1rK2VSRTlIdWVpMWk4SkswYmZqZUI5Z3BFek9ZSEhVZ2oKOTd0NHE2YkZUTnIrbTh3N3FUYzZaU0I2Z20yVUhOYTVIRUtKaXdMdWdvR0V2K0wzamZsUE5idlBReVVRbkFpVAo1TDV3RFYxV2ZBV2FHdk5saXZYWm9sVGloTFpEQWl1S25tRkRmUT09Ci0tLS0tRU5EIENFUlRJRklDQVRFIFJFUVVFU1QtLS0tLQo=
    signerName: network.openshift.io/signer
    usages:
    - ipsec tunnel
    username: system:admin
  status:
    conditions:
    - lastTransitionTime: "2025-03-05T08:24:45Z"
      lastUpdateTime: "2025-03-05T08:24:45Z"
      message: Certificate Signing Request is set with invalid user name, can't sign
        it
      reason: CSRInvalidUser
      status: "True"
      type: Failed
kind: List
metadata:
  resourceVersion: ""

martinkennelly
martinkennelly reviewed Mar 7, 2025
View reviewed changes
Copy link
Contributor

@martinkennelly martinkennelly left a comment

Choose a reason for hiding this comment

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

is there a e2e test somewhere to validate this code works for ipsec?
Looks good to me - nice PR! I like switching to ocp crypto lib too! Was there a reason you switched in this PR?

@pperiyasamy
Validate CSR user name before signing it
093a33c 
Signed-off-by: Periyasamy Palanisamy <[email protected]>
@pperiyasamy
Use cno client to update CSR approval
0b672af 
Signed-off-by: Periyasamy Palanisamy <[email protected]>
@pperiyasamy
Add unit tests for cert signer
b5a792a 
Signed-off-by: Periyasamy Palanisamy <[email protected]>
@pperiyasamy
Use library-go crypto utility for generating CA
c88f5d8 
Signed-off-by: Periyasamy Palanisamy <[email protected]>
@pperiyasamy pperiyasamy force-pushed the signer-username-validation branch from 123242b to c88f5d8 Compare March 7, 2025 16:59
@pperiyasamy
Copy link
Member Author

is there a e2e test somewhere to validate this code works for ipsec?

This is being tested in the ipsec install CI lanes while configuring IPsec in the ipsec init container here, so i think we may not need an e2e test specifically for this.

I like switching to ocp crypto lib too! Was there a reason you switched in this PR?

This is just to reuse library-go utility to create CA which aligns with OCP (instead of CNO creating its own self signed cert) for the unit test.

@pperiyasamy
Copy link
Member Author

/retest-required

@martinkennelly
Copy link
Contributor

@pperiyasamy How can i validate this works in the CI? link?

@martinkennelly
Copy link
Contributor

@pperiyasamy can you point me to the jobs that execute it? e2e-ovn-ipsec-step-registry && e2e-metal-ipi-ovn-ipv6-ipsec ? And post install, can you point me to the origin tests ? I want to educate myself.
I looked here but couldnt find ipsec tests. https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-network-operator/2560/pull-ci-openshift-cluster-network-operator-master-e2e-ovn-ipsec-step-registry/1899044072979435520/artifacts/e2e-ovn-ipsec-step-registry/openshift-e2e-test/artifacts/e2e.log

@pperiyasamy
Copy link
Member Author

pperiyasamy commented Mar 19, 2025 •
edited
Loading

@pperiyasamy can you point me to the jobs that execute it? e2e-ovn-ipsec-step-registry && e2e-metal-ipi-ovn-ipv6-ipsec ? And post install, can you point me to the origin tests ? I want to educate myself. I looked here but couldnt find ipsec tests. https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_cluster-network-operator/2560/pull-ci-openshift-cluster-network-operator-master-e2e-ovn-ipsec-step-registry/1899044072979435520/artifacts/e2e-ovn-ipsec-step-registry/openshift-e2e-test/artifacts/e2e.log

@martinkennelly currently these jobs only verifies the ipsec deployment and runs regular e2e test suite but doesn't run ipsec specific tests. IPsec tests are run with CI lane introduced with this PR openshift/release#61740.

@martinkennelly
Copy link
Contributor

ack - do you think its valuable to at least do a run-with command to the bot in-order to test that PR with this?

@martinkennelly
Copy link
Contributor

Peri, the e2e test suite thats run for the current ip sec jobs would verify this PR is working because intra node traffic wouldnt work if this PR failed right?

@pperiyasamy
Copy link
Member Author

yes @martinkennelly , this particular PR change is already validated in e2e-ovn-ipsec-step-registry job, otherwise ipsec pods would have crashed.

openshift-ovn-kubernetes                           ovn-ipsec-host-2jbff                                              2/2     Running                  0               63m     10.0.125.165   ip-10-0-125-165.ec2.internal   <none>           <none>
openshift-ovn-kubernetes                           ovn-ipsec-host-fzmsx                                              2/2     Running                  0               63m     10.0.120.83    ip-10-0-120-83.ec2.internal    <none>           <none>
openshift-ovn-kubernetes                           ovn-ipsec-host-qc2hv                                              2/2     Running                  0               63m     10.0.101.188   ip-10-0-101-188.ec2.internal   <none>           <none>
openshift-ovn-kubernetes                           ovn-ipsec-host-rm5sl                                              2/2     Running                  0               63m     10.0.14.92     ip-10-0-14-92.ec2.internal     <none>           <none>
openshift-ovn-kubernetes                           ovn-ipsec-host-tbqlt                                              2/2     Running                  0               63m     10.0.112.193   ip-10-0-112-193.ec2.internal   <none>           <none>
openshift-ovn-kubernetes                           ovn-ipsec-host-wc5v2                                              2/2     Running                  0               63m     10.0.29.102    ip-10-0-29-102.ec2.internal    <none>           <none>

@martinkennelly
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 19, 2025
@pperiyasamy
Copy link
Member Author

/retest

@pperiyasamy pperiyasamy mentioned this pull request Mar 31, 2025
Merged
@pperiyasamy
Copy link
Member Author

/retest-required

trozet
trozet approved these changes Apr 7, 2025
View reviewed changes
Copy link
Contributor

@trozet trozet left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 7, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: martinkennelly, pperiyasamy, trozet

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:

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 7, 2025
@trozet
Copy link
Contributor

trozet commented Apr 7, 2025

@pperiyasamy this is a step in the right direction, but I don't think it encompasses everything we need for SDN-5342. In that JIRA it talks about including the kubelet certificate in the CSR so that we can then compare the can confirm the identity, rather than just someone sending us a valid node name.

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 2ca824b and 2 for PR HEAD c88f5d8 in total

@pperiyasamy
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 2ca824b and 2 for PR HEAD c88f5d8 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 66f0e13 and 1 for PR HEAD c88f5d8 in total

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 66f0e13 and 2 for PR HEAD c88f5d8 in total

@pperiyasamy
Copy link
Member Author

/retest-required

@openshift-ci-robot
Copy link
Contributor

/retest-required

Remaining retests: 0 against base HEAD 66f0e13 and 2 for PR HEAD c88f5d8 in total

@pperiyasamy
Copy link
Member Author

/retest-required

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Apr 9, 2025 •
edited
Loading

@pperiyasamy: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/security c88f5d8 link false /test security
ci/prow/4.19-upgrade-from-stable-4.18-e2e-aws-ovn-upgrade c88f5d8 link false /test 4.19-upgrade-from-stable-4.18-e2e-aws-ovn-upgrade
ci/prow/e2e-vsphere-ovn-dualstack-primaryv6 c88f5d8 link false /test e2e-vsphere-ovn-dualstack-primaryv6
ci/prow/4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade c88f5d8 link false /test 4.19-upgrade-from-stable-4.18-e2e-azure-ovn-upgrade
ci/prow/4.19-upgrade-from-stable-4.18-e2e-gcp-ovn-upgrade c88f5d8 link false /test 4.19-upgrade-from-stable-4.18-e2e-gcp-ovn-upgrade
ci/prow/e2e-aws-hypershift-ovn-kubevirt c88f5d8 link false /test e2e-aws-hypershift-ovn-kubevirt

Full PR test history. Your PR dashboard.

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-sigs/prow repository. I understand the commands that are listed here.

@pperiyasamy
Copy link
Member Author

/retest-required

@openshift-merge-bot openshift-merge-bot bot merged commit a1283bf into openshift:master Apr 10, 2025
31 of 37 checks passed
@openshift-bot
Copy link
Contributor

[ART PR BUILD NOTIFIER]

Distgit: cluster-network-operator
This PR has been included in build cluster-network-operator-container-v4.20.0-202504101220.p0.ga1283bf.assembly.stream.el9.
All builds following this will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinkennelly martinkennelly martinkennelly left review comments

@trozet trozet trozet approved these changes

@abhat abhat Awaiting requested review from abhat

@JacobTanenbaum JacobTanenbaum Awaiting requested review from JacobTanenbaum

Assignees

@trozet trozet

@martinkennelly martinkennelly

@jcaamano jcaamano

@kyrtapz kyrtapz

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@pperiyasamy @openshift-ci-robot @martinkennelly @huiran0826 @trozet @openshift-bot @jcaamano @kyrtapz