-
Notifications
You must be signed in to change notification settings - Fork 335
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
tinc-vpn: T766: Initial support for tinc VPN #570
Conversation
195cc7a
to
b5cc71f
Compare
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've done a lot of commenting on the current PR code. and have a few comments:
- All yes/no options should be changed to
Node
object, that is the oposite of the default value. if the value is not specified the default value should be used instead - Change interface names to
tincN
instead ofvtincN
thev
is not needed in this setting - the
type
option needs to be reconsidered, and used more understandable options. - Network name should be hardcoded to the interface name, and the user should not need to keep track of this setting
- Marked some typo's
- the way that tinc-up/down is laid out will not work and need to be refactored.
- doing
systemctl restart tinc
after a config change, will that tear down the interface so it need to be fully reinitialized?
on the other hand i cant find any definition of remote-nodes(hosts?), is this missing or have i just missed it?
other than that this is great work
9ec47ee
to
e99bfac
Compare
One of the connect options is used to set the host address connected to the opposite end (the address must have a file in the hosts directory)
After executing
This file is used to set the basic address and fixed configuration information of the interface. I set the interface class and do more configuration in it
After testing, the network parameters only take effect locally, so cancel this setting option, the network parameters will correspond to the interface name
Regarding equipment type parameters, do you have better suggestions for my reference? |
5a27c74
to
8d2b668
Compare
The address should be configured trough the configuration class and not hardcoded this way. The reason for this is that as the interface is recreated all options needs to be readded to the interface. this means that all options configured from the interface class needs to be readded.. |
About the
and
is the looking at the documentation also states that I think we should not allow modification of |
for the mode setting the default is |
Do you need to keep other options of device.type (except tun and tap) |
I do not see a need to keep any of the other options. i'm proposing removing the |
This is not what i ment. All hosts (remote nodes) needs to be defined with its own configuration files and encryption public keys. these are not defined anywhere as i can see. the ConnectTo option only specifies what primary endpoint should be tried to connect to. all connect statements also needs a corresponding host config |
a671981
to
23f1218
Compare
Normally, the following command is used to generate the host configuration file
The local node configuration file is automatically generated in this way, without other instructions and configuration intervention Some other options also come from the host configuration file, but it seems that they can be written in I plan to download files from remote using the following command: op-mode:
or
To copy the hosts file To use
or
When the file soft link does not exist, it will be copied automatically (soft link is created, and the file pointed to here cannot be deleted, otherwise it will cause an exception) |
Seems to be lost in late conflict resolution, I've manually restored this and kept the mainline in sync |
Tinc facts:
|
you mean? |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
Conflicts have been resolved. A maintainer will review the pull request shortly. |
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.
There are quite a few improvements to make in the CLI. Please address the suggestions about spurious capitalizations, overly long lines, and other style issues (please note that many of those issues appear in many places, so if I commented on anything once, it means it applies to all similar cases, it just wouldn't be practical to duplicate those comments).
Whether we should use the PKI infrastructure for keys instead of file paths is a more interesting question, let's see what other people think. I'd be in favor of moving to PKI for consistency with OpenVPN and other components.
<children> | ||
<leafNode name="node-name"> | ||
<properties> | ||
<help>Local Node Name options(require)</help> |
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.
Why capitalize "Node Name"? Also, it should be [REQUIRED]
as per out convention, although I'm not a big fan of it — if users miss it, the commit verification will fail and they will know, so I'd remove it altogether.
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's the meaning?
</leafNode> | ||
<leafNode name="mtu"> | ||
<properties> | ||
<help>MTU(Bytes,default:1514)</help> |
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.
Why is it larger than the typical Ethernet MTU? To cause fragmentation? ;)
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.
https://manpages.ubuntu.com/manpages/xenial/en/man5/tinc.conf.5.html
Because the description of the man manual is written as the default 1514
This can be consistent with the official default settings of tinc
<valueless/> | ||
</properties> | ||
</leafNode> | ||
<leafNode name="private-keyfile"> |
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.
We should probably use the PKI instrastructure now that OpenVPN etc. are using it already. I mean, set pki ...
. I'd be curious what @sever-sever, @c-po and others think about that.
The last release of TINC was in 2018 and there was no development activity since the last year: https://github.com/gsliepen/tinc I believe this means TINC abandoned and cannot be included. However, if the development activity resumes and they make a stable release, we can reconsider it and reopen the PR. |
Tinc is still alive: gsliepen/tinc#443 |
https://phabricator.vyos.net/T766