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

allow admin to add PDS that are ssl or not #904

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

brianolson
Copy link
Contributor

@brianolson brianolson commented Jan 8, 2025

BGS and Slurper allow mixed SSL or not upstreams
-crawl-ssl-policy|RELAY_CRAWL_NONSSL = any|require|none|prod
/admin/pds/crawl can add either; public requestCrawl normally SSL only
BGS AllowPDSProxies for nested relay setups
-allow-pds-proxy|RELAY_ALLOW_PDS_PROXY = true

-crawl-ssl-policy|RELAY_CRAWL_NONSSL = any|require|none|prod
/admin/pds/crawl can add either; public requestCrawl normally SSL only
BGS AllowPDSProxies for nested relay setups
-allow-pds-proxy|RELAY_ALLOW_PDS_PROXY = true
bgs/bgs.go Outdated
@@ -941,7 +944,9 @@ func (bgs *BGS) handleFedEvent(ctx context.Context, host *models.PDS, env *event
return fmt.Errorf("rebase was true in event seq:%d,host:%s", evt.Seq, host.Host)
}

if host.ID != u.PDS && u.PDS != 0 {
if bgs.config.AllowPDSProxies {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would be neat if we preconfigured the allowed hosts here via env var or something to avoid having the relay connected to itself or weird cases like that.

@bnewbold
Copy link
Collaborator

Ok with this for now to get things unblocked, but would love to un-jank all this more in the future, including all the wss:// vs https:// wrangling throuhgout the codebase.

Maybe storing hostnames separately?

Also a bit worried if it becomes possible to consume from the same host both with SSL and without.

@brianolson
Copy link
Contributor Author

major tweak: 'allow relay' is now a per-source admin action. No more global flag.

snuck this bit of data in next to the rate-limit admin stuff
'rate limit' admin stuff could grow to 'configure PDS/source', or could keep it broken up into API endpoint per function (block, crawl, ratelimit, allow-relay, etc)

also the dashboard now uses the admin version of requestCrawl instead of the public xrpc version

if host.ID != u.PDS && u.PDS != 0 {
if host.RelayAllowed {
// don't check that source is canonical PDS, allow intermediate relays
} else if host.ID != u.PDS && u.PDS != 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What are the implications here of skipping the createExternalUser bit? Do we end up not creating a row for users that are being echoed by a Relay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah
image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is a little weird, as it turns out that narelay does keep a little bit of per-user state so that it can emit a warning if there's a discontinuity in a user's feed ( See #915 )

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 we'd still probably want to keep track of the users being proxied by a Relay so we can ban them and/or change their upstream status and/or count them right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

mehhhh, ok, this needs some more consideration, there's some useful stuff inside .createExternalUser() and I can't just throw out the path of code that was under "user from weird place, wat"

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.

3 participants