WIP: Secure mode #31

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

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
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
to 664df9a163 2022-07-26 04:14:34 +00:00
Compare
Author
Contributor

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
Contributor

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
Contributor

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)
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.
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
Contributor

I think I got everywhere the signToActivityPubGet option was referenced.

I think I got everywhere the `signToActivityPubGet` option was referenced.
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/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
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
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
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 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
Contributor

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
Contributor

What would be a more sensible Cache-Control setting?

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

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
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
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
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
to e5595ca31c
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
2022-08-01 03:57:43 +00:00
Compare
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
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
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
Contributor

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.