OIDC private key JWT / client assertion #155

gulducat · 2025-02-25T17:28:36Z

Hello! I'm a Nomad engineer, implementing "Private Key JWT" a.k.a. client_assertion to our OIDC authentication options, as an alternative to Client Secret auth (although technically, one may sign an asertion JWT with a client secret instead of a private key. this supports that, too).

Reference: https://oauth.net/private-key-jwt/

This comprises two main components:

Creating a JWT with the right shape for providers to verify
- I've added a new clientassertion package to isolate this logic
- This is the bulk of the code in this PR, with tests
Adding the JWT to the auth request for the client to send
- I opted to accept the JWT as a ~~string~~ Serializer, which can be generated using the new package
- This is mostly simple plumbing, and some tests

# 1 does not necessarily need to be in this library, but it seemed reasonable to me. # 2 is required for Nomad's existing usage of hashicorp/cap (example in Nomad repo here)

Nomad will use this sign client assertion JWTs with any of:

Nomad's internal keyring (RS256), which includes a JWKS endpoint for providers that support that
A user-provided RSA key (RS256)
A client secret HMAC (HS256)

Here is a stripped-down example usage of it:

jot, err := NewJWTWithRSAKey(cid, aud, RS256, privKey,
	WithKeyID("some-key-id"), // "kid" header
)
die(err)
req, err := capOIDC.NewRequest(expiration, redirect,
	capOIDC.WithClientAssertionJWT(jot),
)
// and so on as usual

Test coverage of the clientassertion package is high, near 100%. I added tests to the outer oidc library as best I could figure out how it's arranged.

Hopefully this may also make it easier for folks to implement this feature in other products, as they see fit.

for signing JWTs to send as client_assertions ref: https://oauth.net/private-key-jwt/

to send a signed JWT as request's client_assertion ref: https://oauth.net/private-key-jwt/

tgross · 2025-02-25T18:40:24Z

oidc/clientassertion/client_assertion.go

+	ErrMissingFuncNow         = errors.New("missing now func; please use New()")
+)
+
+// New sets up a new ClientAssertion to sign private key JWTs


It doesn't look like we actually have any callers for clientassertion.New here in the cap library. And New is the only non-test code path that calls Validate. I do get that we pulled this out to a package for testability.

But from the perspective of an application consuming this API, I'd question why my application needs to call clientassertion.New instead of passing the appropriate configuration values to oidc.NewConfig. It seems like we're introducing more surface area here (and therefore more ways to get it wrong in the application).

My read of oidc.Config is to configure the Provider, which seems to include some minimum required features of OIDC. Other things (like PKCE, which I followed for my implementation) are more features of individual parts of the process. For client assertions (as with PKCE), it's part of a Request.

I could put more of this logic into the oidc.Option, which is passed in oidc.NewRequest -- currently oidc.WithClientAssertionJWT() just takes a string, but it could instead take all the things that this thing takes, and produce the string itself internally. I'm just not sure how much that buys us, and I figured leaving this open would allow applications to produce the JWT however they may want, making this package convenient but optional.

@johanbrandhorst suggests below that we could pass the ClientAssertion (perhaps renamed to JWT to avoid stuttery package.struct names) instead of a string, then the option or Request constructor could do the signing (if I'm understanding that suggestion right). That would make this extra package a lot less optional for this feature, but definitely harder to hold wrong.

Maybe an in-between would be to accept an interface, which ClientAssertion/JWT implements, so folks could build their own, if they wanted. Harder to hold wrong, but still flexible.

@johanbrandhorst suggests below that we could pass the ClientAssertion (perhaps renamed to JWT to avoid stuttery package.struct names) instead of a string, then the option or Request constructor could do the signing (if I'm understanding that suggestion right). That would make this extra package a lot less optional for this feature, but definitely harder to hold wrong.

Maybe an in-between would be to accept an interface, which ClientAssertion/JWT implements, so folks could build their own, if they wanted. Harder to hold wrong, but still flexible.

Yeah, either of those options sounds pretty solid.

johanbrandhorst

Have you been able to integration test this against any third party OAuth2 servers to see that it works as expected?

oidc/clientassertion/client_assertion.go

oidc/clientassertion/doc.go

oidc/provider.go

oidc/request.go

oidc/clientassertion/client_assertion.go

gulducat · 2025-02-25T21:33:34Z

Regarding your top-level comment @johanbrandhorst

Have you been able to integration test this against any third party OAuth2 servers to see that it works as expected?

Yep! I have tested it using KeyCloak (easy to run on laptop) and Azure EntraID (what our customer who requested this has said they use). I wanted to test with Auth0 too, but this feature is an enterprise feature over there, and I didn't want to jump through those hoops. I haven't checked Okta or others.

Happy to take requests!

mgaffney · 2025-02-25T22:28:42Z

oidc/clientassertion/doc.go

@@ -0,0 +1,13 @@
+package clientassertion


What about a package doc string?

good point, I'll do this tomorrow when I make an example_test.go

v4 is more sensitive to HMAC length

instead of a string, so it's harder to use incorrectly. JWT implements Serializer, but folks may provider their own.

gulducat · 2025-02-26T01:40:42Z

@johanbrandhorst I believe I've addressed the bulk of your feedback. Let me know what ya think?

I'll fix the go.mod mergeflict tomorrow (if I keep going tonight, I'll get carried away 😋), and also an example_test.go along with a package docstring.

gulducat · 2025-02-26T18:44:12Z

Alrighty folks, ready for another pass!

jimlambrt

Ty for the thoughtful contribution. Just a few minor suggestions.

oidc/testing_provider.go

oidc/clientassertion/algorithms.go

oidc/clientassertion/options.go

oidc/clientassertion/client_assertion.go

oidc/provider.go

johanbrandhorst

Thanks for the update, it made me think of some new feedback, hope it makes sense!

oidc/clientassertion/algorithms.go

oidc/clientassertion/client_assertion.go

johanbrandhorst · 2025-02-26T18:56:25Z

oidc/clientassertion/client_assertion.go

+// Validate validates the expected fields
+func (j *JWT) Validate() error {
+	var errs []error
+	if j.genID == nil {
+		errs = append(errs, ErrMissingFuncIDGenerator)
+	}
+	if j.now == nil {
+		errs = append(errs, ErrMissingFuncNow)
+	}
+	// bail early if any internal func errors
+	if len(errs) > 0 {
+		return errors.Join(errs...)
+	}
+
+	if j.clientID == "" {
+		errs = append(errs, ErrMissingClientID)
+	}
+	if len(j.audience) == 0 {
+		errs = append(errs, ErrMissingAudience)
+	}
+	if j.alg == "" {
+		errs = append(errs, ErrMissingAlgorithm)
+	}
+	if j.key == nil && j.secret == "" {
+		errs = append(errs, ErrMissingKeyOrSecret)
+	}
+	if j.key != nil && j.secret != "" {
+		errs = append(errs, ErrBothKeyAndSecret)
+	}
+	// if any of those fail, we have no hope.
+	if len(errs) > 0 {
+		return errors.Join(errs...)
+	}
+
+	// finally, make sure Serialize() works; we can't pre-validate everything,
+	// and this whole thing is useless if it can't Serialize()
+	if _, err := j.Serialize(); err != nil {
+		return fmt.Errorf("serialization error during validate: %w", err)
+	}
+
+	return nil
+}


Do we still need this function? I think if we can get this public API down to NewJWT and .Serialize(), that would be ideal. Can we move the checks that make sense from this function into NewJWT instead? If we're trying to help users using &JWT{} instead of NewJWT(), I don't think we can assume that they will call Validate if they already ignored the constructor, so I'm OK with that case just panicking or erroring in a bad way.

Seems reasonable enough to me -- Okay if I keep it as a separate function, just privatize it? I like small single-purpose functions.

Sure, but I think some of this error checking doesn't really belong here anymore. E.g. the checking if j.key and j.secret are both set aren't necessary if you move the logic into the option as I suggested. Calling Serialize in New also feel really wrong IMO, since it's the primary thing the user wants to do. If that's where we're going with this - why do we need a whole struct (or even package) encapsulating this? We could just have NewSerializedJWT as an exported function (maybe we should?). I think New() and Serialize() can be separate, but if this logic was moved into New, it would be clear from the context of that function what should and shouldn't be necessary to validate. Ideally nothing other than the inputs to the function, which should be all the required parameters to create a JWT, should need to be validated. On that note, since either Key or Secret must be set, should it be a parameter rather than an option?

I'm personally not a huge fan of more smaller functions, because the context in which they are used is not clear and indirection always leads to overhead for the reader. I almost left a comment saying that the structure in Serialize was unnecessarily abstracted into smaller functions, but it's such a minor comment that I didn't in the end feel that it was important enough to bring up, but since we're talking about it now, there you go 😁.

Thanks for the thoughtful reply!

I'll say up front that I'm down to remove the Serialize() call from New - it felt weird when I put it there, so easy to drop it. Also good to move validation into New.

the checking if j.key and j.secret are both set aren't necessary if you move the logic into the option

I like cross-field validation to happen in its own spot, but it amounts to the same result either way, so I'd be happy to put the checks in their respective options. except:

since either Key or Secret must be set, should it be a parameter rather than an option?

The current not-actually-optional-Option thing has been bugging me. Here's some (pardon the pun) other options:

I don't love funcs that take multiple params where any of them are expected to always be zero values like "" / nil

We could do similar to what jose does, and accept an any but assert supported types. Personally this kind of annoys me about jose

Or, what if we have 2 constructors?

func NewJWTWithRSAKey(clientID string, audience []string, alg RSAlgorithm, key *rsa.RSAKey, opts ...Option) (*JWT, error) { } func NewJWTWithHMAC(clientID string, audience []string, alg HSAlgorithm, secret string, opts ...Option) (*JWT, error) { }

That would leave a lot less validation to do, but both could set up and return a JWT, which is mostly shared logic internally.

I like option 3 a lot!

I believe the algorithm will need to be a parameter too, and you could even make it RSAlgorithm and HSAlgorithm respectively :)

