search

cert-manager / trust-manager

trust-manager is an operator for distributing trust bundles across a Kubernetes cluster.
https://cert-manager.io/docs/projects/trust-manager/
Apache License 2.0
243 stars 65 forks source link

Sort certificates in bundles to ensure deterministic behaviour #380

Closed jabdoa2 closed 1 month ago

jabdoa2 commented 1 month ago

fix #310

cert-manager-prow[bot] commented 1 month ago

Hi @jabdoa2. Thanks for your PR.

I'm waiting for a cert-manager 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](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository.
erikgb commented 1 month ago

/ok-to-test

jabdoa2 commented 1 month ago

Should be good to go now. I adjusted the certificate order in smoke and integration test.

jabdoa2 commented 1 month ago

@erikgb is there a chance to get this merged & released soonith? we would prefer to forward fix our issue instead of rolling everything back.

erikgb commented 1 month ago

@erikgb is there a chance to get this merged & released soonith? we would prefer to forward fix our issue instead of rolling everything back.

@jabdoa2 Yes, it should be possible to merge and release this improvement soonish. I am going to review it today, but I think we should get https://github.com/cert-manager/trust-manager/pull/375 merged first. The other PR changes overlapping area of code to this PR, and has been open for a while. So you'd have to be prepared for a rebase. 😉

arsenalzp commented 1 month ago

Thanks for looking into this, @jabdoa2! Looks (almost) good to me. 😉 As already commented, we should probably wait for #375 to get merged.

You can merge it as it is then I can re-write code to remediate all remarks as well as introducing crypto/x509 CertPool I'm on vacation right now, thus no chance to get laptop until Tuesday. I would like to stay the course.

erikgb commented 1 month ago

You can merge it as it is then I can re-write code to remediate all remarks as well as introducing crypto/x509 CertPool I'm on vacation right now, thus no chance to get laptop until Tuesday. I would like to stay the course.

Thanks @arsenalzp!

arsenalzp commented 1 month ago

You can merge it as it is then I can re-write code to remediate all remarks as well as introducing crypto/x509 CertPool I'm on vacation right now, thus no chance to get laptop until Tuesday. I would like to stay the course.

Thanks @arsenalzp!

How it will look like: my PR is the first one then #380 is the second one? Or just merge #380 then I will re-write my PR?

erikgb commented 1 month ago

How it will look like: my PR is the first one then #380 is the second one? Or just merge #380 then I will re-write me PR?

I think https://github.com/cert-manager/trust-manager/pull/380 (this PR) can be merged first, and it also fixes a serious issue. It looks almost GTM to me.

erikgb commented 1 month ago

/lgtm

We should maybe squash commits before/on merge?

jabdoa2 commented 1 month ago

/lgtm

We should maybe squash commits before/on merge?

I squashed everything into one commit

cert-manager-prow[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: erikgb

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](https://github.com/cert-manager/trust-manager/blob/main/OWNERS)~~ [erikgb] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
maelvls commented 1 month ago

I spent Friday afternoon manually testing the bug with label selectors. I was able to reproduce it, and I was able to test that your PR fixes the issue. I have documented the testing I did in the Hackmd page PR 380: Manually Testing that ConfigMaps are No Longer Unexpectedly Updated By The ConfigMap Label Selector Bug.

By the way, would there be by any chance someone relying the fact that the ordering in sources is identical in targets?

erikgb commented 1 month ago

By the way, would there be by any chance someone relying the fact that the ordering in sources is identical in targets?

Order doesn't matter here IMO. The order must be consistent, but I don't think a particular order is required.

jabdoa2 commented 1 month ago

By the way, would there be by any chance someone relying the fact that the ordering in sources is identical in targets?

Order doesn't matter here IMO. The order must be consistent, but I don't think a particular order is required.

This should be very unlikely for trust bundles. Those are not usually ordered and order should not matter.

The story would be different for PEMs containing a certificate chain. For instance, OpenSSL is well known for behaving badly if the first cert is not the leaf cert. Most other libraries will behave well though. Also that is not the usecase for trust-manager.

maelvls commented 3 days ago

Order doesn't matter here IMO. The order must be consistent, but I don't think a particular order is required.

We just found out that order may matter for Java clients when there are intermediate certificates in the bundle: https://github.com/cert-manager/trust-manager/issues/419. But this needs more investigation to be sure that the ordering of the root and intermediate is the cause of the issue.

sagarmujumale commented 2 days ago

By the way, would there be by any chance someone relying the fact that the ordering in sources is identical in targets?

Order doesn't matter here IMO. The order must be consistent, but I don't think a particular order is required.

The Validation Process

When a client verifies a server's certificate, it follows these steps: Starts with the leaf certificate: The client uses the public key in the leaf certificate to verify the signature of the intermediate certificate. Verifies intermediate certificates: The client iterates through the chain, using each certificate's public key to verify the signature of the next certificate. Trusts the root certificate: The final step is to verify the signature of the last intermediate certificate using the public key of the trusted root certificate.

Consequences of Incorrect Order If the certificate chain is not in the correct order, the validation process will fail: Invalid signature: The client will be unable to verify the signature of a certificate using the wrong public key. Broken trust chain: The chain of trust will be incomplete, leading to certificate validation failure.

In summary, the correct order of certificates in a chain is essential for establishing trust and ensuring secure communication.

reordering certificate in bundle is breaking the chain and service is generating "unable to find valid certification path to requested target" exception.

erikgb commented 2 days ago

@sagarmujumale I think you are mixing certificate chains and trust bundles.

jabdoa2 commented 2 days ago

This absolutely matters for certificate chains in some TLS libs (i.e. OpenSSL). However, trust-manager is designed for root of trust (instead of chains).