added post client auth method #289

ilgatnau · 2025-06-22T18:31:43Z

https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication

As per specs, multiple available Client Authentication methods that are used by Clients to authenticate to the Authorization Server when using the Token Endpoint. During Client Registration, the RP (Client) MAY register a Client Authentication method. If no method is registered, the default method is client_secret_basic.

These Client Authentication methods are:

client_secret_basic
client_secret_post
client_secret_jwt
private_key_jwt

This PR implements implementation for client_secret_post (currently only basic possible) plus placeholder for missing other 2 methods.

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

nacx

Thanks!

I've left some comments on the PR. Thanks for raising!

In order to have the PR merged and in s state that can be safely maintained a couple things need to be added:

Unit tests validating that the POST method works as expected. The current PR only configures changes tests for the BASIC. Make sure there are unit tests for the case where no method is set, and when basic and post are set. The three cases need to be explicitly covered.
An end-to-end test that also exercises the post method. We use Keycloak in the e2e tests. Some suite should be updated to use the post mehtod and verify it is working as expected.

Having these two points properly implemented and covered will help us maintain the feature without breaking it accidentally in the future.

Thanks!

nacx · 2025-06-23T11:20:50Z

config/v1/oidc/config.proto

+  // This section define swhether
+  // [Client Authentication](https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication)
+  // is Basic (default) or method post.


Suggested change

// This section define swhether

// [Client Authentication](https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication)

// is Basic (default) or method post.

// Available [Client Authentication](https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication) methods

config/v1/oidc/config.proto

internal/authz/oidc.go

nacx · 2025-06-23T11:25:05Z

internal/authz/oidc.go

-		inthttp.HeaderAuthorization: []string{inthttp.BasicAuthHeader(o.config.GetClientId(), o.config.GetClientSecret())},
+	form, error := buildAuthParams(o.config, codeFromReq, stateFromReq)
+	if error != nil {
+		log.Error("error building form", error)


Suggested change

log.Error("error building form", error)

log.Error("error building auth params", error)

nacx · 2025-06-23T11:26:55Z

internal/authz/oidc.go

+// or if the implementation for the specified method is not supported.
+func buildAuthHeader(config *oidcv1.OIDCConfig) (http.Header, error) {
+
+	headers := http.Header{}


There is no need to allocate something here if it is going to be replaced.

Suggested change

headers := http.Header{}

var headers http.Header

internal/authz/oidc.go

nacx · 2025-06-23T11:28:21Z

internal/authz/oidc.go

+	case oidcv1.OIDCConfig_CLIENT_AUTHENTICATION_METHOD_CLIENT_SECRET_JWT:
+		// Build jwt auth header
+		// TODO: implement jwt auth header
+		return nil, errors.New("not implemented")


Add more details in the error about what is not implemented so users can understand what is the problem when receiving the response back.

nacx · 2025-06-23T11:29:03Z

internal/authz/oidc.go

+	params := url.Values{}
+	switch config.GetMethod() {
+	case oidcv1.OIDCConfig_CLIENT_AUTHENTICATION_METHOD_BASIC:
+	default:


Same two comments as in the previous method.

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

ilgatnau · 2025-06-28T08:10:09Z

Thanks!

I've left some comments on the PR. Thanks for raising!

In order to have the PR merged and in s state that can be safely maintained a couple things need to be added:

Unit tests validating that the POST method works as expected. The current PR only configures changes tests for the BASIC. Make sure there are unit tests for the case where no method is set, and when basic and post are set. The three cases need to be explicitly covered.

An end-to-end test that also exercises the post method. We use Keycloak in the e2e tests. Some suite should be updated to use the post mehtod and verify it is working as expected.

Having these two points properly implemented and covered will help us maintain the feature without breaking it accidentally in the future.

Thanks!

Thank you for your comments @nacx, while I am trying to implement new tests for new client authentication methods I have some doubts how you guys would like to have this done as TestOIDCProcess looks like an uber test difficult to breakdown in more atomic tests.

I have for now duplicated this test as a reusable function I can call with different client authentication methods, would that be acceptable?

See commit ilgatnau@998f12b

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

internal/authz/oidc_test.go

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

nacx

Looking good!

I've left mostly style nits.

There is one important validation thing as well.

Once this is addressed, the only thing left would be to exercise the POST authentication in one of the e2e tests (I'd just modify the keycloak suite to use that) and it should be good to go!

nacx · 2025-07-01T21:08:50Z

config/v1/oidc/config.proto

+
+
+    // Clients create a JWT using an HMAC SHA algorithm, such as HMAC SHA-256 (currently not implemented)
+    CLIENT_AUTHENTICATION_METHOD_CLIENT_SECRET_JWT = 3;
+
+
+    // Clients that have registered a public key sign a JWT using that key (currently not implemented)
+    CLIENT_AUTHENTICATION_METHOD_PRIVATE_KEY_JWT = 4;
+
+


style nit: remove the unnecessary blank lines

Suggested change

// Clients create a JWT using an HMAC SHA algorithm, such as HMAC SHA-256 (currently not implemented)

CLIENT_AUTHENTICATION_METHOD_CLIENT_SECRET_JWT = 3;

// Clients that have registered a public key sign a JWT using that key (currently not implemented)

CLIENT_AUTHENTICATION_METHOD_PRIVATE_KEY_JWT = 4;

// Clients create a JWT using an HMAC SHA algorithm, such as HMAC SHA-256 (currently not implemented)

CLIENT_AUTHENTICATION_METHOD_CLIENT_SECRET_JWT = 3;

// Clients that have registered a public key sign a JWT using that key (currently not implemented)

CLIENT_AUTHENTICATION_METHOD_PRIVATE_KEY_JWT = 4;

nacx · 2025-07-01T21:10:33Z

internal/authz/oidc.go

+
+		// Build basic auth header
+		headers = http.Header{
+			inthttp.HeaderContentType:   []string{inthttp.HeaderContentTypeFormURLEncoded},
+			inthttp.HeaderAuthorization: []string{inthttp.BasicAuthHeader(config.GetClientId(), config.GetClientSecret())},
+		}
+
+	case oidcv1.OIDCConfig_CLIENT_AUTHENTICATION_METHOD_CLIENT_SECRET_POST:
+
+		// Build post auth header
+		headers = http.Header{
+			inthttp.HeaderContentType: []string{inthttp.HeaderContentTypeFormURLEncoded},
+		}
+


style nit: for consistency with other code blocks, remove the unnecessary blank lines here as well:

Suggested change

// Build basic auth header

headers = http.Header{

inthttp.HeaderContentType: []string{inthttp.HeaderContentTypeFormURLEncoded},

inthttp.HeaderAuthorization: []string{inthttp.BasicAuthHeader(config.GetClientId(), config.GetClientSecret())},

}

case oidcv1.OIDCConfig_CLIENT_AUTHENTICATION_METHOD_CLIENT_SECRET_POST:

// Build post auth header

headers = http.Header{

inthttp.HeaderContentType: []string{inthttp.HeaderContentTypeFormURLEncoded},

}

// Build basic auth header

headers = http.Header{

inthttp.HeaderContentType: []string{inthttp.HeaderContentTypeFormURLEncoded},

inthttp.HeaderAuthorization: []string{inthttp.BasicAuthHeader(config.GetClientId(), config.GetClientSecret())},

}

case oidcv1.OIDCConfig_CLIENT_AUTHENTICATION_METHOD_CLIENT_SECRET_POST:

// Build post auth header

headers = http.Header{

inthttp.HeaderContentType: []string{inthttp.HeaderContentTypeFormURLEncoded},

}

nacx · 2025-07-01T21:10:56Z

internal/authz/oidc.go

+	var params url.Values
+	switch config.GetClientAuthenticationMethod() {
+	case oidcv1.OIDCConfig_CLIENT_AUTHENTICATION_METHOD_BASIC:
+


Suggested change

nacx · 2025-07-01T21:11:20Z

internal/authz/oidc.go

+			"client_id":     []string{config.GetClientId()},
+			"client_secret": []string{config.GetClientSecret()},
+		}
+


Suggested change

nacx · 2025-07-01T21:11:27Z

internal/authz/oidc.go

+		// Build jwt auth params
+		// TODO: implement jwt auth params
+		return nil, errors.New("client authentication method client_secret_jwt is not implemented")
+


Suggested change

nacx · 2025-07-01T21:11:39Z

internal/authz/oidc.go

+
+	default:
+


Suggested change

default:

default:

nacx · 2025-07-01T21:13:25Z

internal/authz/oidc.go

+
+		return params, nil
+


style nit: for consistency with other code blocks, just rely on the method return. Let's just return in the cases or in the method, but let's not mix as it is confusing.

Suggested change

return params, nil

nacx · 2025-07-01T21:13:42Z

internal/authz/oidc.go

+
+		return params, nil


Suggested change

return params, nil

nacx · 2025-07-01T21:23:29Z

config/v1/oidc/config.proto

+
+  // Available [Client Authentication](https://openid.net/specs/openid-connect-core-1_0.html#ClientAuthentication)
+  // methods
+  ClientAuthenticationMethod client_authentication_method = 24;


Add the validation for this field. I think the lack of this validation is causing the current tests/examples to always fall to the default cases, as the values configured there are not defined in the enum.
Adding this validation should make the authservice fail early and not allow undefined values.

Suggested change

ClientAuthenticationMethod client_authentication_method = 24;

ClientAuthenticationMethod client_authentication_method = 24 [(validate.rules).enum.defined_only = true];

nacx · 2025-07-01T21:47:00Z

BTW, to let the check test pass, can you run make format to get the code formatted?

added post client auth method

2ea0d69

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

ilgatnau requested review from nacx, ZackButcher and sergicastro as code owners June 22, 2025 18:31

istio-testing added the size/L label Jun 22, 2025

missing generated files

727d485

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

istio-testing added size/XXL and removed size/L labels Jun 22, 2025

fix merge

82e02ea

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

istio-testing added size/XL and removed size/XXL labels Jun 22, 2025

nacx reviewed Jun 23, 2025

View reviewed changes

ilgatnau added 3 commits June 24, 2025 21:23

first round of fixes as per PR comments

54d846c

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

Addressed last set of comments

87bd38b

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

new client authentication methods tests

998f12b

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

istio-testing added size/XXL and removed size/XL labels Jun 28, 2025

JWT not implemented test method

9576002

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

nacx reviewed Jun 30, 2025

View reviewed changes

internal/authz/oidc_test.go Show resolved Hide resolved

reusable test for multiple client authentication methods

07d12ec

Signed-off-by: Ivan Gatnau Lopez <[email protected]>

istio-testing added size/XL and removed size/XXL labels Jul 1, 2025

nacx reviewed Jul 1, 2025

View reviewed changes

	log.Error("error building form", error)
	log.Error("error building auth params", error)

	ClientAuthenticationMethod client_authentication_method = 24;
	ClientAuthenticationMethod client_authentication_method = 24 [(validate.rules).enum.defined_only = true];

added post client auth method #289

Are you sure you want to change the base?

added post client auth method #289

Uh oh!

Conversation

ilgatnau commented Jun 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nacx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ilgatnau commented Jun 28, 2025

Uh oh!

Uh oh!

nacx left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nacx commented Jul 1, 2025

Uh oh!

Uh oh!

ilgatnau commented Jun 22, 2025 •

edited

Loading