# Неожиданности в авторизации\. Что скрывает Keycloak? Авторизация и регистрация пользователей — важная часть приложения как для юзеров, так и для безопасности\. Но какие неожиданности содержит исходный код популярного Open Source решения для авторизации, и как они могут повлиять на работу приложения? ![1142_Keycloak_checking_ru/image1.png](https://import.viva64.com/docx/blog/1142_Keycloak_checking_ru/image1.png) Все, кто когда\-нибудь реализовывал авторизацию для веб\-приложений, наверняка смогут вам рассказать множество историй о том, какие проблемные ситуации она создавала в процессе\. И я не исключение\. Однажды в проекте мне нужно было реализовать авторизацию через мессенджер во Frontend\-части приложения\. Задача казалась самой простой на Земле, но в итоге получилось наоборот: сроки горят, код спотыкается об API мессенджера, а люди вокруг начинают кричать\. После этого случая товарищ показал мне хороший инструмент для того, чтобы в будущем мы не испытывали проблем с авторизацией в наших проектах\. Этим проектом был Keycloak — Open Source решение для реализации единого входа \(Single Sign\-On, SSO\) и управления доступом к современным приложениям и сервисам, написанный на языке Java\. От того, что я сам пользуюсь этим решением, мне стало ещё интереснее залезть в его исходный код и с помощью статического анализатора PVS\-Studio посмотреть на жучков, которые там спрятались\. ## Классический NullPointerException > В дверь постучали\\\. Штирлиц попытался открыть дверь и сломался\\\. "NullPointerException\\\!" — догадался Мюллер\\\. Ошибки, связанные с проверкой на _null_, это тип ошибок, встречающийся почти в каждом проекте, который мы проверяли ранее\. Получается, начнём с классики\. **Фрагмент 1** ```cpp private void checkRevocationUsingOCSP(X509Certificate[] certs) throws GeneralSecurityException { .... if (rs == null) { if (_ocspFailOpen) logger.warnf(....); else throw new GeneralSecurityException(....); } if (rs.getRevocationStatus() == OCSPProvider.RevocationStatus.UNKNOWN) { // <= if (_ocspFailOpen) logger.warnf(....); else throw new GeneralSecurityException(....); .... } ``` Предупреждение: [V6008](https://pvs-studio.ru/ru/docs/warnings/v6008/) Potential null dereference of 'rs'\. В этом фрагменте есть проверка на _null_, но важно то, что в ней происходит\. При истинности переменной _\_oscpFailOpen_ программа не выбросит исключения, а лишь сохранит лог о произошедшем и продолжит выполнение, получив _NullPointerException_ уже в следующем _if_, поскольку в нём переменная _rs_ уже используется\. Может показаться, что если бы не _NullPointerException_, то программа бы просто бросила другое исключение, но это не так, поскольку переменная_\_oscpFailOpen_ является истинной, и программа должна была продолжать выполнение, а в итоге споткнулась об нулевой указатель\. **Фрагмент 2** ```cpp public void writeDateAttributeValue(XMLGregorianCalendar attributeValue) throws ProcessingException { .... StaxUtil.writeAttribute( writer, "xsi", JBossSAMLURIConstants.XSI_NSURI.get(), "type", "xs:" + attributeValue.getXMLSchemaType().getLocalPart() // <= ); if (attributeValue == null) { StaxUtil.writeAttribute( writer, "xsi", JBossSAMLURIConstants.XSI_NSURI.get(), "nil", "true" ); .... } ``` Предупреждение_:_ [V6060](https://pvs-studio.ru/ru/docs/warnings/v6060/) The 'attributeValue' reference was utilized before it was verified against null\. "Лучше поздно, чем никогда" — фраза, подходящая к большому количеству ситуаций, но, к сожалению, не к проверке на _null_\. В приведённом выше фрагменте объект _attributeValue_ используется до того, как он будет проверен на существование, что приводит к возникновению _NullPointerException_\. > \*\*Примечание\*\*\\\. Если вам интересны и другие примеры подобных ошибок, то у нас есть целый \[список\]\(https://pvs\-studio\.ru/ru/blog/examples/v6060/\)\\\! ## Аргументируем\.\.\. Не знаю, как так вышло, но в Keycloak ошибки, связанные с количеством аргументов, встретились именно в функциях, форматирующих строки\. Не то чтобы репрезентативная статистика, просто интересный факт\. **Фрагмент 3** ```cpp protected void process() { .... if (f == null) { .... } else { .... if (isListType(f.getType()) && t instanceof ParameterizedType) { t = ((ParameterizedType) t).getActualTypeArguments()[0]; if (!isBasicType(t) && t instanceof Class) { .... out.printf(", where value is:\n", ts); // <= .... } } else if (isMapType(f.getType()) && t instanceof ParameterizedType) { .... out.printf(", where value is:\n", ts); // <= .... } } } ``` Предупреждение: [V6046](https://pvs-studio.ru/ru/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Arguments not used: 1\. В приведённом фрагменте при вызове функции _printf_ передаётся строка для форматирования и значение, которое в неё необходимо подставить\. Только есть одна проблема: в строке просто нет места для подстановки аргументов\. Также интересен тот факт, что из\-за полного копирования блока из _if_ в _else if_ эта ошибка тоже была продублирована\. А вот в следующем фрагменте, наоборот, произошёл недобор аргументов\.\.\. **Фрагмент 4** ```cpp public String toString() { return String.format( "AuthenticationSessionAuthNoteUpdateEvent [ authSessionId=%s, tabId=%s, clientUUID=%s, authNotesFragment=%s ]", authSessionId, clientUUID, authNotesFragment); // <= } ``` Предупреждение: [V6046](https://pvs-studio.ru/ru/docs/warnings/v6046/) Incorrect format\. A different number of format items is expected\. Missing arguments: 4\. Помимо переданных в функцию аргументов _authSessionId_, _clientUUID_ и _authNotesFragment_ также ожидается четвёртый аргумент, названный в строке _tabId_\. В этом случае метод _String\.format_ бросит исключение _IllegalFormatException_, что может оказаться неприятной неожиданностью\. ## Нечистый код Следующий список предупреждений, которые выдал статический анализатор PVS\-Studio, не влияет непосредственно на работоспособность кода\. Все они скорее касаются того, насколько качественный код был написан\. Обращу внимание, что это не ошибки в истинном понимании этого слова\. "Почему же тогда мы будем на них смотреть?" — возможно подумали вы\. Во\-первых, я считаю, что код должен быть чистым и красивым\. И это не вопрос вкусов, ведь помимо визуальных качеств чистота кода влияет на его читаемость, что, как мне кажется, важно для любого проекта, а тем более для Open Source\. Во\-вторых, мне было интересно показать, что статический анализатор PVS\-Studio помогает не только исправлять ошибки в коде, но и наводить в нём красоту и порядок\. ### Нам нужно больше переменных\! Следующий фрагмент почему\-то представляется в моей голове как злобный план человека, плохо относящегося к переменным: "А давайте\-ка заведём переменные, а после оставим их с ужасом ожидать, пока до них дойдёт сборщик мусора\.\.\." **Фрагмент 5** ```cpp private void onUserRemoved(RealmModel realm, String userId) { int num = em.createNamedQuery("deleteClientSessionsByUser") .setParameter("userId", userId).executeUpdate(); // <= num = em.createNamedQuery("deleteUserSessionsByUser") .setParameter("userId", userId).executeUpdate(); } ``` Предупреждение: [V6021](https://pvs-studio.ru/ru/docs/warnings/v6021/) The value is assigned to the 'num' variable but is not used\. С точки зрения работы программы в этом фрагменте не произошло ничего страшного\. Но неясно, для чего именно это было сделано\. Переменная _num_ содержит количество установленных параметров, которое возвращает метод _executeUpdate_, поэтому я подумал, что, возможно, в этом методе также была проверка на то, произошли ли какие\-то изменения\. Но даже отмотав время назад, я обнаружил лишь, что изначально вызовы этого метода не записывались в переменную, а приняли текущее состояние чуть позднее\. Итого получается бесполезное присваивание переменной, как и в следующем фрагменте\. **Фрагмент 6** ```cpp private void verifyCodeVerifier(String codeVerifier, String codeChallenge, String codeChallengeMethod) throws ClientPolicyException { .... String codeVerifierEncoded = codeVerifier; try { if (codeChallengeMethod != null && codeChallengeMethod.equals(OAuth2Constants.PKCE_METHOD_S256)) { codeVerifierEncoded = generateS256CodeChallenge(codeVerifier); } else { codeVerifierEncoded = codeVerifier; // <= } } catch (Exception nae) { .... } } ``` Предупреждение: [V6026](https://pvs-studio.ru/ru/docs/warnings/v6026/) This value is already assigned to the 'codeVerifierEncoded' variable\. Если обратить внимание на код, то видно, что до этого переменной _codeVerifierEncoded_ уже было присвоено то же самое значение, что присваивается в блоке _else_ условного ветвления\. То есть просто выполнено лишнее действие, которое мало того, что бесполезно, так ещё и утяжеляет код\. А вот следующий фрагмент кода просто показался мне забавным\. **Фрагмент 7** ```cpp private static Type[] extractTypeVariables(Map typeVarMap, Type[] types){ for (int j = 0; j < types.length; j++){ if (types[j] instanceof TypeVariable){ TypeVariable tv = (TypeVariable) types[j]; types[j] = typeVarMap.get(tv.getName()); } else { types[j] = types[j]; // <= } } return types; } ``` Предупреждение: [V6005](https://pvs-studio.ru/ru/docs/warnings/v6005/) The variable 'types\[j\]' is assigned to itself\. Похоже на предыдущий фрагмент, однако здесь степень моего непонимания необходимости представленного кода принимает на порядок более высокие значения\. Изначально я подумал, что здесь мы имеем дело со вложенным циклом, и автор просто перепутал переменные_ i_ и _j_\. Правда, после этого я всё же констатировал, что цикл здесь один\. Другое моё предположение было в том, что такое присваивание появилось в процессе рефакторинга, и что ранее здесь был более сложный код\. Но в итоге я обнаружил, что эта функция изначально была создана именно таким образом \([коммит](https://github.com/keycloak/keycloak/commit/72d134748cc200108a41075b209aaabb27d96d09)\)\. ### Любимый Copy\-Paste Упущения при копировании встречаются довольно часто\. Думаю, что вы даже прямо сейчас могли вспомнить несколько ситуаций, когда такое встречалось у вас в коде\. В Keycloak тоже остались следы от использования Copy\-Paste\. **Фрагмент 8** ```cpp public class IDFedLSInputResolver implements LSResourceResolver { .... static { .... schemaLocations.put("saml-schema-metadata-2.0.xsd", "schema/saml/v2/saml-schema-metadata-2.0.xsd"); schemaLocations.put("saml-schema-x500-2.0.xsd", "schema/saml/v2/saml-schema-x500-2.0.xsd"); schemaLocations.put("saml-schema-xacml-2.0.xsd", "schema/saml/v2/saml-schema-xacml-2.0.xsd"); schemaLocations.put("saml-schema-xacml-2.0.xsd", "schema/saml/v2/saml-schema-xacml-2.0.xsd"); // <= schemaLocations.put("saml-schema-authn-context-2.0.xsd", "schema/saml/v2/saml-schema-authn-context-2.0.xsd"); .... } .... } ``` Предупреждение_:_ [V6033](https://pvs-studio.ru/ru/docs/warnings/v6033/) An item with the same key '"saml\-schema\-xacml\-2\.0\.xsd"' has already been added\. Честно, даже зная о том, что здесь есть опечатка, и какая именно, мне довольно затруднительно было сходу найти её в исходном коде\. Если вы обратите внимание, в вызовах метода _schemaLocations\.put_ передаваемые аргументы довольно похожи друг на друга\. Поэтому, как я предполагаю, человек, писавший этот код, просто скопировал какую\-то строку в качестве образца, а далее менял в ней значения\. Проблема в том, что в процессе копирования в проект закралась одна строка, повторяющая предыдущую\. Подобные "очепятки" могут как приводить к серьёзным последствиям, так и ни на что не влиять\. Конкретно этот пример Copy\-Paste существует в проекте с 21 ноября 2017 года \([коммит](https://github.com/keycloak/keycloak/commit/a993f6fb7545d24fb9e7b868bfe48ea66bc47543)\), и как я думаю, больших трудностей он не создал\. Цель включения этой ошибки в статью исключительно в том, чтобы напомнить, что при копировании нужно быть аккуратнее и следить за тем, что именно меняется\. А подробнее прочитать про опечатки, связанные с Copy\-Paste, можно в [этой статье](https://pvs-studio.ru/ru/blog/posts/csharp/0708/)\. ### Недостижимый код Заголовок даёт небольшую подсказку, какого рода предупреждение ждёт нас в следующем фрагменте\. Но я предлагаю вам самостоятельно обнаружить недостаток\. **Фрагмент 9** ```cpp public void executeOnEvent(ClientPolicyContext context) throws ClientPolicyException { switch (context.getEvent()) { case REGISTER: case UPDATE: .... case RESOURCE_OWNER_PASSWORD_CREDENTIALS_REQUEST: .... executeOnAuthorizationRequest(ropcContext.getParams()); return; default: return; } } ``` Полагаю, что конкретно диагностировать проблему не получилось\. Я ни капли не злорадствую и дал вам самостоятельно попробовать оценить обстановку на глаз исключительно из\-за того, что маленький недостаток этого кода связан с тем, что трудно обнаружить, не зная остального кода\. Ответ на то, какая ошибка спрятана в этом фрагменте, даст содержимое метода _executeOnAuthorizationRequest_: ```cpp private void executeOnAuthorizationRequest(MultivaluedMap params) throws ClientPolicyException { .... throw new ClientPolicyException(....); } ``` Да, этот метод бросает исключение\. То есть весь код, написанный после вызова этого метода, будет недостижимым, что и диагностировал анализатор PVS\-Studio\. ```cpp public void executeOnEvent(ClientPolicyContext context) throws ClientPolicyException { switch (context.getEvent()) { case REGISTER: case UPDATE: .... case RESOURCE_OWNER_PASSWORD_CREDENTIALS_REQUEST: .... executeOnAuthorizationRequest(ropcContext.getParams()); return; // <= default: return; } } ``` Предупреждение: [V6019](https://pvs-studio.ru/ru/docs/warnings/v6019/) Unreachable code detected\. It is possible that an error is present\. Пусть этот недочёт и совсем небольшой, но подобная ситуация могла произойти уже с более серьёзными последствиями\. Тут лишь могу отметить, что статический анализатор поможет избежать таких неприятных моментов\. ### Здесь условия диктую я Теперь заглянем в условные операторы и обратим внимание на то, при каких ситуациях они выполняют свой код\. **Фрагмент 10** ```cpp public boolean validatePassword(AuthenticationFlowContext context, UserModel user, MultivaluedMap inputData, boolean clearUser) { .... if (password == null || password.isEmpty()) { return badPasswordHandler(context, user, clearUser,true); } .... if (password != null && !password.isEmpty() && // <= user.credentialManager() .isValid(UserCredentialModel.password(password))) { .... } } ``` Предупреждения: [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'password \!\= null' is always true\. [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression '\!password\.isEmpty\(\)' is always true\. Сразу два предупреждения в одной строке\! На что же тут ругается анализатор? Проблема в том, что самый первый условный оператор проверяет, что пароль не равен _null_ и не пустой, прекращая в обратном случае выполнение функции\. А вот в той строке, на которую указал анализатор, обе эти проверки повторяются, то есть условия всегда являются положительными\. С одной стороны, лишними проверки не бывают, а с другой — из\-за таких повторений есть вероятность упустить ту часть условия, которая может быть эквивалентна _false_ и действительно влиять на работу программы\. Повторим упражнение\. **Фрагмент 11** ```cpp public KeycloakUriBuilder schemeSpecificPart(String ssp) throws IllegalArgumentException { if (ssp == null) throw new IllegalArgumentException(....); .... if (ssp != null) // <= sb.append(ssp); .... } ``` Предупреждение: [V6007](https://pvs-studio.ru/ru/docs/warnings/v6007/) Expression 'ssp \!\= null' is always true\. В целом, идентичная ситуация\. Если переменная _ssp_ принимает значение _null_, то программа бросает исключение\. Исходя из этого, условие ниже не только постоянно истинно, так ещё и в принципе не нужно, поскольку соответствующий ему блок кода будет выполняться всегда\. Таким же ненужным является и условие из следующего фрагмента\. **Фрагмент 12** ```cpp protected String changeSessionId(HttpScope session) { if (!deployment.turnOffChangeSessionIdOnLogin()) return session.getID(); // <= else return session.getID(); } ``` Предупреждение: [V6004](https://pvs-studio.ru/ru/docs/warnings/v6004/) The 'then' statement is equivalent to the 'else' statement\. В этом методе при разных временах года, фазах луны, и, что самое важное, при разных условиях, выполняется один и тот же код\. То есть, опять же, условие здесь было совсем не нужно\. Немного покопавшись в коде, я обнаружил фрагмент, который похож на этот как две капли воды: ```cpp protected String changeSessionId(HttpSession session) { if (!deployment.turnOffChangeSessionIdOnLogin()) return ChangeSessionId.changeSessionId(exchange, false); else return session.getId(); } ``` Как видно, при выполнении условия вызывается метод, который меняет идентификатор сессии\. Исходя из этого, можно сделать два предположения: либо код просто скопировали и поменяли результат условия, либо в первом условии всё же должна была обновляться сессия, и эта ошибка уже выходит за границы нечистого кода\. Но не только лишними условиями едины\! **Фрагмент 13** ```cpp static String getParameter(String source, String messageIfNotFound) { Matcher matcher = PLACEHOLDER_PARAM_PATTERN.matcher(source); while (matcher.find()) { return matcher.group(1).replaceAll("'", ""); // <= } if (messageIfNotFound != null) { throw new RuntimeException(messageIfNotFound); } return null; } ``` Предупреждение: [V6037](https://pvs-studio.ru/ru/docs/warnings/v6037/) An unconditional 'return' within a loop\. Есть ощущение, что этот _while_ воспитали _if_'ы\. Возможно, этот код имеет какой\-то скрытый замысел, однако мы с анализатором видим здесь одно и то же: цикл, который всегда выполняет одну итерацию\. Возможно, автор этого кода предполагал другой вариант его поведения\. А если даже и нет, то понять смысл этого кода будет чуть сложнее, чем если бы там был просто условный оператор\. ## Сравнение строк Автор кода из следующего фрагмента предположил, что всё просто\. Но оказалось, что это не так\.\.\. **Фрагмент 14** ```cpp public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; Key key = (Key) o; if (action != key.action) return false; // <= .... } ``` Предупреждение: [V6013](https://pvs-studio.ru/ru/docs/warnings/v6013/) Strings 'action' and 'key\.action' are compared by reference\. Possibly an equality comparison was intended\. При сравнении строк подразумевается, что мы сравниваем их содержимое\. Однако на самом деле сравниваются ссылки на объекты\. То же самое происходит не только со строками, а ещё с массивами и коллекциями\. Думаю, понятно, что в определённых ситуациях работа кода может привести к неожиданным последствиям\. Самое главное, что исправить такую ошибку довольно легко: ```cpp public boolean equals(Object o) { .... if (!action.equals(key.action)) return false; .... } ``` Метод _equals_ возвращает сравнение именно к содержанию двух строк\. Победа\! А я обращу ваше внимание, что статический анализатор обнаружил ту ошибку, которую разработчик при осмотре кода, скорее всего, пропустил бы дальше\. Об этой и других причинах использования статического анализа можно почитать в этой [статье](https://pvs-studio.ru/ru/blog/posts/0687/)\. ## Блокировка с двойной проверкой Блокировка с двойной проверкой — это параллельный шаблон проектирования, предназначенный для уменьшения накладных расходов получения блокировки\. Сначала мы проверяем условие блокировки без синхронизации, и, если оно выполняется, поток пытается получить блокировку\. Если упростить, то за счёт этого шаблона блокировка будет получаться только тогда, когда она действительно необходима\. Думаю, вы уже догадались о наличии ошибок с реализацией этого шаблона, раз уж я начал о нём рассказывать\. Собственно, так и есть\. **Фрагмент 15** ```cpp public class WelcomeResource { private AtomicBoolean shouldBootstrap; .... private boolean shouldBootstrap() { if (shouldBootstrap == null) { synchronized (this) { if (shouldBootstrap == null) { shouldBootstrap = new AtomicBoolean(....); } } } return shouldBootstrap.get(); } ``` Предупреждение: [V6082](https://pvs-studio.ru/ru/docs/warnings/v6082/) Unsafe double\-checked locking\. The field should be declared as volatile\. Анализатор ругается, что у поля _shouldBootstrap_ отсутствует модификатор _volatile_\. Но на что это влияет? В таком коде вероятно, что объект может быть использован разными потоками до окончания процесса их инициализации\. И если этот факт может быть не так существенен, то уже в следующем примере компилятор вполне может поменять порядок выполнения действий с не\-_volatile_ полями\. **Фрагмент 16** ```cpp public class DefaultFreeMarkerProviderFactory implements FreeMarkerProviderFactory { private DefaultFreeMarkerProvider provider; // <= .... public DefaultFreeMarkerProvider create(KeycloakSession session) { if (provider == null) { synchronized (this) { if (provider == null) { if (Config.scope("theme").getBoolean("cacheTemplates", true)) { cache = new ConcurrentHashMap<>(); } kcSanitizeMethod = new KeycloakSanitizerMethod(); provider = new DefaultFreeMarkerProvider(cache, kcSanitizeMethod); } } } return provider; } } ``` Предупреждение: [V6082](https://pvs-studio.ru/ru/docs/warnings/v6082/) Unsafe double\-checked locking\. The field should be declared as volatile\. Почему тогда эти фрагменты не исправили, если ошибка такая опасная? Ответ в том, что ошибка не только опасная, но ещё и коварная\. В большинстве случаев всё работает так, как и должно\. А вот неправильное поведение может произойти из\-за множества различных факторов, например, используемой JVM или решений планировщика потоков, из\-за чего воспроизвести условия возникновения ошибки бывает довольно тяжело\. Подробнее про подобные ошибки и причины их возникновения можно прочитать в [статье](https://pvs-studio.ru/ru/blog/posts/java/1128/)\. ## Заключение В завершение этой статьи хочу отметить, что в ней я использовал анализатор не совсем верно\. Моей целью было развлечь вас и показать, какие ошибки есть в проекте\. Однако для непосредственного исправления ошибок и профилактики их дальнейшего возникновения статический анализатор нужно использовать не разово, а систематически в процессе написания кода\. Об этом подробнее можно прочитать [здесь](https://pvs-studio.ru/ru/blog/posts/0669/)\. Несмотря на это, вместе с анализатором мы обнаружили довольно разнообразный набор ошибок, связанных как с работой программы, так и с недостаточной чистотой кода \(всё ещё считаю, что это важно, и вы вряд ли меня переубедите\)\. И ошибки — это не страшно, если они вовремя находятся и исправляются, с чем и поможет статический анализ\. А попробовать анализатор PVS\-Studio и с его помощью проверить проект можно [бесплатно](https://pvs-studio.ru/ru/pvs-studio/try-free/)\.