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
mirjak
wants to merge
2
commits into
ietf-wg-masque:main
Choose a base branch
from
mirjak:patch-1
base: main
Could not load branches
Branch not found: {{ refName }}
Could not load tags
Nothing to show
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
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?
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.
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.
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.
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).
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.
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.
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.
I would make validation a separate requirement instead of conflating it with the IP assignment requirement
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.
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.
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.
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.
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.
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.
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.
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?
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.
What in the wording would you like to see be made more high level? I think it is fine as-is.