refactor(restrict-hostnames): restructure to better reuse code.

[zachomedia]

@justbert I agree with the objective but I have one concern about this change is that it separates the host and path checks on virtual services, which removes the grouping of the two; assuming I'm reading the implementation correctly.

For example:

Allowed:

• domain1.com/path1
• domain2.com/path2

I think this implementation would let you take /path2 at domain1.com.

[justbert]

Can you point in the code where?

I don't think there's any effective change to what's happening here as to in your version; all I did was move code that was reused into functions.

As a side note, a potential fix to the edge-case you described (and a fix for here) is to use the complex objects as they are. We build a set of allowed host/path combos, then build a set of all the possibilities identified in the VS, and ensure that the set from the VS is all contained in the set from the annotation.

What do you think?

Oh and are paths in the annotation exact match or prefix?

[zachomedia]

@justbert I think that what I mentioned is actually an issue in my implementation as well - which I think is actually the same issue as the edge case. I'll leave that to tired brain!

I think your approach would work for handing the virtual service objects! When I look at a VirtualService object, I see a few possible combinations/options:

apiVersion: networking.istio.io/v1alpha3
kind: VirtualService
metadata:
  name: example
spec:
  hosts:
  - domain1.com
  - domain2.com
  http:
  - match:
    - authority:
        exact: domain1.com
       uri:
        prefix: "/path1"
    - uri:
        prefix: "/path2"

We would need to check the following is authorized:

• domain1.com/path1
• domain1.com/path2
• domain2.com/path2

Since path2 doesn't have a more restrictive authority match.

As for exact/prefix, that's a slight oversight in my initial implementation. I was treating them as to what would appear in the objects - but I'm thinking we may want a flag to say if it's exact or prefix. We can use that in both the Ingress (pathType) and the VirtualService validation.

[justbert]

Yeah that sounds good! We can either add both options or just treat it as a prefix. I feel like most of the time, treating it as a prefix will work for our use-cases. This comes from my (perhaps flawed) understanding that if someone is serving at a subpath, we'd usually also allow them to serve from anything underneath it. I can't really see a case where we'll restrict someone to a single link, can you?

As for Authority, do we really need to check it? It doesn't really come into routing decisions, correct? Worst case would be that it is wrong and so there would be issues in requesting the content, but the routing would still be correct?

[zachomedia]

I'm ok with the assumption of prefix since that would generally be our use case. We can always revisit if that changes down the road. I don't know if we want to update the policy to say handle it being a prefix (right now it's an exact match)?

For the authority, I think it's important. Because if we look at the example in my previous message and say the allowed-hosts annotation was:

[
  {
    "host": "domain1.com",
    "path": "/path1"
  },
 {
   "host": "domain2.com",
   "path": "/path2"
  }
]

Then we would need to ensure it fails if they attempt to "claim" domain1.com/path2. But I think the way our logic currently is, it'll just check that path is valid for any host in the domain list. I think anyways, I could be processing the implementation wrong in my head and it would in fact fail.

[justbert]

Aha! Oh I get it! I don't think that the authority section is used for routing on the Istio side. From what I gather from looking into the code, it just adds the filters to Envoy to validate that the Authority header is in place.

So solutions builders could be setting anything in the Authority, but it should just cause some weirdness in the browser due to the mismatch between the HOST. It shouldn't allow anyone to route to another URL.

[justbert]

closed in favour of #24