WIP: Secure mode #31

Closed
norm wants to merge 21 commits from (deleted):feat/secure-fetch into main
Owner
Fixes https://akkoma.dev/FoundKeyGang/FoundKey/issues/29 Commits from https://github.com/misskey-dev/misskey/pull/7709
Author
Owner

Used Deepl to translate the original Japanese strings. I tried to correct any grammar mistakes or other potential mistranslations. Please proofread.

Used Deepl to translate the original Japanese strings. I tried to correct any grammar mistakes or other potential mistranslations. Please proofread.
norm force-pushed feat/secure-fetch from 6067410213 to 664df9a163 2022-07-26 04:14:34 +00:00 Compare
Author
Owner

also seems like the text for the settings doesn't actually show up in the admin console, don't know that much about vue to know why exactly

also seems like the text for the settings doesn't actually show up in the admin console, don't know that much about vue to know why exactly
Owner

Looked through the first commit just to get the idea...
Private mode looks like allowlist federation?
I guess including it also is fine since it's not that lengthy.

Do we already do signing for authorized fetch being enabled elsewhere? (I presume yes since it works).

Looked through the first commit just to get the idea... Private mode looks like allowlist federation? I guess including it also is fine since it's not that lengthy. Do we already do signing for authorized fetch being enabled elsewhere? (I presume yes since it works).
Author
Owner

Yeah there's the signToActivityPubGet option which does exactly that.

Yeah there's the `signToActivityPubGet` option which does exactly that.
norm changed title from WIP: Secure mode to Secure mode 2022-07-26 16:13:55 +00:00
Author
Owner

ok I got the strings to show up now
image

ok I got the strings to show up now ![image](/attachments/0fed0ea5-e8d5-40f8-946f-296ebb6b0e51)
norm added 1 commit 2022-07-26 19:30:49 +00:00
Merge branch 'main' into feat/secure-fetch
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
d27ab85e0f
Owner

Maybe we should remove the signToActivitypubGet setting and have it be always on. I think because some masto instances require it, many people have it on anyways? It would remove the kinda weird dependence between the GUI setting and the config file.

Maybe we should remove the `signToActivitypubGet` setting and have it be always on. I think because some masto instances require it, many people have it on anyways? It would remove the kinda weird dependence between the GUI setting and the config file.
norm added 1 commit 2022-07-27 06:32:53 +00:00
Remove signToActivityPubGet option
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
55b2aebec4
Makes it so that all requests are signed, equivalent to
signToActivityPubGet always being true.
Author
Owner

I think I got everywhere the signToActivityPubGet option was referenced.

I think I got everywhere the `signToActivityPubGet` option was referenced.
norm added 1 commit 2022-07-27 21:08:09 +00:00
Merge branch 'main' into feat/secure-fetch
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
c1547c9159
Owner

Still haven't done a full review of this yet since it's quite big. On a cursory view of a few more files I noticed there are new Japanese comments, e.g. in packages/backend/src/queue/processors/inbox.ts or packages/backend/src/remote/activitypub/check-fetch.ts. I would prefer if we could avoid adding new Japanese comments. Remove or translate them to English?

