Skip to content

backend: oidc: impersonate instead of forwarding oidc token to k8s api #2814

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

backend: oidc: impersonate instead of forwarding oidc token to k8s api #2814

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1bbd62
backend: oidc: impersonate instead of forwarding oidc token to k8s api
iSE-stefan-may Jan 30, 2025
bd747b2
Merge branch 'main' into feat/oidc-impersonate
iSE-stefan-may Feb 7, 2025
53fadbe
backend: oidc: adaptions
iSE-stefan-may Feb 7, 2025
039dde1
backend: oidc: cleanup, oidc groups added, filter for other headers
iSE-stefan-may Feb 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
109 changes: 108 additions & 1 deletion backend/cmd/headlamp.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@
oidcClientID string
oidcClientSecret string
oidcIdpIssuerURL string
oidcImpersonate bool
oidcUserClaim string
oidcGroupsClaim string
oidcProvider *oidc.Provider
baseURL string
oidcScopes []string
proxyURLs []string
Expand Down Expand Up @@ -366,7 +370,7 @@
if config.useInCluster {
context, err := kubeconfig.GetInClusterContext(config.oidcIdpIssuerURL,
config.oidcClientID, config.oidcClientSecret,
strings.Join(config.oidcScopes, ","))
strings.Join(config.oidcScopes, ","), config.oidcImpersonate)
if err != nil {
logger.Log(logger.LevelError, nil, err, "Failed to get in-cluster context")
}
Expand Down Expand Up @@ -896,9 +900,112 @@
})
}

// OIDCImpersonateMiddleware will validate and exchange the authorization JWT from the header with
// the kubernetes service account JWT and instead impersonate the user using the `Impersonate-User`
// header.
// The headlamp service account must have the corresponding roles, see
// https://kubernetes.io/docs/reference/access-authn-authz/authentication/#user-impersonation
// If the token cannot be validated, the request will be forwarded to the next handler.
func (c *HeadlampConfig) OIDCImpersonateMiddleware(next http.Handler) http.Handler {

Check failure on line 909 in backend/cmd/headlamp.go

View workflow job for this annotation

GitHub Actions / build 

Function 'OIDCImpersonateMiddleware' is too long (74 > 60) (funlen)
oidcProvider, err := oidc.NewProvider(context.Background(), c.oidcIdpIssuerURL)
if err != nil {
logger.Log(logger.LevelError, map[string]string{"idpIssuerURL": c.oidcIdpIssuerURL},
err, "failed to get oidc provider")
return next
}

c.oidcProvider = oidcProvider

return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
// skip if not cluster request
if !strings.HasPrefix(r.URL.String(), "/clusters/") {
next.ServeHTTP(w, r)
return
}

// parse cluster and token
cluster, token := parseClusterAndToken(r)
if cluster == "" || token == "" {
next.ServeHTTP(w, r)
return
}

idToken, err := c.getValidToken(r.Context(), token)
if err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please add some logging in the error conditions?

Copy link
Author

Choose a reason for hiding this comment

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

Logging added, but I'm not really sure, whether it is really the best case to log always when the token validation fails. It may also be, that OIDC impersonation is active, but the user still used a serviceaccount token, in those cases a warning in the log would be irritating.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok. That sounds like it makes sense to remove.

Is something like this possible? Otherwise remove that logging?

if ! OIDCImpersonationEnaled and serviceAccount {
    log()
}

Copy link
Author

Choose a reason for hiding this comment

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

Wether the user provided token is a kubernetes serviceaccount token can only be evaluated from the Kubernetes API, therefore this check won't be possible. Only option where this would be possible would be, if we disable the option to login in the ui via a serviceaccount token altogether. In this case we could give a log warning here, but as far as I can see, that's currently not possible.
I'll cleanup the logging for the cases, where it may not be an error case.

next.ServeHTTP(w, r)
return
}

var claims map[string]interface{}
if err := idToken.Claims(&claims); err != nil {
logger.Log(logger.LevelWarn, nil, err, "token claims could not be extracted")
next.ServeHTTP(w, r)
return
}
user, ok := claims[c.oidcUserClaim].(string)
if !ok || user == "" {
logger.Log(logger.LevelWarn, nil, err, "no user found in token")
next.ServeHTTP(w, r)
return
}
var groups []string
if c.oidcGroupsClaim != "" {
switch v := claims[c.oidcGroupsClaim].(type) {
case string:
groups = []string{v}
case []string:
groups = v
}
}

context, err := c.kubeConfigStore.GetContext(cluster)
if err != nil {
logger.Log(logger.LevelError, map[string]string{"cluster": cluster}, err,
"no serviceaccount token found")
next.ServeHTTP(w, r)
return
}

