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

Rewrite of IP assignment section #21

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mirjak
Copy link

@mirjak mirjak commented Jan 20, 2021

No description provided.

@achernya
Copy link
Collaborator

Hi Mirja,

Your PR unfortunately contains lots of typos and language that I am having a hard time parsing. Could you please provide a description of what this rewrite is trying to address? Otherwise, I am not sure I can provide an effective review.

Sincerely,
-Alex

request.
The IP addresss or range of IP addresses can either be assigned by the client
or the proxy. If the address is assigned by the client, proxy has to ensure
that the IP address is from a valid range that the client authorty of and that

Choose a reason for hiding this comment

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

Do we need this detail in the requirements? I am hoping that we should be able to remain unopinionated about what it means for the address to be 'valid' which I suspect will mean different things to different implementers.

I think only specifying that the recipient of the request has the means to reject it should be sufficient. Are there use cases you're thinking of where we need this stronger requirement?

Copy link
Author

Choose a reason for hiding this comment

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

I think this is actually important to ensure that a proxy is safe to deploy. If you deploy two proxies between two datacenters which are both controlled by the same entity, you might skip that validation and trust the client. But if you have proxy that connects the client to the "open" Internet, you should check that the address is not spoofed.

Choose a reason for hiding this comment

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

Hey Mirja,

I absolutely agree that it is important that a proxy has the opportunity to accept/reject a request to establish an ip proxying session -- and to accept/reject a client's choice of ip address.

However, I'm not sure it makes sense to be strongly opinionated about the validation that is required on the proxy (or are we just quibbling on semantics?). As you mention, there are different scenarios where the proxy may want to validate different properties of the request. So long as the protocol provides all necessary hooks for the proxy to perform validation and make accept/reject decisions based upon that validation, I'm not sure what value there is in making it a requirement that the proxy perform some specific form of validation (which seems likely to be deployment-dependent).

Copy link
Author

Choose a reason for hiding this comment

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

I think we are quibbling a bit on the right level that we want to express the requirements. And there might be even more than one requirement here.

There seems to be a function requirement here where we specify who gets to decide. Having a request mechanism that then naturally could also be rejected, could actually be also seen as an implementation details. But again, not sure what's the right level here want to state things on. (That's the hard part of requirement documents).

The other thing that I think we should cover somehow is a security requirement to avoid that the proxy can be misused to make spoofing easier.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I would make validation a separate requirement instead of conflating it with the IP assignment requirement

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not opposed to splitting things up like that, but I worry that by explicitly enumerating them, we will imply that other policies or enforcement points are prohibited/non-requirements. We've already seen that in other discussions. As a result, my preference is to leave this as-is.

Copy link
Author

Choose a reason for hiding this comment

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

The point of my proposed wording (which might also not be optimal yet) is that I think the requirement at the moment has not the right level of abstraction. The requirement talks about a request which is a concrete design, while the requirement should probably stay more on the level of who get's to decide. My reading of this requirement was that you would like to see support of both cases where either the client or the proxy does decide about the assignment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Your reading is correct. The requirement is intentionally written this way to allow assignment by either the client or server, as there are cases where that is required. In the typical VPN use case, the server will be assigning one or more addresses to the client, but in a network-to-network (aka site-to-site) VPN the client is informing the server of the IP ranges it is making accessible.

Copy link
Author

Choose a reason for hiding this comment

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

Can we work on the wording to make it more a high level requirement and less specific to any mechanism on how it could be realised?

Copy link
Collaborator

Choose a reason for hiding this comment

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

What in the wording would you like to see be made more high level? I think it is fine as-is.

@mirjak
Copy link
Author

mirjak commented Jan 21, 2021

Sorry I was in a hurry yesterday. Fixed all the typos and missing words now.

Comment on lines +159 to +161
or the proxy. If the address is assigned by the client, the proxy has to ensure
that the IP address is from a valid range that the client has authority of and that
return traffic is routed correctly over the proxy.
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
or the proxy. If the address is assigned by the client, the proxy has to ensure
that the IP address is from a valid range that the client has authority of and that
return traffic is routed correctly over the proxy.
or the proxy.

I can remove the sentence about validation (because that can indeed be seen as a separate discussion), however, I would still like to see the rest of the PR as there are two cases which should be covered here: 1) the client request a certain IP or IP range, or the proxy just assigns an address and tells the client about it.

@mirjak mirjak mentioned this pull request May 17, 2021
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.

None yet

4 participants