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

AUTH-296: Finish the removal of the root CA #1031

Merged
merged 5 commits into from
Oct 31, 2022

Conversation

stlaz
Copy link
Member

@stlaz stlaz commented Oct 19, 2022

This PR does a couple of things in order to be able to finally remove the root CA:

  1. generates serving certificates for the KAS
  2. fixes the trust for ingress CA which caused the default router cert to be signed by the service-ca
  3. removes the root CA and most of the original crypto code as that becomes unused

depends on:

@openshift-ci openshift-ci bot requested review from mangelajo and pmtk October 19, 2022 12:22
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 19, 2022
@pmtk
Copy link
Member

pmtk commented Oct 25, 2022

/retest-required

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 26, 2022
@stlaz stlaz force-pushed the kas-serving-cert branch 2 times, most recently from 2505ac6 to 3bd4f5a Compare October 27, 2022 10:49
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 27, 2022
@stlaz
Copy link
Member Author

stlaz commented Oct 27, 2022

/retest
this is rather odd

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 27, 2022

@stlaz: The following test 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/periodic-ocp-4.13-images 700e744 link true /test periodic-ocp-4.13-images

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

@stlaz
Copy link
Member Author

stlaz commented Oct 27, 2022

The errors turned out to be because of the changes to assets locations, should be fixed now

kind: Secret
metadata:
namespace: openshift-ingress
name: router-certs-default
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this name match the change in service-internal.yaml?

Copy link
Member Author

Choose a reason for hiding this comment

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

it shouldn't, these are two different secrets

Copy link
Contributor

Choose a reason for hiding this comment

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

This is more aligned with what OCP has, right?

An OCP 4.12 cluster has the following objects:

 oc --kubeconfig kubeconfig-clusterbot get secret router-certs-default -n openshift-ingress
NAME                   TYPE                DATA   AGE
router-certs-default   kubernetes.io/tls   2      41m
[root@maxwell ~]# oc --kubeconfig kubeconfig-clusterbot get svc router-internal-default -n openshift-ingress -o yaml
apiVersion: v1
kind: Service
metadata:
  annotations:
    service.alpha.openshift.io/serving-cert-secret-name: router-metrics-certs-default
    service.alpha.openshift.io/serving-cert-signed-by: openshift-service-serving-signer@1667219307
    service.beta.openshift.io/serving-cert-signed-by: openshift-service-serving-signer@1667219307
  creationTimestamp: "2022-10-31T12:29:41Z"
  labels:
    ingresscontroller.operator.openshift.io/owning-ingresscontroller: default
  name: router-internal-default
  namespace: openshift-ingress
  ownerReferences:
  - apiVersion: apps/v1
    controller: true
    kind: Deployment
    name: router-default
    uid: 158f6632-1e72-42e1-a455-e3412ac5666a
  resourceVersion: "9123"
  uid: daded2dc-55fd-41b1-b690-a46e11a5b171
spec:
  clusterIP: 172.30.158.236
  clusterIPs:
  - 172.30.158.236
  internalTrafficPolicy: Cluster
  ipFamilies:
  - IPv4
  ipFamilyPolicy: SingleStack
  ports:
  - name: http
    port: 80
    protocol: TCP
    targetPort: http
  - name: https
    port: 443
    protocol: TCP
    targetPort: https
  - name: metrics
    port: 1936
    protocol: TCP
    targetPort: 1936
  selector:
    ingresscontroller.operator.openshift.io/deployment-ingresscontroller: default
  sessionAffinity: None
  type: ClusterIP
status:
  loadBalancer: {}

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, OCP separates the trust for external (routes) and internal cluster services.

In OCP, for the router, we use the internal router port to scrape metrics from, and so on the service network we use the service-ca-signed certificate, as that is the certificate Prometheus should trust for these purposes.

Service-CA should only ever sign the internal services so that services running in the cluster can choose to only trust the internal cluster traffic. The external router interface is signed by a different certificate that, unlike the service-ca cert, should appear in the end-user trust bundle.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 28, 2022
@oglok
Copy link
Contributor

oglok commented Oct 31, 2022

@stlaz could you please rebase?

@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 31, 2022
@stlaz
Copy link
Member Author

stlaz commented Oct 31, 2022

@oglok done! 👍

@oglok
Copy link
Contributor

oglok commented Oct 31, 2022

Ok, PR has been reviewed and tested. Great work @stlaz ! Glad to be merging this important milestone!

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 31, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 31, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oglok, stlaz

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-merge-robot openshift-merge-robot merged commit bb0d60f into openshift:main Oct 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants