Add "expose LDAP groups" feature #49

evrardjp-cagip · 2024-12-17T07:55:49Z

Without this patch, we are filtering LDAP groups and take a decision on what to expose.

This is a problem, as it removes the flexibility of rolebindings later.

We intend to expose custom role bindings for extra services (for example Kong), which requires teams to be entering a different model. In that case, the platform team creates a new cluster role, but the grantee of the role might come from a deployment tool, hence outside the operator.

I fixed it by first exposing the groups directly in the token provider. For that, I had to clean the token provider first, then fixing a few existing code's panics, cleaning the http handlers, adding more tests, simplifying auth, removing useless code, streamlining ldap requests, fixing the config validation, exposing errors into the main loop instead of silently ignoring them, and remove dependencies to rely more on standard library.

It's a big PR, so I suggest you to review the code one commit at a time.

Without this patch, we are filtering LDAP groups and take a decision on what to expose. This is a problem, as it removes the flexibility of rolebindings later. We intend to expose custom role bindings for extra services (for example Kong), which requires teams to be entering a different model. In that case, the platform team creates a new cluster role, but the grantee of the role might come from a deployment tool, hence outside the operator. I fixed it by first exposing the groups directly in the token provider. This means that further commits are required to filter properly + generate the right token groups directly.

Without this, the code was duplicated and the generation of claims was not very readable. For example, it contained steps that are part of the issuer initialisation. This is a problem, as it leads to difficult reviews and difficulty to iterate on the topic. This fixes it by creating a new constructor for the token issuer, which clarifies all what's necessary for it, and fatals if the requirements are not met. This makes sure the code does not error forever when the requirements are not met. On top of that, the signature of the token was separated, allowing easier testing. The testing has shown a lack of handling the errors in the JWT signature checks, which should be fixed in a later commit.

Without this, creating a token provider with no private key will still try to sign the token. This should never happen. However, acting on it, even on tests, means a generic panic, instead of a just an error. This handles the pointer dereferencing to ensure the code does not panic, as it should already fatal on main through the constructor.

Without this, the basic auth is included deep in the call stack. This is a problem, as it means multiple calls have the information about passwords, and need to carry useless data around. This fixes it by ensuring a new middleware for basicAuth was added, directly connecting to ldap. The user information is then filtered to keep only username, userDN, and email. It is then passed in a context for use in the next httpHandlers. At the same time, it allowed me to see that the GenerateConfig would not fail if the generation of the token results in an error. I fixed it by adding the same logic in GenerateConfig and in GenerateJWT http handlers.

Without this we will not be able to refactor GenerateJWT and GerateConfig. This further allows testability for the config generation.

This allows simplification of the code.

This makes it easier to reason around the getUser details.

Without this, the naming is a bit hard to follow, and the indentation is not really matching the usual go patterns. This fixes it to make the code more readable.

Without this, we validate the token format twice: one when doing parsewithclaims, one when we do our own validation. This serves no purposes, and introduces fragile code. This fixes it by removing the useless commits.

We use different but similiar constructor methods for the Has* calls. This is a problem, as it makes the ldap methods unnecessary harder to read. This fixes it by ensuring the code is more readable, and allowed us optimisations, like regrouping code to connect, query, and return results with our defaults.

Without this, one might expect to have groups[] to be always populated, and passed to JWTClaims generation. However, this is not the case, as the groups are empty if the user does not have direct access rights to the cluster. This is a problem, as it will prevent further evolution of the group management. To fix this, I made sure that _ALL THE GROUPS_, including the special groups (appops, customerops, cloudops, containerops) have their groups fetched and generated in the JWT. I also moved the code from ldap to project.go, as none of the code was actually relevant from ldap perspective: All the code was manipulating project objects. Tests were added to ensure the behaviour was intact. The code also took the opportunity to remove incorrectly exported functions back to internal functions (and fixing their tests).

Now that authprovider is merely doing ldap functions (it was already doing that), be explicit and call the package ldap.

Presenting the CA is a very small endpoint, lost in the middle of the "services" internal package. This is a problem, as it makes it annoying to find. On top of that, it needed to be passed global variables instead of having direct access to config data. This fixes it by making sure this endpoint (only used once) is directly readable from the main, as it's a two-liner.

Services does not really means what this code does. This is a problem, as it makes the debugging tedious for a new contributor. This fixes it by moving the middlewares to their own package.

Without this, one might wonder where the constants are used. One might even think that Service account is used to provision some parts of kubi, where in fact it is only used for auth. This is a problem, as it could lead to misunderstandings in the code. In other words, I believe that moving the constants will make it more explicit about their scope and maintenance. Therefore it makes clear that the constants moved here are ONLY usable for auth. To avoid issues, this removes the constant from generic "utils" use.

Without this, the helpers become a big mess of functions that are only partially used or are inferior in implementation to what you can find in other parts of the code. This fixes it by: - regroup the config parsing into config.go - inlining functions with little use, which can be replaced by a few idiomatic golang calls. - temporarily moving some implementations until the inlining is safe to implement (needs test coverage) - removing duplicate calls: ldap host parsing in the config, fatal + os.exit(1)...

This commit tidies the go.mod after cleaning the code removing the usage of ozzo validation.

the createAccessToken method is bubbling up errors, but we never show them. This is a problem, as it prevents observability of the errors. This fixes it by logging the issues at error level.

evrardjp-cagip · 2024-12-23T17:13:12Z

internal/ldap/membership.go

+	for _, entry := range m.CloudOpsAccess {
+		groups = append(groups, entry.GetAttributeValue("cn"))
+	}
+	groups = append(groups, m.ListClusterGroups()...)


For the reviewers, this needs testing. Could not do that on devbench.

cmd/api/main.go

internal/ldap/auth.go

internal/ldap/membership.go

internal/services/token-provider.go

internal/utils/prometheus.go

evrardjp-cagip · 2025-01-17T16:32:42Z

We decided to merge all the work into a single PR, #61.

evrardjp-cagip force-pushed the add_groups branch 8 times, most recently from f766390 to 8c67cc2 Compare December 23, 2024 15:33

evrardjp-cagip added 17 commits December 23, 2024 16:56

Extract kubeConfig generation for futureproofing

a6a6ca3

Without this we will not be able to refactor GenerateJWT and GerateConfig. This further allows testability for the config generation.

Use User type for Authenticate user

3dddf81

This allows simplification of the code.

Simplify Authentication flow

477fbb0

This makes it easier to reason around the getUser details.

Refactor code to be more idiomatic

cb5a512

Without this, the naming is a bit hard to follow, and the indentation is not really matching the usual go patterns. This fixes it to make the code more readable.

Do not double validate token

6a7b156

Without this, we validate the token format twice: one when doing parsewithclaims, one when we do our own validation. This serves no purposes, and introduces fragile code. This fixes it by removing the useless commits.

Rename authprovider

c6a7be4

Now that authprovider is merely doing ldap functions (it was already doing that), be explicit and call the package ldap.

Migrate middlewares outside the services folder

fde0e7b

Services does not really means what this code does. This is a problem, as it makes the debugging tedious for a new contributor. This fixes it by moving the middlewares to their own package.

Remove ozzo validation

153ccff

This commit tidies the go.mod after cleaning the code removing the usage of ozzo validation.

evrardjp-cagip force-pushed the add_groups branch from 8c67cc2 to 153ccff Compare December 23, 2024 16:00

Extend logging for token issues

2c2c62d

the createAccessToken method is bubbling up errors, but we never show them. This is a problem, as it prevents observability of the errors. This fixes it by logging the issues at error level.

evrardjp-cagip changed the title ~~[WIP] Add expose LDAP groups~~ Add "expose LDAP groups" feature Dec 23, 2024

evrardjp-cagip commented Dec 23, 2024

View reviewed changes

evrardjp-cagip mentioned this pull request Jan 6, 2025

Code does not pass staticcheck #47

Closed