RFC for mTLS

[david-martin]

Follow on from investigation in Kuadrant/kuadrant-operator#1049 and a possible automated way to enable and manage mTLS for users instead of the manual documented way in Kuadrant/kuadrant-operator#1054

[guicassolato]

We should probably add a section for Envoy Gateway as well.

[david-martin]

I don't know what to add for that.
The Istio approach takes advantage of Istio sidecar capabilities to enable mTLS in the kuadrant pods.
If Istio isn't there, would that be some other solution outside of Envoy Gatway completely?

[guicassolato]

Although this could work for a first implementation, I wonder if it's clear to the user that the certs will be provisioned via Secret discovery service (SDS).

I can also see how non-mesh users could want this but by configuring certs by other means though. If we don't support such other means right away, should the API be more explicit about using SDS perhaps?

[david-martin]

So the available options right now for mTLS seem quite limited (from reading this recent article https://blog.howardjohn.info/posts/mtls-kubernetes/)
There's either a sidecar approach (which this uses), ambient mode, a CNI approach (which doesn't exist yet), or DIY.
Is the abstraction too low if SDS is mentioned in the config?
What about an mTLS mode of 'mesh' being the initial option, with that meaning we'll integrate with the mesh if it's available i.e. if you're using Istio.

[maleck13]

So what would be the minimum to allow configuring the certs. Would it just be to provide some paths to the CA for example? If so it would seem like a relatively simple adjustment to allow the needed paths to be provided under the mtls section. I haven't gone through that link in detail, but I can def see both use cases. One simple as possible just make mtls work. 2 the certs have to be specific certs from a the orgs CA etc

[maleck13]

One thing I am thinking here is we could go with an opinionated mTLS but ensure we allow users to still change values directly if needed. So when reconciling, we would assert that the section is there but not overwrite any new config for example.
So out of the box you get a working version that uses SDS if you want more than that you can configure it directly and we wont overwrite it

[maleck13]

@guicassolato I believe I am correct in saying we don't create these envoy filters until an Auth or RateLimit policy is created?
@david-martin I think we would need to make sure if mtls is enabled that part of marking the policy ready / accepted would mean ensuring this configuration was in place first.

[maleck13]

I think it would be good to call out that if we don't support mTLS configuration for a gateway provider, it will be reflected in the status of the kuadrant resource. Also given how important this can be for users, kuadrant should probably not become ready until the specified mTLS can be handled or the mTLS section is removed.

[maleck13]

Some minor improvements suggest but, I like this as an initial pass at allowing configuration of mTLS when using Istio. Avoids users having to mess with resources owned by the kaudrant operator and also kuadrant users not needing to understand the innards of Envoy. It also seems relatively easy to extend with things like a specific CA etc.

[Boomatang]

I think there is a design issue here. More so when we think of the whole CR and just this feature. As this feature only has two components that is affected this will be harder to visualize, but I will try explain it from a generic features point of view.

spec:
  feature:
    activate: false # optional, boolean, defaults to false
    enabled: [] # optional, list of strings
    disabled: [] # optional, list of strings

Under the feature all fields are optional to ensure backwards compatibility. The feature will not be activated on less the activate field is set to true. This allows the feature to be enabled or disabled without changing the configuration for that feature. A feature would have a list of defaults components that would be affected by the feature being enabled. In this case that would be two, authorino & limitador, but this would not always be the case.

If the user wanted to disable the feature on a component lets say limitador they would add the following spec.

spec:
  feature:
    activate: true 
    disabled: ['limitador']

In this example only limitador will not have the feature enabled, but also if any new components are added to the defaults list that will be auto enabled. Being auto enabled by default may not be what the user wants. That is where the enabled list comes in. If the user wants to only have the feature enabled for authorino no matter what components are add in the future they could use the following spec.

spec:
  feature:
    activate: true 
    enable: ['authorino']

The enable list would be an explicit list. Only components listed would be affected. This ensures that behaviour would not change if a component is added to the support list later in the future. However, if there are a lot of components that is supported by default filling this list in could be tedious, but as it is a string list there is nothing stopping use from supporting keywords like 'defaults'. An example of this being used could be service monitors whereby default we only add them to the operands. A user many also add them to the operator that they care about. The below example shows that looks like.

spec:
  feature:
    activate: true
    enable: 
      - 'defaults'
      - 'authorino-operator'

So advantage that I see with doing this design are.

• All features have the same basic interface.
• New components don't require CRD updates to be supported. The change will only be required with the operator.
• The common interface make documentation simpler

[david-martin]

I like this approach.
Are the enabled & disabled lists mutually exclusive?

[Boomatang]

At first pass I think they should be exclusive, but I am not sure how we would enforce that. There might be something that could be done with CEL black magic and sets. If CEL doesn't work, a validating web hook is always an option. The bigger question around exclusive comes if we decide to allow keywords such as 'defaults'. There may be a component in the default list that the user wants to disable.

The example the comes to mind is a generic feature that works on both the operand and operator, but we decide that by default it only applies to the operand. The default list could be:

- authorino
- limitador

In this case the user may not be using authorino but also wants to use the feature with the limitador operator. The configuration they create would be something like:

spec:
  generic_feature:
    activate: true
    enable:
      - 'defaults'
      - 'limitador-operator'
    disable:
      - 'authorino'

For clarity this is the list that the feature would be applied to with the above configuration.

- limitador
- limitador-operator

In this scenario when building the list of component to apply the feature too, the disable list needs to read secondary. If the list were exclusive the order should not matter.

I think the behaviour around the keywords like 'defaults' needs a bit more refinement to explain what is regarded as expected behaviour.

I understood the question to mean one thing, but on second pass I can see it could be asking a different question to what I answer. The second way could be; are the enabled & disabled lists mutually exclusive within the Kuadrant Spec? As in, you can only have one or the other but not both at the same time. If this was want was meant by the question the answer would be no they are not mutually exclusive. If they were configuration like that give in the scenario above would not be possible, and I think that is where the real value of this interface is.

[Boomatang]

As this would be configured inside the kuadrant CR, it would be worth looking at #112 as it is trying to define a standardized API design for new features added to the kuadrant CR

[laurafitzgerald]

Discussion around this happening -> https://groups.google.com/a/redhat.com/g/kuadrant-dev/c/rbsY4kDaSnE
And at the community call -> https://docs.google.com/document/d/1Dkcqu1MgCJlvJHF_iCYibPNsnSHu1aIeG5-WoqlKtGU/edit?tab=t.0#heading=h.5a78l4pgyj90
Will relay the outcome here via update with an answer to this question.

[laurafitzgerald]

The outcome was decided and communicated via a recent reply to the kuadrant-dev mailing list. We will no expose any config via limitador or authorino cr. Instead we will allow the addition of the label to the deployment.spec.template.labels by external user/component and the operators will not remove them.

[maleck13]

@david-martin not sure what state we are in with this one. We have the feature so is the RFC implemented and can be merged?

[maleck13]

@david-martin is this still relevant? Be good for it reflect the current state and be merged

[david-martin]

@maleck13 I've updated the rfc to reflect the current status (implemented) and how the spec looks.
I think we can merge this now.

[maleck13]

approving as this is in place now updates or changes can be added via a new RFC