Authentication cleanup and credentials handling

[Nadahar]

This PR started out trying to replace String with char[] for storing secrets like passwords and tokens. While preparing for user pinia store storage in core, I had to take a closer look at user management, and discovered this:

openhab-core/bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java

Lines 119 to 124 in 8aa7a04

	char[] chars = password.toCharArray();
	byte[] bytes = salt.getBytes();
	PBEKeySpec spec = new PBEKeySpec(chars, bytes, iterations, KEY_LENGTH);
	Arrays.fill(chars, Character.MIN_VALUE);

I saw what was done there, and assume that it was an attempt at following "security best practices", but kind of missed the whole point by creating the char array from a string, and then clearing the array after use.

A short description of the underlying issue (skip this paragraph if you're already familiar): Strings in Java are immutable, the memory cannot be overwritten until the string has been GC'ed (which can take minutes, depending on circumstances). To make it even worse strings are "interned"/cached for improved performance, and this is controlled entirely by the JVM, so you can risk that any string ends up in this cache, and has an even much longer lifetime. As a result, strings in Java are terribly unsuitable for secrets of any kind, because a memory dump of any kind is likely to contain these secrets. The "best practice" is therefore to use mutable char[] for storage of sensitive data, that you overwrite as soon as the data aren't needed anymore.

I started to try to replace strings with char arrays, and this took me on a journey where I discovered a lot of code that I believe isn't doing anything. #2245 removed the only implementation for a structure of code that was left behind, but couldn't actually do anything, in the form of AuthenticationManager. I removed the components that seemed to be without actual utility, and started testing for consequences - without being able to find any.

I then discovered all the Jaas functionality that didn't seem to do anything, and tried to investigate why it was there. This seems to be remnants from previous attempts to unify Karaf users with UI users. This isn't what is actually used, these are independent user databases today, which seems like the correct design. I "unentangled" and removed the Jaas related code, which ended with me being able to remove the org.openhab.core.auth.jaas bundle in its entirety, together with the org.openhab.core.karaf.internal.jaas package.

I also addressed an OSGi resolution/encapsulation issue with JwtHelper and got rid of WARN logging when tokens expire (which isn't something the user should be concerned with, they are then refreshed and things keep working).

As a result, I don't know quite what to call this PR, it's a bit of "authentication cleanup" combined with some improvement of memory safety for secrets. The initial task of protecting secrets has a somewhat limited effect though, because I quickly discovered that most of this information is delivered to us from other components, like Jetty, already as strings. Which greatly reduces the point of trying to clean them from memory later. It's still an improvement of "security practices", albeit with a perhaps limited real-life impact.

I have done quite a lot of testing on this, and I've been unable to find anything that has broken. Many of these things are "scary" to touch though, and I assume that's why much of this code is still around despite not doing anything. So, I feel somewhat uneasy that there might have been something I've missed, and I invite scrutiny on any consequences of the removed code. But, the removal of all this makes evolving authentication in the future much easier, because the code is much easier to understand/has a cleaner structural design.

Disclaimer: This PR is free from AI generated code.

[florian-h05]

Does the console log in still work?

[Copilot]

Pull request overview

This PR refactors openHAB core authentication by removing unused legacy authentication/Jaas infrastructure, while improving secret-handling by moving password/token credentials toward char[] with explicit disposal and updating authentication flows accordingly.

Changes:

• Remove the legacy AuthenticationManager/HTTP auth handler pipeline and the JAAS integration (including the org.openhab.core.auth.jaas bundle and Karaf JAAS remnants).
• Update UserRegistry/AuthenticationProvider and credential types to support char[] secrets with Credentials#dispose() for best-effort memory clearing.
• Make JwtHelper publicly accessible (package move) and reduce log noise for expired JWT tokens.

Reviewed changes

Copilot reviewed 35 out of 35 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
features/karaf/openhab-core/src/main/feature/feature.xml	Removes the JAAS auth feature and dependencies from runtime features.
bundles/pom.xml	Removes the org.openhab.core.auth.jaas module from the reactor build.
bundles/org.openhab.core/src/test/java/org/openhab/core/internal/auth/UserRegistryImplTest.java	Updates tests for char[] password/token APIs.
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/UserRegistryImpl.java	Implements char[] password/token handling, token generation, and disposal-aware authentication.
bundles/org.openhab.core/src/main/java/org/openhab/core/internal/auth/AuthenticationManagerImpl.java	Deletes the legacy authentication manager implementation.
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/UserRegistry.java	Adds char[] overloads and deprecates String password APIs.
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/UsernamePasswordCredentials.java	Stores passwords as char[] and adds disposal support.
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/UserApiTokenCredentials.java	Stores API tokens as char[] and adds disposal support (with deprecated String ctor).
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/UserApiToken.java	Refactors stored token data to “hash + salt” model.
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/Credentials.java	Introduces dispose() contract for credentials.
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/AuthenticationProvider.java	Adds disposal-aware authentication overload; default call disposes by default.
bundles/org.openhab.core/src/main/java/org/openhab/core/auth/AuthenticationManager.java	Removes the legacy authentication manager interface.
bundles/org.openhab.core.karaf/src/main/java/org/openhab/core/karaf/internal/jaas/ManagedUserRealm.java	Removes Karaf JAAS realm integration.
bundles/org.openhab.core.karaf/src/main/java/org/openhab/core/karaf/internal/jaas/ManagedUserBackingEngineFactory.java	Removes Karaf backing engine factory for managed users.
bundles/org.openhab.core.karaf/src/main/java/org/openhab/core/karaf/internal/jaas/ManagedUserBackingEngine.java	Removes Karaf backing engine for managed users.
bundles/org.openhab.core.io.rest.auth/src/test/java/org/openhab/core/io/rest/auth/AuthFilterTest.java	Adjusts imports due to JwtHelper package move.
bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/JwtHelper.java	Moves JwtHelper out of internal and hardens key dir creation.
bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/internal/TokenResource.java	Updates import to the new JwtHelper location.
bundles/org.openhab.core.io.rest.auth/src/main/java/org/openhab/core/io/rest/auth/AuthFilter.java	Updates credential creation to char[] and downgrades expired-JWT logs to debug.
bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/RedirectHandler.java	Removes unused redirect handler from legacy HTTP auth flow.
bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/CreateAPITokenPageServlet.java	Updates password/token handling to char[].
bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/ChangePasswordPageServlet.java	Updates password handling to char[].
bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthorizePageServlet.java	Updates registration/login password handling to char[].
bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AuthenticationHandler.java	Removes the legacy HTTP authentication handler.
bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/internal/AbstractAuthPageServlet.java	Updates login API to accept char[] password.
bundles/org.openhab.core.io.http.auth/src/main/java/org/openhab/core/io/http/auth/CredentialsExtractor.java	Removes legacy credentials extractor API.
bundles/org.openhab.core.io.console/src/main/java/org/openhab/core/io/console/internal/extension/UserConsoleCommandExtension.java	Updates console user management to char[] password/token APIs.
bundles/org.openhab.core.auth.jaas/src/main/java/org/openhab/core/auth/jaas/internal/ManagedUserLoginModule.java	Removes JAAS login module.
bundles/org.openhab.core.auth.jaas/src/main/java/org/openhab/core/auth/jaas/internal/ManagedUserLoginConfiguration.java	Removes JAAS login configuration.
bundles/org.openhab.core.auth.jaas/src/main/java/org/openhab/core/auth/jaas/internal/JaasAuthenticationProvider.java	Removes JAAS authentication provider.
bundles/org.openhab.core.auth.jaas/pom.xml	Removes the JAAS bundle build descriptor.
bundles/org.openhab.core.auth.jaas/NOTICE	Removes JAAS bundle notice file.
bundles/org.openhab.core.auth.jaas/.project	Removes Eclipse project metadata for the JAAS bundle.
bundles/org.openhab.core.auth.jaas/.classpath	Removes Eclipse classpath metadata for the JAAS bundle.
bom/openhab-core/pom.xml	Removes the JAAS bundle from the core BOM.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Nadahar]

Does the console log in still work?

It should be unaffected - you log in with Karaf users, not "UI users". I must actually build the distribution and run it under Karaf to test it, but I can't understand why it should be affected. I haven't removed anything related to that, as far as I know.

edit: @florian-h05 Just to be sure that there's no misunderstanding here: Are you talking about logging in to the Karaf console using a Karaf user or a "OH user"? Because, this PR removes code that seems to have been an attempt at making that possible, but as far as I can tell, it was incomplete and didn't actually work. I also tested this with an existing 5.2.0 installation running in Docker, and I could not log in with users I created using openhab:users. As I understand it, the user databases are completely separate, Karaf handles its users in some configuration file, while the UI uses the JSONDB user database.

[florian-h05]

Hmm, you should be able to log into the Karaf console using an openHAB user account (I'd expect only with admin privileges, not sure however that is required).

Your feedback that it is not possible anymore to log into the console with an openHAB user account confirms that I recalled the use of the JAAS stuff correctly: It is needed for Karaf login.
I think I figured that out when I was looking into LDAP support, I guess that code is even lying around in some branch but I haven't found the motivation yet to finish that work and submit it as a PR.

[Nadahar]

Hmm, you should be able to log into the Karaf console using an openHAB user account (I'd expect only with admin privileges, not sure however that is required).

Try to do that in a current installation. I couldn't get it to work, and I can't quite see that everything necessary to make it work was present either (not 100% sure about that part, but it appeared to me somewhat "partial").

The idea of logging into Karaf using a OH user is also a bit "strange" - why would you want that? A Karaf console session effectively has full control over the system security - it can add or modify users, it can modify the configuration directly, manipulating how the system behaves. A Karaf account must be considered an "installation root". Also, it would mean that you can't log in to Karaf if JSONDB isn't running - and if you stop that bundle from within Karaf, you invalidate your account.

Are you sure that this is even desirable?

[florian-h05]

Ah sorry, I think I messed a few things up. Actually login on Karaf with an openHAB user isn't possible. I still wonder however for what the JAAS realm was needed ...

[Nadahar]

Ah sorry, I think I messed a few things up. Actually login on Karaf with an openHAB user isn't possible. I still wonder however for what the JAAS realm was needed ...

That's why I removed it. It's not needed as far as I can tell. At one point back in OH 2 I think, there was an attempt at having a "unified user database", common for Karaf and the UI. It was abandoned at some stage as I understand it, but a lot of left-overs are still in the system, making everything quite convoluted and difficult to try to figure out. I think the idea back then was to log in to the UI using Karaf users, so that you didn't need to keep a separate user database. But, because of how "powerful" a Karaf account is, I don't think it's a good idea to begin with. I think of the Karaf users as "installation administration accounts", and the "OH users" as the actual users of the system.

edit: If this end up being merged, I think you'll find implementing e.g. LDAP support quite a lot easier.

[Nadahar]

The Karaf user database is actually userdata/etc/users.properties, and as far as I know, only users that exist there can be used to log in to Karaf.

[Nadahar]

On a sidenote, I discovered that users.properties has global read permissions (644) by default and is hashed with SHA-256 without salt:

openhab = {CRYPT}4F61A0FD056BC0FD8231899EC4D9F9CA06AF0DEC895B2A3B0773F6FBC1C99776{CRYPT},_g_:admingroup
_g_\:admingroup = group,admin,manager,viewer,systembundles

That's a pretty "weak setup" considering that access to the Karaf console gives full access to create administrative OH users, change the Karaf configuration etc. Anybody can search for hits in rainbow tables for the hash found here, and the hash itself isn't very difficult to access. It might be a good idea to change this to use salt at the very least, and perhaps the file should be 600, not 644 by default.

I've looked some into it, and it should be possible it seems, to use the "tags" ({CRYPT} in this case) to define more than one "Karaf authentication module", so that it could support two different algorithms at once, using different "tags". The "new", better one, could be the default that was used when new passwords are generated, while the "old" could make sure that existing password hashes keep working, so that users aren't locked out of Karaf. userdata/etc/org.apache.karaf.jaas.cfg shows that there are some better options available. Bcrypt would have been the best option as far as I can understand, but it requires installing feature spring-security-crypto-encryption. But, we should be able to switch from basic to jasypt without requiring installation of an additional feature.

This doesn't belong in this PR, but it's worth mentioning, since I discovered it now.

[wborn]

Thanks for investigating this area. There are several worthwhile changes here, particularly removing the unused authentication pipeline, fixing the JwtHelper package exposure and avoiding warnings for normal token expiration.

I think the PR currently combines too many independent changes, however:

1. removal of the old AuthenticationManager pipeline;
2. removal of the JAAS integration;
3. exporting JwtHelper and adjusting JWT logging;
4. redesigning public credential APIs around char[];
5. changing the persisted representation of API tokens.

The first three are primarily cleanup and encapsulation improvements. The last two change public API contracts, introduce new ownership semantics for mutable arrays and affect persisted user data. They therefore need substantially different compatibility and upgrade testing.

Could we split this into separate PRs? My preferred split would be:

• removal of the unused authentication pipeline and JAAS code, including the corresponding distribution changes and documentation of removed behaviour;
• JwtHelper export and expired-token logging;
• the char[] credential redesign;
• the API-token persistence change, only if it is still needed and with explicit migration coverage.

This would make the cleanup considerably easier to review and merge without tying it to the credential redesign.

Regarding the char[] conversion specifically: I agree with the principle when a secret is obtained as mutable data and remains mutable throughout its lifetime. In the current implementation, though, there is no common secret wrapper. The individual credential classes store raw arrays, their constructors retain caller-owned arrays, their getters expose those arrays directly, and hashing or disposal can overwrite them.

This creates an unclear ownership contract: callers may not expect authentication to mutate an array they supplied, and credential objects effectively become single-use without that being expressed by the API.

The practical benefit is also limited in the current entry points because HTTP parameters, authorization headers and console arguments already arrive as String values before being converted to arrays. Clearing the later array does not remove the original string. Generated tokens also become strings when printed or placed in an HTML response.

There is still some defense-in-depth value, but I do not think it currently justifies breaking several public APIs or changing persistent data in the same PR. The credential work would benefit from a separate design discussion about whether mutable-secret handling should remain internal or use a dedicated abstraction with clear ownership, copying and disposal semantics.

I also agree with the existing unresolved review comments concerning migration of existing API tokens and authentication when different users have tokens with the same name. I consider those concrete regressions rather than hypothetical concerns.

Review assisted by AI.

[Nadahar]

The last two change public API contracts, introduce new ownership semantics for mutable arrays and affect persisted user data. They therefore need substantially different compatibility and upgrade testing.

As explained above, what I set out to do was the char[] refactoring. The rest were results of me going through the code to make these changes. As some of the Copilot comments show, I was actually sufficiently "distracted" by all the code removal that I forgot to fully complete the original task, with some lapses in synchronization logic etc. I "had the feeling" that this had happened, and spent some time trying to go through the "original changes", but as it turns out, I didn't find them all.

That said, this PR was never intended to break any APIs, it's not supposed to require migration. I have used deprecated methods with the old method signatures where possible/viable, exactly to prevent breaking. Some things can't be handled that way, like changing the return type, but I expect that the impact on any add-ons should be minimal and easily addressed should they turn up.

The issue with serialization was one that I simply didn't think about, I had no idea that the class was actually serialized. I obviously didn't check either, which I should have done, but the point is that this PR was never meant to require migration. The way I've solved it now, it will "migrate automatically", but users who upgrade, run OH, and then downgrade again will end up with invalid API tokens. I think this is acceptable though, because it only invalidates existing API tokens. The user can just delete them, generate new ones and all should be fine. It's not possible to make an "automatic unmigration", because that would require changing the code of already released versions.

If this is considered problematic, I could revert data structure change to that class. It's not very important at all, I just wanted to avoid having to do the string splitting, retrieval and conversion to byte[] every time the salt is needed. So, it only a minor optimization, nothing in any way important. But I think the automatic migration should make it almost a non-issue.

Regarding the char[] conversion specifically: I agree with the principle when a secret is obtained as mutable data and remains mutable throughout its lifetime. In the current implementation, though, there is no common secret wrapper. The individual credential classes store raw arrays, their constructors retain caller-owned arrays, their getters expose those arrays directly, and hashing or disposal can overwrite them.

This creates an unclear ownership contract: callers may not expect authentication to mutate an array they supplied, and credential objects effectively become single-use without that being expressed by the API.

This is true, but it's also in the nature of the problem that you don't want to make a lot of defensive copies. I went through and studied how these were used, and found that most of the time, the objects were created for the purpose of doing a single authentication, which is why I found it acceptable to do it this way. Others might disagree, but this was my evaluation.

The practical benefit is also limited in the current entry points because HTTP parameters, authorization headers and console arguments already arrive as String values before being converted to arrays. Clearing the later array does not remove the original string. Generated tokens also become strings when printed or placed in an HTML response.

I've already point that out. From what I understand, this is usually the case, and "best practices" still recommend doing things like this. The idea is, I guess, that the fewer copies the better, and as various parts of the system is upgraded to use mutable storage for sensitive data, it adds up, and eventually you might end up with a "string free handling of secrets". I don't see that happening anytime soon though.

One argument that is made in favor of doing such things are that the strings produced by e.g. Jetty are "short lived", while the internal storage objects might have a much longer lifespan. The claim is generic and might not actually be true in every real situation, but it's part of the "general principle" from what I understand.

[wborn]

I looked this up, and using char[] for passwords is indeed an established Java pattern. APIs such as JPasswordField, PasswordCallback and PBEKeySpec use it so the password can be cleared after use.

What seems unusual here is that the credentials keep the caller's array, expose it directly and may overwrite it during authentication. PasswordCallback and PBEKeySpec, for example, copy the supplied array rather than taking ownership of it.

Would a small secret wrapper make the ownership, single-use behaviour and disposal semantics clearer?

For API design changes like this, could you also create an issue for discussion before implementing them? It would be easier to agree on these semantics before they become part of a larger PR.

[Nadahar]

What seems unusual here is that the credentials keep the caller's array, expose it directly and may overwrite it during authentication. PasswordCallback and PBEKeySpec, for example, copy the supplied array rather than taking ownership of it.

It was a deliberate choice, both to minimize the number of copies and to make sure that it could be deleted "as soon as it wasn't needed anymore". A part of the equation was also that I looked at where it was called from, and found that it was often more trouble than it was worth to handle the clearing at the caller.

But, I agree that the "contract" might not be as clear as it should, and I had to make some "dubious mechanisms" (like whether to call dispose() or not), because sometimes it needs to be retained for re-use.

If you have an idea for a "cleaner" way to do this, I'm all ears. A wrapper in itself wouldn't "solve" the "who is responsible for clearing it" problem. It would be easy to make copies everywhere and make every piece of code responsible to clean up "their instance", but it would make more copies in total. Maybe it's still "worth it" in that the handling becomes clearer? Or did you have something "more clever" in mind?

For API design changes like this, could you also create an issue for discussion before implementing them? It would be easier to agree on these semantics before they become part of a larger PR.

I've tried that before, it just doesn't work in the real world. It takes months to get to an agreement, and often there are little or no response. There's no way to define "when agreement has been made" either, even if those participating in the issue agree, that doesn't mean that somebody that doesn't participate can't have a different opinion.

By the time all this would have been figured out, I would have forgotten all about what I intended to do. It would be pointless, at least for "my process". I typically work with a problem for a day or two, and after that, I turn to something else, and my memory slowly begins to fade. By the time somebody makes a comment, I might have dealt with 10 different situations. I then have to try to refresh my memory on this situation, make an answer, and then time goes by again. Rinse and repeat.

I'm not able to get anything done that way, so I've concluded that generally, it's better to just made a suggestion, and then see how it's received. That way, I'm at least able to work with it while I have that situation fresh in my mind. I get a somewhat similar situation when the PR is reviewed later, in that I probably have forgotten a lot of what considerations I did, but when I have a "complete PR", it's at least easier for me to go back and look at what I did and try to remember.

If I were to make some major, structural changes, I would of course seek to discuss it first. I tend to try to avoid those because of the whole timescale issue, and rather pick things where I don't need to do that. I always try to do whatever changes that must take place "as defensively as possible".

I never considered this PR to do "major API changes". If you define "the API" as every piece of public and protected methods, it's hard to do anything without changing something. But, as far as I know, I've made no changes here that are likely to break existing code, and the changes I've done that are "breaking" are necessary to achieve the main goal of the PR - to transition from String to char[] storage of secrets.

• AbstractAuthPageServlet: Changed the password from String to char[] in one protected method. The class is internal and thus not exported outside the bundle, I've updated all references within the bundle.
• AuthenticationProvider: Added a method, the existing method is unchanged, no change for callers, but a new method is required for implementations (in reality just a small change to the existing method).
• Credentials: Added a method, only affects implementations. I could have given the new method a default no-op implementation, but I considered the likelihood for non-core implementations small.
• UserApiToken: No changes to existing methods, callers not affected.
• UserApiTokenCredentials: Added a constructor, the existing constructor is deprecated but unchanged. Breaking: getApiToken() has changed return type from String to char[].
• UserRegistry: Added two methods and deprecated two existing methods, impacts implementations but not callers. Breaking: addUserApiToken() has changed return type from String to char[].
• UsernamePasswordCredentials: Breaking: password parameter in constructor changed from String to char[] and return type of getPassword() has changed from String to char[].
• UserRegistryImpl: Reflects the breaking change from UserRegistry, otherwise nothing.

I agree that the "disposal logic" probably can be improved, but apart from that, the API changes are strictly necessary to move away from string storage. I thus see that rejecting the API changes = rejecting the idea. So, the discussion takes place here and now, not upfront, and I think that's the way that makes the most sense.

The different "aspects" of this PR are done in clearly distinctive commits, so there's no problem to extract just the "cleanup part" and don't do the String → char[] transition. But, there's a good reason why both is in the same PR, and that is that the opposite isn't true: You can't easily do the String → char[] transition without the cleanup, because the transition required a number of changes also in some of the removed classes. It's when I discovered this, that I was modifying classes that served no purpose, that I switched to the "cleanup" before completing the other work. I had to throw away some refactoring that I had already done before I discovered it. This is also the reason why I moved the "cleanup commits" first - otherwise I'd have to make changes only to delete that code in a later commit (if I wanted to make all commits compile, which I as a general rule do).

I also expected, and asked for, scrutiny for this PR, I'm usually more confident that nothing significant will break, in this case I find it hard to be as confident as I'd like before submitting. As such, I never intended for what I submitted to be "final", and expected further changes.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rbac-model-in-openhab-and-potential-security-vulnerability-found/136419/20