-
Notifications
You must be signed in to change notification settings - Fork 5.9k
feat(unstable): --allow-net subdomain wildcards #29327
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
feat(unstable): --allow-net subdomain wildcards #29327
Conversation
|
IMO, users may need to be concerned about attacks via dangling records, while this feature is safe to use in most cases. https://www.form3.tech/blog/engineering/dangling-danger |
bartlomieju
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exciting!
|
Please open a PR to https://github.com/denoland/docs that updates explanation of the |
dsherret
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM once the boolean parameter is removed.
dsherret
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See latest comment.
dsherret
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great. Thanks!
| || NetDescriptor::parse(host_and_port).is_ok() | ||
| || NetDescriptor::parse_for_list( | ||
| host_and_port, | ||
| UnstableSubdomainWildcards::Enabled, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sweet. Thanks! This also makes it slightly easier to remove in the future because we'll just need to do a find all references for this type.
| fn parse_net_query( | ||
| &self, | ||
| text: &str, | ||
| ) -> Result<NetDescriptor, NetDescriptorParseError>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One tiny thing, is considering this is permissions, I wonder if we should use a different type for query like NetQuery instead of NetDescriptor so there's a bit more protection in not accidentally mixing them up? I'm not sure it makes sense. Just a thought.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my first try. It's hard to walk through it all but I essentially hit a roadblock implementing QueryDescriptor::from_allow() (this method fundamentally doesn't make sense) for NetQuery { type AllowDesc = NetDescriptor }, and the change exploded in scope trying to address everything. So I restarted.
However, I'm also doubtful we should have separate 'query' and 'allow-listed range' types, since the latter should also be query-able through the runtime API. It also needs to be queried when checking for escalations when spawning child workers, which is precisely where we use QueryDescriptor::from_allow().
There might have been technical causes to do this type AllowDesc stuff for env or run permissions, but from a wider lens it seems flawed.
Closes #6532.
Supports one wildcard which must be the first label. Doesn't support wildcards in a middle label like
cloudformation.*.amazonaws.comas one user requested. Based on https://developers.cloudflare.com/dns/manage-dns-records/reference/wildcard-dns-records/#aspects-to-consider.Let's decline these ones.