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.

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

serverca: expand signed cert and cert chain PEMs with multiple cert blocks inside #54422

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

serverca: expand signed cert and cert chain PEMs with multiple cert blocks inside #54422

wants to merge 1 commit into from

Conversation

zliu-rh
Copy link

@zliu-rh zliu-rh commented Dec 19, 2024 •
edited
Loading

Please provide a description of this PR:

Fixes istio/ztunnel#1061

In multiple places such as the cert chain from CA bundle, or returned singed certs, there could be a single string representing a PEM with multiple blocks/certs.

Since IstioCertificateResponse's signature is []string, we should semantically expand those multi-cert PEMs.

See detailed analysis on how this is causing issues with ztunnel in particular istio/ztunnel#1061 (comment)

@zliu-rh zliu-rh requested a review from a team as a code owner December 19, 2024 16:50
@istio-policy-bot istio-policy-bot added area/ambient Issues related to ambient mesh area/security labels Dec 19, 2024
@linux-foundation-easycla Linux Foundation: EasyCLA
Copy link

linux-foundation-easycla bot commented Dec 19, 2024 •
edited
Loading

CLA Not Signed

@istio-policy-bot
Copy link

😊 Welcome @zliu-rh! This is either your first contribution to the Istio istio repo, or it's been
a while since you've been here.

You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines
by referring to Contributing to Istio.

Thanks for contributing!

Courtesy of your friendly welcome wagon.

@istio-testing istio-testing added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-ok-to-test labels Dec 19, 2024
@istio-testing
Copy link
Collaborator

Hi @zliu-rh. Thanks for your PR.

I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

howardjohn
howardjohn reviewed Dec 19, 2024
View reviewed changes
Copy link
Member

@howardjohn howardjohn left a comment

Choose a reason for hiding this comment

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

Makes sense to me overall.

cc @costinm @jaellio can you take a look as well

security/pkg/server/ca/server.go Outdated
response := &pb.IstioCertificateResponse{
CertChain: respCertChain,

// flatten `respCertChain` since each element might be a concatenated multi-cert PEM
Copy link
Member

Choose a reason for hiding this comment

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

very nitpicky, but this seems the opposite of flattening, we are expanding it?

Copy link
Author

Choose a reason for hiding this comment

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

haha I'm thinking of "flattening" in the sense similar to a nested-array, but yes expanding is more accurate

@howardjohn
Copy link
Member

/ok-to-test

@istio-testing istio-testing added ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. and removed needs-ok-to-test labels Dec 19, 2024
@zliu-rh zliu-rh force-pushed the serverca-multi-cert-in-chain branch from c0d0ee6 to dc4e333 Compare December 19, 2024 18:01
@zliu-rh zliu-rh changed the title serverca: flatten signed cert and cert chain PEMs with multiple cert blocks inside serverca: expand signed cert and cert chain PEMs with multiple cert blocks inside Dec 19, 2024
@zliu-rh zliu-rh force-pushed the serverca-multi-cert-in-chain branch from dc4e333 to f895747 Compare December 19, 2024 19:29
@istio-testing istio-testing added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Dec 19, 2024
@zliu-rh zliu-rh force-pushed the serverca-multi-cert-in-chain branch 3 times, most recently from 3a46236 to 7f111a3 Compare December 19, 2024 19:39
@zliu-rh zliu-rh force-pushed the serverca-multi-cert-in-chain branch from 7f111a3 to 3834a21 Compare December 19, 2024 19:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@howardjohn howardjohn howardjohn left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
area/ambient Issues related to ambient mesh area/security ok-to-test Set this label allow normal testing to take place for a PR not submitted by an Istio org member. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Invalid peer certificate on ztunnel, when using subordinate AWS private CA
4 participants
@zliu-rh @istio-policy-bot @istio-testing @howardjohn