Still haven't done a full review of this yet since it's quite big. On a cursory view of a few more files I noticed there are new Japanese comments, e.g. in `packages/backend/src/queue/processors/inbox.ts` or `packages/backend/src/remote/activitypub/check-fetch.ts`. I would prefer if we could avoid adding new Japanese comments. Remove or translate them to English?
norm added 20 commits 2022-07-30 22:59:34 +00:00
refactor components/form/textarea.vue to composition API
Some checks failed
ci/woodpecker/push/build Pipeline failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/test Pipeline failed
ci/woodpecker/push/lint-client Pipeline failed
9a236bd862
refactor: visitor.vue to composition api
Some checks failed
ci/woodpecker/push/build Pipeline failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/test Pipeline failed
ci/woodpecker/push/lint-client Pipeline failed
1f3b3abf68
API: visiblity cannot be less restrictive
Some checks failed
ci/woodpecker/push/lint-backend Pipeline failed
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
40d9aa6219
Removed a now unnecessary provision from services/note/create as well.
client: search button is a no-op
Some checks failed
ci/woodpecker/push/lint-backend Pipeline failed
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
9abbe94108
Don't be evil.
reference: #2 (comment)
fix: lint error in create.ts
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
b7c0e26da9
fix: lint errors in modal.vue
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
86b94e213e
refactor: common.vue to composition api
Some checks failed
ci/woodpecker/push/build Pipeline failed
ci/woodpecker/push/lint-backend Pipeline failed
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
a485d13e8a
refactor: object-view.value.vue to composition api
Some checks failed
ci/woodpecker/push/build Pipeline failed
ci/woodpecker/push/lint-backend Pipeline failed
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
6c8eb4c4df
refactor: header.vue to composition api
Some checks failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
16833b8cd8
refactor: sample.vue to composition api
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
9e8b59f886
refactor: welcome.timeline.vue to composition api
Some checks failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
9fc3fcaf18
refactor components/widgets.vue to composition API
Some checks failed
ci/woodpecker/push/lint-backend Pipeline was successful
ci/woodpecker/push/build Pipeline was successful
ci/woodpecker/push/lint-client Pipeline failed
ci/woodpecker/push/test Pipeline failed
340420c48a
norm added 1 commit 2022-07-30 23:00:06 +00:00
Merge branch 'main' into feat/secure-fetch
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
530c7bb5e1
norm added 1 commit 2022-07-31 22:36:00 +00:00
norm added 1 commit 2022-07-31 22:38:10 +00:00
Merge branch 'main' into feat/secure-fetch
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
daa286e333
Johann150 requested changes 2022-08-01 00:42:04 +00:00
Johann150 left a comment
Owner

First of all I'm not really fond of the idea of both of these modes (see also https://honk.tedunangst.com/u/tedu/h/3PkQ9bJXNBgJ7w7bD8). Not to mention that the current Fediverse "agreement"¹ of HTTP signatures is a huge mess.

¹ Mastdon uses a pretty old version of an RFC draft that is no longer current but now everyone else is stuck with that version.


I'm not really happy with how the pug templates have unless privatemode everywhere. I think it would make more sense to not render the template in the first place if you are in private mode? I think you should be able to just skip to next in the router as if you didn't find something.