oidc/clientassertion/client_assertion.go

oidc/clientassertion/options.go

it is getting late in the day!

if i'd just run my own tests (:

gulducat · 2025-02-27T14:52:14Z

Thanks y'all! I think I've addressed all your comments, with a couple small exceptions that I could still be convinced of. :) Let me know what ya think!

jimlambrt

Ty for all the changes. I had just 2 more I hoping you'll consider.

oidc/clientassertion/algorithms.go

oidc/provider.go

jimlambrt

Ty again for the contribution.

gulducat · 2025-02-27T17:28:57Z

@johanbrandhorst let me know if you'd like to press on any of your unresolved comments, or if there's anything else I can do. :)

oidc/clientassertion/algorithms.go

oidc/clientassertion/client_assertion.go

oidc/clientassertion/options.go

johanbrandhorst · 2025-02-27T21:46:14Z

@gulducat looks like there are still some unresolved replies on the existing comments that I replied to

gulducat · 2025-02-27T22:00:34Z

still some unresolved replies

My bad, I got ahead of myself seeing only the new ones. ❤️

i was a bit overzealous with my tiny methods. i couldn't quite bring myself to delete signer() :P

gulducat · 2025-02-28T02:29:49Z

Got a big ol refactor for ya @johanbrandhorst, spurred by #155 (comment). I do think it's a nice improvement to the ergonomics.

I think I covered your other smaller concerns, too?

The diff is pretty large (including completely rewritten tests), but the public interface is only slightly so, as demonstrated by the barely-changed example_test.go

johanbrandhorst

This looks great, thank you so much for being so responsive and accepting of feedback! I just had a few nits left but I'm happy to merge as is if you want.

oidc/clientassertion/client_assertion.go

oidc/clientassertion/client_assertion_test.go

gulducat · 2025-02-28T17:58:09Z

Thanks for helping me make this thing the best it can be! Latest feedback commit is both easier and harder to read ignoring whitespace: 8051410?w=1 -- Conveniently, I could just turn my code comments into subtest names. :)

gulducat added 3 commits February 25, 2025 12:09

add oidc/clientassertion package

f8b3e76

for signing JWTs to send as client_assertions ref: https://oauth.net/private-key-jwt/

add WithClientAssertionJWT Option

d640633

to send a signed JWT as request's client_assertion ref: https://oauth.net/private-key-jwt/

test WithClientAssertionJWT

ca292a6

gulducat requested a review from a team as a code owner February 25, 2025 17:28

add changelog, fix test

8fca05f

tgross reviewed Feb 25, 2025

View reviewed changes

johanbrandhorst requested changes Feb 25, 2025

View reviewed changes

gulducat added 2 commits February 25, 2025 16:43

s/ClientAssertion/JWT/g in clientassertion pkg

2fe4cae

move options to options.go

b40fd48

mgaffney reviewed Feb 25, 2025

View reviewed changes

gulducat added 3 commits February 25, 2025 19:03

go-jose/v3->v4

4dfbc63

v4 is more sensitive to HMAC length

more and different option validation

d8b89d1

WithClientAssertionJWT accepts a Serializer

a71134d

instead of a string, so it's harder to use incorrectly. JWT implements Serializer, but folks may provider their own.

gulducat added 3 commits February 26, 2025 00:37

Merge branch 'main' into oidc-client-assertion

53388a4

protect kid from headers

25a0e5c

add ExampleJWT test

8f9aa91

mgaffney requested a review from jimlambrt February 26, 2025 18:23

gulducat added 3 commits February 26, 2025 13:33

brief package docstring, rename a const

1d1813c

add copyright headers

cf24de8

missed a rename

