Skip to content

fix(middleware): added client credential login to middleware#1844

Merged
kian99 merged 1 commit intocanonical:v3from
alesstimec:middleware-client-creds-login
Feb 2, 2026
Merged

fix(middleware): added client credential login to middleware#1844
kian99 merged 1 commit intocanonical:v3from
alesstimec:middleware-client-creds-login

Conversation

@alesstimec
Copy link
Collaborator

@alesstimec alesstimec commented Jan 30, 2026 •
edited
Loading

Description

Enables client credential login to http endpoints we proxy.

Engineering checklist

  • Documentation updated
  • Covered by unit tests
  • Covered by integration tests

Test instructions

  1. apply the plan below
  2. change resource revision to 2 and reapply the plan
terraform {
	required_providers {
		juju = {
			source  = "juju/juju"
			version = "1.2.0"
		}
	}
	required_version = ">= 1.5.0"
}


provider "juju" {
	client_id     = "test-client-id"
	client_secret = "2M2blFbO4GX4zfggQpivQSxwWX1XGgNf"
	controller_addresses = "jimm.localhost:443"
}

# Pull existing cloud credential for LXD from local Juju file store
resource "juju_credential" "lxd" {
	name   = "lxd-credential"

    cloud {
      name = "lxd"
    }

	auth_type = "certificate"

	attributes = {
		client-cert = <<-EOT
<REDACTED>
		EOT
		client-key  = <<-EOK
<REDACTED>
		EOK
		server-cert = <<-EOS
<REDACTED>
		EOS
	}
}


resource "juju_model" "test-model" {
	name        = "test-model"

	credential = juju_credential.lxd.name

    cloud {
      name = "lxd"
    }
}

resource "juju_application" "test" {
	name 	 = "test-app"
	model_uuid = juju_model.test-model.uuid

	charm {
		name = "juju-qa-test"
		revision = 26
		channel = "latest/stable"
	}

	resources = {
		"foo-file" = "1"
	}
}

@alesstimec alesstimec requested a review from a team as a code owner January 30, 2026 15:29
SimoneDutto
SimoneDutto approved these changes Jan 30, 2026
View reviewed changes
Copy link
Contributor

@SimoneDutto SimoneDutto left a comment

Choose a reason for hiding this comment

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

rip: JIMMAuthner

luci1900
luci1900 approved these changes Jan 30, 2026
View reviewed changes
Copy link
Contributor

@luci1900 luci1900 left a comment

Choose a reason for hiding this comment

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

That's nice.

ale8k
ale8k reviewed Jan 30, 2026
View reviewed changes
internal/middleware/authn.go Outdated
if err == nil {
next.ServeHTTP(w, r.WithContext(withIdentity(ctx, user)))
}
// then try client credentials authentication
Copy link
Contributor

@ale8k ale8k Jan 30, 2026

Choose a reason for hiding this comment

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

I'd prefer an explicit method for client creds, I wouldn't consider them basic auth as that implies it is a person. It also doesn't read well as the method is called AuthenticateWithSessionTokenViaBasicAuth, so oyou could call it AuthenticateWithSessionTokenViaBasicAuthOrClientCredentials or just make a new method.

@alesstimec alesstimec force-pushed the middleware-client-creds-login branch 2 times, most recently from d9becbf to 34d35c6 Compare February 2, 2026 11:26
@canonical canonical deleted a comment from github-actions bot Feb 2, 2026
kian99
kian99 approved these changes Feb 2, 2026
View reviewed changes
internal/middleware/authn.go
ctx := r.Context()
// extract auth token
_, password, ok := r.BasicAuth()
username, password, ok := r.BasicAuth()
Copy link
Contributor

@kian99 kian99 Feb 2, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed with Alex that we are overloading things here. It would make more sense to parse the Authorization header. If it's basic auth, i.e. Basic <base64value> we treat it as a client-id/secret, and if it's Bearer <token> we treat it as a session token. The auth schemes are covered here. Unfortunately we can't change the handling for tokens since that would break backwards compatibility.

We could change the format for the ClientCredentials login provider since only JIMM parses it and clearly that doesn't work yet. I.e. on the Juju client side use a different scheme rather than Basic. There aren't any good schemes that cover this though. And it also takes me back to the idea that JIMM "technically" shouldn't receive a client-id/secret, the client should exchange those with the identity server for a token, then it's always bearer auth.

A long term option would be to change the Juju client's SessionTokenLoginProvider to send a Bearer <token> header and support both for a time. A bunch of effort for not much gain though.

In the end, what you've done is fine. To avoid the try option 1 then try option 2, you could choose to check if username != "" and do the client-credentials login else do SessionToken login but that's a nit imo.

internal/middleware/authn.go Outdated
Comment on lines 91 to 92
// AuthenticateViaBasicAuth performs basic auth authentication and puts an identity in the request's context.
// The basic-auth is composed of an empty user, and as a password a jwt token that we parse and use to authenticate the user.
Copy link
Contributor

@kian99 kian99 Feb 2, 2026

Choose a reason for hiding this comment

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

I'd expand the docstring to explain why we have to try one option then another.

@alesstimec
fix(middleware): added client credential login to middleware
52e8b5d 
Enables client credential login to http endpoints we proxy.
@alesstimec alesstimec force-pushed the middleware-client-creds-login branch from 34d35c6 to 52e8b5d Compare February 2, 2026 12:41
@kian99 kian99 merged commit b9b1190 into canonical:v3 Feb 2, 2026
8 of 9 checks passed
@alesstimec alesstimec mentioned this pull request Feb 2, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ale8k ale8k ale8k left review comments

@luci1900 luci1900 luci1900 approved these changes

@SimoneDutto SimoneDutto SimoneDutto approved these changes

@kian99 kian99 kian99 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alesstimec @luci1900 @SimoneDutto @ale8k @kian99