GithubStar/UA-.NETStandard
1
0
Fork 0
mirror of https://github.com/OPCFoundation/UA-.NETStandard.git synced 2026-10-29 07:07:24 +08:00
Code Issues Packages Projects Releases Wiki Activity
2,874 Commits 77 Branches 105 Tags
master
Commit Graph
2 Commits
Author SHA1 Message Date
romanettandClaude Opus 5.5 9b034f8291 Enable nullable reference types centrally; migrate all unset and annotations-only projects (#4700) 
# Description

Nullable reference types are now enabled once, in `common.props`, and
every project in the repository builds warning-clean with them.

**Configuration**
- `<Nullable>enable</Nullable>` lives in `common.props`. All per-project
`<Nullable>` settings are removed: 178 project files (enable,
annotations, and the `disable` in the three Aot test projects).
- 419 per-file `#nullable enable`/`restore` directives and the 4
`#nullable disable` directives are removed.
- Kept on purpose:
- The 5 `<auto-generated>` schema files (`UANodeSet.cs`, `UA Type
Dictionary.cs`, `OPCBinarySchema.cs`, `SecuredApplication.cs`,
`PInvokeHelper.cs`) keep `#nullable enable`. The compiler ignores the
project setting in generated code, so the directive is required there.
- `tests/Opc.Ua.Interop.LegacyPeer` keeps `disable`. It builds against
the 1.5.378 packages and does not import the repository props.
- `docs/DeveloperGuide.md` and `.github/copilot-instructions.md` now
describe the central setting.

**Migration** (the 43 unset and 13 annotations-only projects, roughly
10k diagnostics)
- **Production code: annotated at the declarations.** Covers
`tools/Opc.Ua.SourceGeneration.Core`, `src/Opc.Ua.Lds.Server` and the
LDS sample.
- Members, parameters and returns are nullable where null actually
flows.
- Null-check helpers carry `[NotNullWhen]` (`IsNull`, `IsValidXmlName`,
`IsMethodTypeDesign`).
- `CoreUtils.Format`, `Utils.Format`, `ITemplateWriter.WriteLine` take
`params object?[]`. `Template.AddReplacement` takes `string?` (rendering
already skips null replacements).
- `IRegisteredServerStore.RegisterAsync` returns
`Task<RegistrationEntry?>`: it returns null when a registration is
removed.
- Dereferences backed by an invariant (for example, the validated model)
use `!`, so runtime behaviour is unchanged.
- **Behavioural changes in production code** (small, deliberate):
- `NodeSetToModelDesign` throws `InvalidOperationException` when the
file is not a NodeSet. Before, it threw a NullReferenceException on the
next line.
- `ModelDesignValidator` starts its lookup tables empty instead of null.
A redundant `if (m_nodes == null) return;` guard is gone.
- **Test code: minimal annotations.**
  - `?` on values that can be null, `!` where the test asserts non-null.
  - `= null!` for fixture members assigned in SetUp.
- `TryGetStructure<T>(out T? x)` where the inferred `T?` broke the
`IEncodeable` constraint.
- A few redundant `== null` checks on structs (`NodeId`, `ArrayOf<T>`)
folded into the adjacent `IsNull`/`Count` checks.

**Notes for reviewers**
- The diff is large (~1,500 files) but almost entirely mechanical: `?`,
`!` and directive removal. The non-annotation changes are listed above.
- Known follow-up, not done here:
`IModelDesign.Namespaces/TargetNamespace/NamespaceUris` stay
non-nullable even though the validator returns null before it loads a
dictionary. Making them nullable would ripple through every generator
call site.

## Related Issues

- Fixes #4439

## Checklist

- [ ] I have signed the
[CLA](https://opcfoundation.org/license/cla/ContributorLicenseAgreementv1.0.pdf)
and read the
[CONTRIBUTING](https://github.com/OPCFoundation/UA-.NETStandard/blob/master/CONTRIBUTING.md)
doc.
- [ ] I have added tests that prove my fix is effective or that my
feature works and increased code coverage.
- [x] I have added all necessary documentation.
- [x] I have verified that my changes do not introduce (new) build or
analyzer warnings.
- [ ] I ran **all** tests locally using the **UA.slnx** solution against
at least .net **framework** and .net **10**, and all passed.
- [ ] I fixed **all** failing and flaky tests in the CI pipelines and
**all** CodeQL warnings.
- [ ] I have addressed **all** PR feedback received.

The "no new warnings" box covers net10.0 and net48: full non-incremental
builds of `UA.slnx`, `tools/SourceGeneration.slnx`,
`tools/MigrationAnalyzer.slnx` and the Aot test projects finished with 0
warnings and 0 errors under the repository's warnings-as-errors
settings. net9.0 and net8.0 also had no nullable diagnostics, but they
were not rebuilt after the final fixes. Tests have not been run locally
yet.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-08 08:48:13 +02:00
romanettandClaude Opus 5.5 920ebda69b Fix transport channel and encryption audit findings (#4626) 
## Summary

Fixes the Critical, High and Medium findings of a read-only audit of the
low-level transport channels (UA-TCP / UASC, HTTPS, WSS, Kestrel TCP,
buffer management) and the encryption layer (key derivation, nonces,
RSA/ECC, EncryptedSecret, security policies). Every finding was traced
in source and checked against the specification (Parts 2, 4, 6, 7)
before fixing.

Scope: 2 Critical, 1 High, 21 Medium → **21 fixed, 3 partial**. Low
findings are not part of this PR.

### Critical / High

**T7-1: HTTPS session isolation (Critical).**
- **Affects:** servers that expose a secured `opc.https` endpoint.
- **Problem:** over HTTPS all clients share one SecureChannel (Part 6
7.4.1), so the session AuthenticationToken is the only thing that keeps
sessions apart. The spec therefore requires it to be at least 32 random
bytes. For secured channels the server issued tokens that are not hard
to predict.
- **Impact:** another client on the same HTTPS endpoint could act within
a session that does not belong to it, with that session's identity and
permissions.
- **Fix:** `SessionManager` now always issues a random 32-byte opaque
token, on every transport (Part 6 7.4.1, Part 4 7.35).

**T1-1: secured client channels could lose their protection
(Critical).**
- **Affects:** opc.tcp clients connecting to Sign or SignAndEncrypt
endpoints.
- **Problem:** the client did not verify that the OpenSecureChannel
response used the security policy and mode it had requested, as Part 4
5.6.2.1 and Part 6 6.7.4 require.
- **Impact:** an attacker in the network path could make a secured
channel continue without message security. The check on the server
signature in CreateSession limits this on first connect. The session
reconnect path has no equivalent check. Traffic of an existing session
could then lose confidentiality and integrity without the application
noticing. Other users of the channel (discovery, registration, custom
service clients) were not protected at all.
- **Fix:** a client channel for a secured endpoint is now bound to the
requested policy and mode. A response with any other policy or mode, or
without the server certificate, faults the channel, on connect and on
reconnect.

**T8-1: Kestrel opc.tcp listener availability (High).**
- **Affects:** servers using `KestrelTcpTransportListener`.
- **Problem:** the listener did not expire idle channels and did not
reclaim unused ones when it reached `MaxChannelCount`, unlike the
classic listener. Part 4 5.6.2.1 requires closing the oldest unused
channel without a session before refusing new ones.
- **Impact:** an unauthenticated peer could exhaust the channel slots
and deny service to legitimate clients.
- **Fix:** activity tracking, an inactivity sweep based on
ChannelLifetime, and reclaiming the least recently used channel without
a session at the limit.

**E2-1: user token decryption errors (Medium, High where lockout is
disabled).**
- **Problem:** the result of a failed user identity token decryption
could reveal more than Part 4 7.40.2.1 allows, which says every
decryption error must look like `Bad_IdentityTokenInvalid`.
- **Mitigation already in place:** the default failed-authentication
lockout limits how this can be used.
- **Fix:** clients now receive only `Bad_IdentityTokenInvalid`, with the
detail logged on the server. Token decryption takes the same steps
whichever check fails.

### Medium
- **Key derivation / sequence numbers:** E1-1/T2-1 renewal XORs the
previous secret only for SecureChannelEnhancements policies (Part 6
6.8.1 Step 2); T2-2 Sign-mode HKDF length uses enc key / IV length 0
(Part 6 6.8.1); T2-3 server chains a second renewal from the pending
renewed token like the client; T2-4 receiver rejects sequence-number
gaps (Part 2 5.1.14).
- **OPN / client channel:** T1-2 (partial) OPN sender certificate
checked against the policy's key family / curve and RSA key-length
window (Part 4 6.1.3, Part 6 6.1); T3-1 renewal completion no longer
blocks on a newer handshake while holding the gate; T3-9 reverse-connect
listener bounded (1000 pending, 500/s, burst 1000).
- **TCP listener / buffers:** T5-1 Basic128Rsa15 abuse tracker fails
open for untracked IPs when full; T5-2 refused connections get
`Bad_TcpNotEnoughResources` after the HEL (Part 6 7.1.2.3); T6-2
(partial) idle sockets no longer hold a receive buffer; T6-3
`AsyncResultBase` completes exactly once, reverse-connect timeout
cancels the connect.
- **HTTPS:** T7-3 chunked request bodies supported (Part 6 7.4.1); T7-4
mTLS CreateSession compares only the leaf certificate and answers a
mismatch with a ServiceFault; T7-5 (partial) opcua+openapi WSS path gets
the discovery-only gate; T7-6 default rate limiter partitioned per
client IP; T7-7 shared Kestrel host owns its TLS certificate and
rotation rebuilds it for all listeners.
- **WSS / Kestrel TCP:** T8-2 SecureChannel and certificate audit events
(Part 4 6.5.5); T8-3 queued senders fail with BadConnectionClosed on
close instead of hanging; T8-4 WSS clients keep the mTLS client
certificate alive; T8-5 WSS+JSON close bounded by timeout and
cancellation.

### Partial / not fixed
- T1-2: missing CertificateSignatureAlgorithm check per chain
certificate, issuer key length ≥ issued key length, and checks on ECC
issuers.
- T6-2: `BufferManagerMemoryLimiter` still blocks the calling thread
when real traffic exhausts the budget (needs an async TakeBuffer API).
- T7-5: the fallback to any SecurityMode=None endpoint stays, because
HTTPS listeners advertise `HttpsOpenApiTransport`.
- Not covered: the HTTPS listener's WSS uacp channels likely need the
same idle cleanup / reclaim as T8-1.
- Not covered: the certificate/policy key check (T1-2) applies to UASC
channels; the opc.https session path is a follow-up.

## Behaviour changes reviewers should look at

**Wire-incompatible with older versions of this SDK (both conform to the
spec now):**
1. Renewals on ECC_nistP256/P384, ECC_brainpoolP256r1/P384r1,
ECC_curve25519/448 no longer mix in the previous secret; a renewal
between this and an older SDK fails on the first message after renewal.
2. Sign-mode channels on those CBC+HMAC ECC policies derive different
keys; Sign-mode channels to older SDKs fail.

**Other:**
3. Every session AuthenticationToken is a 32-byte opaque ByteString
NodeId, on every transport.
4. Any sequence-number gap faults the channel with
`BadSequenceNumberInvalid` (client side previously reported
`BadSecurityChecksFailed`). Only a reconnect OpenSecureChannel may skip
ahead.
5. OPN peer certificates outside the policy's key-length window or with
the wrong key family/curve are rejected (e.g. 4096-bit RSA on Basic256 /
Basic128Rsa15).
6. Refused opc.tcp connections get an ERR after their Hello; a refused
socket can stay open up to 2 s waiting for it.
7. Kestrel opc.tcp listener closes idle channels and, at
MaxChannelCount, the oldest unused channel (BadNoCommunication) instead
of refusing the new client.
8. HTTPS mTLS CreateSession mismatch → ServiceFault
`Bad_SecurityChecksFailed` instead of HTTP 401; oversized HTTPS bodies →
413 instead of 500; default HTTPS rate limit is 100 req/s per client IP.
9. Certificate rotation on a shared (host, port) restarts the shared
Kestrel host once; TLS connections on that port reconnect.
10. `EncryptedSecret.TryDecrypt/TryDecryptAsync` return false on the RSA
branch instead of throwing (public API behaviour); `TryDecryptRsa`
reports `Bad_SecurityChecksFailed` for padding/layout/signature
failures. A KeyData whose encrypted length differs from what the policy
and receiver key imply is rejected the same way, before decryption.
11. Client reverse-connect listener limits pending connections and
connection rate; at its cap the oldest connection that has not sent a
ReverseHello is closed to make room. On opc.wss the cap applies only to
connections not yet handed off.
12. `ApplicationInstance` creates its own RSA application certificate
with at least 2048 bits (except the RsaMin type), even when
`MinimumCertificateKeySize` is lower; that setting only bounds accepted
peers.
13. Kestrel opc.tcp listener: `ChannelLifetime` <= 0 disables the idle
sweep; otherwise the sweep runs at most once per second.
14. Certificate rotation on a shared Kestrel host closes the listener's
WebSocket connections and outbound reverse-connect channels even when
the TLS certificate is unchanged; if the new host fails to start, a host
with the previous TLS certificate is restored and the error is rethrown.
Listeners on the same (host, port) share a Kestrel host only when their
host-level settings (mutual TLS, admission limits, middleware) match;
otherwise the listener uses its own host, as for a TLS certificate
mismatch.
15. On a second renewal before the first renewed token was used, the
server makes the first renewed token current (raising `TokenActivated`
then) instead of dropping it; a reconnect chains from the current token.
16. Sign-only HKDF channels report empty IV/key plus the signing key
length in channel diagnostics and the Wireshark keyset.
17. `UserNameIdentityTokenHandler.DecryptAsync` throws the specific
RSAEncryptedSecret failure; the server still answers clients with a
plain `Bad_IdentityTokenInvalid` and logs the cause.
18. Refused ReverseHello messages up to the maximum valid size also get
the `Bad_TcpNotEnoughResources` reply; `TcpByteTransport` rents receive
buffers at the chunk size.

## Tests

New regression tests for every Critical/High fix fail before the fix.
Tests that pinned old behaviour were updated:
`TcpReconnectKeyAgreementTests`,
`UaSCBinaryChannelSequenceRolloverTests`,
`TcpMessageFailureRegressionTests`,
`TcpTransportListenerDeterministicTests`.

Combined branch, net10.0:

| Suite | Passed | Skipped | Failed |
|---|---|---|---|
| Opc.Ua.Core.Tests | 6036 | 88 | 0 |
| Opc.Ua.Bindings.Https.WebApi.Tests | 405 | 0 | 0 |
| Opc.Ua.Server.Tests | 7806 | 5 | 0 |
| Opc.Ua.Client.Tests | 2733 | 0 | 0 |
| Opc.Ua.Gds.Tests | 1234 | 56 | 0 |
| Opc.Ua.Sessions.Tests | 790 | 379 | 0 |

net48 builds of the Core, Server, Client and WebApi test projects and
the netstandard2.1 Release build: 0 errors, no new warnings. One
telemetry test (`ActivityTraceFlags_Metrics_ShouldRecordMetricsOnly`)
failed once under parallel load and passes on rerun.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
 2026-10-02 12:08:16 +02:00