// User was successfully authenticated, execute request as this user
r.Header.Set("Authorization", "Bearer "+context.BearerToken)
// Since we are doing requests with the serviceaccount, make sure there are no
// other Impersonate-* headers added
for key, _ := range r.Header {

Check failure on line 973 in backend/cmd/headlamp.go

View workflow job for this annotation

GitHub Actions / build 

File is not `gofmt`-ed with `-s` (gofmt)
if strings.HasPrefix(strings.ToLower(key), "impersonate-") {
r.Header.Set(key, "")
}
}
r.Header.Set("Impersonate-User", user)
for _, group := range groups {
r.Header.Set("Impersonate-Group", group)
Copy link

Choose a reason for hiding this comment

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

I believe this should use Add instead of Set.

Suggested change
r.Header.Set("Impersonate-Group", group)
r.Header.Add("Impersonate-Group", group)

}
next.ServeHTTP(w, r)
})
}

func (c *HeadlampConfig) getValidToken(ctx context.Context, token string) (*oidc.IDToken, error) {
oidcConfig := &oidc.Config{
ClientID: c.oidcClientID,
}
oauth2Token, err := c.oidcProvider.Verifier(oidcConfig).Verify(ctx, token)

Check failure on line 991 in backend/cmd/headlamp.go

View workflow job for this annotation

GitHub Actions / build 

File is not `gofumpt`-ed (gofumpt)
if err != nil {
return nil, fmt.Errorf("token is not valid: %w", err)
}

return oauth2Token, nil
}

