KongPlugin Konnect sync fails when Konnect sanitizer enabled and configFrom used about kubernetes-ingress-controller HOT 12 CLOSED

For @mheap to decide there are essentially four options, laid out below.

My preference is trying to go for the Koko-side break glass skip plugin validation display whatever approach. If viable it's less work than building out either the generators or placeholder plugin configurations. If we opt for a KIC-only approach, my preference is for the placeholder plugin approach. It's simple, albeit rather stupid, and arguably a stopgap--we should commit to some sort of upstream handling in the future if we go that route.

Send placeholder plugins

KIC team writes static minimum viable plugin configurations for each plugin. When a plugin uses ConfigFrom, we submit the stand-in plugin to Konnect. When a plugin uses ConfigPatch, we overlay (probably via mergo) the known fields in Config over the stand-in plugin configuration to backfill any missing fields.

In addition to the overlay code, this requires some busywork to build out all the stand-in configs. We can maybe do so iteratively with patch releases that include additional plugins if we want to target a subset first.

Konnect will display fake values as if they were actual values. There will be no Konnect-side indication that these are actually masked fields.

Fill plugin fields from schema-aware generators

KIC team writes generators for Kong DAO schema types. KIC reads plugin schemas and tries to generate appropriate garbage values for strings, ints, and more complex types like PEM. Required fields absent from Config are filled using generators.

This takes more code work, but covers all plugins, including custom, once complete. It has a higher chance of failure for ConfigFrom, as these KongPlugins require generating all required fields, and the results may violate some unexpected rules.

Upstream gateway changes

Update the Kong DAO to recognize something akin to the existing referenceable quality that applies to all fields automatically. The Kong DAO allows a known masked value placeholder in any field. This would hold true even for non-Konnect cases, as the Konnect validation relies on upstream Kong for plugin validation, and we'd accept that attempts to use the placeholder value in a non-Konnect context would have unexpected effects. We can reasonably expect that non-Konnect users will not submit the placeholder value, but they technically could, with weird results specific to individual plugin code.

Upstream Koko changes

We add some Koko-only plugin flag that tells Koko it shouldn't try to validate a given plugin instance and instead just accept and display whatever configuration we submit (either empty configuration for ConfigFrom or partial configuration for ConfigPatch).

This is probably much more reasonable than the gateway approach, since we know that Konnect doesn't really need to validate KIC-submitted configuration, as Konnect is never going to send that configuration to a DP.

from kubernetes-ingress-controller.

I think both of the options (omitting whole plugin vs filling with defaults) will bring some confusion. However, when users are using configFrom for the plugins, they should know the existence of the plugins aware that they have some special points. So I think the second option with some annotations in tags could bring the least confusion.
Also we should make some discussion between Konnect team, and let they to recommend some better solution for noting the special points of such plugins. @czeslavo

from kubernetes-ingress-controller.

If you encounter this issue, a temporary workaround could be turning SanitizeKonnectConfigDumps feature gate off (i.e. by setting --feature-gates=SanitizeKonnectConfigDumps=false). Please be aware that this will make KIC synchronize private keys with Konnect Admin API though.

from kubernetes-ingress-controller.

Re

we should make some discussion between Konnect team, and let they to recommend some better solution

I think upstream is the best way to handle this. I can't think of much that we can do to handle this on our end as-is: if we have to provide a valid value for any field that doesn't accept a vault reference, we're going to be playing whack-a-mole tracking down every non-referenceable field and writing code to generate a placeholder value that conforms to the field schema.

Using patches instead of configFrom doesn't really help, since there will be some fields that are required that you'd want to keep in a Secret. We need a solution that works for any field.

Initial review of how the upstream API handles this found Kong/koko#449, but it doesn't look like that's where the "if reference, use special validation" logic lives. It defines how to recognize the reference magic string, but doesn't have obvious general-case validation of it.

I did find https://github.com/Kong/koko-private/blob/main/internal/plugin/validators/lua_validator.go and https://github.com/Kong/goks while searching around, and that suggests Koko is offloading schema validation to the Lua, which makes sense--not much reason to maintain parallel schemas, and Lua's easy to embed.

If that's indeed the case, we'd need to add something similar to the existing referenceable validation in the Kong Lua DAO. The existing validation is only available for string fields, whereas we'd need a case for (probably) everything. Implementing something here is less than ideal, as it wouldn't be Konnect-only--the actual Kong API would accept the magic string, and plugin code would attempt to actually operate on the placeholder, without much success. Unfortunately I don't see an obvious way to make a Koko-only implementation that doesn't clash with the existing plugin validation offload code.

If we can swing that, we'd pass a {hidden://Namespace:Name} magic string for a field that's stored in the Namespace/Name Secret.

That said, we'd still have a bit of an issue on our end: we don't quite know what fields we need to mask. ConfigFrom simply provides everything; ConfigPatch provides a path that may be an object. We'd presumably need to walk the JSON object and replace any non-object value field with the magic string.

Lastly, it looks like this Lua offload only applies to plugins, and that plugins simply hold the vast majority of referenceable fields. A brief review of the Kong Lua code suggests it's just certificates and some of the AI/LLM code that has referenceable fields outside them.

from kubernetes-ingress-controller.

That said, we'd still have a bit of an issue on our end: we don't quite know what fields we need to mask. ConfigFrom simply provides everything; ConfigPatch provides a path that may be an object. We'd presumably need to walk the JSON object and replace any non-object value field with the magic string.

We could use Admin API's /schemas/plugins/{} endpoint and inspect every Plugin's configuration field for referenceable value. We can probably assume that if a field is referenceable, it means it's a secret and we should replace it with a dummy reference ({vault://redacted-value}). This switches the way we determine if a field is a secret from "it was provided as a part of Kubernetes secret, it is a secret" to "it's a secret because Kong considers it so". Using this approach, we should always generate configs that are acceptable by Koko validation, while still sanitizing all the secret fields as expected. It's also implementable with no upstream modifications.

Example of a plugin schema that includes a referenceable field:

$ curl --request GET \
  --url http://localhost:8001/schemas/plugins/session \
  --header 'accept: */*' | jq
...
"fields": [
          {
            "secret": {
              "encrypted": true,
              "default": "rxXaQBYq2IkINwzRZgXKDNEsneUReYBIMzLHBzaPc6Uj",
              "type": "string",
              "required": false,
              "referenceable": true,
              "description": "The secret that is used in keyed HMAC generation."
            }
          },
...

from kubernetes-ingress-controller.

I think upstream is the best way to handle this.

I agree with this viewpoint.

Although we can currently achieve our goals through some solutions, if this is not clearly communicated with the upstream, it may be disrupted at some point in the future.

from kubernetes-ingress-controller.

I have added a blocked label for this issue, because I want to get some response from @mheap
And we need to be able to reach a consensus.

https://kongstrong.slack.com/archives/C02KEASTTRC/p1716306680429769

from kubernetes-ingress-controller.

We could maybe create a stopgap that doesn't require fancy field generation by creating "reference" copies of every plugin, where we just write a static minimum viable configuration for each and send that for plugins with secret fields? For ConfigFrom we'd send the whole thing and for ConfigPatch we run mergo to populate the secret fields only.

I don't think plugins are allowed to declare config fields that need globally unique values, or at least I've never encountered one.

This would not handle custom plugins however, and AFAIK Konnect does have some mechanism that allows those.

from kubernetes-ingress-controller.

The correct solution here is for Koko not to validate empty fields in a KIC backed configuration. However, this was too much work for the team during initial discussions.

I think we should proceed as follows:

• Resolve all fields, including configs using configFrom
• Use the schemas endpoints to work out which fields are referenceable
• Mask all of those fields using a vault reference

This will not catch all sensitive data, and that's ok. The initial feature request for this was tightly scoped to "certificate private keys", and somehow expanded to "anything from a secret".

Long term, we should have a way in Gateway to mark fields as sensitive in a schema, which would then allow KIC to send a zero value, and Koko to skip validation for empty values for KIC in Konnect

from kubernetes-ingress-controller.

Unassigning self, #5932 going to consume more time than expected. Re:

The correct solution here is for Koko not to validate empty fields in a KIC backed configuration. However, this was too much work for the team during initial discussions.

We'd just be shifting complex work from one team to another with the above proposal. Is it too much work in the "we will never have time to implement this" sense or in the "we don't have time to implement this now" sense?

The "correct. but too much" bit suggests we're looking for more of a stopgap, and I don't think we have a ready-made schema walker and merger that we can use to produce one quickly KIC-side.

The merger bit that smooshes the actual and fake fill-in-the-gap plugin you need for either KIC-side solution. IDK if you're likely to get both that and the schema-walker to figure out which specific fields to replace in the 3.2 release timeline.

from kubernetes-ingress-controller.

It's closer to "we will never have time to implement this". The correct way to do this is have Gateway mark items as sensitive, then have Koko use that marker with KIC in Konnect to loosen validations. Given that takes schema changes across two products, I think we fix in KIC for now and work on getting schema changes in to Gateway in the future

from kubernetes-ingress-controller.

@mheap and I synced up here. It seems like the best compromise between complexity/time is to go back to the original requirement and only obfuscate certificate private keys and not any secrets.

Users will come back complaining about their secrets not being obfuscated anymore and we can direct them towards:

• Entreprise gateway with Vault support
• Gateway to support schemas with sensitive type.

from kubernetes-ingress-controller.