First of all I'm not really fond of the idea of both of these modes (see also https://honk.tedunangst.com/u/tedu/h/3PkQ9bJXNBgJ7w7bD8). Not to mention that the current Fediverse "agreement"¹ of HTTP signatures is a huge mess. ¹ Mastdon uses a pretty old version of an RFC draft that is no longer current but now everyone else is stuck with that version. ----- I'm not really happy with how the pug templates have `unless privatemode` everywhere. I think it would make more sense to not render the template in the first place if you are in private mode? I think you should be able to just skip to `next` in the router as if you didn't find something.
@ -0,0 +7,4 @@
import DbResolver from '@/remote/activitypub/db-resolver.js';
import { getApId } from '@/remote/activitypub/type.js';
export default async function checkFetch(req: IncomingMessage): Promise<number> {
Owner

If you want you could maybe take a stab at reusing this in packages/backend/src/queue/processors/inbox.ts since its essentially the same code for validating a HTTP signature.

If you want you could maybe take a stab at reusing this in `packages/backend/src/queue/processors/inbox.ts` since its essentially the same code for validating a HTTP signature.
Author
Owner

Not enitrely sure how to extract out the common logic here, may leave it to someone else to handle that.

Not enitrely sure how to extract out the common logic here, may leave it to someone else to handle that.
Johann150 marked this conversation as resolved
@ -189,3 +268,4 @@
// emoji
router.get('/emojis/:emoji', async ctx => {
const verify = await checkFetch(ctx.req);
Owner

I think emojis are often not proxied by servers so clients will try to load the emoji from the original server. With requiring HTTP signatures even for this that will cause problems.

I think emojis are often not proxied by servers so clients will try to load the emoji from the original server. With requiring HTTP signatures even for this that will cause problems.
norm marked this conversation as resolved
@ -203,1 +288,3 @@
ctx.set('Cache-Control', 'public, max-age=180');
const meta = await fetchMeta();
if (meta.secureMode || meta.privateMode) {
ctx.set('Cache-Control', 'private, max-age=0, must-revalidate');
Owner

Is there a significant difference between this an no-store?

I think this is a bit too strong. Just because something is private doesn't mean the max-age is suddenly zero.

Same goes for other cache directives further down and in other files.

Is there a significant difference between this an `no-store`? I think this is a bit too strong. Just because something is private doesn't mean the `max-age` is suddenly zero. Same goes for other cache directives further down and in other files.
Author
Owner

What would be a more sensible Cache-Control setting?

What would be a more sensible `Cache-Control` setting?
Author
Owner

Decided to just change it to no-store for now

Decided to just change it to `no-store` for now
norm marked this conversation as resolved
@ -134,0 +148,4 @@
set.secureMode = ps.secureMode;
}
if (ps.mascotImageUrl !== undefined) {
Owner

I think you re-added mascotImageUrl on accident. We don't have that any more.

I think you re-added `mascotImageUrl` on accident. We don't have that any more.
norm marked this conversation as resolved
@ -288,2 +298,4 @@
langs: instance.langs,
tosUrl: instance.ToSUrl,
repositoryUrl: instance.repositoryUrl,
feedbackUrl: instance.feedbackUrl,
Owner

We do not have repositoryUrl and feedbackUrl any more. Same as mascotImageUrl above.

We do not have `repositoryUrl` and `feedbackUrl` any more. Same as `mascotImageUrl` above.
norm marked this conversation as resolved
@ -67,6 +96,16 @@ function save() {
});
}
function saveInstance() {
Owner

Why is this a separate function and not in the other save function?

Why is this a separate function and not in the other `save` function?
norm marked this conversation as resolved
norm added 1 commit 2022-08-01 02:54:15 +00:00
Remove deprecated URLs
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
7485d8d360
norm added 1 commit 2022-08-01 02:59:27 +00:00
security: combine save functions
Some checks failed
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
cce01c9a70
norm force-pushed feat/secure-fetch from d042603b4f to e5595ca31c 2022-08-01 03:57:43 +00:00 Compare
norm added 1 commit 2022-08-01 04:00:40 +00:00
Remove check for signature in emoji fetch
Some checks failed
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
429cb3ad7b
norm changed title from Secure mode to WIP: Secure mode 2022-08-01 04:20:22 +00:00
norm added 1 commit 2022-08-10 06:31:50 +00:00
Merge branch 'main' into feat/secure-fetch
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
ebc34ab09c
norm added 1 commit 2022-08-11 14:06:51 +00:00
Set Cache-Control to 'no-store' in private/secure mode
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed
a1b50a5ba5
norm referenced this pull request from a commit 2022-08-28 14:46:46 +00:00
Author
Owner

Superceded by #169

Superceded by #169
norm closed this pull request 2022-09-22 15:28:11 +00:00
norm deleted branch feat/secure-fetch 2022-09-22 16:45:30 +00:00
Some checks failed
ci/woodpecker/pr/lint-backend Pipeline was successful
ci/woodpecker/pr/build Pipeline was successful
ci/woodpecker/pr/lint-client Pipeline failed
ci/woodpecker/pr/test Pipeline failed

Pull request closed

Sign in to join this conversation.
No reviewers
No labels
feature
fix
upkeep
No milestone
No project
No assignees
3 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: FoundKeyGang/FoundKey#31
No description provided.