func StartHeadlampServer(config *HeadlampConfig) {
handler := createHeadlampHandler(config)

if config.oidcImpersonate {
handler = config.OIDCImpersonateMiddleware(handler)
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain why this middleware is after the OIDCTokenRefreshMiddleware?

Copy link
Author

Choose a reason for hiding this comment

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

It was the most fitting position I thought of, but not a particular reason. Do you have better recommendations?

Copy link
Contributor

Choose a reason for hiding this comment

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

I was wondering how it would interact with the other middleware? Wondering if it would make sense to go before OIDCTokenRefreshMiddleware or not?

Because would the token refresh need to use the impersonated stuff? If that's the case I was wondering if the OIDCImpersonateMiddleware should go first?

Copy link
Author

Choose a reason for hiding this comment

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

You are right, the OIDCTokenRefreshMiddleware needs the original token, therefore it must be evaluated before OIDCImpersonateMiddleware. I'll change the order of those two and add a comment.


logger.Log(logger.LevelInfo, nil, nil, "OIDC impersonate active")
}
// OIDCTokenRefreshMiddleware must always be first evaluated, since it needs the original
// OIDC token and other middlewares might exchange it
handler = config.OIDCTokenRefreshMiddleware(handler)

// Start server
Expand Down
3 changes: 3 additions & 0 deletions backend/cmd/server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,9 @@ func main() {
oidcClientSecret: conf.OidcClientSecret,
oidcIdpIssuerURL: conf.OidcIdpIssuerURL,
oidcScopes: strings.Split(conf.OidcScopes, ","),
oidcImpersonate: conf.OidcImpersonate,
oidcUserClaim: conf.OidcUserClaim,
oidcGroupsClaim: conf.OidcGroupsClaim,
baseURL: conf.BaseURL,
proxyURLs: strings.Split(conf.ProxyURLs, ","),
enableHelm: conf.EnableHelm,
Expand Down
8 changes: 8 additions & 0 deletions backend/pkg/config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,9 @@ type Config struct {
OidcClientSecret string `koanf:"oidc-client-secret"`
OidcIdpIssuerURL string `koanf:"oidc-idp-issuer-url"`
OidcScopes string `koanf:"oidc-scopes"`
OidcImpersonate bool `koanf:"oidc-impersonate"`
OidcUserClaim string `koanf:"oidc-user-claim"`
OidcGroupsClaim string `koanf:"oidc-groups-claim"`
}

func (c *Config) Validate() error {
Expand Down Expand Up @@ -166,6 +169,11 @@ func flagset() *flag.FlagSet {
f.String("oidc-idp-issuer-url", "", "Identity provider issuer URL for OIDC")
f.String("oidc-scopes", "profile,email",
"A comma separated list of scopes needed from the OIDC provider")
f.Bool("oidc-impersonate", false,
Copy link
Contributor

Choose a reason for hiding this comment

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

@knrt10 would this need to be added to the helm chart as well?

Copy link
Author

Choose a reason for hiding this comment

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

Added to the helm chart, however I'm not convinced that I did it in a good way, since the chart is a little bit confusing for me. So please recheck.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our main helm chart expert is on holidays at the moment. So this will take a bit longer to review. Bear with me :)

"Kubernetes API calls are done with the service account and impersonated as the user in the OIDC token")
f.String("oidc-user-claim", "preferred_username",
"The username claim of the OIDC token to be used when -oidc-impersonate=true")
f.String("oidc-groups-claim", "", "The groups claim of the OIDC token to be used when -oidc-impersonate=true")

return f
}
Expand Down
13 changes: 11 additions & 2 deletions backend/pkg/kubeconfig/kubeconfig.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ type Context struct {
Source int `json:"source"`
OidcConf *OidcConfig `json:"oidcConfig"`
proxy *httputil.ReverseProxy `json:"-"`
BearerToken string `json:"bearerToken"`
Internal bool `json:"internal"`
Error string `json:"error"`
}
Expand Down Expand Up @@ -867,7 +868,7 @@ func splitKubeConfigPath(path string) []string {
// GetInClusterContext returns the in-cluster context.
func GetInClusterContext(oidcIssuerURL string,
oidcClientID string, oidcClientSecret string,
oidcScopes string,
oidcScopes string, oidcImpersonate bool,
) (*Context, error) {
clusterConfig, err := rest.InClusterConfig()
if err != nil {
Expand All @@ -887,7 +888,10 @@ func GetInClusterContext(oidcIssuerURL string,

inClusterAuthInfo := &api.AuthInfo{}

var oidcConf *OidcConfig
var (
oidcConf *OidcConfig
bearerToken string
)

if oidcClientID != "" && oidcClientSecret != "" && oidcIssuerURL != "" && oidcScopes != "" {
oidcConf = &OidcConfig{
Expand All @@ -896,6 +900,10 @@ func GetInClusterContext(oidcIssuerURL string,
IdpIssuerURL: oidcIssuerURL,
Scopes: strings.Split(oidcScopes, ","),
}

if oidcImpersonate {
bearerToken = clusterConfig.BearerToken
}
}

return &Context{
Expand All @@ -904,6 +912,7 @@ func GetInClusterContext(oidcIssuerURL string,
Cluster: cluster,
AuthInfo: inClusterAuthInfo,
OidcConf: oidcConf,
BearerToken: bearerToken,
}, nil
}

Expand Down
52 changes: 51 additions & 1 deletion charts/headlamp/templates/deployment.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
{{- $clientSecret := "" }}
{{- $issuerURL := "" }}
{{- $scopes := "" }}
{{- $impersonate := false }}
{{- $userClaim := "" }}
{{- $groupsClaim := "" }}

# This block of code is used to extract the values from the env.
# This is done to check if the values are non-empty and if they are, they are used in the deployment.yaml.
Expand All @@ -21,6 +24,15 @@
{{- if eq .name "OIDC_SCOPES" }}
{{- $scopes = .value }}
{{- end }}
{{- if eq .name "OIDC_IMPERSONATE" }}
{{- $impersonate = (eq .value "true") }}
{{- end }}
{{- if eq .name "OIDC_USER_CLAIM" }}
{{- $userClaim = .value }}
{{- end }}
{{- if eq .name "OIDC_GROUPS_CLAIM" }}
{{- $groupsClaim = .value }}
{{- end }}
{{- end }}

apiVersion: apps/v1
Expand Down Expand Up @@ -66,9 +78,23 @@ spec:
envFrom:
- secretRef:
name: {{ $oidc.externalSecret.name }}
{{- if .Values.env }}
{{- if or .Values.env $oidc.impersonate }}
env:
{{- if $oidc.impersonate }}
- name: OIDC_IMPERSONATE
value: {{ $oidc.impersonate | quote }}
{{- end }}
{{- if $oidc.userClaim }}
- name: OIDC_USER_CLAIM
value: {{ $oidc.userClaim }}
{{- end }}
{{- if $oidc.groupsClaim }}
- name: OIDC_GROUPS_CLAIM
value: {{ $oidc.groupsClaim }}
{{- end }}
{{- if .Values.env }}
{{- toYaml .Values.env | nindent 12 }}
{{- end }}
{{- end }}
{{- else }}
env:
Expand Down Expand Up @@ -119,6 +145,18 @@ spec:
value: {{ $oidc.scopes }}
{{- end }}
{{- end }}
{{- if $oidc.impersonate }}
- name: OIDC_IMPERSONATE
value: {{ $oidc.impersonate | quote }}
{{- end }}
{{- if $oidc.userClaim }}
- name: OIDC_USER_CLAIM
value: {{ $oidc.userClaim }}
{{- end }}
{{- if $oidc.groupsClaim }}
- name: OIDC_GROUPS_CLAIM
value: {{ $oidc.groupsClaim }}
{{- end }}
{{- if .Values.env }}
{{- toYaml .Values.env | nindent 12 }}
{{- end }}
Expand Down Expand Up @@ -153,6 +191,18 @@ spec:
- "-oidc-idp-issuer-url=$(OIDC_ISSUER_URL)"
- "-oidc-scopes=$(OIDC_SCOPES)"
{{- end }}
{{- if or ($oidc.impersonate) ($impersonate) }}
# Check if impersonate is enabled either from env or oidc.config
- "-oidc-impersonate=$(OIDC_IMPERSONATE)"
{{- end }}
{{- if and (or ($oidc.impersonate) ($impersonate)) (or (ne $oidc.userClaim "") (ne $userClaim "")) }}
# Check if user claim is non empty either from env or oidc.config
- "-oidc-user-claim=$(OIDC_USER_CLAIM)"
{{- end }}
{{- if and (or ($oidc.impersonate) ($impersonate)) (or (ne $oidc.groupsClaim "") (ne $groupsClaim "")) }}
# Check if groups claim is non empty either from env or oidc.config
- "-oidc-groups-claim=$(OIDC_GROUPS_CLAIM)"
{{- end }}
{{- with .Values.config.baseURL }}
- "-base-url={{ . }}"
{{- end }}
Expand Down
12 changes: 12 additions & 0 deletions charts/headlamp/tests/expected_templates/oidc-create-secret.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,12 @@ spec:
secretKeyRef:
name: oidc
key: scopes
- name: OIDC_IMPERSONATE
value: "true"
- name: OIDC_USER_CLAIM
value: preferred_username
- name: OIDC_GROUPS_CLAIM
value: groups
args:
- "-in-cluster"
- "-plugins-dir=/headlamp/plugins"
Expand All @@ -138,6 +144,12 @@ spec:
- "-oidc-idp-issuer-url=$(OIDC_ISSUER_URL)"
# Check if scopes are non empty either from env or oidc.config
- "-oidc-scopes=$(OIDC_SCOPES)"
# Check if impersonate is enabled either from env or oidc.config
- "-oidc-impersonate=$(OIDC_IMPERSONATE)"
# Check if user claim is non empty either from env or oidc.config
- "-oidc-user-claim=$(OIDC_USER_CLAIM)"
# Check if groups claim is non empty either from env or oidc.config
- "-oidc-groups-claim=$(OIDC_GROUPS_CLAIM)"
ports:
- name: http
containerPort: 4466
Expand Down
12 changes: 12 additions & 0 deletions charts/headlamp/tests/expected_templates/oidc-directly-env.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,12 @@ spec:
value: testIssuerURL
- name: OIDC_SCOPES
value: testScope
- name: OIDC_IMPERSONATE
value: "true"
- name: OIDC_USER_CLAIM
value: preferred_username
- name: OIDC_GROUPS_CLAIM
value: groups
args:
- "-in-cluster"
- "-plugins-dir=/headlamp/plugins"
Expand All @@ -122,6 +128,12 @@ spec:
- "-oidc-idp-issuer-url=$(OIDC_ISSUER_URL)"
# Check if scopes are non empty either from env or oidc.config
- "-oidc-scopes=$(OIDC_SCOPES)"
# Check if impersonate is enabled either from env or oidc.config
- "-oidc-impersonate=$(OIDC_IMPERSONATE)"
# Check if user claim is non empty either from env or oidc.config
- "-oidc-user-claim=$(OIDC_USER_CLAIM)"
# Check if groups claim is non empty either from env or oidc.config
- "-oidc-groups-claim=$(OIDC_GROUPS_CLAIM)"
ports:
- name: http
containerPort: 4466
Expand Down
12 changes: 12 additions & 0 deletions charts/headlamp/tests/expected_templates/oidc-directly.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,12 @@ spec:
value: testIssuerURL
- name: OIDC_SCOPES
value: testScope
- name: OIDC_IMPERSONATE
value: "true"
- name: OIDC_USER_CLAIM
value: preferred_username
- name: OIDC_GROUPS_CLAIM
value: groups
args:
- "-in-cluster"
- "-plugins-dir=/headlamp/plugins"
Expand All @@ -114,6 +120,12 @@ spec:
- "-oidc-idp-issuer-url=$(OIDC_ISSUER_URL)"
# Check if scopes are non empty either from env or oidc.config
- "-oidc-scopes=$(OIDC_SCOPES)"
# Check if impersonate is enabled either from env or oidc.config
- "-oidc-impersonate=$(OIDC_IMPERSONATE)"
# Check if user claim is non empty either from env or oidc.config
- "-oidc-user-claim=$(OIDC_USER_CLAIM)"
# Check if groups claim is non empty either from env or oidc.config
- "-oidc-groups-claim=$(OIDC_GROUPS_CLAIM)"
ports:
- name: http
containerPort: 4466
Expand Down
Loading
Loading