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

Only allow internal addresses to update FS and RTP caches #44

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

two56
Copy link

@two56 two56 commented Dec 6, 2023

I've updated the internal check in the OPTIONS handler so it doesn't rely on the presence of the X-RTP-Status or X-FS-Status header, instead it checks the source address against the internal network CIDR.

Copy link
Contributor

@davehorton davehorton left a comment

Choose a reason for hiding this comment

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

two things:

  1. I think I would prefer if this were refactored into drachtio middleware
  2. If an external system is sending an OPTIONS that includes these headers, I think we should return a failure result, maybe a simple 503, and log a distinctive info message (so that if we wanted we could have something like fail2ban block the source)
  3. In other places in the project we use the CIDRMatcher package for this sort of thing, can we use that instead of the custom code?

@two56 two56 force-pushed the fix/secure-options branch from feb4353 to f5114ff Compare December 7, 2023 12:06
@two56
Copy link
Author

two56 commented Dec 7, 2023

Hi Dave,

I probably haven't got the knowledge of the drachtio middleware to move the logical backwards. If you can give me some pointers I'm happy to give it a go.

I've updated the code to use cidr-matcher but I haven't added the 503 as, in my mind anyway, that would give someone a clue they're doing the "right" thing. Happy to change this if you disagree.

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.

2 participants