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:...).
There was a problem hiding this comment.
The "broadcast" ranges should also be prohibited as part of to_localnet.
Done in branch commits 2e6d513 (IPv4) and cd71aeb (IPv6).
For pre-define that would be
ipv4_broadcast(255.255.255.255) andipv6_broadcast(FFFF:...).
For IPv6, there is no such thing as "broadcast" AFAICT. There is multicast that uses FF:: rather than FFFF: prefix. Commit cd71aeb adds multicast.
we should make to_linklocal a pre-define
I agree. The question is whether to do that in this PR or a separate one? Given many potential disagreements on what "aliases" and "predefines" should be added (not to mention "how" they should be implemented), I suggest doing that in a separate followup PR(s), leaving this PR definitions where they are now. Are you OK with that plan?
I hope that this part of the discussion belongs to a different dedicated PR:
unlike localnet the admin does not need to remove entries
I agree that admins may define "local" differently than RFCs do.
I disagree that those legitimate admin adjustment needs should determine what is "predefined" by Squid. I think it is a mistake to (continue to) design ACLs based on our http_access template in squid.conf.documented. That historical approach makes it difficult to interpret configurations correctly, increasing security risks:
- What does X mean?
- Ah, do not worry about it, it is a Squid-defined ACL. Squid guys know what they are doing!
- Ah, OK, I will look for the problem elsewhere.
- Oh, wait, we redefined X five years ago in this custom included file, before we enabled IPv6 traffic!
- OMG! That explains why we allowed malicious traffic into our network!
Instead, Squid-provided definitions should be constant. If admins want to customize their access controls, they sure can, but they must use their own names for those customizations! I bet that best-written custom rules will not actually need to change what "localnet" or "all" means. They will just add http_access rules that use those built-in and other/custom ACLs to achieve the correct result.
Developers have learned the hard way that variables are dangerous. Named things should be constant by default. The same logic applies to writing complex access rules. If we were to start from scratch, we could make ACLs constant as well, but that is too big of a change. We can and should make Squid-defined ACLs and parameter aliases constant though (despite some inconveniences for existing configurations that attempt to modify them).
| # 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"
There was a problem hiding this comment.
I would rather not make the title so generic, especially since followup PRs may also "improve default policy". IMO, the existing "Also show how to allow internally-generated requests" paragraph in the PR description is enough to disclose this extra addition, but if you can suggest a more specific PR title that includes both topics, I would be happy to use it. FWIW, I tried to find one before posting this PR and failed.
|
|
||
| # | ||
| # 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.
| # 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.
There was a problem hiding this comment.
Which would allow adding AIA as an exception to the proposed deny ... to_locanet addition earlier.
AFAICT, adding such an exception is not a good idea because we do not want to structure our default template to allow AIA access to localnet (after some allow rules are added as we instruct the admin to do in INSERT YOUR OWN RULE(S) HERE). In other words, we do want to deny AIA requests for localnet resources (in most cases).
I also disagree that it makes sense to pre-define/name such simple ACLs as internallyGenerated, but that is secondary.
Co-authored-by: Amos Jeffries <yadij@users.noreply.github.com>
... for the same reasons IPv4 broadcast (handled in the previous commit) should be a part of that ACL.
rousskov
left a comment
rousskov
left a comment
There was a problem hiding this comment.
I hope I have addressed all in-scope concerns, especially if we keep aliases/pre-defines outside this PR scope.
| # 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.
The "broadcast" ranges should also be prohibited as part of to_localnet.
Done in branch commits 2e6d513 (IPv4) and cd71aeb (IPv6).
For pre-define that would be
ipv4_broadcast(255.255.255.255) andipv6_broadcast(FFFF:...).
For IPv6, there is no such thing as "broadcast" AFAICT. There is multicast that uses FF:: rather than FFFF: prefix. Commit cd71aeb adds multicast.
we should make to_linklocal a pre-define
I agree. The question is whether to do that in this PR or a separate one? Given many potential disagreements on what "aliases" and "predefines" should be added (not to mention "how" they should be implemented), I suggest doing that in a separate followup PR(s), leaving this PR definitions where they are now. Are you OK with that plan?
I hope that this part of the discussion belongs to a different dedicated PR:
unlike localnet the admin does not need to remove entries
I agree that admins may define "local" differently than RFCs do.
I disagree that those legitimate admin adjustment needs should determine what is "predefined" by Squid. I think it is a mistake to (continue to) design ACLs based on our http_access template in squid.conf.documented. That historical approach makes it difficult to interpret configurations correctly, increasing security risks:
- What does X mean?
- Ah, do not worry about it, it is a Squid-defined ACL. Squid guys know what they are doing!
- Ah, OK, I will look for the problem elsewhere.
- Oh, wait, we redefined X five years ago in this custom included file, before we enabled IPv6 traffic!
- OMG! That explains why we allowed malicious traffic into our network!
Instead, Squid-provided definitions should be constant. If admins want to customize their access controls, they sure can, but they must use their own names for those customizations! I bet that best-written custom rules will not actually need to change what "localnet" or "all" means. They will just add http_access rules that use those built-in and other/custom ACLs to achieve the correct result.
Developers have learned the hard way that variables are dangerous. Named things should be constant by default. The same logic applies to writing complex access rules. If we were to start from scratch, we could make ACLs constant as well, but that is too big of a change. We can and should make Squid-defined ACLs and parameter aliases constant though (despite some inconveniences for existing configurations that attempt to modify them).
| # 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.
Which would allow adding AIA as an exception to the proposed deny ... to_locanet addition earlier.
AFAICT, adding such an exception is not a good idea because we do not want to structure our default template to allow AIA access to localnet (after some allow rules are added as we instruct the admin to do in INSERT YOUR OWN RULE(S) HERE). In other words, we do want to deny AIA requests for localnet resources (in most cases).
I also disagree that it makes sense to pre-define/name such simple ACLs as internallyGenerated, but that is secondary.
| # acl internallyGenerated transaction_initiator internal | ||
| # http_access allow internallyGenerated |
There was a problem hiding this comment.
I would rather not make the title so generic, especially since followup PRs may also "improve default policy". IMO, the existing "Also show how to allow internally-generated requests" paragraph in the PR description is enough to disclose this extra addition, but if you can suggest a more specific PR title that includes both topics, I would be happy to use it. FWIW, I tried to find one before posting this PR and failed.
rousskov
added
the
S-waiting-for-reviewer
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.