trust-manager deduplication doesnt work

Jiawei0227 commented 7 months ago

The deduplication feature does not work according to a simple test.

Lets say we have two ca file which contains the exact same CA data

a.crt

b.crt

And then we create them as configmap

kubectl create configmap  ca1 --from-file=./a.crt -n cert-manager
kubectl create configmap  ca2 --from-file=./b.crt -n cert-manager

Lastly we create a bundle

apiVersion: trust.cert-manager.io/v1alpha1
kind: Bundle
metadata:
  name: trust-store-root-ext
spec:
  sources:
  - configMap:
      key: a.crt
      name: ca1
  - configMap:
      key: b.crt
      name: ca2
  target:
    configMap:
      key: ca.crt

I expect the output only has one CA data but instead it still has duplicate CAs.

Jiawei0227 commented 7 months ago

I thought this is supposed to be fixed already by https://github.com/cert-manager/trust-manager/pull/184?

Maybe something is missing? @arsenalzp

arsenalzp commented 7 months ago

/assign

arsenalzp commented 7 months ago

I'm going to check it.

Jiawei0227 commented 7 months ago

Thanks Oleksandr! Also it would be great to check the certPool can return deterministic ordering of CA certs to keep the output consistent.

arsenalzp commented 7 months ago

The issue is that the certificate pool is created for each source, so it is necessary to maintain it for each bundle and it should be persistent. Let's discuss this topic, how to implement this feature. As always - I'm ready and want to improve this project!

arsenalzp commented 7 months ago

This code process each source separately, however util.ValidateAndSanitizePEMBundleWithOptions() should accept either all sources or just hashes only, to be able to maintain hashes across invocation.

Jiawei0227 commented 7 months ago

Should we process de-dup process after the for loop? We could also just move the entire validateAndSanitize out of the loop and add it here: https://github.com/cert-manager/trust-manager/blob/cd860d6da4580e9aa1d193b33d726e4a01ad3af0/pkg/bundle/sync.go#L126

I dont feel there are performance or other implication if we do that.

Jiawei0227 commented 7 months ago

actually... I think the validation of the source should still happen within the for loop. But the deduplication should really happen after the for loop. So we might need to do a dedicated dedup after the resolvedBundle is generated.

Jiawei0227 commented 7 months ago

A simple POC shows that

    // NB: empty bundles are not valid so check and return an error if one somehow snuck through.

    if len(bundles) == 0 {
        return bundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle")
    }

    combinedBundle := strings.Join(bundles, "\n") + "\n"
    finalBundle, err := util.ValidateAndSanitizePEMBundle([]byte(combinedBundle))
    if err != nil {
        return bundleData{}, fmt.Errorf("invalid PEM data in source: %w", err)
    }
    resolvedBundle.data = string(finalBundle)

    return resolvedBundle, nil
}

If we do this, then it will work. But here basically we are validating the PEM again which is not ideal. The best is probably to only dedup here? I am okay with any solution that could fix this.

arsenalzp commented 7 months ago

A simple POC shows that

  // NB: empty bundles are not valid so check and return an error if one somehow snuck through.

  if len(bundles) == 0 {
      return bundleData{}, fmt.Errorf("couldn't find any valid certificates in bundle")
  }

  combinedBundle := strings.Join(bundles, "\n") + "\n"
  finalBundle, err := util.ValidateAndSanitizePEMBundle([]byte(combinedBundle))
  if err != nil {
      return bundleData{}, fmt.Errorf("invalid PEM data in source: %w", err)
  }
  resolvedBundle.data = string(finalBundle)

  return resolvedBundle, nil
}

If we do this, then it will work. But here basically we are validating the PEM again which is not ideal. The best is probably to only dedup here? I am okay with any solution that could fix this.

I agree with you it would be better to move a dedup from downstream calls to buildSourceBundle() call.

arsenalzp commented 7 months ago

I tried to fix this issue with no harm to the existing code and test. It is reasonable to separate a sanitizing and a deduplication processes, instead to develop huge food processor in one function.

arsenalzp commented 5 months ago

Hello, Can we close this issue as it was fixed by #303 ?

arsenalzp commented 3 months ago

Can we close this issue as PR #303 has been introduced?

erikgb commented 3 months ago

Fixed by https://github.com/cert-manager/trust-manager/pull/303

/close

cert-manager-prow[bot] commented 3 months ago

@erikgb: Closing this issue.

In response to [this](https://github.com/cert-manager/trust-manager/issues/299#issuecomment-2194306888): >Fixed by https://github.com/cert-manager/trust-manager/pull/303 > >/close 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.

cert-manager / trust-manager

trust-manager deduplication doesnt work #299