Feat: Add authentik idp#8619
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces support for Authentik as an Identity Provider (IdP) in SSSD, including documentation, a new lookup implementation, and JSON parsing enhancements. Several critical issues were identified in the review: a syntax error due to missing parentheses in a conditional check, incorrect parameter types and redundant calls during URL encoding of device codes, and logic errors in identifying Authentik users and groups. Specifically, using 'pk' for both user and group identifiers prevents proper object differentiation, and the user lookup logic should be updated to check for the 'pk' attribute to align with Authentik's API.
e2158a2 to
3cdafa1
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request adds support for Authentik as an Identity Provider, including documentation, a new lookup implementation, and enhanced JSON parsing. Feedback points out a potential crash in parse_result due to a missing NULL check when encoding the device_code and a typo in store_json_user where uid was used instead of uuid.
3cdafa1 to
677e00c
Compare
sssd-bot
left a comment
There was a problem hiding this comment.
Review done using Claude Code / claude-opus-4-6
Functional Issues
-
get_user_identifierregression: NULLuser_identifier_attrbreaks all IdP types (src/oidc_child/oidc_child_json.c:539)The old code handled
user_identifier_attr == NULLexplicitly:const char *id_attr_list[] = { "sub", "id", NULL }; if (user_identifier_attr != NULL) { id_attr_list[0] = user_identifier_attr; id_attr_list[1] = NULL; }
The new code:
const char *id_attr_list[] = { user_identifier_attr, "sub", "id", NULL };
When
user_identifier_attris NULL (which is the default — it's an optional--user-identifier-attributeCLI argument),id_attr_list[0]is NULL, terminating the loop atid_attr_list[c] != NULLimmediately. The fallback attributes"sub"and"id"are never tried. This breaks user authentication for all IdP types (Entra ID, Keycloak, and Authentik) when the user-identifier-attribute is not explicitly configured. -
NULL pointer dereference in
authentik_lookupdone block (src/oidc_child/oidc_child_id.c:617-623)When
get_json_string_array_from_json_stringreturns NULL at line 614,retis set toENOMEMat line 619, but execution falls through toadd_posix_to_json_string_array(mem_ctx, ..., 0, tmp, out)at line 621 withtmp == NULL.add_posix_to_json_string_arraycallsjson_loads(in, ...)with a NULLin, causing a crash. Agoto done-style early return or anelseblock is needed after the NULL check (note: this function only has one label so a second error path must usereturnor restructure). -
Device code URL-encoding in shared code path may break other IdP types (
src/oidc_child/oidc_child_json.c:394-399)parse_result()is called for all IdP types, not just Authentik. The addedurl_encode_string()call on the device code will encode it for every IdP whenuser_codeis present. If Entra ID or Keycloak device codes contain characters that are already URL-safe but get modified by encoding (e.g.,+,=, or%-encoded sequences getting double-encoded), this will break their device authorization flows. This change needs to be conditional on the IdP type, or it needs evidence that Entra ID and Keycloak device codes are invariant under URL encoding. -
get_json_stringinteger-to-string conversion is a global behavior change (src/oidc_child/oidc_child_json.c:42-52)get_json_stringis called throughout the OIDC child code (device code parsing, token handling, userinfo, etc.). Making it silently convert integers to strings changes behavior for every call site. Code paths that previously received NULL for integer-typed JSON fields and treated that as "field not present" will now silently get a stringified integer. This should be a separate function (e.g.,get_json_string_or_int) or the integer handling should be done at the Authentik-specific call sites. -
Debug message / code mismatch in
store_json_user(src/providers/idp/idp_id_eval.c:66-70)The code checks for
"uid"as fallback:uuid = json_object_get(user, "uid");
But the error message says:
"JSON user object does not contain 'id' or 'uuid' string.\n"It should say
'id' or 'uid'. This will mislead debugging. -
Missing
:relnote:tagThis PR adds a new user-visible IdP type (
authentik). Neither commit message contains a:relnote:tag.
Nits & Non-functional Issues
-
Missing space after
ifandswitchkeywords (src/oidc_child/oidc_child_json.c:42,src/oidc_child/oidc_child_id.c:504)SSSD coding style requires a space:
if (...),switch (...). The PR hasif(json_is_integer(tmp))andswitch(oidc_cmd). -
Inconsistent indentation in
parse_result(src/oidc_child/oidc_child_json.c:395-399)The
ifblock uses 6-space indentation instead of 4:dc_enc = get_json_string(dc_ctx, root, "device_code"); if (dc_enc != NULL && dc_ctx->user_code != NULL) { ... }
-
CamelCase variable name (
src/oidc_child/oidc_child_json.c:44)iValshould bei_valor similar per SSSD style ("Use lower cased names with words separated by underscores"). -
Leading whitespace before
obj_id(src/oidc_child/oidc_child_id.c:563)Extra leading space:
obj_id = get_str_attr_from_embed_json_string(— should be 4-space indent. -
Double blank line (
src/oidc_child/oidc_child_id.c:631-632)Two consecutive blank lines between
authentik_lookupandoidc_get_id. -
Extra blank line (
src/oidc_child/oidc_child_json.c:541)Double blank line before the
forloop inget_user_identifier. -
Trailing space before comma in
talloc_asprintfcalls (src/oidc_child/oidc_child_id.c:515,533,537)Several instances of
"...%s" ,base_urlwith a space before the comma instead of after. -
Confusing boolean condition (
src/oidc_child/oidc_child_id.c:512)if (sep == NULL && sep != input)— whensepis NULL,sep != inputis always true (assuming non-NULL input), so this is equivalent to justif (sep == NULL). This is the same as the pre-existing Keycloak code at line 342, but worth cleaning up. The Entra ID version at line 122 uses the clearerif (sep == NULL || sep == input). -
Hard-coded
page_size=2000(src/oidc_child/oidc_child_id.c:575,579)The page size for group membership and user group queries is hard-coded to 2000. For environments with many groups or members, this may silently truncate results with no indication to the user.
-
Typo "SCERET" in manpage (
src/man/sssd-idp.5.xml:305)YOUR-CLIENT-SCERETshould beYOUR-CLIENT-SECRET. (Note: this same typo exists in the pre-existing Entra ID and Keycloak examples.)
Confirmed Issues from Existing Review Comments
-
gemini-code-assist:
if(json_is_integer(tmp))missing space — The syntax error flagged (missing parentheses) appears to have been fixed in the latest revision. The actual post-PR code atsrc/oidc_child/oidc_child_json.c:42hasif(json_is_integer(tmp))which compiles but violates style (missing space afterif). -
gemini-code-assist:
url_encode_stringparameter type — The latest code atsrc/oidc_child/oidc_child_json.c:398correctly usesdc_ctx->rest_ctxas the first argument. This appears to have been fixed. -
gemini-code-assist:
uidvsuuidinstore_json_user— The PR author states that Authentik's API usesuid(notuuid) for users, referencing the Authentik API docs. However, the debug message at line 70 still says'uuid'instead of'uid', which is misleading regardless of which field name is correct. -
gemini-code-assist:
name_and_type_identifierusingpkfor both user and group identifiers — The latest code atsrc/oidc_child/oidc_child_id.c:487-491now correctly uses"username"foruser_identifier_attrand"name"forgroup_identifier_attr. This appears to have been fixed.
@Maddosaurus, take this with a grain of salt as some points might be off. If you prefer, feel free to postpone addressing this until somebody from maintainers performs a review. |
677e00c to
bd08e60
Compare
| "User identifier not found in user info data, " | ||
| "checking id token.\n"); | ||
|
|
||
| <<<<<<< HEAD |
There was a problem hiding this comment.
Looks like you accidentally committed a conflict?
There was a problem hiding this comment.
I've overlooked an artifact while rebasing, thanks for flagging that.
Going forward, I'll commit any feedback I address into their own commits and then squash these into the final commits at the end
bd08e60 to
f0a57c4
Compare
|
@alexey-tikhonov I'd appreciate a human review at this point, as it seems that recent changes on master resulted in this code not working when rebased against the latest master. |
:relnote: Add Authentik as an IDP integration option.
|
Could you please rebase on the top of the current 'master' branch? |
1c58c51 to
ba1d25b
Compare
|
I've rebased on master - to describe the current situation: Authentik uses random special characters for the device code, and loading/saving the JSON object in SSSD via the jansson lib strips some of the escaping. I suspect that the device code needs to be url sanitized when loaded from the buffer ( I suspect a3c506d to be one of the reasons for a change in behaviour. This escaping of my change is not needed anymore because of improved URL data escaping, I will remove that. |
|
This currently doesn't build: |
|
I've removed the not needed function I mentioned earlier. Could you advise what the best strategy would be to ensure proper url safe encoding of the token that is stored in An example device token that Authentik returns: |
Hi, I'm a bit confused because the error in the logs you've send is coming from the Can you recompile after running bye, |
I see, thanks for the clarification. I'm not entirely sure where to escape the device code best after the recent changes. Any advice is appreciated.
You should find the logs in your inbox momentarily. |
|
@sumit-bose / @alexey-tikhonov From my point of view the PR is ready for a review. I can confirm that Authentik OIDC now works correctly. |
Thank you. Take a note it will take some time though, as we are going thru release cycle now. |
|
|
||
| token_data = json_loadb((const char *) buf, token_buflen, 0, &json_error); | ||
| if (token_data == NULL) { | ||
| // FIXME: Authentik fails at this point, |
This PR add Authentik as an available IDP provider besides Entra and Keycloak.
From a quick look around the Authentik repo, it looks they do daily test runs against their Compose deployment (see this workflow).
For testing, starting from a fresh Authentik instance (Docker compose instructions), the following additional configuration is needed:
sssd-prod-prov) and auth flow (e.g.default-provider-authorization-implicit-consent (Authorize Application))authentik default OAuth Mapping: authentik API accessto "Selected Scopes"sssd-prod-appand select the previously created provider (e.g.sssd-prod-prov)ak-<application>-client_credentials)authentik user goup read-only)authentik CoreselectCan view GroupandCan view Userakservice user of your application: Users -> $serviceaccout -> Roles -> Add to existing role -> select the created roleCombined with the following
sssd.confthis will result in a running setup for ssh login and sudo management:Note that authentik does not have application-specific endpoints for token, userinfo and device auth, but instead global endpoints. For example, the token endpoint for a given Authentik server would be
https://prod.idp.tld.com/application/o/token(onlyauthentik.companyis a placeholder in this config).Fixes #8613