-
Notifications
You must be signed in to change notification settings - Fork 99
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.
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
Add session command data and enforce-session
builtin
#1171
base: master
Are you sure you want to change the base?
Conversation
enforce-session
builtin
This reverts commit 6fb10da.
src/Pact/Types/Command.hs
Outdated
@@ -93,20 +94,23 @@ import Pact.Types.Scheme (PPKScheme(..), defPPKScheme) | |||
data Command a = Command | |||
{ _cmdPayload :: !a | |||
, _cmdSigs :: ![UserSig] | |||
, _cmdSessionPubKey :: Maybe PublicKeyText |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like something is getting lost in translation. My previous comment:
Sessions are a container-provided trusted value, whereas Commands are signed by user.
Thus, the container should be directly instrumenting any Session credentials directly into the environment.
"directly instrument" simply means "call setupEvalEnv
in the container". There are many reasons why sessions have nothing whatsoever to do with transactions or commands:
- The user has no way to specify; sessions are entirely controlled by the container.
- The session obviously spans multiple commands and indeed is unaffected by the execution of any given command.
Command
is the single most important type in our API as it's literally how every single blockchain transaction is executed so it's trust paradigm cannot be modified. The triple of(hash,sigs,payload)
is an invariant: thehash
directly reflects thepayload
and the only extraneous information aresigs
that sign nothing more than the hash.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, thanks for the explanation! The PR is updated now so that Command
and Payload
do not include a session public-key. So session-pubkey isn't exposed via any API of pact nor any of the downstream projects that use pact, but a hypothetical new container service would be able to set it while provisioning its EvalEnv
. For example a hypothetical new pact server than manages user authentication and sessions could set up session pubkey. (As we discussed on slack. restating it here for the github record).
src-ghc/Pact/GasModel/GasTests.hs
Outdated
tests = | ||
createGasUnitTests | ||
updateEnvMsgSession | ||
updateEnvMsgSession |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateEnvMsgSession | |
updateEnvMsgSession |
src/Pact/Eval.hs
Outdated
case sessionPubKey of | ||
Nothing -> error "enforce-session called while there is no session pubkey in the environment" | ||
Just (publicKeyText, caps) -> do | ||
let matchingKeys = M.filterWithKey matchKey $ M.singleton publicKeyText caps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this code just checking whether matchKey
equals publicKeyText
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a tad more - matchKey
isn't a single key, but a predicate that checks if publicKeyText
is an element of the keyset keys.
The code here is confusing. I've tweaked this part to make it more clear.
Fix formatting for enforceSession Co-authored-by: John Wiegley <johnw@newartisans.com>
Co-authored-by: John Wiegley <johnw@newartisans.com>
Co-authored-by: John Wiegley <johnw@newartisans.com>
Co-authored-by: John Wiegley <johnw@newartisans.com>
Co-authored-by: John Wiegley <johnw@newartisans.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good so far!
|
||
Set PUBLIC-KEY as the session public key. | ||
```lisp | ||
(env-session "my-key" []) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the syntax for a cap here? Can we get a more complex example?
enforceKeySetSession i ksn KeySet{..} = do | ||
sessionPubKey <- view eeSessionSig | ||
case sessionPubKey of | ||
Nothing -> error "enforce-session called while there is no session pubkey in the environment" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to error this hard instead of calling evalError'
or dying some other way? I seem to remember some discussion of this but I'd like to see it fleshed out here in the PR.
Asana task
Goals:
(enforce-session keyset-foo)
, which enforces that "keyset-foo" is available in the environment as a "session signer".Command
. A session signer is the public key of a user who has somehow been authenticated, for example by an authentication token that relates a user to a public key in some external system.Command
through toEvalEnv
, so it's available to the implementation ofenforce-session
.Wish list for review:
Reevaluate assumptions about adding sessionSig to Payload. (I'm not 100% sure this is still needed). For example, we may be able to remove it fromPayload
andMsgData
, and make it the responsibility of whoever invokes the Pact interpreter, to override_eeSessionSig
in the environment. I'm not sure if this has other drawbacks (for example, is it insecure to fail to record a SessionSig that authenticated a transaction?)SessionSigner
doesn't belong in the payload, so I've moved it out. Whoever sets up theEvalEnv
now sets SessionSigner directly.Is it ok that I had to change the golden tests for encoded continuations?That's probably not ok, soSessionSigner
is no longer part of the Payload, the hashes no longer change. 👍