[PR] GUACAMOLE-558: Store secret connection parameters encrypted (JDBC). [guacamole-client]

["zivshits (via GitHub)" <gi...@apache.org>]

zivshits opened a new pull request, #1256:
URL: https://github.com/apache/guacamole-client/pull/1256

   Resolves [GUACAMOLE-558](https://issues.apache.org/jira/browse/GUACAMOLE-558) for the JDBC authentication extensions (PostgreSQL, MySQL, SQL Server).
   
   ### Problem
   
   Connection and sharing-profile parameters, including the passwords, passphrases and private keys used to log in to remote desktops, are stored as plain text. Anyone with read access to the parameter tables, or to a dump, backup or snapshot of them, can read every stored credential. Hashing is not possible: Guacamole is the client, and guacd must present the real secret to the RDP/SSH/VNC server, so the value has to be recoverable.
   
   ### Design
   
   - **Opt-in.** With no key configured, behaviour is unchanged.
   - **Cipher:** AES-256-GCM from the JDK (no new dependency), 96-bit random IV, 128-bit tag, stored as `base64(IV ‖ ciphertext ‖ tag)`. The associated data `owner-type ‖ owner-id ‖ parameter-name` binds each value to its row.
   - **What is secret:** every `PASSWORD` field in any loaded protocol descriptor, plus `private-key`, `sftp-private-key`, `client-key`, plus configured extra names. Non-secret parameters (hostname, port, …) stay plain text.
   - **Schema:** nullable `parameter_encryption` column (key ID; NULL = plain text) on `guacamole_connection_parameter` and `guacamole_sharing_profile_parameter`; `parameter_value` widened to `varchar(8192)` / `nvarchar(max)` so an encrypted 4096-bit PEM key fits. No backfill. Plain-text secrets over 6,000 bytes are rejected on save.
   - **Writes:** `ConnectionService` and `SharingProfileService` encrypt secret values before insert, with a fresh IV and the current key on every save.
   - **Reads:** the tunnel path (`AbstractGuacamoleTunnelService`) fails the session with `GuacamoleServerException` if a value cannot be decrypted, so ciphertext never reaches guacd. The REST/admin path omits that value and logs an ERROR. Messages name the owner, parameter and key ID only, never a value or key.
   - **Startup sweep:** a background thread (never delays startup) encrypts existing plain-text secrets, re-keys rows on non-current keys, or decrypts everything in `decrypt` mode. Each row is updated with a compare-and-set `UPDATE`, so several nodes or a concurrent admin save are safe without locking. With encryption disabled it only counts encrypted rows and logs an ERROR if any exist.
   - **Fail fast:** an invalid keyring entry (reported by position, never by its text), a key that is not 32 bytes, a current key missing from the keyring, or encryption properties set without a keyring stop the extension at load. Misconfiguration never silently falls back to plain text.
   - **Key provider:** keys come from a small `ParameterKeyProvider` interface, loaded through `ServiceLoader`. The built-in `property` provider reads the keyring from `guacamole.properties` / environment variables.
   
   ### Properties
   
   `<db>` is `postgresql`, `mysql` or `sqlserver`; each has the usual environment-variable form.
   
   | Property | Default | Meaning |
   |---|---|---|
   | `<db>-parameter-encryption-keys` | unset (feature off) | `kid:base64key[,kid:base64key…]`, each key 32 bytes (`openssl rand -base64 32`). |
   | `<db>-parameter-encryption-current-key` | required if keys set | Key ID used for new encryption. |
   | `<db>-parameter-encryption-provider` | `property` | Key provider name. |
   | `<db>-parameter-encryption-mode` | `encrypt` | `encrypt` or `decrypt`. |
   | `<db>-parameter-encryption-extra-names` | unset | Extra parameter names to treat as secret. |
   
   ### Upgrade
   
   Adds `schema/upgrade/upgrade-pre-1.6.1.sql` for each database (and folds the change into `001-create-schema.sql`). The upgrade is required for this version even with encryption disabled, because the mappers read and write the new column. It is additive; older versions ignore the column.
   
   Note: the open PRs #1254 and #1255 also add `upgrade-pre-1.6.1.sql`. Whichever merges later should append its section to the existing file; I am happy to rebase.
   
   Backups taken before the sweep still contain plain text, so protection of backups is complete only once those have expired.
   
   ### Rotation and downgrade
   
   - **Rotation, two phases:** (1) add the new key to the keyring on every node, keeping `current-key`; (2) switch `current-key` on every node. The sweep re-keys every row on a non-current key and logs `remaining on non-current keys 0` when done; only then remove the old key. Keep retired keys archived for as long as backups that need them exist.
   - **Downgrade:** run with `mode=decrypt` (keyring still configured) until the sweep logs `remaining encrypted 0`, then deploy the older version. Otherwise an older version would read ciphertext as the password, and saving such a connection there would lose the real secret.
   
   ### Testing
   
   - 51 new JUnit 5 tests in `guacamole-auth-jdbc-base` (`ParameterCipherTest` 10, `KeyringTest` 8, `PropertyParameterKeyProviderTest` 3, `ParameterKeyProviderFactoryTest` 5, `SecretParameterPolicyTest` 3, `ParameterEncryptionServiceTest` 8, `TunnelDecryptFailureTest` 1, `ParameterEncryptionSweepTest` 13), covering tampering, owner/name binding, wrong keys, size limits, configuration validation, rotation, decrypt mode, concurrent sweeps and racing admin writes. JUnit 5 test dependencies were added to `guacamole-auth-jdbc-base`, which had no tests before.
   - `docker build` of this branch: `BUILD SUCCESS` with all module tests passing.
   - Smoke test of this branch on PostgreSQL 16: applied `main`'s `001`/`002` schema, then the new `upgrade-pre-1.6.1.sql`, started with a fresh key, created an SSH connection with a password through REST. The row was stored encrypted with key ID `k1` (other parameters plain), the REST API returned the original value, and an SSH session through guacd succeeded (`SSH connection successful`). No key or password appeared in the web application log.
   - End-to-end on PostgreSQL 16, MySQL 8.0 and SQL Server 2022 was run on the equivalent commits in a downstream fork (same 558 code, different surrounding history; that run predates the last review-fix commits, which are covered by the unit tests and the smoke test above): existing plain-text passwords were encrypted by the sweep, SSH sessions worked with the decrypted value, `decrypt` mode restored plain text and an unmodified build then connected again. On PostgreSQL also rotation, a missing-key failure confined to the affected connection, and a sweep of 600 connections in 0.21 s.
   
   ### AI assistance
   
   This contribution was developed with Claude Code (Claude Opus 5.5) under human direction; commits carry a `Co-Authored-By` trailer. The design was reviewed by a human, and the test runs above were executed and their output reviewed before submission.
   
   🤖 Generated with [Claude Code](https://claude.com/claude-code)
   


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] GUACAMOLE-558: Store secret connection parameters encrypted (JDBC). [guacamole-client]

["necouchman (via GitHub)" <gi...@apache.org>]

necouchman commented on PR #1256:
URL: https://github.com/apache/guacamole-client/pull/1256#issuecomment-6478667265

   @zivshits Thanks for the contribution, here. We've discussed this sort of thing several times in the past, and I'm still of the opinion that this really just implements a means of obfuscation rather than real security - the keys for the encryption are stored in the guacamole.properties file, which means anyone who can read the location of the database and the credentials to access the database can also read the encryption keys.
   
   I'm willing to be convinced otherwise, but why should we enable a mediocre level of protection of the data in the database rather than encourage people _not_ to store secrets in the database, and, instead, to use prompting, secrets managers, etc.?


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: dev-unsubscribe@guacamole.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org