oidc_child: change default with no auth method#8819
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a new CAM_DEFAULT authentication method to handle cases where no explicit client authentication method is specified. However, the reviewer identified a critical regression: CAM_DEFAULT is not handled in other parts of the codebase, such as append_to_creds_to_post_data, which breaks authentication. To resolve this, the reviewer suggests determining the default method directly as either CAM_SECRET or CAM_NONE within set_client_auth_method depending on the presence of a client secret, thereby eliminating the need for the new CAM_DEFAULT enum value entirely.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| } else { | ||
| opts->client_auth_method = CAM_SECRET; | ||
| opts->client_auth_method = CAM_DEFAULT; | ||
| } | ||
|
|
||
| switch (opts->client_auth_method) { | ||
| case CAM_DEFAULT: | ||
| /* For compatibility with oder version both CAM_SECRET and CAM_NONE | ||
| * are allowed by default. */ | ||
| if (has_creds_pkcs12(opts)) { | ||
| fprintf(stderr, "\nPath to PKCS12 file is not needed for " | ||
| "authentication with or without client secret.\n"); | ||
| return false; | ||
| } | ||
| break; |
There was a problem hiding this comment.
Introducing CAM_DEFAULT as a new enum value causes a critical regression because it is not handled in append_to_creds_to_post_data (located in src/oidc_child/oidc_child_curl.c). As a result, when no explicit authentication method is specified, the client secret is never appended to the POST request, completely breaking authentication for clients that require a secret.
Instead of introducing a new pseudo-state CAM_DEFAULT that needs to be handled across multiple files, we can resolve the default authentication method directly in set_client_auth_method. If no authentication method is specified:
- If a client secret is provided, default to
CAM_SECRET. - Otherwise, default to
CAM_NONE.
This restores the original behavior cleanly without requiring any changes to other files or the client_auth_method enum.
} else {
if (has_creds_client_secret(opts)) {
opts->client_auth_method = CAM_SECRET;
} else {
opts->client_auth_method = CAM_NONE;
}
}
switch (opts->client_auth_method) {There was a problem hiding this comment.
/gemini see update
There was a problem hiding this comment.
Thank you for the update. The implementation in the latest patch correctly resolves the default authentication method by checking for the presence of client credentials directly within set_client_auth_method. This approach avoids introducing a new enum value and effectively restores the expected behavior for public clients without causing regressions in other parts of the codebase.
| CAM_DEFAULT, /* can be CAM_SECRET or CAM_NONE */ | ||
| CAM_SENTINEL |
Before the new client-auth-method option was added oidc_child was accepting a client or nothing in case the IdP client is a public one. With the introduction of the client-auth-method option the default if not option was given was set the 'secret' which cause a regression for public clients. With this patch the old default behavior, accepting a secret or nothing, is restored.
a2065fe to
cc77ca4
Compare
Before the new client-auth-method option was added oidc_child was accepting a client or nothing in case the IdP client is a public one. With the introduction of the client-auth-method option the default if not option was given was set the 'secret' which cause a regression for public clients.
With this patch the old default behavior, accepting a secret or nothing, is restored.