Replace legacy encryption with AES-GCM

This commit is contained in:
Andy Howard
2026-09-16 18:40:32 +01:00
parent 6989c5f9b6
commit a0a5ef970c
13 changed files with 456 additions and 278 deletions
+1
View File
@@ -8,6 +8,7 @@ An optional informational overview of sibling workspaces is available in `../../
- `topics.md` tracks prioritised work, work in progress, and completed work.
- `component-reviews.md` records which projects, paths, and classes have been reviewed.
- `authenticated-encryption-migration.md` proposes the shared versioned authenticated-encryption design and migration sequence.
- `legacy-xamarin.md` explains how the retained Xamarin source is isolated from the supported build.
## Project Trackers
+100
View File
@@ -0,0 +1,100 @@
# Authenticated Encryption Migration
<style>
table th,
table td {
vertical-align: top;
}
</style>
Date reviewed: 2026-09-16
## Implemented Clean-Break Decision
The migration is implemented as a deliberate clean break. Common now writes and reads only the `SACSENC2.` AES-GCM envelope, using a fresh random 12-byte nonce, a 16-byte authentication tag, and required purpose-bound associated data. The nonce is the GCM equivalent of an IV: it is stored with the ciphertext, is not secret, and must never repeat with the same key.
The existing in-code key strings are retained as agreed. `SacsAesGcmCrypter` derives a 256-bit key from its existing string with SHA-256; raw-key consumers continue to supply their existing Base64-encoded keys. Fixed IVs, per-session IV state, CBC operations, and legacy decryption have been removed from active APIs. Invalid, tampered, wrong-key, wrong-purpose, and legacy values fail closed rather than being returned as plaintext.
Existing encrypted cookies, links, session parameters, and service messages are intentionally invalidated. Coupled service clients and servers must be deployed together. Moving keys into protected configuration and adding key identifiers and rotation remains a future hardening task, not part of this compatibility-free change.
Tests cover empty, ASCII, and Unicode round trips; URL-safe envelopes; nonce uniqueness; tampering; wrong keys and purposes; malformed, incomplete, oversized, and legacy input; invalid key sizes; and both shared wrappers.
## Superseded Staged-Migration Proposal
The remainder of this document records the earlier compatibility-preserving proposal for context. Its dual-read, retention-window, key-ring, and staged-rollout requirements were superseded by the clean-break decision above and are not implementation instructions.
## Scope
This document proposed the shared design for migrating the former `SacsAesIvCrypter` API, now replaced by `SacsAesGcmCrypter`, and `SacsEncryptionHelper` from unauthenticated AES-CBC to versioned authenticated encryption. BusinessMgr-specific consumers and retention requirements are recorded in its repository-owned [`docs/encryption-consumers.md`](../../../BusinessMgr/Workspace/docs/encryption-consumers.md).
The recent static-PBKDF2 change deliberately preserved the legacy format. It removed the .NET 10 warning but did not address CBC integrity, fixed password salt, low iteration count, embedded application keys, or key rotation.
## Current Formats
| API | Key material | IV handling | Serialized form | Compatibility concern |
|---|---|---|---|---|
| Former `SacsAesIvCrypter` string API | Password converted to 256-bit key with PBKDF2-SHA256, a fixed eight-byte salt, and 300 iterations | Random 16-byte IV appended to each value | `<base64 ciphertext>;<base64 IV>` | Used for BusinessMgr service request and response traffic. Existing clients and servers must continue to understand the legacy form during a coordinated rollout. |
| `SacsEncryptionHelper` | Caller supplies a Base64-encoded 256-bit key | Caller supplies a 16-byte IV; several BusinessMgr flows reuse a fixed IV | Base64 ciphertext only | Used for cookies, emailed links, and session URLs. The IV and key are external to the value, so the ciphertext is not self-describing and cannot support algorithm or key rotation safely. |
Both formats use AES-CBC without an authentication tag. Successful decryption therefore does not prove that the ciphertext and associated context are authentic.
## Proposed Version 2 Envelope
Use AES-GCM through .NET's `AesGcm` API with a fresh random 12-byte nonce and a 16-byte authentication tag for every encryption operation. Never reuse a nonce with the same key.
The serialized value should be URL-safe and self-describing:
```text
SACSENC2.<key-id>.<kdf-id>.<kdf-parameters>.<base64url nonce>.<base64url ciphertext>.<base64url tag>
```
The exact binary/text encoding should be finalised with test vectors before implementation. Required properties are:
- An unambiguous version marker that cannot be confused with either legacy format.
- A non-secret key identifier so decryptors can select the correct key during rotation.
- Explicit KDF identity and parameters for password-derived keys; raw-key callers use a `none` KDF marker.
- A fresh 12-byte nonce and a 16-byte tag stored with every ciphertext.
- Optional authenticated associated data identifying the application and purpose, such as `BusinessMgr:Control:KeepLoggedIn`, to prevent a valid value being replayed in another context.
- Base64url encoding for cookies and query-string values without additional URL-encoding ambiguity.
Password-based encryption must use a new random salt per value and a separately agreed, benchmarked PBKDF2 iteration count. Legacy fixed-salt/300-iteration derivation remains read-only compatibility code and must not be relabelled as version 2.
## API Shape
Keep legacy methods available for compatibility, but prevent new callers from accidentally creating legacy ciphertext. The implementation should introduce purpose-aware versioned operations along these lines:
```csharp
String EncryptVersioned(String plaintext, EncryptionKey key, String purpose);
Boolean TryDecryptVersioned(String value, EncryptionKeyRing keys, String purpose, out String plaintext, out EncryptionFormat format);
```
`EncryptionFormat` would have distinguished authenticated version 2, the former `SacsAesIvCrypter` format, legacy `SacsEncryptionHelper`, and unencrypted input. Password-based and raw-key entry points could have remained separate internally, but both would have emitted the same versioned envelope rules.
Authentication failures must return a distinct failure result, must not return attacker-controlled plaintext, and must be logged without recording keys, ciphertext, cookies, tokens, or decrypted personal data.
## Rollout Sequence
1. Add envelope parsing, AES-GCM encryption/decryption, deterministic test vectors, tamper tests for every envelope field, wrong-key/purpose tests, and legacy-read tests in Common.
2. Introduce configuration-backed key rings with an active write key and retained read keys. Do not commit production keys.
3. Add dual-read behavior while retaining legacy writes; deploy it to every producer and consumer of a shared format.
4. Switch one consumer category at a time to version 2 writes. New writes use AES-GCM; reads dispatch by the envelope marker and continue to accept the applicable legacy format.
5. Retire legacy reads only after the category-specific retention window and rollout constraints in BusinessMgr's inventory have elapsed.
6. Remove embedded keys and fixed IVs after configured key deployment is verified.
## Required Tests
| Area | Required coverage |
|---|---|
| Envelope | Version parsing, Base64url round trip, empty and maximum expected plaintext, Unicode, unknown versions, missing/extra fields, and size limits before allocation. |
| AES-GCM | Fixed test vector, random nonce uniqueness, modified nonce/ciphertext/tag rejection, wrong key, wrong purpose/associated data, and unsupported platform behavior. |
| Password derivation | Fixed version 2 vector, random salt uniqueness, parameter bounds, wrong password, and exact legacy ciphertext compatibility. |
| Key rotation | Active-key writes, retained-key reads, unknown and retired key identifiers, and no fallback from a failed authenticated value to legacy parsing. |
| Consumers | Legacy and version 2 cookies/links/service messages, expiration handling, mixed-version service rollout, and failure behavior that never treats invalid ciphertext as plaintext. |
## Decisions Required Before Implementation
- Where each application obtains its encryption key ring and how keys are deployed and rotated.
- Whether service request and response traffic can move in one coordinated release or needs explicit capability negotiation.
- The maximum lifetime of login cookies and emailed links, which determines how long legacy reads remain.
- The purpose strings and key separation required for Control, Portal, service requests, service responses, password reset, demo registration, and session parameters.
- The benchmarked PBKDF2 iteration count if password-derived version 2 encryption remains necessary; configured random keys are preferable for server applications.
+2 -2
View File
@@ -25,5 +25,5 @@ Last updated: 2026-09-16
| `SacsStandardBase/Base/Net/SacsHttpClient.cs` | Class | Zstandard dependency upgraded and covered | 2026-09-15 | Retain the loopback response test for future HTTP and ZstdNet updates. | ZstdNet 1.5.7 replaces 1.4.5. An in-process HTTP test exercises the real response path with native Zstandard compression and verifies successful UTF-8 decompression without external network access. |
| `SacsStandardProc/Proc/Excel/SaMsSpreadsheet_OpenXml.cs` | Class | Dependency upgrade complete; implementation remains incomplete | 2026-09-15 | Scope functional completion and dedicated wrapper tests separately if this implementation is to be revived. | DocumentFormat.OpenXml 3.5.1 builds and passes the existing spreadsheet and downstream validation. The Open XML implementation contains numerous pre-existing unsupported or placeholder operations, so this update does not present it as production-complete. |
| `SacsStandardProc/Proc/Excel/SaMsSpreadsheet_POI.cs` | Class | Dependency upgrade complete | 2026-09-12 | Retain the focused spreadsheet suite for future NPOI updates; do not adopt NPOI 2.8.0 without an explicit licensing decision. | NPOI 2.7.6 is paired with an explicit `System.Security.Cryptography.Xml` 10.0.12 reference and NuGet reports no vulnerable packages for `SacsStandardProc`. Nullable date and indexed-colour API changes were adapted. Ten focused tests and all 111 Common tests pass; BusinessMgr builds and its 21 tests pass. |
| `SacsStandardBase/Base/Helpers/SacsEncryptionHelper.cs` and `SacsStandardBase/Base/Data/SacsAesIvCrypter.cs` | Encryption compatibility and hardening | Compatibility warning resolved; architectural action still required | 2026-09-16 | Design a versioned authenticated-encryption format with legacy migration and tamper tests. | Static PBKDF2 now preserves the legacy fixed-salt, 300-iteration SHA-256 derivation without the .NET 10 warning, and fixed legacy ciphertext protects compatibility. Nineteen focused tests cover both helpers. CBC remains unauthenticated, and the password-based format still uses a fixed salt and low iteration count; changing those properties requires a versioned migration rather than an in-place update. |
| `eng/validate.ps1` | Repository validation | Complete; passing | 2026-09-16 | Use as the standard validation entry point and extend it when repository-wide checks change. | Builds serially, runs the 11-test focused SQL/ODBC-provider suite, the 10-test focused NPOI suite, and all 167 Common tests; audits both package-owning projects; verifies changed-file line endings and diffs; then invokes BusinessMgr validation. |
| `SacsAuthenticatedEncryption`, `SacsEncryptionHelper`, and `SacsAesGcmCrypter` | Authenticated-encryption migration | Clean-break migration implemented | 2026-09-16 | Retain the focused suite; later move embedded application keys to protected configuration and add key rotation. | The `SACSENC2.` envelope uses AES-GCM with a fresh 12-byte nonce, a 16-byte tag, and purpose-bound associated data. Existing in-code keys are retained by decision, while CBC, fixed/session IVs, legacy reads, and plaintext fallback are removed. Thirty focused tests pass. See [`authenticated-encryption-migration.md`](authenticated-encryption-migration.md). |
| `eng/validate.ps1` | Repository validation | Complete; passing | 2026-09-16 | Use as the standard validation entry point and extend it when repository-wide checks change. | Builds serially, runs the 11-test focused SQL/ODBC-provider suite, the 10-test focused NPOI suite, and all 178 Common tests; audits both package-owning projects; verifies changed-file line endings and diffs; then invokes BusinessMgr validation. |
+3 -2
View File
@@ -13,7 +13,7 @@ Last updated: 2026-09-16
| Priority | Topic | Date noted | Next step | Notes |
|---|---|---|---|---|
| P1 | Design a versioned authenticated-encryption migration | 2026-09-08 | Inventory persisted and transient ciphertext consumers, define an AES-GCM envelope, and specify legacy-read/new-write migration behavior before implementation. | The compatibility-preserving PBKDF2 warning fix and direct regression coverage are complete. Existing CBC formats are unauthenticated; the password-derived format also uses a fixed salt and 300 iterations. An in-place algorithm change would make existing data unreadable. |
| P2 | Externalise and rotate encryption keys | 2026-09-16 | Move the retained in-code application keys to protected configuration or a secret store, then add key identifiers and a rotation policy in a separately scoped change. | The agreed clean-break AES-GCM migration is complete and intentionally retains the existing key strings. This follow-up is defense-in-depth and requires an operational key-deployment decision. |
| P3 | Review raster image handling and ImageSharp | 2026-09-16 | Retain ImageSharp 3.1.12 for now and revisit the wrapper, licensing, and alternatives as a separately scoped functional review. | Version 3.1.12 has no reported vulnerability and five focused tests cover the current narrow use. Later options are: obtain an eligible free or commercial licence and upgrade to ImageSharp 4.1.2; retain a supported secure 3.x release; migrate to MIT-licensed SkiaSharp, accepting native runtime assets and a wrapper rewrite; or evaluate Magick.NET/ImageMagick as a candidate with broad image-format and processing support but native binaries and a heavier dependency footprint. This review is separate from BusinessMgr's Windows printer integration. |
| P1 | Prevent synchronous and asynchronous execution from overlapping in `SaProcess_Base` | 2026-09-09 | Route `RunProcess()` and `RunAsync()` through one guarded start operation and extend the lifecycle tests. | A synchronous call can currently reuse and dispose the asynchronous worker's task data. |
| P2 | Retain cancellation as a completed process outcome | 2026-09-09 | Store a durable cancellation result until the next process reset and test post-completion state. | `IsInterrupted` currently loses token-based cancellation after task-data cleanup. |
@@ -45,7 +45,8 @@ Last updated: 2026-09-16
| Topic | Date noted | Date started | Date completed | Outcome |
|---|---|---|---|---|
| Remove the obsolete PBKDF2 usage and establish encryption compatibility coverage | 2026-09-10 | 2026-09-16 | 2026-09-16 | Replaced the obsolete `Rfc2898DeriveBytes` constructor with static `Pbkdf2` using the same password, fixed salt, 300 iterations, SHA-256, and key length. Fixed legacy ciphertext proves derived-key and stored-data compatibility. Expanded direct coverage across both encryption helpers, corrected whitespace-only key acceptance, and disposed AES and transform resources. The broader versioned authenticated-encryption migration remains separately tracked. |
| Replace legacy CBC encryption with authenticated encryption | 2026-09-08 | 2026-09-16 | 2026-09-16 | Implemented the `SACSENC2.` AES-GCM envelope with fresh nonces, 16-byte authentication tags, and purpose-bound associated data. Removed active CBC, fixed/session IVs, legacy reads, and plaintext fallback. Retained existing in-code keys by explicit decision. Thirty focused Common tests and BusinessMgr consumer tests cover the new behavior; see [`authenticated-encryption-migration.md`](authenticated-encryption-migration.md). |
| Remove the obsolete PBKDF2 usage and establish encryption compatibility coverage | 2026-09-10 | 2026-09-16 | 2026-09-16 | Replaced the obsolete `Rfc2898DeriveBytes` constructor with static `Pbkdf2` using the same password, fixed salt, 300 iterations, SHA-256, and key length. Fixed legacy ciphertext proved derived-key and stored-data compatibility before the subsequent clean-break authenticated-encryption migration replaced that legacy format. Expanded direct coverage across both encryption helpers, corrected whitespace-only key acceptance, and disposed AES and transform resources. |
| Harden and cover `AssemblyUtil` PE timestamp handling | 2026-09-10 | 2026-09-16 | 2026-09-16 | Replaced the unchecked partial reads and marshalled COFF structure with exact primitive reads, PE-signature validation, and graceful handling of missing, malformed, or truncated files. Added a public `TryGetBuildDateTime` extension while preserving `GetBuildDateTime`, plus five focused tests. The API documentation notes that deterministic-build COFF timestamps need not represent physical build time. |
| Upgrade Microsoft.Data.SqlClient and expand SQL provider coverage | 2026-09-15 | 2026-09-15 | 2026-09-15 | Updated Microsoft.Data.SqlClient from 5.2.3 to 7.0.3. Expanded the database-free provider suite from eight to eleven tests, adding parsed SQL connection security and authentication settings plus complete SQL scalar and typed-null parameter coverage. The tests explicitly verify SqlClient 7's mandatory-encryption option. Full Common and downstream BusinessMgr validation passes. |
| Align System.Data.Odbc with .NET 10 and expand provider coverage | 2026-09-15 | 2026-09-15 | 2026-09-15 | Updated the Common ODBC dependency from 6.0.2 to 10.0.12 and removed six redundant direct references from BusinessMgr consumers. Expanded the database-free provider suite from four to eight tests, covering non-SQL connections, adapter creation, parameter order, scalar types, and typed nulls. The tests exposed and prompted correction of missing explicit `Int64`, `Decimal`, `Boolean`, `Guid`, and string mappings. All 141 Common tests and both BusinessMgr suites pass. |