Skip to content

ssl client principal propagation#1

Open
ujjwalkalia wants to merge 84 commits intomainfrom
ssl_principal_propagation
Open

ssl client principal propagation#1
ujjwalkalia wants to merge 84 commits intomainfrom
ssl_principal_propagation

Conversation

@ujjwalkalia
Copy link
Owner

Type of change

Select the type of your PR

  • Bugfix
  • Enhancement / new feature
  • Refactoring
  • Documentation

Description

Please describe your pull request

Additional Context

Why are you making this pull request?

Checklist

Please go through this checklist and make sure all applicable tasks have been done

  • PR raised from a fork of this repository and made from a branch rather than main.
  • Write tests
  • Update documentation
  • Make sure all unit/integration tests pass
  • Make sure all Sonarcloud warnings are addressed or are justifiably ignored.
  • If applicable to the change, trigger the system test suite. Make sure tests pass.
  • If applicable to the change, trigger the performance test suite. Ensure that any degradations to performance numbers are understood and justified.
  • Ensure the PR references relevant issue(s) so they are closed on merging.
  • For user facing changes, update CHANGELOG.md (remember to include changes affecting the API of the test artefacts too).

NOTE: You must be a member of @kroxylicious/developers to trigger the system test and performance test suites. If you are not part of this group, comment on the PR requesting a trigger, tagging @kroxylicious/developers.

return downStreamCertificatePrincipal;
}

private @Nullable String downStreamCertificatePrincipal;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add this before getDownStreamCertificatePrincipal

