-
Notifications
You must be signed in to change notification settings - Fork 148
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
Refactor locking, kb interactive changes and add authorized_keys api #448
Conversation
modules/[email protected]
Outdated
This statement is present so the mandatory descendant | ||
nodes do not imply that this node must be | ||
configured."; | ||
presence "Indicates that PAM configuration file name has been configured and that |
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.
This description should be updated. Also, I propose to rename this container to something like use-system-auth
.
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 proposed solution to this is to add a mandatory choice node to the presence container.
container keyboard-interactive {
presence "";
choice method {
mandatory true;
leaf pam-based {
type empty;
}
}
}
This choice will, for now, only have a single case, that is pam-based
. In the future as the library grows other cases may be added to the choice node allowing for different approaches when it comes to the keyboard interactive authentication.
The description of the pam-based
leaf will reflect how it's handled in the library.
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 suboptimal because it leaks some implementation details ("it's using PAM") into a configuration which, in my opinion, should just say "this is authenticating in the same way as the rest of the Linux system".
Another problem which we talked about with Michal over Slack the other day is that this concept allows different authentication backends to be used for the password
and keyboard-interactive
SSH authentication schemes, which makes no sense to me. We were not able to reach a mutual understanding about this point, though.
In short, I prefer the configuration to describe how the system behaves to an external observer. To a "NETCONF admin", the choice is really between "let me configure this explicitly defined list of users for this endpoint", which the IETF models are all about, vs. "delegate auth to the system for this endpoint", which I think is the purpose of this extension. In contrast to that, the choice between PAM and getpwent
/getspnam
/... is something that the system architect should be making. I cannot imagine that someone might want to provision a system where one half of SSH clients authenticate through the system's /etc/shadow
while the rest goes through the system's PAM, especially since PAM's default config delegates everything to /etc/shadow
, and when the decision is essentially only determined by the level of support of SSH features in the client' SSH library.
For me, the perfect approach will be a single, empty presence container np2:user-system-auth
which, if present, will use PAM (with a PAM service name that's passed from a build-time-configurable default in Netopeer2 to libnetconf2 through an API call) for both kbd-interactive and password authentication. Or event better -- make it a non-presence container with a single leaf enabled
so that I can add an augment which sets its default value to true
. To me, this configuration is all about policies, not about fine-tuning library behavior in a level of detail that is, IMHO, not needed at all. The system is either built with PAM, which is a huge majority of any "classic" Linux systemd, or it is built without PAM which might be the case on some really restricted embedded boxes. Sure, have an #ifdef
in code for that if you want to care about such systems, but using and bypassing PAM at the same time is something that I see no use case for.
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 have just updated it, feel free to take a look @jktjkt .
modules/[email protected]
Outdated
uses keyboard-interactive-grouping; | ||
} | ||
|
||
augment "/ncs:netconf-server/ncs:listen/ncs:endpoint/ncs:transport" { | ||
case unix-socket { | ||
container unix-socket { | ||
description | ||
"Defines a new transport called UNIX socket."; | ||
"UNIX socket listening configuration for inbound connections."; |
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.
From the sysadmin's perspective, this allows the NACM admin to create a socket on disk as root. I question the need for this; it's not an operational choice such as "what port to use for NETCONF", or "do we allow TLS at all". It's a feature which applies exclusively to the local machine, and there's no remote access. As such, I'm convinced that it should be driven only by API, not by YANG data. If his feature stays in, we will have to explicitly disable that by a deviation I suppose. I just hope there's still some CLI flag for netopeer2-server
to force it to listen on a Unix socket somewhere...
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.
As far as I am aware this ability was removed from netopeer2 as it didn't seem to be necessary (so it's configurable by data only). However, I can see your point and will discuss it.
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.
@jktjkt I think there is a difference of our understanding of YANG. You consider it a modelling language for configuration managed by network protocols such as NETCONF or RESTCONF. While that was the initial purpose, since then has YANG became largely independent and its most narrow purpose being simply a modelling language for any configuration. And that is how we try to use it and in that way it makes sense to have UNIX socket configuration there, for example.
But you are right that this independence is still to some extent theoretical and latest YANG and especially NETCONF RFCs do not fully reflect this yet. So there may be some practical problems, which is probably the reason you are against similar changes such as this one.
Now, hopefully having better understand of each other's approach, what exactly is the problem of having this configuration in YANG? Why cannot you configure the UNIX socket by locally changing the YANG configuration?
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.
TL;DR: if you think you have users who need this, please make this configurable only when a YANG-level feature is enabled, and keep a CLI flag in Netopeer2 to pass a socket path to listen on. We will keep this feature disabled on our boxes.
The main problem is that one will have to propagate this configuration "all over the place". For example, our devices use the netconf-cli
as an /etc/passwd
-level login shell for unprivileged users. This "login shell" connects to Netopeer over its Unix socket (so that we can use SO_PEERCRED
for NACM, for example). If this v3/v4 code gets used in our devices in its current form, the console would have to ask sysrepo for the current config of the listening backend and use that as the path to open. It is doable, but I seriously wonder "what for". Another source of complexity is selinux (and I believe AppArmor as well) where you assign labels to objects in filesystem, and you define access policies based on these labels -- a classic example of an RBAC system for Linux. You would have to essentially recompile the access policy database, and that's a big no-no from the compliance point of view (as well as resource usage, btw, but that's a minor detail of selinux). Going further, there are mount namespaces where one process' /var/run/xx.sock
might not be reachable at all from other processes on the system, etc etc etc.
In short, this approach is crossing a certain fuzzy boundary between "system configuration" and "internal system setup". I understand why it looks so nice from the perspective of a library developer. I'm also all for configuring "the system" completely over YANG -- that's what we've been using sysrepo et al for for the past five years, after all, so I think I have some practical experience in this topic. However, there's a line between what "can be done with YANG" (and the answer to that is "everything", really), and what's practical and reasonable. There are sill many features of a running Netopeer2 process that cannot be configured in the current devel
branch, for example:
- you cannot configure the UID that the daemon runs as,
- you cannot set a process priority, set resource usage limits, or configure the cgroup properties of the process tree,
- you cannot set the sysrepo recovery UID, SHM prefixes, etc,
- you do not set up the kernel's UTS, mount, user, network,... namespaces,
- you do not get to configure your init system's dependencies, etc
And I'm 100% convinced that there is a very good reason for that -- there is typically no use case for changing these on-the-fly and remotely. If you let your remote NACM-admin user change them, there's a non-trivial chance that the system will either become inaccessible, or that you will introduce a security issue into a well-designed system.
IMHO, the only reason why Netopeer2 exists is that people want to use it in their systems as a NETCONF server. These systems have to be designed in some way, and part of this design is a decision on how they will look like "internally". This is a job that we've been doing for years, and a part of this task is deciding how user accounts are going to be managed, how the FS layout will look like, how to perform firmware updates, etc. Another part of the job is defining what part of the system behavior is configurable, and what is fixed, or decided up front in the design phase. Now, I understand that in theory, someone else might be thrilled by an option to configure the Unix socket to listen at over YANG (I wonder why, and I'm sincerely interested in whether you actually got user feedback about this, but at least in theory I accept that it's possible). The problem that I think you currently do not recognize is that there are people like us who do not want to make this configurable, and who do not even want to have this Unix path visible in the system's config because doing that would cause extra support load and user confusion.
For us, exposing config toggles where any other value but the default one leads to a broken system is something that we would very much like to avoid.
@@ -68,73 +68,6 @@ server_thread(void *arg) | |||
return NULL; | |||
} | |||
|
|||
static char * |
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.
PAM can be still tested even without the confdir
support with pam_wrapper
. Here's how we're doing that in another CMake-based project:
https://github.com/CESNET/rousette/blob/master/CMakeLists.txt#L171
One of the gotchas is that pam_wrapper
performs some serious magic in the background which involves per-process temporary .so
files as well as binary patching that libpam.so
on disk. That's why we ended up running it in a mount namespace with a separate /tmp
. Previously, we used pam_userdb
, but that one will be undergoing some important changes in the next release of modern distros, so we ported away from that.
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.
Thank you, I will take a look at this. I wasn't happy about removing the PAM module and the testing of the kbint method, but I thought it wasn't possible.
e8f18e0
to
846ca43
Compare
846ca43
to
27d3da3
Compare
fcc6dfe
to
a2f6e5e
Compare
These changes include: - PAM filename is set globally for all clients - PAM directory not configurable (security reasons) - Removal of the ln2 PAM module and its use in a test - Description added to the ln2 YANG module - Change of add_user_interactive API to only enable kbint for a user
Removed unix socket from configuration and moved it back to API.
a2f6e5e
to
94ac467
Compare
No description provided.