9f0b393

gulducat requested a review from johanbrandhorst February 26, 2025 18:44

gulducat mentioned this pull request Feb 26, 2025

oidc: support PKCE and client assertion / private key JWT hashicorp/nomad#25231

Merged

jimlambrt requested changes Feb 26, 2025

View reviewed changes

test the new method on test provider

95b5c03

johanbrandhorst requested changes Feb 26, 2025

View reviewed changes

gulducat added 2 commits February 26, 2025 16:45

godocs and ops everywhere!

c06d559

privatize a couple things, const KeyIDHeader

70ea140

gulducat added 4 commits February 26, 2025 16:51

errors go in error.go

c871e25

error if missing kid

e6d416e

fix obvious mistake

62aaf97

it is getting late in the day!

fix less obvious but still pretty obvious error

9981bce

if i'd just run my own tests (:

jimlambrt requested changes Feb 27, 2025

View reviewed changes

oidc/clientassertion/algorithms.go Outdated Show resolved Hide resolved

oidc/provider.go Outdated Show resolved Hide resolved

add a couple missing err ops

d6932e0

jimlambrt previously approved these changes Feb 27, 2025

View reviewed changes

johanbrandhorst reviewed Feb 27, 2025

View reviewed changes

oidc/clientassertion/algorithms.go Outdated Show resolved Hide resolved

johanbrandhorst reviewed Feb 27, 2025

View reviewed changes

oidc/clientassertion/client_assertion.go Outdated Show resolved Hide resolved

oidc/clientassertion/options.go Outdated Show resolved Hide resolved

pr feedback bits

cd236a5

gulducat dismissed jimlambrt’s stale review via cd236a5 February 27, 2025 21:19

gulducat requested a review from johanbrandhorst February 27, 2025 21:44

gulducat added 3 commits February 27, 2025 17:43

clarify hmac min len

97b3588

refactor to NewJWTWithRSAKey and NewJWTWithHMAC

e8dfe19

remove most single-use private methods

a94da54

i was a bit overzealous with my tiny methods. i couldn't quite bring myself to delete signer() :P

johanbrandhorst previously approved these changes Feb 28, 2025

View reviewed changes

more lovely PR feedback

8051410

gulducat dismissed johanbrandhorst’s stale review via 8051410 February 28, 2025 17:53

johanbrandhorst approved these changes Feb 28, 2025

View reviewed changes

johanbrandhorst merged commit 44f7e72 into hashicorp:main Feb 28, 2025
5 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OIDC private key JWT / client assertion #155

OIDC private key JWT / client assertion #155

gulducat commented Feb 25, 2025 •

edited

Loading

tgross Feb 25, 2025

gulducat Feb 25, 2025

tgross Feb 25, 2025

johanbrandhorst left a comment

gulducat commented Feb 25, 2025

mgaffney Feb 25, 2025

gulducat Feb 26, 2025

gulducat commented Feb 26, 2025

gulducat commented Feb 26, 2025

jimlambrt left a comment

johanbrandhorst left a comment

johanbrandhorst Feb 26, 2025

gulducat Feb 26, 2025

johanbrandhorst Feb 27, 2025

gulducat Feb 27, 2025 •

edited

Loading

johanbrandhorst Feb 27, 2025

johanbrandhorst Feb 27, 2025

gulducat commented Feb 27, 2025 •

edited

Loading

jimlambrt left a comment

jimlambrt left a comment

gulducat commented Feb 27, 2025

johanbrandhorst commented Feb 27, 2025

gulducat commented Feb 27, 2025

gulducat commented Feb 28, 2025

johanbrandhorst left a comment

gulducat commented Feb 28, 2025

OIDC private key JWT / client assertion #155

OIDC private key JWT / client assertion #155

Conversation

gulducat commented Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

gulducat commented Feb 25, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gulducat commented Feb 26, 2025

gulducat commented Feb 26, 2025

jimlambrt left a comment

Choose a reason for hiding this comment

johanbrandhorst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gulducat Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gulducat commented Feb 27, 2025 • edited Loading

jimlambrt left a comment

Choose a reason for hiding this comment

jimlambrt left a comment

Choose a reason for hiding this comment

gulducat commented Feb 27, 2025

johanbrandhorst commented Feb 27, 2025

gulducat commented Feb 27, 2025

gulducat commented Feb 28, 2025

johanbrandhorst left a comment

Choose a reason for hiding this comment

gulducat commented Feb 28, 2025

gulducat commented Feb 25, 2025 •

edited

Loading

gulducat Feb 27, 2025 •

edited

Loading

gulducat commented Feb 27, 2025 •

edited

Loading