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

Improve filtered certs error reporting #276

Open Hoega opened 7 months ago

Hoega commented 7 months ago

When creating a bundle from a source (configmap, secret, or inline) with exclusively expired certificates, the bundle will fail and indicate that the source contains no PEM data. There is no indication that certs are expired and it can be tricky to troubleshoot.

This is because 'ValidateAndSanitizePEMBundleWithOptions' parses sources individually, therefore if a source has only expired cert(s), the cert(s) will be skipped and the sanitize function will find an empty output thus returning an error.

For example, if I have:

The bundle will fail with this error message: Warning SourceBuildError 1s (x12 over 12s) bundles Failed to build bundle sources: invalid PEM data in source: bundle contains no PEM certificates

Also, the bundle is pending: 

NAME         TARGET   SYNCED   REASON   AGE
my-org.com                              5m15s

Whereas If I add 1 valid cert to the inline source the bundle would be OK. This is because the sanitize function requires at least one valid cert by source.

I propose to parse and sanitize sources globally and/or at least to improve the error reporting to highlight a cert is expired.

arsenalzp commented 7 months ago

Can we accept this as the new feature? I think I can work on it once the decision is made.

erikgb commented 7 months ago

Can we accept this as the new feature? I think I can work on it once the decision is made.

Please work on it! I think we all agree that the error handling could be improved here. Could you start by describing the proposed solution in more detail? I agree this would become a lesser problem if a bundle is sanitized after reading all sources. But what if the resulting bundle would contain exclusively expired certificates?

arsenalzp commented 7 months ago

Can we accept this as the new feature? I think I can work on it once the decision is made.

Please work on it! I think we all agree that the error handling could be improved here. Could you start by describing the proposed solution in more detail? I agree this would become a lesser problem if a bundle is sanitized after reading all sources. But what if the resulting bundle would contain exclusively expired certificates?

Hello, Thank you for letting me work again! I guess it would be optionally possible either allow or disallow to include expired certificates into the resulting bundle. But I don't know any reason why users want to have expired certificates in bundles, except legacy requirements, etc.

I would look something like allowExpiredCerts: true with default false. Then we can suppress expired certificate warnings and include it in the bundle, or just write ones and exclude expired certificates from the bundle.

erikgb commented 7 months ago

Ah, you haven't seen https://github.com/cert-manager/trust-manager/pull/273? There is now an opt-in to filter out expired certificates in trust-manager. I understand this issue mostly as a usability improvement to this recently added feature. It could make sense to include an expired certificate in a bundle to get better error messages.

arsenalzp commented 7 months ago

Ah, you haven't seen #273? There is now an opt-in to filter out expired certificates in trust-manager. I understand this issue mostly as a usability improvement. It could make sense to include an expired certificate in a bundle to get better error messages.

Oh, I didn't see that because pull request wasn't linked to this particular issue.

erikgb commented 1 month ago

It seems most reasonable to validate to validate at least one certificate per bundle, and not per bundle source.

/priority important-soon