SHIP Pairing Implementation and API redesign#66
Open
DerAndereAndi wants to merge 20 commits intodevfrom
Open
Conversation
Co-Authored-By: Mostafa Abdelfatah <Mostafa.Abdelfattah@coretech-innovations.com>
…plete Co-Authored-By: Mostafa Abdelfatah <Mostafa.Abdelfattah@coretech-innovations.com>
- enforce devA-side forPar matching and runtime trustCurve support checks - require devZ target trust before StartAnnouncementTo and validate target secret bound - harden _shippairing TXT ingestion (enum validation + nonce/digest format checks) - normalize pairing secret length handling across config/listener/announcer paths - add and update TDD coverage for listener, hub, mdns and secret boundary cases
- [HubInterface] Add SHIP Pairing Service APIs and other missing APIs that were not covered by the interface. - [SHIP Pairing Interfaces & Implementation]: Remove dead code and re-evaluate some parameters/fields types. - [Announcement Lifetime Tracker]: changed timers map to take lifetimeTimer as a value instead of a pointer reference. + Improve StopAll() - [Hub] GeneratePairingQR(): Remove secret key parameter - it is fetched internally instead - Update mocks based on updated APIs
1. In multiple instances, a new instance of ServiceIdentity is created from SKIToServiceIdentity(), which is incorrect because it creates a ServiceIdentity object with SKI only. The correct way is to get the ServiceIdentity from the ServiceDetails itself, unless ServiceDetails does not exist. 2. In ReportServiceShipID(), we do not need to call RemoteServiceConnected again, as this gets called after the handshake process is completed.
kirollosnct
force-pushed
the
feature/shippairing-2
branch
from
197ec1b to
a59d876
Compare
…on initiation While iterating through discovered services on mDNS, the SKI would be used to search the ServiceDetails for a trust, which would trigger a connection attempt. In some cases, during a SHIP Pairing process, devZ can add to its trust store received information from a devA (SHIP ID and Fingerprint). This commit will look for the SHIP ID if the SKI was not found for this entry, on a condition that the fingerprint is present.
kirollosnct
force-pushed
the
feature/shippairing-2
branch
3 times, most recently
from
52b5f20 to
27a8423
Compare
…e correct instanceID
kirollosnct
force-pushed
the
feature/shippairing-2
branch
from
27a8423 to
2b11574
Compare
sthelen-enqs
requested changes
Mar 30, 2026
Contributor
sthelen-enqs
left a comment
sthelen-enqs
left a comment
There was a problem hiding this comment.
I've checked over the first few files and found a couple of minor nits and some other small changes ad improvements we could make here.
I'll continue looking over the rest of this PR, but wanted to post these comments so they aren't blocked until I finish the whole review.
| // Used by devA only. | ||
| // | ||
| // Returns: the fingerprint and ShipID of any trusted AddCu device, respectively. Or empty string if none | ||
| HasTrustedAddCuDevice() (string, string) |
Contributor
There was a problem hiding this comment.
I would normally expect a Has.*() function to only return bool, could we change this to GetTrustedAddCuDevice() instead?
I also don't love the idea of returning 2 strings here and expecting the user to know which is the fingerprint and which is the ShipID. Can we return the ServiceDetail here directly instead?
That way a user could call ShipID() or Fingerprint() on it to get the information they want.
| // a service was paired using SHIP Pairing. | ||
| // | ||
| // Note: This needs to be persisted in the applications trust store and provided with | ||
| // RegisterRemoveService call if known! |
Contributor
There was a problem hiding this comment.
Suggested change
| // RegisterRemoveService call if known! | |
| // RegisterRemoteService call if known! |
| // It will be provided instead of SKI when a service is paired using SHIP Pairing. | ||
| // | ||
| // Note: This needs to be persisted in the applications trust store and provided with | ||
| // RegisterRemoveService call if known! |
Contributor
There was a problem hiding this comment.
Suggested change
| // RegisterRemoveService call if known! | |
| // RegisterRemoteService call if known! |
| // a service was paired using SHIP Pairing. | ||
| // | ||
| // Note: This needs to be persisted in the applications trust store and provided with | ||
| // RegisterRemoveService call if known! |
Contributor
There was a problem hiding this comment.
Suggested change
| // RegisterRemoveService call if known! | |
| // RegisterRemoteService call if known! |
| // paired using SHIP Pairing this will be set to PairingTypeAddCu (1) | ||
| // | ||
| // Note: This needs to be persisted in the applications trust store and provided with | ||
| // RegisterRemoveService call! |
Contributor
There was a problem hiding this comment.
Suggested change
| // RegisterRemoveService call! | |
| // RegisterRemoteService call! |
Comment on lines
+193
to
+195
| // | ||
| // This interface replaces PairingHistoryProviderInterface to simplify application implementation | ||
| // by moving ring buffer algorithm complexity into the library where it belongs. |
Contributor
There was a problem hiding this comment.
Suggested change
| // | |
| // This interface replaces PairingHistoryProviderInterface to simplify application implementation | |
| // by moving ring buffer algorithm complexity into the library where it belongs. |
see below
Comment on lines
+247
to
+284
| // PairingHistoryProviderInterface - DEPRECATED: Use RingBufferPersistence instead | ||
| // This interface will be removed in a future version. Applications should migrate to | ||
| // RingBufferPersistence which simplifies implementation by handling ring buffer logic | ||
| // in the library rather than requiring applications to implement it. | ||
| // | ||
| // Applications implement this to provide persistent storage for SHIP pairing history | ||
| type PairingHistoryProviderInterface interface { | ||
| // HasSeenDigest checks if an HMAC digest has been used in a previous pairing attempt. | ||
| // This implements replay attack protection as required by SHIP Pairing Service | ||
| // specification section 9.2. Applications must maintain a history of seen digests | ||
| // to prevent attackers from reusing captured pairing requests. | ||
| // | ||
| // Parameters: | ||
| // - alg: The HMAC algorithm used (typically "hmacSha256") | ||
| // - digest: The hex-encoded HMAC digest to check | ||
| // | ||
| // Returns: | ||
| // - true if this digest has been seen before (potential replay attack) | ||
| // - false if this is a new, previously unseen digest | ||
| // | ||
| // Implementation note: Applications should use efficient storage (e.g., hash set) | ||
| // and implement ring buffer behavior per SHIP spec section 11 | ||
| HasSeenDigest(alg, digest string) bool | ||
|
|
||
| // RecordPairing stores a successful pairing's HMAC digest for replay protection. | ||
| // This implements the digest history requirement from SHIP Pairing Service | ||
| // specification section 11. Applications must maintain a ring buffer of recent | ||
| // successful pairings to prevent digest reuse. | ||
| // | ||
| // Parameters: | ||
| // - alg: The HMAC algorithm used (typically "hmacSha256") | ||
| // - digest: The hex-encoded HMAC digest from the successful pairing | ||
| // | ||
| // Implementation note: Applications should implement ring buffer behavior | ||
| // to limit memory usage while maintaining sufficient history for security | ||
| RecordPairing(alg, digest string) | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
Why are we adding a new API that's already deprecated? Can't we just delete this so there's only one API? There shouldn't be any consumers of this
Suggested change
| // PairingHistoryProviderInterface - DEPRECATED: Use RingBufferPersistence instead | |
| // This interface will be removed in a future version. Applications should migrate to | |
| // RingBufferPersistence which simplifies implementation by handling ring buffer logic | |
| // in the library rather than requiring applications to implement it. | |
| // | |
| // Applications implement this to provide persistent storage for SHIP pairing history | |
| type PairingHistoryProviderInterface interface { | |
| // HasSeenDigest checks if an HMAC digest has been used in a previous pairing attempt. | |
| // This implements replay attack protection as required by SHIP Pairing Service | |
| // specification section 9.2. Applications must maintain a history of seen digests | |
| // to prevent attackers from reusing captured pairing requests. | |
| // | |
| // Parameters: | |
| // - alg: The HMAC algorithm used (typically "hmacSha256") | |
| // - digest: The hex-encoded HMAC digest to check | |
| // | |
| // Returns: | |
| // - true if this digest has been seen before (potential replay attack) | |
| // - false if this is a new, previously unseen digest | |
| // | |
| // Implementation note: Applications should use efficient storage (e.g., hash set) | |
| // and implement ring buffer behavior per SHIP spec section 11 | |
| HasSeenDigest(alg, digest string) bool | |
| // RecordPairing stores a successful pairing's HMAC digest for replay protection. | |
| // This implements the digest history requirement from SHIP Pairing Service | |
| // specification section 11. Applications must maintain a ring buffer of recent | |
| // successful pairings to prevent digest reuse. | |
| // | |
| // Parameters: | |
| // - alg: The HMAC algorithm used (typically "hmacSha256") | |
| // - digest: The hex-encoded HMAC digest from the successful pairing | |
| // | |
| // Implementation note: Applications should implement ring buffer behavior | |
| // to limit memory usage while maintaining sufficient history for security | |
| RecordPairing(alg, digest string) | |
| } |
Comment on lines
+391
to
+397
| // IsValidLength reports whether the secret length is acceptable for SHIP pairing. | ||
| // This implementation supports: | ||
| // - 16 bytes (raw 128-bit secret) | ||
| // - 32 bytes (commonly used textual/encoded representation) | ||
| func (s PairingSecret) IsValidLength() bool { | ||
| return len(s) == 16 || len(s) == 32 | ||
| } |
Contributor
There was a problem hiding this comment.
Are we allowing PairingSecrets both in hex as well as raw byte form without differentiating between the two? This feels like it can only go wrong. I can't find any code in the HMAC calculation that differentiates between the two forms and it looks like it assumes it will always get the byte form.
If my understanding is correct we should drop this to only check for len(s) == 16
Suggested change
| // IsValidLength reports whether the secret length is acceptable for SHIP pairing. | |
| // This implementation supports: | |
| // - 16 bytes (raw 128-bit secret) | |
| // - 32 bytes (commonly used textual/encoded representation) | |
| func (s PairingSecret) IsValidLength() bool { | |
| return len(s) == 16 || len(s) == 32 | |
| } | |
| // IsValidLength reports whether the secret length is acceptable for SHIP pairing. | |
| // This implementation supports: | |
| // - 16 bytes (raw 128-bit secret) | |
| func (s PairingSecret) IsValidLength() bool { | |
| return len(s) == 16 | |
| } |
Comment on lines
+169
to
+170
| // ServiceConnectionStateChanged method removed - this was not part of HubReaderInterface | ||
| // Connection state updates are handled through ServicePairingDetailUpdate |
Contributor
There was a problem hiding this comment.
Suggested change
| // ServiceConnectionStateChanged method removed - this was not part of HubReaderInterface | |
| // Connection state updates are handled through ServicePairingDetailUpdate |
| return false | ||
| } | ||
|
|
||
| return t.pairedDeviceShipID == shipID && t.pairedDeviceShipID != "" |
Contributor
There was a problem hiding this comment.
Redundant check for empty string. t.pairedDeviceShipID will never be "" here since it must be equal to shipID and shipID was checked to not be ""
Either of t.pairedDeviceShipID != "" or shipId == "" can be removed.
kirollosnct
force-pushed
the
feature/shippairing-2
branch
from
072c7f5 to
b8bc14a
Compare
kirollosnct
force-pushed
the
feature/shippairing-2
branch
2 times, most recently
from
dee9ed6 to
ad19726
Compare
kirollosnct
force-pushed
the
feature/shippairing-2
branch
from
ad19726 to
1aef1cf
Compare
Member
|
Thank you @sthelen-enqs for your review and recommendations. Can you please check my latest changes? For the PairingHistoryProviderInterface , I agree that it should neither be marked as a deprecated interface nor exposed for application use. I moved the interface into the pairing package because the interface is used by the pairing struct and also needed for mocking for the unit tests. What do you think about that? |
CreateListener and CreateAnnouncer signature changed to take no parameter, the methods internally will fetch a copy of ServiceDetails instance and pass it along to the listener/announcer instances.
Replace the provider-instance-ID-as-key approach in pairingInstances with a new announcedPairing struct and announcedPairings map. AnnouncePairingService now returns a stable logical ID that callers hold permanently; the internal providerID is updated transparently on interface-change re-announcements without invalidating caller-held IDs.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…pairing - Hub pairing tests now pass real SKI/fingerprint/shipID to NewServiceDetails instead of empty strings, to be able to create a ServiceDetails. - pairing.NewService nil-checks the result of api.NewServiceDetails before storing it, surfacing a clear error instead of a later nil panic
NewServiceDetails previously returned nil on invalid input (empty SKI and fingerprint), making it easy for callers to silently propagate a nil *ServiceDetails and panic at the use site. Change the signature to (*ServiceDetails, error) so callers must handle the error explicitly. Update all call sites across api/, hub/, pairing/, and examples/ to unpack the two return values. Also nil-guard hub/hub_pairing.go so that an invalid-identity path logs and returns cleanly instead of panicking.
Replace `defer t.mutex.Unlock()` with an explicit unlock before calling the timeout callback. sync.RWMutex is not reentrant — holding the write lock while onTimeout re-enters the tracker (e.g. IsInReplacementWindow) caused a self-deadlock in the timer goroutine.
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
4 participants
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.
Implements SHIP Pairing Service specification including automatic device pairing, HMAC-based authentication, and Device Replacement Timing Logic for AddCu devices.
BREAKING CHANGE: ServiceIdentity is now used instead of ServiceDetails, or manual SKI, fingerprint, shipID arguments
Key components:
Architecture improvements:
Security features:
Specification compliance: