feat: sign traffic between RS and AS #31
Conversation
Signed-off-by: Wouter Termont <[email protected]>
Signed-off-by: Wouter Termont <[email protected]>
Signed-off-by: Wouter Termont <[email protected]>
There was a problem hiding this comment.
I tested it both with node v20 and with v18 and the test flows work.
Though there is a need to have .nvmrc and the docs pointing to the same version for consistency, if that is fixed I'll approve.
Something entirely else, I've found a bug with the enforcement flow (some missing exports). I'll make a new branch to fix this
|
|
||
| try { | ||
| const permissionEndpoint = (await fetchUmaConfig(issuer)).permission_endpoint; | ||
| return fetchPermissionTicket(requestedModes, permissionEndpoint, pat); | ||
| endpoint = (await this.fetchUmaConfig(issuer)).permission_endpoint; | ||
| } catch (e: any) { | ||
| throw new Error(`Error while retrieving ticket: ${(e as Error).message}`); |
There was a problem hiding this comment.
FYI CSS has a createErrorMessage utility function.
| protected options: UmaVerificationOptions = {}, | ||
| ) {} | ||
|
|
||
| public async signedFetch(url: string, request: RequestInit & Omit<SignRequest, 'url'>): Promise<Response> { |
There was a problem hiding this comment.
To understand, this is the replacement for the current PAT right? Is the plan to eventually somehow have 1 key per data owner? Or one per server and see the entire server as a single owner?
There was a problem hiding this comment.
Correct. Plans are not clear yet. UMA says PAT per owner, GNAP says HTTP Message Signatures between servers (similar to this implementation). Not sure yet about my preference. Any thoughts?
There was a problem hiding this comment.
Having it be server-side definitely seems easier, as otherwise users somehow have to be responsible for providing their own PAT. Unless they let the server generate it for them, but at that point you might as well have just one for the server. If you trust the server to store all your data, I don't really see the point in having a personal PAT.
| } | ||
|
|
||
| private process(): void { | ||
| this.processing = true; |
There was a problem hiding this comment.
Why have these? As the loop and shifting is sync, I don't think it's necessary, as no other calls will be completed until the backlog has been emptied.
There was a problem hiding this comment.
Ha, I guess you're right! Being too careful...
| * Any object of which a status can be changed. | ||
| */ | ||
| export interface StatusDependant<T> { | ||
| changeStatus(status: T): Promise<void>, |
There was a problem hiding this comment.
Isn't that almost any object? 😅 Very subjective but name feels weird to me. I'm thinking more of something like setState and as interface name StateMachine or something similar?
There was a problem hiding this comment.
Yeah, I know, terrible name 😆 I was wondering for the CSS: do you prefer we just implement this specifically for the Fetchers, or do we keep this and change the name to something better?
There was a problem hiding this comment.
I'm ok with a generic interface as not all fetchers will need this, and there might be other components in the future that also depend on something similar. But I would prefer a better name 😀
| import type { StatusDependant } from './StatusDependant'; | ||
|
|
||
| /** | ||
| * A {@link ServerConfigurator} that maps events of a {@link Server} so a status, |
There was a problem hiding this comment.
| * A {@link ServerConfigurator} that maps events of a {@link Server} so a status, | |
| * A {@link ServerConfigurator} that maps events of a {@link Server} to a status, |
| const trigger = new EventEmitter(); | ||
| const result = new Promise<Response>(resolve => { | ||
| trigger.on(PROCESS, () => { | ||
| this.fetcher.fetch(...args).then(response => resolve(response)) |
There was a problem hiding this comment.
| this.fetcher.fetch(...args).then(response => resolve(response)) | |
| this.fetcher.fetch(...args).then(resolve) |
There was a problem hiding this comment.
🤔 is this necessary, or are you instinctively applying ESlint rules? 😄
There was a problem hiding this comment.
It just felt wrong to not at least mention it 😉
|
|
||
| async fetch(...args: FetchParams): Promise<Response> { | ||
| const trigger = new EventEmitter(); | ||
| const result = new Promise<Response>(resolve => { |
There was a problem hiding this comment.
Minor optimization, but might be nicer to already check the status here and immediately return the fetch without the promise/eventemitter if the fetcher is active.
There was a problem hiding this comment.
True, I even thought about it, but I was lazy. Will do for CSS PR ...
|
@joachimvh review and merge must have crossed, because I only see this now due to notifications ... Will take most things up somewhere else, but feel free to still reply here. |
This PR contains a lot of changes to do a simple thing: sign requests between RS and AS, and check those signatures.
It was a messy trial-and-error development process, so my apologies for the lack of more granular commits, and the rough edges in some places.
Closes #29