try {
downStreamCertificatePrincipal = sslHandler.engine().getSession().getPeerPrincipal().toString();
} catch (SSLPeerUnverifiedException e) {
LOGGER.debug("No client principal received, setting principal as ANONYMOUS");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add channel information in the logs ?
Lets check how kroxylicious adds debug logs

downStreamCertificatePrincipal = sslHandler.engine().getSession().getPeerPrincipal().toString();
} catch (SSLPeerUnverifiedException e) {
LOGGER.debug("No client principal received, setting principal as ANONYMOUS");
downStreamCertificatePrincipal = "ANONYMOUS";
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this from KafkaPrincipal class from kafka ?

Copy link

@hrishabhg hrishabhg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Expects few minor things to be taken care.

String sniHostname();

@Nullable
String downStreamCertificatePrincipal();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
String downStreamCertificatePrincipal();
String downstreamCertificatePrincipal();

String sniHostname();

@Nullable
String downStreamCertificatePrincipal();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did it not break during build? We should be backward compatible with existing impl of the interface. It should be

default String downStreamCertificatePrincipal() { return null; }

downStreamCertificatePrincipal = sslHandler.engine().getSession().getPeerPrincipal().toString();
} catch (SSLPeerUnverifiedException e) {
LOGGER.debug("No client principal received, setting principal as ANONYMOUS");
downStreamCertificatePrincipal = "ANONYMOUS";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. define constant.
  2. This should be in finally. 2nd time ssl event should not return previously resolved value.
Suggested change
downStreamCertificatePrincipal = "ANONYMOUS";
downStreamCertificatePrincipal = ANONYMOUS;
  1. log.warn or log.info instead of debug.

/**
* Test the normal flow, in a number of configurations.
*
* @param clientAuthConfigured

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: add description

})) // reverses order
.stream()
.map(f -> new FilterHandler(getOnlyElement(FilterAndInvoker.build(f.getClass().getSimpleName(), f)), timeoutMs, null, testVirtualCluster, inboundChannel))
.map(f -> new FilterHandler(getOnlyElement(FilterAndInvoker.build(f.getClass().getSimpleName(), f)), timeoutMs, null, null, testVirtualCluster, inboundChannel))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should add parametrised test in FilterHandlerTest also to cover non-null scenario.
Add parameterised test for inheritors if needed.

ujjwalkalia and others added 26 commits June 9, 2025 02:47
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Ujjwal <ukalia@confluent.io>
…nal (kroxylicious#2248)

Signed-off-by: k-wall <18440250+k-wall@users.noreply.github.com>
Co-authored-by: k-wall <18440250+k-wall@users.noreply.github.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
…2.6.9.Fi…"

This reverts commit 6021132.

Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Bumps [org.junit:junit-bom](https://github.com/junit-team/junit5) from 5.12.2 to 5.13.0.
- [Release notes](https://github.com/junit-team/junit5/releases)
- [Commits](junit-team/junit-framework@r5.12.2...r5.13.0)

---
updated-dependencies:
- dependency-name: org.junit:junit-bom
  dependency-version: 5.13.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Bumps [org.apache.maven.plugins:maven-clean-plugin](https://github.com/apache/maven-clean-plugin) from 3.4.1 to 3.5.0.
- [Release notes](https://github.com/apache/maven-clean-plugin/releases)
- [Commits](apache/maven-clean-plugin@maven-clean-plugin-3.4.1...maven-clean-plugin-3.5.0)

---
updated-dependencies:
- dependency-name: org.apache.maven.plugins:maven-clean-plugin
  dependency-version: 3.5.0
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Bumps [dawidd6/action-download-artifact](https://github.com/dawidd6/action-download-artifact) from 8 to 10.
- [Release notes](https://github.com/dawidd6/action-download-artifact/releases)
- [Commits](dawidd6/action-download-artifact@v8...v10)

---
updated-dependencies:
- dependency-name: dawidd6/action-download-artifact
  dependency-version: '10'
  dependency-type: direct:production
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Co-authored-by: Keith Wall <kwall@redhat.com>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
…etrics

* Frame now carries api key id/version
* pushed responsibility for emitting metrics to dedicated handlers.

Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
…ponse

Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Co-authored-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
…f API

Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
sonar

Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
SamBarker and others added 28 commits June 9, 2025 02:47
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Did some other renaming to try and make things easier to grok.

Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
If any referenced KafkaProxyIngress contains a loadBalancer property, we:
1. create a single Service with type LoadBalancer for the proxy. This is
   to be shared by all LoadBalancer ingresses.
2. add a shared-sni container port to the proxy, 9291. This is the port
   all of the shared Service ports will target.

Then, for each loadbalancer KafkaProxyIngress referred to by a VKC we
add a gateway to that VKC's proxy config. The gateway uses SNI and we
pass through:
- the bootstrapAddress from the KPI custom resource, with the shared-sni
  container port appended, as this is the address the proxy will listen
  on.
- the advertisedBrokerAddressPattern from the KPI custom resource, with
  the client-facint Service port appended (currently defaulted to 9083)
  as this is the port clients will have to connect to.

In future the port exposed by the shared Service for a KafkaProxyIngress
will be customizable, the intent is that the shared Service will
accumulate all the client-facing ports across all ingresses and map them
all to the shared-sni port on the proxy container.

We have not implemented VKC ingress status for loadbalancer ingresses
yet.

Signed-off-by: Robert Young <robeyoun@redhat.com>
Co-authored-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Formatter didn't fix it as it was in a massive block of formatter off.

Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
…ls to resolve to virtual cluster.

Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: ShubhamRwt <shubhamrwt02@gmail.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: k-wall <18440250+k-wall@users.noreply.github.com>
Co-authored-by: k-wall <18440250+k-wall@users.noreply.github.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Keith Wall <kwall@apache.org>
Signed-off-by: Ujjwal <ukalia@confluent.io>
For TLS clusterIP ingress we manifest:
1. a bootstrap service named ${virtualClusterName}-${ingressName}, the
   same naming we use for the service carrying the TCP bootstrap port.
2. a service for each upstream node named like
   ${virtualClusterName}-${ingressName}-${upstreamNodeId}.
3. we configure the proxy virtual cluster gateway like:
   sniHostIdentifiesNode:
     bootstrapAddress: "${virtualClusterName}-${ingressName}.${proxyNamespace}.svc.cluster.local:9191"
     advertisedBrokerAddressPattern: "${virtualClusterName}-${ingressName}-$(nodeId).${proxyNamespace}.svc.cluster.local:9292"

The user must ensure their downstream TLS certificates contain SANs for the
bootstrap and each upstream node.

Why:
Previously we limited the user to a single VKC+ingress using identifying
ports due to the risks of using port for identification (during
reconciliation, the identity of a port can change, so clients could
connect to an unexpected vcluster/node).

By changing to SNI as the mechanism, the identity is unambiguous as
VKCs/gateways are added and removed, as the hostnames should be stable
and uniquely address a gateway bootstrap or node. So we can safely allow
numerous TLS clusterIP ingresses to co-exist within a cluster without
implementing a safer port allocation strategy for identifying ports.

Signed-off-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
kubectl exited non-zero when we try to delete some resources, when k8s
has no knowledge of those CRDs. This command is intended to clean out
existing resources and CRDs from an established minikube, as well as
install into a fresh minikube. So we can tactically ignore those
failures.

Signed-off-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Why:
We want to be able to suffix the proxy name to create a shared
LoadBalancer service. Limiting the KafkaProxy name gives us some wiggle
room since we can only create Services with names up to 63 characters.

Signed-off-by: Robert Young <robeyoun@redhat.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
Fixes: kroxylicious#1852
Signed-off-by: Sam Barker <sam@quadrocket.co.uk>
Signed-off-by: Ujjwal <ukalia@confluent.io>
* first try

Signed-off-by: Francisco Vila <fvila@redhat.com>

* make olm installation work

Signed-off-by: Francisco Vila <fvila@redhat.com>

* added some default values

Signed-off-by: Francisco Vila <fvila@redhat.com>

* fix installation method

Signed-off-by: Francisco Vila <fvila@redhat.com>

* add olm deployment name as env variable

Signed-off-by: Francisco Vila <fvila@redhat.com>

* replace install type env variable

Signed-off-by: Francisco Vila <fvila@redhat.com>

* fix format

Signed-off-by: Francisco Vila <fvila@redhat.com>

* added undeclared dependencies

Signed-off-by: Francisco Vila <fvila@redhat.com>

* set new default value for olm deployement

Signed-off-by: Francisco Vila <fvila@redhat.com>

* small nits from Keith

Signed-off-by: Francisco Vila <fvila@redhat.com>

---------

Signed-off-by: Francisco Vila <fvila@redhat.com>
Signed-off-by: Ujjwal <ukalia@confluent.io>
@ujjwalkalia ujjwalkalia force-pushed the ssl_principal_propagation branch from 037b011 to b0e4f40 Compare June 8, 2025 21:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants