diff --git a/CHANGELOG.md b/CHANGELOG.md index bc9c0ada..12a65f4f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,20 @@ ## oidc-agent 5.1.1 +### Change / Enhancement / Bugfix + +The previous release stated that: + + When an account configuration is generated and the OP returns scopes in the initial token flow, the account + configuration is updated with those scopes. + +This did not work as intended. We made the following changes: + +- Fixed a bug, so that the agent now actually behaves as described. +- Implemented separate scope lists for the initial token flow and the refreshing of tokens. Only the refresh-scope-list + is updated. This way access tokens can be obtained with the correct (updated) scope, but re-authentication flows can + still use the original scope list. + ### Enhancements - `oidc-add` can now also take an issuer url to load the default account for this issuer, i.e. `oidc-add ` diff --git a/src/account/account.c b/src/account/account.c index e26c78cf..213dba5c 100644 --- a/src/account/account.c +++ b/src/account/account.c @@ -135,15 +135,17 @@ struct oidc_account* getAccountFromJSON(const char* json) { AGENT_KEY_CONFIG_ENDPOINT, AGENT_KEY_SHORTNAME, OIDC_KEY_CLIENTID, OIDC_KEY_CLIENTSECRET, OIDC_KEY_USERNAME, OIDC_KEY_PASSWORD, OIDC_KEY_REFRESHTOKEN, AGENT_KEY_CERTPATH, - OIDC_KEY_REDIRECTURIS, OIDC_KEY_SCOPE, - OIDC_KEY_DEVICE_AUTHORIZATION_ENDPOINT, OIDC_KEY_CLIENTNAME, - AGENT_KEY_DAESETBYUSER, OIDC_KEY_AUDIENCE, AGENT_KEY_OAUTH, - AGENT_KEY_USESPUBCLIENT, AGENT_KEY_MYTOKENPROFILE); + OIDC_KEY_REDIRECTURIS, OIDC_KEY_SCOPE, AGENT_KEY_AUTHSCOPE, + AGENT_KEY_REFRESHSCOPE, OIDC_KEY_DEVICE_AUTHORIZATION_ENDPOINT, + OIDC_KEY_CLIENTNAME, AGENT_KEY_DAESETBYUSER, OIDC_KEY_AUDIENCE, + AGENT_KEY_OAUTH, AGENT_KEY_USESPUBCLIENT, + AGENT_KEY_MYTOKENPROFILE); GET_JSON_VALUES_RETURN_NULL_ONERROR(json); KEY_VALUE_VARS(issuer_url, issuer, mytoken_url, config_endpoint, shortname, client_id, client_secret, username, password, refresh_token, - cert_path, redirect_uris, scope, device_authorization_endpoint, - clientname, daeSetByUser, audience, oauth, pub, profile); + cert_path, redirect_uris, scope, auth_scope, refresh_scope, + device_authorization_endpoint, clientname, daeSetByUser, + audience, oauth, pub, profile); struct oidc_account* p = secAlloc(sizeof(struct oidc_account)); struct oidc_issuer* iss = secAlloc(sizeof(struct oidc_issuer)); if (_issuer_url) { @@ -174,7 +176,21 @@ struct oidc_account* getAccountFromJSON(const char* json) { account_setPassword(p, _password); account_setRefreshToken(p, _refresh_token); account_setCertPath(p, _cert_path); - account_setScopeExact(p, _scope); + if (strValid(_auth_scope)) { + // set auth scope + account_setAuthScopeExact(p, _auth_scope); + if (strValid(_refresh_scope)) { + // set refresh scope + account_setRefreshScope(p, _refresh_scope); + } else { + secFree(_refresh_scope); + } + secFree(_scope); + } else { + secFree(_refresh_scope); + secFree(_auth_scope); + account_setAuthScopeExact(p, _scope); + } account_setAudience(p, _audience); account_setUsedMytokenProfile(p, _profile); list_t* redirect_uris = JSONArrayStringToList(_redirect_uris); @@ -228,8 +244,10 @@ cJSON* _accountToJSON(const struct oidc_account* p, int useCredentials) { strValid(account_getRefreshToken(p)) ? account_getRefreshToken(p) : "", AGENT_KEY_CERTPATH, cJSON_String, strValid(account_getCertPath(p)) ? account_getCertPath(p) : "", - OIDC_KEY_SCOPE, cJSON_String, - strValid(account_getScope(p)) ? account_getScope(p) : "", + AGENT_KEY_AUTHSCOPE, cJSON_String, + strValid(account_getAuthScope(p)) ? account_getAuthScope(p) : "", + AGENT_KEY_REFRESHSCOPE, cJSON_String, + strValid(account_getRefreshScope(p)) ? account_getRefreshScope(p) : "", OIDC_KEY_AUDIENCE, cJSON_String, strValid(account_getAudience(p)) ? account_getAudience(p) : "", AGENT_KEY_OAUTH, cJSON_Number, account_getIsOAuth2(p), @@ -287,7 +305,8 @@ void secFreeAccountContent(struct oidc_account* p) { account_setIssuer(p, NULL); account_setClientId(p, NULL); account_setClientSecret(p, NULL); - account_setScopeExact(p, NULL); + account_setAuthScopeExact(p, NULL); + account_setRefreshScope(p, NULL); account_setAudience(p, NULL); account_setUsername(p, NULL); account_setPassword(p, NULL); @@ -337,7 +356,7 @@ int hasRedirectUris(const struct oidc_account* account) { } list_t* defineUsableScopeList(const struct oidc_account* account) { - char* wanted_str = account_getScope(account); + char* wanted_str = account_getAuthScope(account); list_t* wanted = delimitedStringToList(wanted_str, ' '); if (wanted == NULL) { wanted = createList(1, NULL); diff --git a/src/account/account.h b/src/account/account.h index bc8bbf81..cd547646 100644 --- a/src/account/account.h +++ b/src/account/account.h @@ -20,7 +20,8 @@ struct oidc_account { char* clientname; char* client_id; char* client_secret; - char* scope; + char* refresh_scope; + char* auth_scope; char* audience; char* used_mytoken_profile; char* username; diff --git a/src/account/setandget.c b/src/account/setandget.c index 4edb2f6f..18011939 100644 --- a/src/account/setandget.c +++ b/src/account/setandget.c @@ -76,8 +76,12 @@ char* account_getClientSecret(const struct oidc_account* p) { return p ? p->client_secret : NULL; } -char* account_getScope(const struct oidc_account* p) { - return p ? p->scope : NULL; +char* account_getAuthScope(const struct oidc_account* p) { + return p ? p->auth_scope : NULL; +} + +char* account_getRefreshScope(const struct oidc_account* p) { + return p ? p->refresh_scope : NULL; } char* account_getAudience(const struct oidc_account* p) { @@ -225,19 +229,27 @@ void account_setClientSecret(struct oidc_account* p, char* client_secret) { p->client_secret = client_secret; } -void account_setScopeExact(struct oidc_account* p, char* scope) { - if (p->scope == scope) { +void account_setRefreshScope(struct oidc_account* p, char* scope) { + if (p->refresh_scope == scope) { + return; + } + secFree(p->refresh_scope); + p->refresh_scope = scope; +} + +void account_setAuthScopeExact(struct oidc_account* p, char* scope) { + if (p->auth_scope == scope) { return; } - secFree(p->scope); - p->scope = scope; + secFree(p->auth_scope); + p->auth_scope = scope; } -void account_setScope(struct oidc_account* p, char* scope) { - account_setScopeExact(p, scope); +void account_setAuthScope(struct oidc_account* p, char* scope) { + account_setAuthScopeExact(p, scope); if (strValid(scope)) { char* usable = defineUsableScopes(p); - account_setScopeExact(p, usable); + account_setAuthScopeExact(p, usable); } } @@ -247,8 +259,8 @@ void account_setIssuer(struct oidc_account* p, struct oidc_issuer* issuer) { } secFreeIssuer(p->issuer); p->issuer = issuer; - if (issuer && strValid(account_getScope(p))) { - account_setScopeExact(p, defineUsableScopes(p)); + if (issuer && strValid(account_getAuthScope(p))) { + account_setAuthScopeExact(p, defineUsableScopes(p)); } } @@ -262,7 +274,7 @@ void account_setScopesSupported(struct oidc_account* p, } issuer_setScopesSupported(p->issuer, scopes_supported); char* usable = defineUsableScopes(p); - account_setScopeExact(p, usable); + account_setAuthScopeExact(p, usable); } void account_setAudience(struct oidc_account* p, char* audience) { diff --git a/src/account/setandget.h b/src/account/setandget.h index 891ff604..3e49f93b 100644 --- a/src/account/setandget.h +++ b/src/account/setandget.h @@ -20,7 +20,8 @@ char* account_getName(const struct oidc_account* p); char* account_getClientName(const struct oidc_account* p); char* account_getClientId(const struct oidc_account* p); char* account_getClientSecret(const struct oidc_account* p); -char* account_getScope(const struct oidc_account* p); +char* account_getRefreshScope(const struct oidc_account* p); +char* account_getAuthScope(const struct oidc_account* p); char* account_getAudience(const struct oidc_account* p); char* account_getUsedMytokenProfile(const struct oidc_account* p); char* account_getUsername(const struct oidc_account* p); @@ -50,8 +51,9 @@ void account_setName(struct oidc_account* p, char* shortname, const char* client_identifier); void account_setClientId(struct oidc_account* p, char* client_id); void account_setClientSecret(struct oidc_account* p, char* client_secret); -void account_setScopeExact(struct oidc_account* p, char* scope); -void account_setScope(struct oidc_account* p, char* scope); +void account_setAuthScopeExact(struct oidc_account* p, char* scope); +void account_setAuthScope(struct oidc_account* p, char* scope); +void account_setRefreshScope(struct oidc_account* p, char* scope); void account_setIssuer(struct oidc_account* p, struct oidc_issuer* issuer); void account_setScopesSupported(struct oidc_account* p, char* scopes_supported); void account_setAudience(struct oidc_account* p, char* audience); diff --git a/src/defines/agent_values.h b/src/defines/agent_values.h index 7245a15c..ab1da22c 100644 --- a/src/defines/agent_values.h +++ b/src/defines/agent_values.h @@ -2,6 +2,8 @@ #define AGENT_MAGIC_VALUES_H #define AGENT_SCOPE_ALL "max" +#define AGENT_KEY_AUTHSCOPE "auth_scope" +#define AGENT_KEY_REFRESHSCOPE "refresh_scope" #define AGENT_KEY_ISSUERURL "issuer_url" #define AGENT_KEY_DAESETBYUSER "daeSetByUser" #define AGENT_KEY_CONFIG_ENDPOINT "config_endpoint" diff --git a/src/oidc-agent/oidc/flows/code.c b/src/oidc-agent/oidc/flows/code.c index 0afa0880..87736e2d 100644 --- a/src/oidc-agent/oidc/flows/code.c +++ b/src/oidc-agent/oidc/flows/code.c @@ -109,9 +109,10 @@ char* buildCodeFlowUri(const struct oidc_account* account, char** state_ptr, secFree(*state_ptr); *state_ptr = tmp; } - char* scope = only_at ? removeScope(oidc_strcopy(account_getScope(account)), - OIDC_SCOPE_OFFLINE_ACCESS) - : oidc_strcopy(account_getScope(account)); + char* scope = only_at + ? removeScope(oidc_strcopy(account_getAuthScope(account)), + OIDC_SCOPE_OFFLINE_ACCESS) + : oidc_strcopy(account_getAuthScope(account)); list_t* postData = createList( LIST_CREATE_DONT_COPY_VALUES, OIDC_KEY_RESPONSETYPE, OIDC_RESPONSETYPE_CODE, OIDC_KEY_CLIENTID, account_getClientId(account), diff --git a/src/oidc-agent/oidc/flows/device.c b/src/oidc-agent/oidc/flows/device.c index debca0a9..3c862c74 100644 --- a/src/oidc-agent/oidc/flows/device.c +++ b/src/oidc-agent/oidc/flows/device.c @@ -13,7 +13,7 @@ char* generateDeviceCodePostData(const struct oidc_account* a) { return generatePostData(OIDC_KEY_CLIENTID, account_getClientId(a), - OIDC_KEY_SCOPE, account_getScope(a), NULL); + OIDC_KEY_SCOPE, account_getAuthScope(a), NULL); } char* generateDeviceCodeLookupPostData(const struct oidc_account* a, diff --git a/src/oidc-agent/oidc/flows/oidc.c b/src/oidc-agent/oidc/flows/oidc.c index d7ed48e8..e932007b 100644 --- a/src/oidc-agent/oidc/flows/oidc.c +++ b/src/oidc-agent/oidc/flows/oidc.c @@ -109,7 +109,9 @@ char* parseTokenResponseCallbacks( // if we get a scope value back from the OP when the initial AT is obtained, // we update the config, because it might be possible that the OP made // changes to the scopes. - account_setScopeExact(a, _scope); + // We use this updated scope value in future refresh request. For a + // reauthenticate we will use the initial authorization scopes. + account_setRefreshScope(a, _scope); } else { secFree(_scope); } diff --git a/src/oidc-agent/oidc/flows/password.c b/src/oidc-agent/oidc/flows/password.c index 56c17ddc..62302350 100644 --- a/src/oidc-agent/oidc/flows/password.c +++ b/src/oidc-agent/oidc/flows/password.c @@ -24,10 +24,10 @@ char* generatePasswordPostData(const struct oidc_account* a, list_rpush(postDataList, list_node_new(account_getUsername(a))); list_rpush(postDataList, list_node_new(OIDC_KEY_PASSWORD)); list_rpush(postDataList, list_node_new(account_getPassword(a))); - if (scope || strValid(account_getScope(a))) { + if (scope || strValid(account_getAuthScope(a))) { list_rpush(postDataList, list_node_new(OIDC_KEY_SCOPE)); list_rpush(postDataList, - list_node_new((char*)scope ?: account_getScope(a))); + list_node_new((char*)scope ?: account_getAuthScope(a))); } char* aud_tmp = NULL; if (strValid(account_getAudience(a))) { diff --git a/src/oidc-agent/oidc/flows/refresh.c b/src/oidc-agent/oidc/flows/refresh.c index 15e723de..de720189 100644 --- a/src/oidc-agent/oidc/flows/refresh.c +++ b/src/oidc-agent/oidc/flows/refresh.c @@ -13,13 +13,13 @@ char* generateRefreshPostData(const struct oidc_account* a, const char* scope, const char* audience) { char* refresh_token = account_getRefreshToken(a); - char* scope_tmp = oidc_strcopy( - strValid(scope) ? scope - : account_getScope( - a)); // if scopes are explicitly set use these, if - // not we use the same as for the used refresh - // token. Usually this parameter can be - // omitted. For unity we have to include this. + if (NULL == scope) { + scope = strValid(account_getRefreshScope(a)) ? account_getRefreshScope(a) + : account_getAuthScope(a); + // if scopes are explicitly set use these, if not we use the same as for the + // used refresh token. Usually this parameter can be omitted. For unity we + // have to include this. + } list_t* postDataList = list_new(); // list_rpush(postDataList, list_node_new(OIDC_KEY_CLIENTID)); // list_rpush(postDataList, list_node_new(account_getClientId(a))); @@ -42,9 +42,9 @@ char* generateRefreshPostData(const struct oidc_account* a, const char* scope, } } - if (strValid(scope_tmp)) { + if (strValid(scope)) { list_rpush(postDataList, list_node_new(OIDC_KEY_SCOPE)); - list_rpush(postDataList, list_node_new(scope_tmp)); + list_rpush(postDataList, list_node_new((void*)scope)); } char* aud_tmp = NULL; if (strValid(audience)) { @@ -59,7 +59,6 @@ char* generateRefreshPostData(const struct oidc_account* a, const char* scope, } char* str = generatePostDataFromList(postDataList); list_destroy(postDataList); - secFree(scope_tmp); secFree(aud_tmp); return str; } diff --git a/src/oidc-agent/oidc/flows/registration.c b/src/oidc-agent/oidc/flows/registration.c index c1f3294d..7d4b9679 100644 --- a/src/oidc-agent/oidc/flows/registration.c +++ b/src/oidc-agent/oidc/flows/registration.c @@ -42,7 +42,7 @@ char* getRegistrationPostData(const struct oidc_account* account, list_t* flows, OIDC_KEY_APPLICATIONTYPE, cJSON_String, application_type, OIDC_KEY_CLIENTNAME, cJSON_String, client_name, OIDC_KEY_RESPONSETYPES, cJSON_Array, response_types, OIDC_KEY_GRANTTYPES, cJSON_Array, - grant_types, OIDC_KEY_SCOPE, cJSON_String, account_getScope(account), + grant_types, OIDC_KEY_SCOPE, cJSON_String, account_getAuthScope(account), OIDC_KEY_REDIRECTURIS, cJSON_Array, redirect_uris_json, NULL); secFree(response_types); secFree(grant_types); diff --git a/src/oidc-agent/oidcd/oidcd_handler.c b/src/oidc-agent/oidcd/oidcd_handler.c index e4e98ed7..945b0a6a 100644 --- a/src/oidc-agent/oidcd/oidcd_handler.c +++ b/src/oidc-agent/oidcd/oidcd_handler.c @@ -154,7 +154,7 @@ void _handleGenFlows(struct ipcPipe pipes, struct oidc_account* account, } else if (strcaseequal(current_flow->val, FLOW_VALUE_DEVICE) || strcaseequal(current_flow->val, FLOW_VALUE_MT_OIDC)) { if (scope) { - account_setScopeExact(account, oidc_strcopy(scope)); + account_setAuthScopeExact(account, oidc_strcopy(scope)); } struct oidc_device_code* dc = strcaseequal(current_flow->val, FLOW_VALUE_MT_OIDC) @@ -248,9 +248,10 @@ void oidcd_handleGen(struct ipcPipe pipes, const char* account_json, } const int only_at = strToInt(only_at_str); - char* scope = only_at ? removeScope(oidc_strcopy(account_getScope(account)), - OIDC_SCOPE_OFFLINE_ACCESS) - : NULL; + char* scope = only_at + ? removeScope(oidc_strcopy(account_getAuthScope(account)), + OIDC_SCOPE_OFFLINE_ACCESS) + : NULL; _handleGenFlows(pipes, account, flow, scope, only_at, nowebserver_str, noscheme_str, arguments); diff --git a/src/oidc-agent/oidcp/oidcp.c b/src/oidc-agent/oidcp/oidcp.c index 0a6d4541..ef024d0c 100644 --- a/src/oidc-agent/oidcp/oidcp.c +++ b/src/oidc-agent/oidcp/oidcp.c @@ -556,14 +556,14 @@ void handleAutoGen(struct ipcPipe pipes, int sock, } if (strequal(scopes, AGENT_SCOPE_ALL)) { if (account_getMytokenUrl(account)) { - account_setScopeExact(account, oidc_strcopy(scopes)); + account_setAuthScopeExact(account, oidc_strcopy(scopes)); } else { - account_setScope(account, usedUserClient - ? getScopesForUserClient(account) - : getScopesForPublicClient(account)); + account_setAuthScope(account, usedUserClient + ? getScopesForUserClient(account) + : getScopesForPublicClient(account)); } } else { - account_setScope(account, oidc_strcopy(scopes)); + account_setAuthScope(account, oidc_strcopy(scopes)); } agent_log(DEBUG, "Prompting user for confirmation for autogen for '%s'", diff --git a/src/oidc-gen/gen_handler.c b/src/oidc-gen/gen_handler.c index 9b9c59ab..94c3017e 100644 --- a/src/oidc-gen/gen_handler.c +++ b/src/oidc-gen/gen_handler.c @@ -779,7 +779,7 @@ struct oidc_account* registerClient(struct arguments* arguments) { secFreeJson(merged_json); exit(EXIT_FAILURE); } - const char* requested_scope = account_getScope(account); + const char* requested_scope = account_getAuthScope(account); if (strequal(requested_scope, "max") && _max_scopes) { requested_scope = _max_scopes; } diff --git a/src/oidc-gen/promptAndSet/scope.c b/src/oidc-gen/promptAndSet/scope.c index 85bd765c..591640ff 100644 --- a/src/oidc-gen/promptAndSet/scope.c +++ b/src/oidc-gen/promptAndSet/scope.c @@ -40,23 +40,24 @@ void askOrNeedScope(struct oidc_account* account, void _askOrNeedScope(char* supportedScope, struct oidc_account* account, const struct arguments* arguments, int optional) { if (readScope(account, arguments)) { - if (strequal(account_getScope(account), AGENT_SCOPE_ALL)) { - account_setScope(account, supportedScope); + if (strequal(account_getAuthScope(account), AGENT_SCOPE_ALL)) { + account_setAuthScope(account, supportedScope); } return; } ERROR_IF_NO_PROMPT(optional, ERROR_MESSAGE("scope", OPT_LONG_SCOPE)); printNormal("The following scopes are supported: %s\n", supportedScope); - if (!strValid(account_getScope(account))) { - account_setScope(account, oidc_strcopy(DEFAULT_SCOPE)); + if (!strValid(account_getAuthScope(account))) { + account_setAuthScope(account, oidc_strcopy(DEFAULT_SCOPE)); } char* res = _gen_promptMultipleSpaceSeparated( - "Scopes or '" AGENT_SCOPE_ALL "'", account_getScope(account), optional); + "Scopes or '" AGENT_SCOPE_ALL "'", account_getAuthScope(account), + optional); if (res) { - account_setScopeExact(account, res); + account_setAuthScopeExact(account, res); } - if (strequal(account_getScope(account), AGENT_SCOPE_ALL)) { - account_setScope(account, supportedScope); + if (strequal(account_getAuthScope(account), AGENT_SCOPE_ALL)) { + account_setAuthScope(account, supportedScope); } else { secFree(supportedScope); } @@ -64,14 +65,14 @@ void _askOrNeedScope(char* supportedScope, struct oidc_account* account, int readScope(struct oidc_account* account, const struct arguments* arguments) { if (arguments->scope) { - void (*setter)(struct oidc_account*, char*) = account_setScope; + void (*setter)(struct oidc_account*, char*) = account_setAuthScope; if (strequal(arguments->scope, AGENT_SCOPE_ALL)) { - setter = account_setScopeExact; + setter = account_setAuthScopeExact; } setter(account, oidc_strcopy(arguments->scope)); return 1; } - if (prompt_mode() == 0 && strValid(account_getScope(account))) { + if (prompt_mode() == 0 && strValid(account_getAuthScope(account))) { return 1; } return 0; diff --git a/test/src/account/account/tc_defineUsableScopes.c b/test/src/account/account/tc_defineUsableScopes.c index 1dc41110..d194e60f 100644 --- a/test/src/account/account/tc_defineUsableScopes.c +++ b/test/src/account/account/tc_defineUsableScopes.c @@ -10,7 +10,7 @@ extern void _printList(list_t* l); START_TEST(test_null) { struct oidc_account account = {}; - account_setScope(&account, NULL); + account_setAuthScope(&account, NULL); list_t* list = defineUsableScopeList(&account); // _printList(list); ck_assert_ptr_ne(list, NULL); @@ -21,7 +21,7 @@ END_TEST START_TEST(test_nullGoogle) { struct oidc_account account = {}; - account_setScope(&account, NULL); + account_setAuthScope(&account, NULL); account_setIssuerUrl(&account, GOOGLE_ISSUER_URL); list_t* list = defineUsableScopeList(&account); // _printList(list); @@ -33,8 +33,8 @@ END_TEST START_TEST(test_valid) { struct oidc_account account = {}; - account_setScope(&account, - oidc_strcopy("profile email offline_access handy")); + account_setAuthScope(&account, + oidc_strcopy("profile email offline_access handy")); list_t* list = defineUsableScopeList(&account); // _printList(list); ck_assert_ptr_ne(list, NULL);