Skip to content
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

Ozone ACLs #2252

Merged
merged 28 commits into from
Mar 6, 2024
Merged

Ozone ACLs #2252

merged 28 commits into from
Mar 6, 2024

Conversation

dholms
Copy link
Collaborator

@dholms dholms commented Feb 29, 2024

Switches out basic auth for ACLs in Ozone backend.

  • list of moderators & triagers in ozone env
  • appview & pds/entryway are configured with a modServiceDid for all moderation routes
    • retain (narrowly distributed) admin tokens on appview & pds as an escape hatch
  • all requests that go to Ozone travel through the user's PDS
    • PDS signs a service auth token for the user & sends it to ozone backend (similar to appview auth)
    • Ozone will then handle the request or sign another request out to the relevant service using it's own service DID

Supersedes #2058 but broken up such that it does not introduce breaking changes

@dholms dholms mentioned this pull request Mar 1, 2024
@dholms dholms marked this pull request as ready for review March 1, 2024 04:05
Comment on lines +260 to +262
creds.credentials.type === 'mod_service' ||
(creds.credentials.type === 'standard' &&
this.isModService(creds.credentials.iss))
Copy link
Collaborator

@devinivy devinivy Mar 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making sure I understand correctly—this means the mod service can view takedowns either:

  • as a mod service, using the mod service signing key (also used for e.g. signing labels).
  • as an actor, using their repo signing key.

Does that sound right? If so, when does the latter come into play?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that does sound right - maybe it could be communicated better.

But the latter comes into play on some of the view routes where we still want to show taken down content - getProfile for instance

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use a separate authVerifier there like optionalStandardOrModService but I thought it was a bit clearer to just use optionalStandard and then do a follow on check to see if it came from a trusted did


type NullOutput = {
credentials: {
type: 'null'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I struggle a little bit with the string 'null' here, mostly because I wonder whether it's a typo when I read checks like credentials.type === 'null'.

I like keeping a string here so there's a value to discriminate credentials on. This covers the case where the user did not bring any credentials: would 'none' work?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like that 👍

const { iss, aud } = await this.verifyServiceJwt(reqCtx, {
aud: this.ownDid,
iss: [this.adminDid],
iss: [this.modServiceDid, `${this.modServiceDid}#atproto_mod`],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think #atproto_mod is the id of the key. Do we want to use the id of the service here, map it to the id of the key when verifying?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup we chatted about this - I changed this to be the serviceId to keep it inline with possible future updates 👍

Comment on lines +50 to +52
admins: string[]
moderators: string[]
triage: string[]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a hunch we'll end-up wanting to toss these in the database before long, but seems like this is the right place to start 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup yup agreed

throw new AuthRequiredError('Untrusted issuer', 'UntrustedIss')
}
const payload = await this.verifyServiceJwt(reqCtx, {
aud: this.dids.entryway ?? this.dids.pds,
iss: [this.dids.admin],
iss: [this.dids.modService, `${this.dids.modService}#atproto_mod`],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might consider using the service id rather than the key id for the issuer here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup done 👍

Comment on lines -22 to +48
dbPostgresSchema: 'pds_admin_auth',
dbPostgresSchema: 'pds_moderator_auth',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the rebrand from "admin" to "moderator" 👍

Copy link
Collaborator

@devinivy devinivy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the main thing to nail down here is whether the credential issuer should be the service id rather than the key id. I am pro-service-id but we can chat it through! Pumped to have the auth story in a much better state!

dholms and others added 9 commits March 4, 2024 18:49
add runit to the services/bsky Dockerfile
* Allow tags to lead with and contain only numbers

* Break tags on other whitespace characters

* Export regexes from rich text detection

* Add test

* Add test

* Disallow number-only tags

* Avoid combining enclosing screen chars

* Allow full-width number sign

* Clarify tests

* Fix punctuation edge case

* Reorder

* Simplify, add another test

* Another test, comment
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
* ozone delegates email sending to user's pds

* lexicon: add content field to mod email event

* test email sending via mod event
@dholms dholms merged commit 582109c into main Mar 6, 2024
10 checks passed
@dholms dholms deleted the ozone-acls-take2 branch March 6, 2024 01:04
@dholms dholms mentioned this pull request Mar 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants