Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

#211 Namespace validation #215

Merged
merged 15 commits into from
May 19, 2022
Merged

#211 Namespace validation #215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
9ede85b
Add validation test cases
May 17, 2022
cf06ab2
Implemented namespace validation
May 17, 2022
f0115d4
Removed previous namespace code
May 17, 2022
24360fc
Removed previous namespace code
May 17, 2022
c107293
Added peer validation matching test cases
May 17, 2022
31e514c
Implemented namespace peer validation
May 17, 2022
d04d0a9
Added approver side namespace checks
May 17, 2022
f858656
Added proposer side namespace checks on approval
May 17, 2022
2ff4871
Resolve merge conflict
May 17, 2022
f084808
Removed leftover unnecessary check
May 17, 2022
7b06353
Moved namespace validation when proposal is received
May 19, 2022
b954a02
Remove unnecessary todo
May 19, 2022
2bd7fd7
Merge callbacks on proposal response
May 19, 2022
90f7b60
Setting proposal to nil after using it on settlement
May 19, 2022
eedabee
Resolved merge conflict
May 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions .swiftpm/xcode/xcshareddata/xcschemes/WalletConnect.xcscheme
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,34 @@
ReferencedContainer = "container:">
</BuildableReference>
</BuildActionEntry>
<BuildActionEntry
buildForTesting = "YES"
buildForRunning = "YES"
buildForProfiling = "YES"
buildForArchiving = "YES"
buildForAnalyzing = "YES">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "WalletConnectAuth"
BuildableName = "WalletConnectAuth"
BlueprintName = "WalletConnectAuth"
ReferencedContainer = "container:">
</BuildableReference>
</BuildActionEntry>
<BuildActionEntry
buildForTesting = "YES"
buildForRunning = "YES"
buildForProfiling = "YES"
buildForArchiving = "YES"
buildForAnalyzing = "YES">
<BuildableReference
BuildableIdentifier = "primary"
BlueprintIdentifier = "WalletConnectRelay"
BuildableName = "WalletConnectRelay"
BlueprintName = "WalletConnectRelay"
ReferencedContainer = "container:">
</BuildableReference>
</BuildActionEntry>
</BuildActionEntries>
</BuildAction>
<TestAction
Expand Down
5 changes: 3 additions & 2 deletions Sources/WalletConnectAuth/AuthClient.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,7 +160,7 @@ public final class AuthClient {
namespaces: [String: SessionNamespace]
) throws {
//TODO - accounts should be validated for matching namespaces BEFORE responding proposal
guard let (sessionTopic, proposal) = pairingEngine.respondSessionPropose(proposerPubKey: proposalId) else {return}
guard let (sessionTopic, proposal) = pairingEngine.approveProposal(proposerPubKey: proposalId, validating: namespaces) else {return}
try sessionEngine.settle(topic: sessionTopic, proposal: proposal, namespaces: namespaces)
}

Expand Down Expand Up @@ -327,7 +327,8 @@ public final class AuthClient {
sessionEngine.onSessionResponse = { [unowned self] response in
delegate?.didReceive(sessionResponse: response)
}
pairingEngine.onProposeResponse = { [unowned self] sessionTopic in
pairingEngine.onProposeResponse = { [unowned self] sessionTopic, proposal in
sessionEngine.settlingProposal = proposal
sessionEngine.setSubscription(topic: sessionTopic)
}
}
Expand Down
43 changes: 25 additions & 18 deletions Sources/WalletConnectAuth/Engine/Common/PairingEngine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import WalletConnectKMS

final class PairingEngine {
var onSessionProposal: ((Session.Proposal)->())?
var onProposeResponse: ((String)->())?
var onProposeResponse: ((String, SessionProposal)->())?
var onSessionRejected: ((Session.Proposal, SessionType.Reason)->())?

private let proposalPayloadsStore: KeyValueStore<WCRequestSubscriptionPayload>
Expand Down Expand Up @@ -71,7 +71,7 @@ final class PairingEngine {

func propose(pairingTopic: String, namespaces: [String: ProposalNamespace], relay: RelayProtocolOptions) async throws {
logger.debug("Propose Session on topic: \(pairingTopic)")
try Validator.validate(namespaces)
try Namespace.validate(namespaces)
let publicKey = try! kms.createX25519KeyPair()
let proposer = Participant(
publicKey: publicKey.hexRepresentation,
Expand Down Expand Up @@ -116,33 +116,34 @@ final class PairingEngine {
// todo - delete pairing if inactive
}

func respondSessionPropose(proposerPubKey: String) -> (String, SessionProposal)? {
func approveProposal(proposerPubKey: String, validating sessionNamespaces: [String: SessionNamespace]) -> (String, SessionProposal)? {
guard let payload = try? proposalPayloadsStore.get(key: proposerPubKey),
case .sessionPropose(let proposal) = payload.wcRequest.params else {
//TODO - throws
return nil
}
let proposedNamespaces = proposal.requiredNamespaces
proposalPayloadsStore.delete(forKey: proposerPubKey)

let selfPublicKey = try! kms.createX25519KeyPair()
var agreementKey: AgreementKeys!

do {
agreementKey = try kms.performKeyAgreement(selfPublicKey: selfPublicKey, peerPublicKey: proposal.proposer.publicKey)
try Namespace.validate(sessionNamespaces)
try Namespace.validateApproved(sessionNamespaces, against: proposedNamespaces)
Comment on lines +129 to +130
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think those two validations should not be in do{}catch{} block but rather throw an error so user can see that minimal requirement(or other requirement) has not been met.

I am also not sure if it make sense to respond with error for them in catch{} block
that is not valid error for a peer.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the validation error catch, it has to be thrown. I just used it as a temporary solution to make the validation start working. An issue can be created for that to improve error reading.

The catch respond error already existed before, but only when the key agreement failed. Should this be changed also?

let selfPublicKey = try! kms.createX25519KeyPair()
let agreementKey = try kms.performKeyAgreement(selfPublicKey: selfPublicKey, peerPublicKey: proposal.proposer.publicKey)
//todo - extend pairing
let sessionTopic = agreementKey.derivedTopic()

try! kms.setAgreementSecret(agreementKey, topic: sessionTopic)
guard let relay = proposal.relays.first else {return nil}
let proposeResponse = SessionType.ProposeResponse(relay: relay, responderPublicKey: selfPublicKey.hexRepresentation)
let response = JSONRPCResponse<AnyCodable>(id: payload.wcRequest.id, result: AnyCodable(proposeResponse))
logger.debug("Responding session propose")
networkingInteractor.respond(topic: payload.topic, response: .response(response)) { _ in }
return (sessionTopic, proposal)
} catch {
networkingInteractor.respondError(for: payload, reason: .missingOrInvalid("agreement keys"))
return nil
}
//todo - extend pairing
let sessionTopic = agreementKey.derivedTopic()

try! kms.setAgreementSecret(agreementKey, topic: sessionTopic)
guard let relay = proposal.relays.first else {return nil}
let proposeResponse = SessionType.ProposeResponse(relay: relay, responderPublicKey: selfPublicKey.hexRepresentation)
let response = JSONRPCResponse<AnyCodable>(id: payload.wcRequest.id, result: AnyCodable(proposeResponse))
logger.debug("Responding session propose")
networkingInteractor.respond(topic: payload.topic, response: .response(response)) { _ in }
return (sessionTopic, proposal)
}

//MARK: - Private
Expand All @@ -162,6 +163,12 @@ final class PairingEngine {

private func wcSessionPropose(_ payload: WCRequestSubscriptionPayload, proposal: SessionType.ProposeParams) {
logger.debug("Received Session Proposal")
do {
try Namespace.validate(proposal.requiredNamespaces)
} catch {
// TODO: respond error
return
}
try? proposalPayloadsStore.set(payload, forKey: proposal.proposer.publicKey)
onSessionProposal?(proposal.publicRepresentation())
}
Expand Down Expand Up @@ -228,7 +235,7 @@ final class PairingEngine {

try? kms.setAgreementSecret(agreementKeys, topic: sessionTopic)
try! sessionToPairingTopic.set(pairingTopic, forKey: sessionTopic)
onProposeResponse?(sessionTopic)
onProposeResponse?(sessionTopic, proposal)

case .error(let error):
if !pairing.active {
Expand Down
80 changes: 48 additions & 32 deletions Sources/WalletConnectAuth/Engine/Common/SessionEngine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@ final class SessionEngine {
var onSessionDelete: ((String, SessionType.Reason)->())?
var onEventReceived: ((String, Session.Event, Blockchain?)->())?

var settlingProposal: SessionProposal?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could it be overriden?
could some runtime key value storage be safer?


private let sessionStore: WCSessionStorage
private let pairingStore: WCPairingStorage
private let sessionToPairingTopic: KeyValueStore<String>
Expand Down Expand Up @@ -59,6 +61,30 @@ final class SessionEngine {
sessionStore.getAcknowledgedSessions().map{$0.publicRepresentation()}
}

func settle(topic: String, proposal: SessionProposal, namespaces: [String: SessionNamespace]) throws {
let agreementKeys = try! kms.getAgreementSecret(for: topic)!

let selfParticipant = Participant(publicKey: agreementKeys.publicKey.hexRepresentation, metadata: metadata)

let expectedExpiryTimeStamp = Date().addingTimeInterval(TimeInterval(WCSession.defaultTimeToLive))
guard let relay = proposal.relays.first else {return}
let settleParams = SessionType.SettleParams(
relay: relay,
controller: selfParticipant,
namespaces: namespaces,
expiry: Int64(expectedExpiryTimeStamp.timeIntervalSince1970))//todo - test expiration times
let session = WCSession(
topic: topic,
selfParticipant: selfParticipant,
peerParticipant: proposal.proposer,
settleParams: settleParams,
acknowledged: false)
logger.debug("Sending session settle request")
Task { try? await networkingInteractor.subscribe(topic: topic) }
sessionStore.setSession(session)
networkingInteractor.request(.wcSessionSettle(settleParams), onTopic: topic)
}

func delete(topic: String, reason: Reason) async throws {
logger.debug("Will delete session for reason: message: \(reason.message) code: \(reason.code)")
try await networkingInteractor.request(.wcSessionDelete(reason.internalRepresentation()), onTopic: topic)
Expand Down Expand Up @@ -139,34 +165,24 @@ final class SessionEngine {
}
}.store(in: &publishers)
}

func settle(topic: String, proposal: SessionProposal, namespaces: [String: SessionNamespace]) throws {
try Validator.validate(namespaces) // FIXME: Validation should happen before responding proposal, before settlement
let agreementKeys = try! kms.getAgreementSecret(for: topic)!

let selfParticipant = Participant(publicKey: agreementKeys.publicKey.hexRepresentation, metadata: metadata)

let expectedExpiryTimeStamp = Date().addingTimeInterval(TimeInterval(WCSession.defaultTimeToLive))
guard let relay = proposal.relays.first else {return}
let settleParams = SessionType.SettleParams(
relay: relay,
controller: selfParticipant,
namespaces: namespaces,
expiry: Int64(expectedExpiryTimeStamp.timeIntervalSince1970))//todo - test expiration times
let session = WCSession(
topic: topic,
selfParticipant: selfParticipant,
peerParticipant: proposal.proposer,
settleParams: settleParams,
acknowledged: false)
logger.debug("Sending session settle request")
Task { try? await networkingInteractor.subscribe(topic: topic) }
sessionStore.setSession(session)
networkingInteractor.request(.wcSessionSettle(settleParams), onTopic: topic)
}


private func onSessionSettle(payload: WCRequestSubscriptionPayload, settleParams: SessionType.SettleParams) {
logger.debug("Did receive session settle request")
guard let proposedNamespaces = settlingProposal?.requiredNamespaces else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. you set settlingProposal onApprovalResponse so it happens on responder's engine as responder calls "approve"
  2. you retrieve that settlingProposal onSessionSettle so it happens on proposer's engine as responder calls "settle"

🤔 am I correct? will it work?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

splitting engines/methods for controller/non-controller later will really help.

Copy link
Author

@sekimondre sekimondre May 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually they both happen on proposer's side, so it will work. The thing is that they happen on different engines. There may be a better solution to this.

// TODO: respond error
return
}
settlingProposal = nil
let sessionNamespaces = settleParams.namespaces
do {
try Namespace.validate(proposedNamespaces)
try Namespace.validate(sessionNamespaces)
try Namespace.validateApproved(sessionNamespaces, against: proposedNamespaces)
} catch {
// TODO: respond error
return
}

let topic = payload.topic

let agreementKeys = try! kms.getAgreementSecret(for: topic)!
Expand All @@ -177,12 +193,12 @@ final class SessionEngine {
updatePairingMetadata(topic: pairingTopic, metadata: settleParams.controller.metadata)
}

// TODO: Validate namespaces
let session = WCSession(topic: topic,
selfParticipant: selfParticipant,
peerParticipant: settleParams.controller,
settleParams: settleParams,
acknowledged: true)
let session = WCSession(
topic: topic,
selfParticipant: selfParticipant,
peerParticipant: settleParams.controller,
settleParams: settleParams,
acknowledged: true)
sessionStore.setSession(session)
networkingInteractor.respondSuccess(for: payload)
onSessionSettle?(session.publicRepresentation())
Expand Down
12 changes: 0 additions & 12 deletions Sources/WalletConnectAuth/Engine/Common/SessionStateMachineValidating.swift
View file
Open in desktop

This file was deleted.

5 changes: 2 additions & 3 deletions Sources/WalletConnectAuth/Engine/Controller/ControllerSessionStateMachine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import WalletConnectUtils
import WalletConnectKMS
import Combine

final class ControllerSessionStateMachine: SessionStateMachineValidating {
final class ControllerSessionStateMachine {
var onNamespacesUpdate: ((String, [String: SessionNamespace])->())?
var onExpiryUpdate: ((String, Date)->())?

Expand All @@ -27,11 +27,10 @@ final class ControllerSessionStateMachine: SessionStateMachineValidating {
}.store(in: &publishers)
}

// TODO: Change to new namespace spec
func update(topic: String, namespaces: [String: SessionNamespace]) async throws {
var session = try getSession(for: topic)
try validateControlledAcknowledged(session)
try Validator.validate(namespaces)
try Namespace.validate(namespaces)
logger.debug("Controller will update methods")
session.updateNamespaces(namespaces)
sessionStore.setSession(session)
Expand Down
4 changes: 2 additions & 2 deletions Sources/WalletConnectAuth/Engine/NonController/NonControllerSessionStateMachine.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import WalletConnectUtils
import WalletConnectKMS
import Combine

final class NonControllerSessionStateMachine: SessionStateMachineValidating {
final class NonControllerSessionStateMachine {

var onNamespacesUpdate: ((String, [String: SessionNamespace])->())?
var onExpiryUpdate: ((String, Date) -> ())?
Expand Down Expand Up @@ -42,7 +42,7 @@ final class NonControllerSessionStateMachine: SessionStateMachineValidating {
// TODO: Update stored session namespaces
private func onSessionUpdateNamespacesRequest(payload: WCRequestSubscriptionPayload, updateParams: SessionType.UpdateParams) {
do {
try Validator.validate(updateParams.namespaces)
try Namespace.validate(updateParams.namespaces)
} catch {
networkingInteractor.respondError(for: payload, reason: .invalidUpdateNamespaceRequest)
return
Expand Down
Loading