-
Notifications
You must be signed in to change notification settings - Fork 3
feat: add session key authentication support #103
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
Conversation
Adds session key mode as an alternative to private key authentication, allowing delegated authorisation without wallet prompts. Session keys are verified to have 30+ minutes remaining validity at initialization. The owner wallet's private key isn't required for CreateDataSet or AddPieces operations in this mode, so we only require the owner address. - add AddressOnlySigner for read-only owner wallet access - add WALLET_ADDRESS and SESSION_KEY env var support to add/import commands - fix FIL balance check to use owner wallet instead of session key NOTE: This will fail if the owner wallet doesn't meet the payments setup requirements, there's no ability to auto-fix funding or authorisation setup without being able to sign with that wallet.
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.
in general lgtm. tried testing locally but some commands still depending on private-key.
Questions:
- how do we revoke a session key? We should probably make that available so we can do it easily if filecoin-pin-website starts being used maliciously
function validateAuthConfig(config: SynapseSetupConfig): 'standard' | 'session-key' { | ||
const hasStandardAuth = config.privateKey != null | ||
const hasSessionKeyAuth = config.walletAddress != null && config.sessionKey != null | ||
|
||
if (!hasStandardAuth && !hasSessionKeyAuth) { | ||
throw new Error('Authentication required: provide either a privateKey or walletAddress + sessionKey') | ||
} | ||
|
||
if (hasStandardAuth && hasSessionKeyAuth) { | ||
throw new Error('Conflicting authentication: provide either a privateKey or walletAddress + sessionKey, not both') | ||
} | ||
|
||
return hasStandardAuth ? 'standard' : 'session-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.
we should use a typeguard and two strict types (descriminating union). maybe its mostly personal preference, but resolving optional types everywhere has always been a pain for me, but it should also prevent invalid permutations of a wide/loose interface. It also gives us better type validation at build time rather than runtime, makes refactoring/extending easier.. etc..
export type SynapseSetupConfig = SynapseSetupConfigPrivKey | SynapseSetupConfigSession
function isValidConfig(config: SynapseSetupConfig): config is SynapseSetupConfig { /* ... */ }
function isValidSessionConfig(config: SynapseSetupConfig): config is SynapseSetupConfigSession
function isValidPrivKeyConfig(config: SynapseSetupConfig): config is SynapseSetupConfigPrivKey
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.
yeah, go for it, or just a boolean
}, | ||
'Synapse SDK initialized' | ||
) | ||
logger.info({ event: 'synapse.init.success', network }, 'Synapse SDK initialized') |
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.
this is overall much cleaner than previous..
// The easiest way is to re-read the CAR and write a new one with the correct root | ||
|
||
// Import CarReader to read the existing CAR | ||
const { CarReader } = await import('@ipld/car') |
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.
oops
import type { SynapseSetupConfig } from './core/synapse/index.js' | ||
|
||
export function createLogger(config: Pick<SynapseSetupConfig, 'logLevel'>): Logger { | ||
export function createLogger(config: { logLevel?: string }): Logger { |
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.
Maybe this should be logLevel?: PinoLogLevels
if they provide a logLevel type
console.log('::notice::Building CAR file but upload will be blocked') | ||
} | ||
|
||
const { parseInputs, resolveContentPath } = await import('./inputs.js') |
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.
thank you. oops
// Verify permissions - fail fast if expired or expiring soon | ||
const expiries = await sessionKey.fetchExpiries([CREATE_DATA_SET_TYPEHASH, ADD_PIECES_TYPEHASH]) | ||
const now = Math.floor(Date.now() / 1000) | ||
const bufferTime = 30 * 60 // 30 minutes in seconds |
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.
30min is probably good for now, but do we need 30 minutes validity for the future? most operations take < 10min
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 started this with 5 minutes, but then I thought that when we start dealing with larger files we're doing this check potentially very long before we even get to submit something to the chain, so let's leave a big buffer
try { | ||
const provider = synapse.getProvider() | ||
const signer = synapse.getSigner() | ||
const signer = synapse.getClient() // owner wallet |
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.
can rename this to user or client
`Cannot sign ${thing} - this is an address-only signer for session key authentication. Signing operations should be performed by the session key.` | ||
|
||
export class AddressOnlySigner extends AbstractSigner { | ||
readonly address: string |
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.
surprising that ethers doesn't already have something like this
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.
Hugo promises me that Viem solves this for us so we can separate read and write wallets so I'm hopeful that this file is temporary until we get those changes into Synapse.
const hasStandardAuth = options.privateKey != null | ||
const hasSessionKeyAuth = walletAddress != null && sessionKey != null | ||
|
||
if (!hasStandardAuth && !hasSessionKeyAuth) { |
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.
this is duplicated in validateAuthConfig
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.
yes, I know, it only exists to deal with TypeScript complaints, original version didn't have it, then linter made me make it jankier, there's probably a better way of dealing with this but this was my expedient version
@SgtPooki if you're going to be refactoring the typeguard for that function then maybe this can be solved at the same time?
const sessionWallet = new Wallet(sessionKey, provider) | ||
|
||
// Initialize with owner signer, then activate session key | ||
synapse = await Synapse.create({ ...synapseOptions, signer: ownerSigner }) |
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.
This will fail if this app does any payments stuff. Does it?
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.
if you have a properly funded wallet then it should be fine, it's one of the reasons I didn't add any documentation or CLI args for this feature because I didn't want to have to deal with the complexity of that - it should only be used by wallets that have enough funds and are authorised at the right level
@SgtPooki added some help in #110 to make that problem a little less annoying, but now I think we're at the problem of avoiding the checks and ending up with reverts if we really don't have a set up wallet.
const hasStandardAuth = options.privateKey != null | ||
const hasSessionKeyAuth = walletAddress != null && sessionKey != null | ||
|
||
if (!hasStandardAuth && !hasSessionKeyAuth) { |
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.
this also duplicates the validation logic
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.
same as above, I hate it, but not enough to wrestle with it
Adds session key mode as an alternative to private key authentication, allowing delegated authorisation without wallet prompts. Session keys are verified to have 30+ minutes remaining validity at initialization. The owner wallet's private key isn't required for CreateDataSet or AddPieces operations in this mode, so we only require the owner address.
NOTE: This will fail if the owner wallet doesn't meet the payments setup requirements, there's no ability to auto-fix funding or authorisation setup without being able to sign with that wallet.
Here's a Bash script that'll make a new session key from your PRIVATE_KEY valid for 2 days:
@SgtPooki I'm assuming that you just need to extend your web code to use these new fields in the options for this to work? We'll need to figure out how long we want the session key to be valid for for the demo, but since it's quite limited and we can revoke it at any time we may as well make it quite long. For now let's just get it working in the demo with address+sessionkey for now so the private key goes away.