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-323: Separate Aggregate Client trust chain from rootCA #1831

Closed
wants to merge 1 commit into from
Closed

AUTH-323: Separate Aggregate Client trust chain from rootCA #1831

wants to merge 1 commit into from

Conversation

ibihim
Copy link
Contributor

@ibihim ibihim commented Oct 26, 2022

What

This is a WIP for a distinct CA for the aggregated client certs.

Why

Signed-off-by: Krzysztof Ostrowski kostrows@redhat.com

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

  • Subject and description added to both, commit and PR.
  • Relevant issues have been referenced.
  • This change includes docs.
  • This change includes unit tests.

@ibihim ibihim changed the title cpo/c/hcp/*: Separate Aggregate Client trust chain [wip] Separate Aggregate Client trust chain Oct 26, 2022
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 26, 2022
@openshift-ci openshift-ci bot requested review from csrwng and enxebre October 26, 2022 20:10
@ibihim ibihim changed the title [wip] Separate Aggregate Client trust chain [wip] Separate Aggregate Client trust chain from rootCA Oct 26, 2022
@netlify
Copy link

netlify bot commented Oct 26, 2022

Deploy Preview for hypershift-docs ready!

Name Link
🔨 Latest commit 2e62397
🔍 Latest deploy log https://app.netlify.com/sites/hypershift-docs/deploys/6359941df227f00009b9237e
😎 Deploy Preview https://deploy-preview-1831--hypershift-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@stlaz
Copy link
Member

stlaz commented Oct 31, 2022

/retest

}
}

func KASAggregatorSignerSecret(ns string) *corev1.Secret {
Copy link
Member

Choose a reason for hiding this comment

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

unused

func AggregateClientCAConfigMap(ns string) *corev1.ConfigMap {
return &corev1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "aggregator-client-ca",
Copy link
Member

Choose a reason for hiding this comment

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

this needs to be wired into the KAS config

Copy link
Member

Choose a reason for hiding this comment

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

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 31, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ibihim
Once this PR has been reviewed and has the lgtm label, please assign enxebre for approval by writing /assign @enxebre in a comment. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@stlaz
Copy link
Member

stlaz commented Oct 31, 2022

/lgtm
/retitle Separate Aggregate Client trust chain from rootCA

@openshift-ci openshift-ci bot changed the title [wip] Separate Aggregate Client trust chain from rootCA Separate Aggregate Client trust chain from rootCA Oct 31, 2022
@openshift-ci openshift-ci bot added lgtm Indicates that a PR is ready to be merged. and removed do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 31, 2022
@stlaz
Copy link
Member

stlaz commented Oct 31, 2022

CI is broken

@stlaz
Copy link
Member

stlaz commented Oct 31, 2022

/retitle AUTH-323: Separate Aggregate Client trust chain from rootCA

@openshift-ci openshift-ci bot changed the title Separate Aggregate Client trust chain from rootCA AUTH-323: Separate Aggregate Client trust chain from rootCA Oct 31, 2022
Signed-off-by: Krzysztof Ostrowski <kostrows@redhat.com>
@openshift-ci openshift-ci bot removed 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

New changes are detected. LGTM label has been removed.

@sjenning
Copy link
Contributor

/retest-required

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Oct 31, 2022

@ibihim: 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/kubevirt-e2e-kubevirt-azure-ovn 30e75f1 link false /test kubevirt-e2e-kubevirt-azure-ovn
ci/prow/e2e-aws 30e75f1 link true /test e2e-aws
ci/prow/capi-provider-agent-sanity 30e75f1 link false /test capi-provider-agent-sanity

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.

Comment on lines +1291 to +1296
kasAggregateClientCA := manifests.AggregateClientCAConfigMap(hcp.Namespace)
if _, err := createOrUpdate(ctx, r, kasAggregateClientCA, func() error {
return pki.ReconcileAggregateClientCA(kasAggregateClientCA, p.OwnerRef, kasAggregateClientSigner)
}); err != nil {
return fmt.Errorf("failed to reconcile combined CA: %w", err)
}
Copy link
Member

Choose a reason for hiding this comment

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

It seems that the configmap is missing in the control-plane of the aws run?

Also, naming should be:

Suggested change
kasAggregateClientCA := manifests.AggregateClientCAConfigMap(hcp.Namespace)
if _, err := createOrUpdate(ctx, r, kasAggregateClientCA, func() error {
return pki.ReconcileAggregateClientCA(kasAggregateClientCA, p.OwnerRef, kasAggregateClientSigner)
}); err != nil {
return fmt.Errorf("failed to reconcile combined CA: %w", err)
}
kasAggregatorClientCA := manifests.AggregatorClientCAConfigMap(hcp.Namespace)
if _, err := createOrUpdate(ctx, r, kasAggregatorClientCA, func() error {
return pki.ReconcileAggregatorClientCA(kasAggregatorClientCA, p.OwnerRef, kasAggregatorClientSigner)
}); err != nil {
return fmt.Errorf("failed to reconcile combined CA: %w", err)
}

This is not an aggregate of anything but the client cert of the KAS API aggregator.

@stlaz
Copy link
Member

stlaz commented Nov 1, 2022

/close
While a good start, this will not work as is today. I found some other rather interesting places within the Hypershift codebase that need to be fixed first.

Continuing in my PR #1837

@openshift-ci openshift-ci bot closed this Nov 1, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Nov 1, 2022

@stlaz: Closed this PR.

In response to this:

/close
While a good start, this will not work as is today. I found some other rather interesting places within the Hypershift codebase that need to be fixed first.

Continuing in my PR #1837

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants