Explicitly deny to_localnet requests in squid.conf.default - #2491
Explicitly deny to_localnet requests in squid.conf.default#2491rousskov wants to merge 2 commits into
Conversation
This ban is not appropriate for some use cases, but our default configuration template should prioritize safety over coverage breadth. This work can be seen as a followup to 2022 commit 6d2f8ed. The new `to_localnet` ban appears to use a more complex http_access rule than existing to `to_localhost` and `to_linklocal bans`, but that additional complexity is misleading: In fact, existing bans should be interpreted in their `http_access allow localhost` context. In other words, existing bans can be viewed as if they were written like this: http_access deny !localhost to_localhost http_access deny !localhost to_linklocal Existing bans exclude localhost requests because a client running on localhost can send banned requests directly to those protected services, bypassing Squid. We use the same "do not ban access to what the clients may be able to access without going through Squid" logic for the new `to_localnet` rule, resulting in the `localnet` clients exclusion: http_access deny !localnet to_localnet Unlike the `localhost` case, we do not want to allow all `localnet` clients by default. That caveat necessitates a different rule shape. XXX: This duplicates "local" address range definitions. TODO: Add "local", similar to existing "all", "ipv4" and "ipv6" aliases? Also show how to allow internally-generated requests. Without that or some other "allow" rule that matches internally-generated requests, our configuration template bans all AIA requests, effectively disabling fetching of missing intermediate X.509 certificates. AIA requests are currently the only known use case of an internally-generated request not associated with a client-to-Squid connection. Typical src-based `http_access allow` rules cannot cover such requests.
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I am marking this PR as a draft because I want to discuss two changes that may necessitate significant PR changes. Please see specific change requests for details.
| # Adapt to list your (internal) IP networks. | ||
| # It is common to deny access to these addresses, but your needs may vary. | ||
| acl to_localnet dst 0.0.0.1-0.255.255.255 # RFC 1122 "this" network (LAN) | ||
| acl to_localnet dst 10.0.0.0/8 # RFC 1918 local private network (LAN) |
There was a problem hiding this comment.
PR description:
XXX: This duplicates "local" address range definitions.
TODO: Add "local", similar to existing "all", "ipv4" and "ipv6" aliases?
These duplication problems and the elevated difficulty of addressing them were the reasons these changes were explicitly excluded from our earlier related work in #1161. Assuming we are still in agreement regarding this overall direction, we have two primary options here:
- Merge this PR with/despite this duplication of localnet address ranges. Follow up with a PR that adds a "local" alias, similar to existing "all", "ipv4" and "ipv6" aliases? That followup PR should also document "all", "ipv4", and "ipv6" aliases, I guess. Adding support for a "local" alias will not be trivial because existing alias-handling code cannot handle multiple distinct address ranges.
- Do the opposite: Post a PR that adds a "local" alias first. Then update this PR to use that alias to define
to_localnet.
There was a problem hiding this comment.
This I think we should make to_linklocal a pre-define, unlike localnet the admin does not need to remove entries. They shoudl make a different ACL and allow X instead.
Possibly with small sub-defines for the ipv4_linklocal, ipv6_linklocal, ipv4_cgn, rfc1918, rfc1122, etc
There was a problem hiding this comment.
The "broadcast" ranges should also be prohibited as part of to_localnet. These are not relevant to the received (from-localnet), but can be for the outgoing to_localnet traffic.
For pre-define that would be ipv4_broadcast (255.255.255.255) and ipv6_broadcast (FFFF:...).
| # acl internallyGenerated transaction_initiator internal | ||
| # http_access allow internallyGenerated |
There was a problem hiding this comment.
This rule addition can be moved into a dedicated followup PR. AIA issues were the last straw that triggered creation of this long anticipated PR, and adding this AIA rule affects other comments in this section, so merging this work into this PR is borderline OK IMO, but please let me know if I should separate the two changes/concerns.
There was a problem hiding this comment.
Okay with me, though it does seem to expand the scope. So title should be something more like "Improve default http_access security policy"
|
|
||
| # | ||
| # INSERT YOUR OWN RULE(S) HERE TO ALLOW ACCESS FROM YOUR CLIENTS | ||
| # INSERT YOUR OWN RULE(S) HERE TO ALLOW PERMITTED TRANSACTIONS. |
There was a problem hiding this comment.
I adjusted this line because Squid-generated transactions, like those AIA requests, are not "FROM YOUR CLIENTS".
I also wanted to de-emphasize the "from" aspect of access control. These rules are also about "to" or "destination" controls, even though our examples do not illustrate that concern directly.
This comment annotates changes without requesting any discussion affecting this PR "draft" status.
* `foo` quotes code or configuration * "bar" uses quotes for standard English language purposes
There was a problem hiding this comment.
This whole issue smells like overriding admin misconfiguration to me.
FTR; The default policy is currently to block always-dangerous, allow only admin allowed traffic, then to block *all (including to-LAN traffic).
To do this properly without breaking admin rules - the deny to_localnet should ideally come after admin preferences so they can allow local servers through, but then it is redundant with deny all.
|
|
||
| # For example, to allow access from your local networks, you may uncomment the | ||
| # following rule (and/or add rules that match your definition of "local"): | ||
| # Uncomment to allow access from "local" networks to any destination that was |
There was a problem hiding this comment.
| # Uncomment to allow access from "local" networks to any destination that was | |
| # Uncomment to allow access from local network (LAN) to any destination that was |
| # missing intermediate X.509 certificates) to any destination that was not | ||
| # banned earlier (e.g., all `to_localnet` destinations were banned for such | ||
| # requests). | ||
| # acl internallyGenerated transaction_initiator internal |
There was a problem hiding this comment.
IMO this can be a pre-define. Which would allow adding AIA as an exception to the proposed deny ... to_locanet addition earlier.
| # Adapt to list your (internal) IP networks from where browsing | ||
| # should be allowed | ||
| # Define what "from your local network" means. | ||
| # Adapt to list your (internal) IP networks. |
There was a problem hiding this comment.
While we are editing these lines please fix the long-standing documentation issue that we did not use the well-known LAN term in this text.
| # Adapt to list your (internal) IP networks. | |
| # Adapt to list your (internal) IP networks (LAN). |
| # Protect services running inside your "local" networks. These services often | ||
| # assume that only "local" users can access them. This rule also denies | ||
| # internally-generated AIA requests for missing intermediate X.509 | ||
| # certificates when those requests target "local" services. |
There was a problem hiding this comment.
This rule is a bit dangerous and slightly non-intuitive due to the ! usage.
| # certificates when those requests target "local" services. | |
| # Protect services running inside your local network (LAN). These services often | |
| # assume that only LAN users can access them. This rule also denies | |
| # internally-generated AIA requests for missing intermediate X.509 | |
| # certificates when those requests target LAN services. |
or if internallyGenerated becomes a pre-define:
| # certificates when those requests target "local" services. | |
| # Protect services running inside your local network (LAN). These services often | |
| # assume that only LAN users can access them. |
2 participants
This ban is not appropriate for some use cases, but our default
configuration template should prioritize safety over coverage breadth.
This work can be seen as a followup to 2022 commit 6d2f8ed.
The new
to_localnetban appears to use a more complex http_access rulethan existing to
to_localhostandto_linklocal bans, but thatadditional complexity is misleading: In fact, existing bans should be
interpreted in their
http_access allow localhostcontext. In otherwords, existing bans can be viewed as if they were written like this:
Existing bans exclude localhost requests because a client running on
localhost can send banned requests directly to those protected services,
bypassing Squid. We use the same "do not ban access to what the clients
may be able to access without going through Squid" logic for the new
to_localnetrule, resulting in thelocalnetclients exclusion:Unlike the
localhostcase, we do not want to allow alllocalnetclients by default. That caveat necessitates a different rule shape.
XXX: This duplicates "local" address range definitions.
TODO: Add "local", similar to existing "all", "ipv4" and "ipv6" aliases?
Also show how to allow internally-generated requests. Without that or
some other "allow" rule that matches internally-generated requests, our
configuration template bans all AIA requests, effectively disabling
fetching of missing intermediate X.509 certificates. AIA requests are
currently the only known use case of an internally-generated request not
associated with a client-to-Squid connection. Typical src-based
http_access allowrules cannot cover such requests.