fix(binding-opcua) opcua security #1401 - #1552
Open
erossignon wants to merge 3 commits into
Open
Conversation
setSecurity() ignored any scheme it did not recognise and returned true, so the client connected with its defaults - no encryption, no user identity - and told the caller it had worked. The same held inside the resolvers, where an unknown messageMode became None and an unknown tokenType became Anonymous. Anonymous is the right default for an identity that was never requested; it is the wrong answer for one we failed to recognise. A scheme in the uav: namespace that we cannot honour is now an error. Schemes belonging to other bindings are still ignored, as before. Refs eclipse-thingweb#1401
Renames the channel security scheme and the property names of both schemes to the spellings published in OPC 10101 "OPC UA for WoT Binding" v1.00, and documents them - the binding had no security documentation at all before. The old names are removed rather than aliased, but not simply dropped: they raise an error naming their replacement, since the previous commit made an unrecognised uav: scheme fatal. Also closes the gaps found reading 6.3.3 line by line: IssuedToken is recognised and refused instead of falling back to Anonymous, securityPolicy "None" is accepted alongside securityMode "None" as the spec marks both terms required, and the full policy URI now resolves. BREAKING CHANGE: see the migration table in the README. Refs eclipse-thingweb#1401
OPC 10101 6.3.2 and 6.3.3 both state that credentials are not shared in thing descriptions and must be supplied separately; the spec examples declare only uav:userIdentityToken. The schemes no longer carry userName, password, certificate or privateKey - they come from servient.addCredentials(), keyed by the thing id, which setSecurity already received and never read. Credentials arrive in two shapes: an array for form-level security, a single object for thing-level, so both are accepted. A scheme whose credentials are missing is refused rather than downgraded to an anonymous session. Adds a narrated end-to-end test that asks the server which session it granted, so the assertions are about what reached the wire rather than what was asked for, plus a runnable example. BREAKING CHANGE: a thing description carrying credentials must move them to the servient. Refs eclipse-thingweb#1401
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the OPC UA security schemes in three steps: stop failing open, adopt the vocabularypublished in OPC 10101 v1.00 §6.3, and take credentials out of the Thing Description.
Discussion and the decision to take the naming as a clean break rather than an alias: #1401.
Why
A Thing Description written to the published spec did not connect the way it asked to, and said
nothing about it.
setSecurity()ignored schemes it did not recognise and returnedtrue, sothe client fell back to
MessageSecurityMode.None/SecurityPolicy.None/ anonymous and reportedsuccess. Nothing downstream — application or test — could detect it.
The same shape appeared twice more inside the resolvers:
IssuedTokenis a value OPC 10101 §6.3.3 defines, so a TD copied out of the spec's own OAuth2example landed here and connected anonymously. An unexpected
uav:securityModewas likewisedowngraded to
None.Anonymous is the correct default for an identity that was never requested. It is the wrong answer
for one we failed to recognise: the author asked for something and got silence.
The commits
1.
refuse security schemes we do not understandA scheme in the
uav:namespace that we cannot honour is now an error, as are an unknownmessageModeand an unknowntokenType. Schemes belonging to other bindings are still ignored,as before — that part of the old behaviour was correct.
Written against the current vocabulary, so it is independent of the rename below and can be
cherry-picked on its own.
2.
align security vocabulary with OPC 10101 v1.00— breakingAdopts the published names. The old ones are removed rather than aliased; because commit 1 made an
unrecognised
uav:scheme fatal, they raise an error naming their replacement instead of silentlydegrading.
Also closes what a line-by-line read of §6.3 turned up:
IssuedTokenis recognised and refused explicitly (node-opcua cannot do issued tokens — seeFollow-ups), instead of falling through to
Anonymous.uav:securityPolicyis marked required in §6.3.3 and admitsNone; our types forbade itwhen the mode was
None. Now accepted, andNoneis refused when the mode asks for security.the short names and its values are the URIs. Both forms now resolve.
Adds a security section to the binding README. There was none before, so this is new material
rather than an edit, and it covers the documentation checkbox on #1401.
3.
take credentials from the credential store— breaking§6.3.2 and §6.3.3 both state that credentials "are not shared in WoT Thing Descriptions and must be
provided separately, e.g., through a separate credential store"; every example in the spec declares
uav:userIdentityTokenand nothing more.A Thing Description is published, served over the network and committed to repositories by design,
so it is the one artefact where a password must not be written. The inline form is removed rather
than deprecated, because a deprecated path here is still a working way to leak a password.
The machinery already existed:
Servient.addCredentials()is there andsetSecurity()alreadyreceived the credentials — the binding never read them. Credentials arrive in two shapes (an array
via
retrieveCredentials()for form-level security, a single object viagetCredentials()forthing-level); both are accepted, and there are tests for each so a future core cleanup cannot break
one silently. A scheme whose credentials are missing is refused, not downgraded.
Breaking changes
Security vocabulary:
"scheme": "uav:channel-security""scheme": "uav:channelsec""messageMode": "none" / "sign" / "sign_encrypt""uav:securityMode": "None" / "Sign" / "SignAndEncrypt""policy": "...""uav:securityPolicy": "...""tokenType": "anonymous" / "username" / "certificate""uav:userIdentityToken": "Anonymous" / "UserName" / "Certificate""scheme": "uav:authentication"Credentials move from the Thing Description to the servient:
Credentials are keyed by thing id, so a TD that did not declare one now needs it.
Both migrations are mechanical, and every removed spelling produces an error naming its
replacement. A survey of the OPC UA TDs reachable from here — this repo's examples, both TD
generators in
node-wot-opcua-tools, andopcua-wot-binding— found all of them onnosec, andnone carrying credentials.
Deliberate extensions
Labelled as such in the README, so they are not mistaken for spec:
Basic128,Basic192,Basic192Rsa15,Basic256Rsa15)combowithoneOf, where the first alternative wins — the spec only usesallOfTesting
Full suite green at each of the three commits taken in isolation (77 / 82 / 97).
test/opcua-security-e2e-test.tsis new: a narrated tour against a real OPC UA server, whichconnects and then asks the server which session it granted, so the assertions are about what
reached the wire rather than what the TD requested — the distinction the original bug hid behind.
packages/examples/src/bindings/opcua/demo-opcua-secure.tsis a runnable version.Follow-ups, not in this PR
report a problem in every binding. Raised on Security for OPC UA #1401, better settled in core than here.
uav:issueToken/IssuedTokenneeds upstream work: node-opcua's client-facingUserIdentityInfounion has no issued-token variant, so there is nothing for a binding to callinto. Refused explicitly until then.