From 6a74fa11c687c4754bacf400a445bc5cee325c09 Mon Sep 17 00:00:00 2001 From: Jens L Date: Thu, 13 Apr 2023 15:06:28 +0200 Subject: [PATCH] providers/oauth2: inconsistent client secret generation (#5241) * use simpler char set for client secret Signed-off-by: Jens Langhammer * also adjust radius Signed-off-by: Jens Langhammer * use similar logic in web to generate ids and secrets Signed-off-by: Jens Langhammer * dont use math.random Signed-off-by: Jens Langhammer --------- Signed-off-by: Jens Langhammer --- ...me_authorizationcode_auth_time_and_more.py | 12 ++++++++ authentik/providers/oauth2/models.py | 7 ++++- authentik/providers/oauth2/tests/test_api.py | 3 -- .../providers/oauth2/tests/test_authorize.py | 6 +--- .../providers/oauth2/tests/test_introspect.py | 4 +-- .../providers/oauth2/tests/test_revoke.py | 4 +-- .../providers/oauth2/tests/test_token.py | 14 --------- .../providers/oauth2/tests/test_token_cc.py | 3 -- .../oauth2/tests/test_token_cc_jwt_source.py | 6 ++-- .../oauth2/tests/test_token_device.py | 4 +-- .../providers/oauth2/tests/test_userinfo.py | 4 +-- .../radius/migrations/0001_initial.py | 2 +- authentik/providers/radius/models.py | 4 +-- .../providers/oauth2/OAuth2ProviderForm.ts | 12 ++++++-- .../providers/radius/RadiusProviderForm.ts | 29 ++++++++++--------- web/src/admin/sources/plex/PlexSourceForm.ts | 4 +-- web/src/common/utils.ts | 21 +++++++++++--- web/src/elements/forms/SearchSelect.ts | 4 +-- 18 files changed, 74 insertions(+), 69 deletions(-) diff --git a/authentik/providers/oauth2/migrations/0015_accesstoken_auth_time_authorizationcode_auth_time_and_more.py b/authentik/providers/oauth2/migrations/0015_accesstoken_auth_time_authorizationcode_auth_time_and_more.py index 7b84077a1..32aed86aa 100644 --- a/authentik/providers/oauth2/migrations/0015_accesstoken_auth_time_authorizationcode_auth_time_and_more.py +++ b/authentik/providers/oauth2/migrations/0015_accesstoken_auth_time_authorizationcode_auth_time_and_more.py @@ -3,6 +3,8 @@ import django.utils.timezone from django.db import migrations, models +import authentik.providers.oauth2.models + class Migration(migrations.Migration): dependencies = [ @@ -37,4 +39,14 @@ class Migration(migrations.Migration): ), preserve_default=False, ), + migrations.AlterField( + model_name="oauth2provider", + name="client_secret", + field=models.CharField( + blank=True, + default=authentik.providers.oauth2.models.generate_client_secret, + max_length=255, + verbose_name="Client Secret", + ), + ), ] diff --git a/authentik/providers/oauth2/models.py b/authentik/providers/oauth2/models.py index c82f20c80..a17216270 100644 --- a/authentik/providers/oauth2/models.py +++ b/authentik/providers/oauth2/models.py @@ -27,6 +27,11 @@ from authentik.providers.oauth2.id_token import IDToken, SubModes from authentik.sources.oauth.models import OAuthSource +def generate_client_secret() -> str: + """Generate client secret with adequate length""" + return generate_id(128) + + class ClientTypes(models.TextChoices): """Confidential clients are capable of maintaining the confidentiality of their credentials. Public clients are incapable.""" @@ -132,7 +137,7 @@ class OAuth2Provider(Provider): max_length=255, blank=True, verbose_name=_("Client Secret"), - default=generate_key, + default=generate_client_secret, ) redirect_uris = models.TextField( default="", diff --git a/authentik/providers/oauth2/tests/test_api.py b/authentik/providers/oauth2/tests/test_api.py index 4f93c902e..2bd0c5c98 100644 --- a/authentik/providers/oauth2/tests/test_api.py +++ b/authentik/providers/oauth2/tests/test_api.py @@ -7,7 +7,6 @@ from rest_framework.test import APITestCase from authentik.blueprints.tests import apply_blueprint from authentik.core.models import Application from authentik.core.tests.utils import create_test_admin_user, create_test_flow -from authentik.lib.generators import generate_id, generate_key from authentik.providers.oauth2.models import OAuth2Provider, ScopeMapping @@ -18,8 +17,6 @@ class TestAPI(APITestCase): def setUp(self) -> None: self.provider: OAuth2Provider = OAuth2Provider.objects.create( name="test", - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://testserver", ) diff --git a/authentik/providers/oauth2/tests/test_authorize.py b/authentik/providers/oauth2/tests/test_authorize.py index 76688fdda..91cdc330a 100644 --- a/authentik/providers/oauth2/tests/test_authorize.py +++ b/authentik/providers/oauth2/tests/test_authorize.py @@ -9,7 +9,7 @@ from authentik.core.models import Application from authentik.core.tests.utils import create_test_admin_user, create_test_flow from authentik.events.models import Event, EventAction from authentik.flows.challenge import ChallengeTypes -from authentik.lib.generators import generate_id, generate_key +from authentik.lib.generators import generate_id from authentik.lib.utils.time import timedelta_from_string from authentik.providers.oauth2.constants import TOKEN_TYPE from authentik.providers.oauth2.errors import AuthorizeError, ClientIdError, RedirectUriError @@ -298,7 +298,6 @@ class TestAuthorize(OAuthTestCase): provider: OAuth2Provider = OAuth2Provider.objects.create( name=generate_id(), client_id="test", - client_secret=generate_key(), authorization_flow=flow, redirect_uris="http://localhost", signing_key=self.keypair, @@ -361,7 +360,6 @@ class TestAuthorize(OAuthTestCase): provider: OAuth2Provider = OAuth2Provider.objects.create( name=generate_id(), client_id="test", - client_secret=generate_key(), authorization_flow=flow, redirect_uris="http://localhost", signing_key=self.keypair, @@ -417,7 +415,6 @@ class TestAuthorize(OAuthTestCase): provider = OAuth2Provider.objects.create( name=generate_id(), client_id=generate_id(), - client_secret=generate_key(), authorization_flow=flow, redirect_uris="http://localhost", signing_key=self.keypair, @@ -467,7 +464,6 @@ class TestAuthorize(OAuthTestCase): provider = OAuth2Provider.objects.create( name=generate_id(), client_id=generate_id(), - client_secret=generate_key(), authorization_flow=flow, redirect_uris="http://localhost", signing_key=self.keypair, diff --git a/authentik/providers/oauth2/tests/test_introspect.py b/authentik/providers/oauth2/tests/test_introspect.py index a55a52825..dd35d5f6f 100644 --- a/authentik/providers/oauth2/tests/test_introspect.py +++ b/authentik/providers/oauth2/tests/test_introspect.py @@ -8,7 +8,7 @@ from django.utils import timezone from authentik.core.models import Application from authentik.core.tests.utils import create_test_admin_user, create_test_cert, create_test_flow -from authentik.lib.generators import generate_id, generate_key +from authentik.lib.generators import generate_id from authentik.providers.oauth2.constants import ACR_AUTHENTIK_DEFAULT from authentik.providers.oauth2.models import AccessToken, IDToken, OAuth2Provider, RefreshToken from authentik.providers.oauth2.tests.utils import OAuthTestCase @@ -21,8 +21,6 @@ class TesOAuth2Introspection(OAuthTestCase): super().setUp() self.provider: OAuth2Provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="", signing_key=create_test_cert(), diff --git a/authentik/providers/oauth2/tests/test_revoke.py b/authentik/providers/oauth2/tests/test_revoke.py index 43c5de7c5..74acbec02 100644 --- a/authentik/providers/oauth2/tests/test_revoke.py +++ b/authentik/providers/oauth2/tests/test_revoke.py @@ -8,7 +8,7 @@ from django.utils import timezone from authentik.core.models import Application from authentik.core.tests.utils import create_test_admin_user, create_test_cert, create_test_flow -from authentik.lib.generators import generate_id, generate_key +from authentik.lib.generators import generate_id from authentik.providers.oauth2.models import AccessToken, IDToken, OAuth2Provider, RefreshToken from authentik.providers.oauth2.tests.utils import OAuthTestCase @@ -20,8 +20,6 @@ class TesOAuth2Revoke(OAuthTestCase): super().setUp() self.provider: OAuth2Provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="", signing_key=create_test_cert(), diff --git a/authentik/providers/oauth2/tests/test_token.py b/authentik/providers/oauth2/tests/test_token.py index acea27f64..79b3b13fe 100644 --- a/authentik/providers/oauth2/tests/test_token.py +++ b/authentik/providers/oauth2/tests/test_token.py @@ -38,8 +38,6 @@ class TestToken(OAuthTestCase): """test request param""" provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://TestServer", signing_key=self.keypair, @@ -67,8 +65,6 @@ class TestToken(OAuthTestCase): """test request param""" provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://testserver", signing_key=self.keypair, @@ -90,8 +86,6 @@ class TestToken(OAuthTestCase): """test request param""" provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://local.invalid", signing_key=self.keypair, @@ -120,8 +114,6 @@ class TestToken(OAuthTestCase): """test request param""" provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://local.invalid", signing_key=self.keypair, @@ -163,8 +155,6 @@ class TestToken(OAuthTestCase): """test request param""" provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://local.invalid", signing_key=self.keypair, @@ -215,8 +205,6 @@ class TestToken(OAuthTestCase): """test request param""" provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://local.invalid", signing_key=self.keypair, @@ -263,8 +251,6 @@ class TestToken(OAuthTestCase): """test request param""" provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://testserver", signing_key=self.keypair, diff --git a/authentik/providers/oauth2/tests/test_token_cc.py b/authentik/providers/oauth2/tests/test_token_cc.py index a578c3be9..e9213e211 100644 --- a/authentik/providers/oauth2/tests/test_token_cc.py +++ b/authentik/providers/oauth2/tests/test_token_cc.py @@ -8,7 +8,6 @@ from jwt import decode from authentik.blueprints.tests import apply_blueprint from authentik.core.models import USER_ATTRIBUTE_SA, Application, Group, Token, TokenIntents from authentik.core.tests.utils import create_test_admin_user, create_test_cert, create_test_flow -from authentik.lib.generators import generate_id, generate_key from authentik.policies.models import PolicyBinding from authentik.providers.oauth2.constants import ( GRANT_TYPE_CLIENT_CREDENTIALS, @@ -31,8 +30,6 @@ class TestTokenClientCredentials(OAuthTestCase): self.factory = RequestFactory() self.provider = OAuth2Provider.objects.create( name="test", - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://testserver", signing_key=create_test_cert(), diff --git a/authentik/providers/oauth2/tests/test_token_cc_jwt_source.py b/authentik/providers/oauth2/tests/test_token_cc_jwt_source.py index 3daf55e6e..a95c7c3a5 100644 --- a/authentik/providers/oauth2/tests/test_token_cc_jwt_source.py +++ b/authentik/providers/oauth2/tests/test_token_cc_jwt_source.py @@ -9,7 +9,7 @@ from jwt import decode from authentik.blueprints.tests import apply_blueprint from authentik.core.models import Application, Group from authentik.core.tests.utils import create_test_cert, create_test_flow -from authentik.lib.generators import generate_id, generate_key +from authentik.lib.generators import generate_id from authentik.policies.models import PolicyBinding from authentik.providers.oauth2.constants import ( GRANT_TYPE_CLIENT_CREDENTIALS, @@ -39,7 +39,7 @@ class TestTokenClientCredentialsJWTSource(OAuthTestCase): slug=generate_id(), provider_type="openidconnect", consumer_key=generate_id(), - consumer_secret=generate_key(), + consumer_secret=generate_id(), authorization_url="http://foo", access_token_url=f"http://{generate_id()}", profile_url="http://foo", @@ -52,8 +52,6 @@ class TestTokenClientCredentialsJWTSource(OAuthTestCase): self.provider: OAuth2Provider = OAuth2Provider.objects.create( name="test", - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://testserver", signing_key=self.cert, diff --git a/authentik/providers/oauth2/tests/test_token_device.py b/authentik/providers/oauth2/tests/test_token_device.py index a9536e0ab..0d7474f92 100644 --- a/authentik/providers/oauth2/tests/test_token_device.py +++ b/authentik/providers/oauth2/tests/test_token_device.py @@ -7,7 +7,7 @@ from django.urls import reverse from authentik.blueprints.tests import apply_blueprint from authentik.core.models import Application from authentik.core.tests.utils import create_test_admin_user, create_test_cert, create_test_flow -from authentik.lib.generators import generate_code_fixed_length, generate_id, generate_key +from authentik.lib.generators import generate_code_fixed_length, generate_id from authentik.providers.oauth2.constants import GRANT_TYPE_DEVICE_CODE from authentik.providers.oauth2.models import DeviceToken, OAuth2Provider, ScopeMapping from authentik.providers.oauth2.tests.utils import OAuthTestCase @@ -22,8 +22,6 @@ class TestTokenDeviceCode(OAuthTestCase): self.factory = RequestFactory() self.provider = OAuth2Provider.objects.create( name="test", - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="http://testserver", signing_key=create_test_cert(), diff --git a/authentik/providers/oauth2/tests/test_userinfo.py b/authentik/providers/oauth2/tests/test_userinfo.py index 576dbca52..451c86fa3 100644 --- a/authentik/providers/oauth2/tests/test_userinfo.py +++ b/authentik/providers/oauth2/tests/test_userinfo.py @@ -9,7 +9,7 @@ from authentik.blueprints.tests import apply_blueprint from authentik.core.models import Application from authentik.core.tests.utils import create_test_admin_user, create_test_cert, create_test_flow from authentik.events.models import Event, EventAction -from authentik.lib.generators import generate_id, generate_key +from authentik.lib.generators import generate_id from authentik.providers.oauth2.models import AccessToken, IDToken, OAuth2Provider, ScopeMapping from authentik.providers.oauth2.tests.utils import OAuthTestCase @@ -23,8 +23,6 @@ class TestUserinfo(OAuthTestCase): self.app = Application.objects.create(name=generate_id(), slug=generate_id()) self.provider: OAuth2Provider = OAuth2Provider.objects.create( name=generate_id(), - client_id=generate_id(), - client_secret=generate_key(), authorization_flow=create_test_flow(), redirect_uris="", signing_key=create_test_cert(), diff --git a/authentik/providers/radius/migrations/0001_initial.py b/authentik/providers/radius/migrations/0001_initial.py index caf504b8f..d1cf7fd43 100644 --- a/authentik/providers/radius/migrations/0001_initial.py +++ b/authentik/providers/radius/migrations/0001_initial.py @@ -31,7 +31,7 @@ class Migration(migrations.Migration): ( "shared_secret", models.TextField( - default=authentik.lib.generators.generate_key, + default=authentik.lib.generators.generate_id, help_text="Shared secret between clients and server to hash packets.", ), ), diff --git a/authentik/providers/radius/models.py b/authentik/providers/radius/models.py index 1fb146d05..7ba39b47e 100644 --- a/authentik/providers/radius/models.py +++ b/authentik/providers/radius/models.py @@ -6,7 +6,7 @@ from django.utils.translation import gettext_lazy as _ from rest_framework.serializers import Serializer from authentik.core.models import Provider -from authentik.lib.generators import generate_key +from authentik.lib.generators import generate_id from authentik.outposts.models import OutpostModel @@ -14,7 +14,7 @@ class RadiusProvider(OutpostModel, Provider): """Allow applications to authenticate against authentik's users using Radius.""" shared_secret = models.TextField( - default=generate_key, + default=generate_id, help_text=_("Shared secret between clients and server to hash packets."), ) diff --git a/web/src/admin/providers/oauth2/OAuth2ProviderForm.ts b/web/src/admin/providers/oauth2/OAuth2ProviderForm.ts index f8a9a99c0..b925aa0ec 100644 --- a/web/src/admin/providers/oauth2/OAuth2ProviderForm.ts +++ b/web/src/admin/providers/oauth2/OAuth2ProviderForm.ts @@ -1,6 +1,6 @@ import { RenderFlowOption } from "@goauthentik/admin/flows/utils"; import { DEFAULT_CONFIG } from "@goauthentik/common/api/config"; -import { first, randomString } from "@goauthentik/common/utils"; +import { ascii_letters, digits, first, randomString } from "@goauthentik/common/utils"; import "@goauthentik/elements/forms/FormGroup"; import "@goauthentik/elements/forms/HorizontalFormElement"; import { ModelForm } from "@goauthentik/elements/forms/ModelForm"; @@ -203,7 +203,10 @@ export class OAuth2ProviderFormPage extends ModelForm { > @@ -215,7 +218,10 @@ export class OAuth2ProviderFormPage extends ModelForm { > diff --git a/web/src/admin/providers/radius/RadiusProviderForm.ts b/web/src/admin/providers/radius/RadiusProviderForm.ts index a8566f976..892f03ba5 100644 --- a/web/src/admin/providers/radius/RadiusProviderForm.ts +++ b/web/src/admin/providers/radius/RadiusProviderForm.ts @@ -1,6 +1,6 @@ import { RenderFlowOption } from "@goauthentik/admin/flows/utils"; import { DEFAULT_CONFIG } from "@goauthentik/common/api/config"; -import { first, randomString } from "@goauthentik/common/utils"; +import { ascii_letters, digits, first, randomString } from "@goauthentik/common/utils"; import { rootInterface } from "@goauthentik/elements/Base"; import "@goauthentik/elements/forms/FormGroup"; import "@goauthentik/elements/forms/HorizontalFormElement"; @@ -98,22 +98,25 @@ export class RadiusProviderFormPage extends ModelForm {

${t`Flow used for users to authenticate.`}

- - - ${t`Protocol settings`}
+ + + { get defaultInstance(): PlexSource | undefined { return { - clientId: randomString(40), + clientId: randomString(40, ascii_letters + digits), } as PlexSource; } diff --git a/web/src/common/utils.ts b/web/src/common/utils.ts index c016e7fd6..d1e59e4a9 100644 --- a/web/src/common/utils.ts +++ b/web/src/common/utils.ts @@ -83,10 +83,23 @@ export function hexEncode(buf: Uint8Array): string { .join(""); } -export function randomString(len: number): string { - const arr = new Uint8Array(len / 2); - window.crypto.getRandomValues(arr); - return hexEncode(arr); +// Taken from python's string module +export const ascii_lowercase = "abcdefghijklmnopqrstuvwxyz"; +export const ascii_uppercase = "ABCDEFGHIJKLMNOPQRSTUVWXYZ"; +export const ascii_letters = ascii_lowercase + ascii_uppercase; +export const digits = "0123456789"; +export const hexdigits = digits + "abcdef" + "ABCDEF"; +export const octdigits = "01234567"; +export const punctuation = "!\"#$%&'()*+,-./:;<=>?@[\\]^_`{|}~"; + +export function randomString(len: number, charset: string): string { + const chars = []; + const array = new Uint8Array(len); + self.crypto.getRandomValues(array); + for (let index = 0; index < len; index++) { + chars.push(charset[Math.floor(charset.length * (array[index] / Math.pow(2, 8)))]); + } + return chars.join(""); } export function dateTimeLocal(date: Date): string { diff --git a/web/src/elements/forms/SearchSelect.ts b/web/src/elements/forms/SearchSelect.ts index 27760b954..a3ed9a7f5 100644 --- a/web/src/elements/forms/SearchSelect.ts +++ b/web/src/elements/forms/SearchSelect.ts @@ -1,5 +1,5 @@ import { EVENT_REFRESH } from "@goauthentik/common/constants"; -import { groupBy, randomString } from "@goauthentik/common/utils"; +import { ascii_letters, digits, groupBy, randomString } from "@goauthentik/common/utils"; import { AKElement } from "@goauthentik/elements/Base"; import { PreventFormSubmit } from "@goauthentik/elements/forms/Form"; @@ -89,7 +89,7 @@ export class SearchSelect extends AKElement { }); }); this.observer.observe(this); - this.dropdownUID = `dropdown-${randomString(10)}`; + this.dropdownUID = `dropdown-${randomString(10, ascii_letters + digits)}`; } toForm(): unknown {