Skip to content

GUACAMOLE-1743: Add salted PBKDF2 password hashing for user-mapping.xml. - #1254

Open
zivshits wants to merge 2 commits into
apache:mainfrom
honeybadger-technologies:GUACAMOLE-1743
Open

zivshits wants to merge 2 commits into
apache:mainfrom
honeybadger-technologies:GUACAMOLE-1743

Conversation

@zivshits

@zivshits zivshits commented Oct 10, 2026 •

Copy link
Copy Markdown

Resolves GUACAMOLE-1743 for user-mapping.xml.

The JDBC part originally in this PR has been removed, per review; it will be raised separately.

Summary

  • user-mapping.xml: adds encoding="pbkdf2", a salted PBKDF2-HMAC-SHA256 hash carried in a self-describing string, as discussed on the issue. Existing plain-text, md5 and sha256 entries behave exactly as before, so the file format stays backward-compatible.
  • Shared hasher: the hashing lives in a reusable class in guacamole-ext, org.apache.guacamole.net.auth.PBKDF2PasswordHasher. It uses only the JDK (SecretKeyFactory PBKDF2WithHmacSHA256), adds no dependency, and is Java 8 compatible.

Design

Shared hasher (guacamole-ext)

  • PBKDF2-HMAC-SHA256 from the JDK.
  • Default iteration count is 600,000, OWASP's current guidance for PBKDF2-HMAC-SHA256.
  • New salts are 32 random bytes from SecureRandom, and new hashes are 32 bytes long.
  • Comparisons are constant-time (MessageDigest.isEqual). A null password never matches.
  • The self-describing string format is pbkdf2-sha256$<iterations>$<base64 salt>$<base64 hash>:
    • The salt and hash use standard RFC 4648 base64, and padding is optional.
    • The hash length sets the derived key length.
  • A known-answer test checks the output against the RFC 7914 §11 PBKDF2-HMAC-SHA256 vector.

user-mapping.xml

<user-mapping>
    <authorize username="alice"
               password="pbkdf2-sha256$600000$mx3KKJsqhJcUnWKye2IQNg==$ygOV6uG1wUoPHoIEs/pfTPSRIwwo59JjpoORws+6vG8="
               encoding="pbkdf2">
        <connection name="example">
            <protocol>ssh</protocol>
            <param name="hostname">localhost</param>
        </connection>
    </authorize>
</user-mapping>

(That hash is S3cret pass. It was generated by the one-liner below, and the Java code verified it.)

  • Malformed strings never authenticate. This covers a wrong prefix, a wrong field count, a non-numeric or non-positive iteration count, bad base64, and an empty salt or hash. The file still loads and other users are unaffected.
  • A malformed string logs one WARN naming the user. The value is never logged:
    Password of user "user" is not a valid PBKDF2 hash. The user cannot log in until it is corrected.
  • The existing plain, md5 and sha256 encodings are unchanged.

Generating a hash. Use this the same way sha256sum is documented for encoding="sha256". It prompts for the password without echoing it:

python3 -c 'import base64,getpass,hashlib,os;p=getpass.getpass().encode();s=os.urandom(16);i=600000;b=lambda x:base64.b64encode(x).decode();print("pbkdf2-sha256$%d$%s$%s"%(i,b(s),b(hashlib.pbkdf2_hmac("sha256",p,s,i))))'

Testing

  • Unit tests: guacamole-ext and guacamole test suites pass on this branch (based on main). The new tests are:
    • PBKDF2PasswordHasherTest: determinism, salt sensitivity, the RFC 7914 §11 known-answer vector, a fixture produced by the Python one-liner above, and 12 malformed strings.
    • AuthorizationTest: a valid PBKDF2 entry, a wrong password, malformed entries, and that the legacy encodings still work.

AI assistance

This change, its tests and this description were written with Claude Code. The author reviewed the change. All test results above come from real runs.

🤖 Generated with Claude Code

zivshits and others added 2 commits October 10, 2026 19:34
…amole-ext.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
….xml.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@necouchman

Copy link
Copy Markdown
Contributor

I haven't had a chance to review changes, yet; however, while I'm likely okay, in general, with updating the encryption of the user-mapping.xml file, I'm not so sure about the JDBC changes. As has been discussed multiple times in the forums and on the various Jira issues, encryption for JDBC necessarily has to be reversible in a way that makes the value of adding this pretty limited. I'd rather encourage admins to not store password values in the database at all and, instead, use a keyvault or prompt users for the values. I believe it adds needless complexity at a limited return.

@zivshits

Copy link
Copy Markdown
Author

Thanks, Nick. I think there may be some cross-over with the connection-parameter encryption discussion. This PR doesn't touch connection parameters; it only changes how JDBC user login passwords (guacamole_user.password_hash) are hashed. Those are already one-way (single-round salted SHA-256) and never need to be reversed. The change makes them slow-to-brute-force if the database leaks, the same as the user-mapping.xml change.

That said, I understand the schema migration and upgrade/downgrade cost may not be worth it to you. I'll reduce this PR to the user-mapping.xml change and the shared hasher, and I'll open a separate issue for the JDBC part so it can be discussed on its own.

@zivshits zivshits changed the title GUACAMOLE-1743: Add salted PBKDF2 password hashing for user-mapping.xml and JDBC. GUACAMOLE-1743: Add salted PBKDF2 password hashing for user-mapping.xml. Oct 12, 2026
@necouchman

Copy link
Copy Markdown
Contributor

Thanks, Nick. I think there may be some cross-over with the connection-parameter encryption discussion. This PR doesn't touch connection parameters; it only changes how JDBC user login passwords (guacamole_user.password_hash) are hashed. Those are already one-way (single-round salted SHA-256) and never need to be reversed. The change makes them slow-to-brute-force if the database leaks, the same as the user-mapping.xml change.

Ah, yes, I see what you mean - thanks for clarifying that.

That said, I understand the schema migration and upgrade/downgrade cost may not be worth it to you. I'll reduce this PR to the user-mapping.xml change and the shared hasher, and I'll open a separate issue for the JDBC part so it can be discussed on its own.

Yeah, I think that would be best. In particular for the JDBC option, if we're making changes to schema, we need to make sure we maintain compatibility - so, if passwords are currently stored in one format we need to make sure those can still be used until they are changed and re-written in the new format, since there isn't any way to transparently migrate them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants